refactor(output-targets): registry + coverage assertion for response builders
``_target_to_response`` in ``api/routes/output_targets.py`` used to be
an isinstance ladder over the three OutputTarget subclasses with a
silent fallback that fabricated a ``LedOutputTargetResponse`` for
unknown types (audit finding H3). The fallback masked exactly the
kind of bug we hit on the CSS side in Phase 1.1: a new target subclass
slipped past the ladder and got mis-shaped on the wire.
Replace the ladder with a ``_TARGET_RESPONSE_BUILDERS`` dict keyed by
the concrete subclass plus an import-time
``_assert_target_response_coverage()`` that requires the registry to
exactly match ``{WledOutputTarget, HALightOutputTarget,
Z2MLightOutputTarget}``. ``_target_to_response`` now raises
``RuntimeError`` instead of silently fabricating a LED response for an
unknown subclass — coverage is asserted at import so this branch is
unreachable in normal operation.
Tests: 5 new regression tests cover bijection between expected classes
and registered builders, callable shape, the rogue-target-raises
contract, and missing/extra entry rejection in the assertion. 24
existing output-target tests stay green; ruff clean.
This commit is contained in:
@@ -175,26 +175,57 @@ def _validate_color_value_source(
|
||||
)
|
||||
|
||||
|
||||
def _target_to_response(target) -> OutputTargetResponse:
|
||||
"""Convert any OutputTarget to the appropriate typed response."""
|
||||
if isinstance(target, WledOutputTarget):
|
||||
return _led_target_to_response(target)
|
||||
elif isinstance(target, HALightOutputTarget):
|
||||
return _ha_light_target_to_response(target)
|
||||
elif isinstance(target, Z2MLightOutputTarget):
|
||||
return _z2m_light_target_to_response(target)
|
||||
else:
|
||||
# Fallback for unknown types — use LED response with defaults
|
||||
return LedOutputTargetResponse(
|
||||
id=target.id,
|
||||
name=target.name,
|
||||
description=target.description,
|
||||
tags=target.tags,
|
||||
created_at=target.created_at,
|
||||
updated_at=target.updated_at,
|
||||
_TARGET_RESPONSE_BUILDERS: dict = {
|
||||
WledOutputTarget: _led_target_to_response,
|
||||
HALightOutputTarget: _ha_light_target_to_response,
|
||||
Z2MLightOutputTarget: _z2m_light_target_to_response,
|
||||
}
|
||||
|
||||
|
||||
def _assert_target_response_coverage() -> None:
|
||||
"""Verify the response registry covers every concrete OutputTarget subclass.
|
||||
|
||||
Runs at module import. Surfaces a missing builder eagerly instead of
|
||||
letting a request fall through to the previous silent fallback (which
|
||||
used to return a defaults-filled LedOutputTargetResponse and quietly
|
||||
misshape the payload for unknown target types).
|
||||
"""
|
||||
expected = {WledOutputTarget, HALightOutputTarget, Z2MLightOutputTarget}
|
||||
registered = set(_TARGET_RESPONSE_BUILDERS.keys())
|
||||
missing = expected - registered
|
||||
extra = registered - expected
|
||||
if missing or extra:
|
||||
problems = []
|
||||
if missing:
|
||||
problems.append(f"missing builders: {sorted(c.__name__ for c in missing)}")
|
||||
if extra:
|
||||
problems.append(f"unregistered classes: {sorted(c.__name__ for c in extra)}")
|
||||
raise RuntimeError(
|
||||
"_TARGET_RESPONSE_BUILDERS is out of sync with the OutputTarget "
|
||||
"subclass set: " + "; ".join(problems)
|
||||
)
|
||||
|
||||
|
||||
_assert_target_response_coverage()
|
||||
|
||||
|
||||
def _target_to_response(target) -> OutputTargetResponse:
|
||||
"""Convert any OutputTarget to the appropriate typed response.
|
||||
|
||||
Dispatches via :data:`_TARGET_RESPONSE_BUILDERS` keyed by concrete
|
||||
subclass. Raises ``RuntimeError`` for an unregistered subclass —
|
||||
coverage is asserted at import, so this should never fire in
|
||||
practice; if it does, the storage layer added a new OutputTarget
|
||||
subclass without a matching response builder here.
|
||||
"""
|
||||
builder = _TARGET_RESPONSE_BUILDERS.get(type(target))
|
||||
if builder is None:
|
||||
raise RuntimeError(
|
||||
f"No response builder registered for OutputTarget subclass " f"{type(target).__name__}"
|
||||
)
|
||||
return builder(target)
|
||||
|
||||
|
||||
# ===== CRUD ENDPOINTS =====
|
||||
|
||||
|
||||
|
||||
@@ -0,0 +1,69 @@
|
||||
"""Tests for the output-target response-builder registry.
|
||||
|
||||
Locks in the safety net that replaced the previous silent
|
||||
``LedOutputTargetResponse(defaults)`` fallback in
|
||||
``output_targets._target_to_response``.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from ledgrab.api.routes import output_targets
|
||||
from ledgrab.api.routes.output_targets import (
|
||||
_assert_target_response_coverage,
|
||||
_target_to_response,
|
||||
_TARGET_RESPONSE_BUILDERS,
|
||||
)
|
||||
from ledgrab.storage.ha_light_output_target import HALightOutputTarget
|
||||
from ledgrab.storage.wled_output_target import WledOutputTarget
|
||||
from ledgrab.storage.z2m_light_output_target import Z2MLightOutputTarget
|
||||
|
||||
|
||||
EXPECTED_TARGET_CLASSES = {WledOutputTarget, HALightOutputTarget, Z2MLightOutputTarget}
|
||||
|
||||
|
||||
def test_registry_covers_every_known_target_subclass():
|
||||
assert set(_TARGET_RESPONSE_BUILDERS.keys()) == EXPECTED_TARGET_CLASSES
|
||||
|
||||
|
||||
def test_every_registered_builder_is_callable():
|
||||
for cls, builder in _TARGET_RESPONSE_BUILDERS.items():
|
||||
assert callable(builder), f"builder for {cls.__name__} is not callable"
|
||||
|
||||
|
||||
def test_unregistered_target_class_raises_in_target_to_response():
|
||||
"""Reaching ``_target_to_response`` with an unmapped class raises loudly
|
||||
instead of silently returning a malformed LedOutputTargetResponse.
|
||||
"""
|
||||
|
||||
class _RogueTarget:
|
||||
id = "tgt_rogue"
|
||||
name = "rogue"
|
||||
|
||||
with pytest.raises(RuntimeError, match="No response builder"):
|
||||
_target_to_response(_RogueTarget())
|
||||
|
||||
|
||||
def test_coverage_assertion_raises_on_drift(monkeypatch):
|
||||
"""Removing a builder from the registry makes the import-time check fail."""
|
||||
pruned = {
|
||||
cls: builder
|
||||
for cls, builder in _TARGET_RESPONSE_BUILDERS.items()
|
||||
if cls is not WledOutputTarget
|
||||
}
|
||||
monkeypatch.setattr(output_targets, "_TARGET_RESPONSE_BUILDERS", pruned)
|
||||
with pytest.raises(RuntimeError, match="WledOutputTarget"):
|
||||
_assert_target_response_coverage()
|
||||
|
||||
|
||||
def test_coverage_assertion_raises_on_extra_entry(monkeypatch):
|
||||
"""An entry keyed by an unknown class is also caught."""
|
||||
|
||||
class _RogueTargetClass:
|
||||
pass
|
||||
|
||||
extended = {**_TARGET_RESPONSE_BUILDERS, _RogueTargetClass: lambda t: None}
|
||||
monkeypatch.setattr(output_targets, "_TARGET_RESPONSE_BUILDERS", extended)
|
||||
with pytest.raises(RuntimeError, match="_RogueTargetClass"):
|
||||
_assert_target_response_coverage()
|
||||
Reference in New Issue
Block a user