From b3f897c36edd30dd6776792ea7a37a176d32799d Mon Sep 17 00:00:00 2001 From: James Mackay Date: Sun, 11 Oct 2026 07:04:28 +0100 Subject: [PATCH] fix(config_flow): clearing an entity field no longer fails validation (t47 C) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit t46 F1, reproduced against real Home Assistant: submitting the options page with a cleared entity field (script_turn_on='') produced "Schema validation failed at script_turn_on". HA validates the submitted page against the schema the flow last returned, and real EntitySelector rejects BOTH '' and None — "Entity is neither a valid entity ID nor a valid UUID" (verified directly against homeassistant's EntitySelector). Clearing therefore depended on the frontend OMITTING the key, on every entity field of the page, not only the script ones t44 added. _ClearingEntitySelector subclasses EntitySelector and returns '' for None or an empty/whitespace string, delegating everything else to the parent — so a real entity id is still domain-checked, and the class is still an EntitySelector: its serialized form is byte-identical to the real selector's ({"selector": {"entity": {"domain": ["script"], "multiple": false, "reorder": false}}}), so the frontend keeps rendering an entity picker. The step also normalizes a submitted None to '' before it acts: an explicit empty value clears the setting (the script dict drops it, the patched options store '' which every reader already treats as unset), while an OMITTED key still means "the page did not touch it" for the patched options — and still means "cleared" for the script dict, which is rebuilt from the page (t44's editor-is-the-truth rule, unchanged). The test harness now models the trap instead of hiding it: EntitySelector is a CLASS (entity_stublib.EntitySelectorStub) that rejects ''/None like real HA, defined once and shared by all five stub installers through install_selector_faces(), so the stubs cannot drift apart. Tests (tests/test_config_flow_ux.py::TestClearingAnEntityField, 8): the schema accepts omitted/None/'' while passing a real entity id through; the plain selector is pinned as rejecting the same values (the trap itself); '' and None each clear a script AND a pre-existing entity field (jellyfin, the older instance of the pattern) while untouched fields survive; an omitted script field is cleared; an omitted entity field is kept; and a prefill→submit round trip changes nothing. RED PROOF: with the five selector sites switched back to the plain EntitySelector, exactly the five tolerance tests fail (5 failed, 4 passed) and the rest of the file stays green; restored, 70 pass. Real-HA probe run separately (see the task output): strict rejects '' and None, the subclass accepts both and returns '', and serialization is unchanged. Verified: 423 passed / 2 skipped, ruff clean, drift engine exit 0 under real HA. --- .../nvidia_shield/config_flow.py | 35 +++- tests/entity_stublib.py | 36 +++- tests/test_config_flow_pairing.py | 6 +- tests/test_config_flow_protocol.py | 6 +- tests/test_config_flow_ux.py | 178 ++++++++++++++++-- tests/test_standalone_setup.py | 6 +- 6 files changed, 233 insertions(+), 34 deletions(-) diff --git a/custom_components/nvidia_shield/config_flow.py b/custom_components/nvidia_shield/config_flow.py index acf90cd..bffafbf 100644 --- a/custom_components/nvidia_shield/config_flow.py +++ b/custom_components/nvidia_shield/config_flow.py @@ -179,20 +179,42 @@ def _user_schema_dict(sel, defaults: dict | None = None) -> dict: return schema_dict +class _ClearingEntitySelector(selector.EntitySelector): + """An entity selector that accepts a CLEARED field (t47 F1). + + Clearing an optional entity field used to depend on the frontend OMITTING the + key. It does not have to: a submission can carry ``''`` or ``None``, and real + Home Assistant's EntitySelector rejects both — the reviewer's real-HA run + produced "Schema validation failed at script_turn_on" from an explicit clear. + That made clearing look like a broken form rather than a choice, on every + entity field of this page (the script selectors are the newest, not the only + ones). + + Returning the empty value unchanged lets the flow read it as "no selection", + while a real entity id still goes through the parent's validation. The class + stays an EntitySelector, so the frontend still renders an entity picker. + """ + + def __call__(self, data): + if data is None or (isinstance(data, str) and not data.strip()): + return "" + return super().__call__(data) + + def _build_selectors(): """Build reusable selector instances.""" return { - "media_player": selector.EntitySelector( + "media_player": _ClearingEntitySelector( selector.EntitySelectorConfig(domain="media_player") ), - "receiver": selector.EntitySelector( + "receiver": _ClearingEntitySelector( selector.EntitySelectorConfig(domain="media_player", device_class="receiver") ), - "power": selector.EntitySelector( + "power": _ClearingEntitySelector( selector.EntitySelectorConfig(domain=["switch", "binary_sensor"]) ), # t44: a script entity per command override - "script": selector.EntitySelector( + "script": _ClearingEntitySelector( selector.EntitySelectorConfig(domain="script") ), } @@ -846,6 +868,11 @@ class NvidiaShieldOptionsFlow(config_entries.OptionsFlowWithReload): """Single-page options.""" errors = {} if user_input is not None: + # t47 F1: a field submitted as None means "not supplied" — only an + # explicit empty value ('' or a cleared selector) clears a setting, so + # the two cannot be confused. Note the schema itself already tolerates + # the empty value (see _ClearingEntitySelector). + user_input = {key: value for key, value in user_input.items() if value is not None} # P5-2: validate the digit delay (0.05–1.0 s, §7 window) delay = user_input.pop(CONF_CHANNEL_DIGIT_DELAY, None) if delay is not None: diff --git a/tests/entity_stublib.py b/tests/entity_stublib.py index f3198f0..5d59a4b 100644 --- a/tests/entity_stublib.py +++ b/tests/entity_stublib.py @@ -42,6 +42,37 @@ ENTITIES_STUB_NAMES = ( ) +class EntitySelectorStub: + """Stand-in for HA's EntitySelector that REJECTS '' and None, like the real one. + + Real HA answers a cleared entity field with "Schema validation failed at …" + (t46 F1), so a stub that accepted anything would hide exactly the trap the flow + has to tolerate; tests/test_config_flow_ux.py validates a schema with both + values to prove it. + """ + + def __init__(self, config=None): + self.config = config + + def __call__(self, data): + if not isinstance(data, str) or not data.strip(): + raise ValueError("Entity is not a valid entity id") + return data + + +def install_selector_faces(selector_module) -> None: + """Populate a ``homeassistant.helpers.selector`` stub module. + + One definition for every installer (entity_stublib and the four per-file + installers in the flow tests), so the stubs cannot drift apart. + """ + selector_module.EntitySelector = EntitySelectorStub + selector_module.EntitySelectorConfig = lambda *args, **kwargs: "" + selector_module.SelectSelector = lambda *args, **kwargs: None + selector_module.SelectSelectorConfig = lambda **kwargs: "" + selector_module.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") + + class MediaPlayerState(enum.StrEnum): OFF = "off" ON = "on" @@ -428,7 +459,7 @@ def install() -> None: event.async_track_state_change_event = lambda *args, **kwargs: (lambda: None) helpers.event = event helpers.selector = types.SimpleNamespace( - EntitySelector=lambda *a, **k: None, + EntitySelector=EntitySelectorStub, EntitySelectorConfig=lambda *a, **k: "", SelectSelector=lambda *a, **k: None, SelectSelectorConfig=lambda **k: "", @@ -489,8 +520,7 @@ def install() -> None: # `_STUB_FACES` honest: install() then really does provide every face a test # file may have popped, which is what its guard checks. selector = types.ModuleType("homeassistant.helpers.selector") - selector.EntitySelector = lambda *args, **kwargs: None - selector.EntitySelectorConfig = lambda *args, **kwargs: "" + install_selector_faces(selector) selector.SelectSelector = lambda *args, **kwargs: None selector.SelectSelectorConfig = lambda **kwargs: "" selector.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") diff --git a/tests/test_config_flow_pairing.py b/tests/test_config_flow_pairing.py index 96ad0b7..81a1c0b 100644 --- a/tests/test_config_flow_pairing.py +++ b/tests/test_config_flow_pairing.py @@ -127,11 +127,7 @@ def _install_import_stubs() -> None: helpers.device_registry = device_registry sys.modules["homeassistant.helpers.device_registry"] = device_registry selector = types.ModuleType("homeassistant.helpers.selector") - selector.EntitySelector = lambda *a, **k: None - selector.EntitySelectorConfig = lambda *a, **k: "" - selector.SelectSelector = lambda *a, **k: None - selector.SelectSelectorConfig = lambda **k: "" - selector.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") + entity_stublib.install_selector_faces(selector) sys.modules["homeassistant.helpers.selector"] = selector diff --git a/tests/test_config_flow_protocol.py b/tests/test_config_flow_protocol.py index c255d23..5d07911 100644 --- a/tests/test_config_flow_protocol.py +++ b/tests/test_config_flow_protocol.py @@ -220,11 +220,7 @@ def _install_stubs() -> None: helpers.device_registry = device_registry sys.modules["homeassistant.helpers.device_registry"] = device_registry selector = types.ModuleType("homeassistant.helpers.selector") - selector.EntitySelector = lambda *a, **k: None - selector.EntitySelectorConfig = lambda *a, **k: "" - selector.SelectSelector = lambda *a, **k: None - selector.SelectSelectorConfig = lambda **k: "" - selector.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") + entity_stublib.install_selector_faces(selector) sys.modules["homeassistant.helpers.selector"] = selector diff --git a/tests/test_config_flow_ux.py b/tests/test_config_flow_ux.py index fcb6af8..1a71dd5 100644 --- a/tests/test_config_flow_ux.py +++ b/tests/test_config_flow_ux.py @@ -37,6 +37,7 @@ from nsh.const import ( CONF_HOST, CONF_JELLYFIN_ENTITY, CONF_PAIRING_CODE, + CONF_POWER_ENTITY, CONF_PROTOCOL_ENABLED, CONF_SKIP_PAIRING, DEFAULT_ADB_PORT, @@ -227,11 +228,7 @@ def _install_stubs() -> None: sys.modules["homeassistant.helpers.device_registry"] = device_registry selector = types.ModuleType("homeassistant.helpers.selector") - selector.EntitySelector = lambda *a, **k: None - selector.EntitySelectorConfig = lambda *a, **k: "" - selector.SelectSelector = lambda *a, **k: None - selector.SelectSelectorConfig = lambda **k: "" - selector.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") + entity_stublib.install_selector_faces(selector) helpers.selector = selector sys.modules["homeassistant.helpers.selector"] = selector sys.modules["homeassistant.helpers"] = helpers @@ -1451,6 +1448,17 @@ class TestCopyContract: assert field in step["data"], f"options.{field} has no label" +def _options_flow(tmp_path, options=None): + """An options flow over an entry with `options`, on a fake hass.""" + module = _form_module() + flow = module.NvidiaShieldOptionsFlow() + flow.config_entry = SimpleNamespace( + options=dict(options or {}), entry_id="e1", title="Shield" + ) + flow.hass = FakeHass(str(tmp_path)) + return flow + + class TestCommandScriptOptions: """t44: one script selector per command, stored as one dict option. @@ -1460,13 +1468,7 @@ class TestCommandScriptOptions: """ def _flow(self, tmp_path, options=None): - module = _form_module() - flow = module.NvidiaShieldOptionsFlow() - flow.config_entry = SimpleNamespace( - options=dict(options or {}), entry_id="e1", title="Shield" - ) - flow.hass = FakeHass(str(tmp_path)) - return flow + return _options_flow(tmp_path, options) @staticmethod def _submitted(schema): @@ -1542,6 +1544,158 @@ class TestCommandScriptOptions: ) +class TestClearingAnEntityField: + """t47 F1: clearing must work whichever shape the field arrives in. + + Home Assistant validates the submitted page against the schema the flow last + returned, and real HA's EntitySelector rejects BOTH ``''`` and ``None`` + ("Entity is neither a valid entity ID nor a valid UUID") — so an explicit clear + was reported as a malformed form. The three shapes are pinned here, validated + through the schema exactly as HA does, and the happy path is pinned too so the + tolerance cannot start swallowing real edits. + """ + + @staticmethod + def _validate(form, data): + """Run a submitted page through the schema the form returned, as HA does. + + This is the step between the frontend and the flow, and it is where the + clearing trap used to fire. The harness's voluptuous mirror carries marker + defaults but does not validate (``vol.Schema`` is the identity there), so + each field is sent through its own schema entry — the same call HA makes, + and the one that used to raise for a cleared entity field. + """ + raw = form["data_schema"] + validators = { + getattr(key, "schema", None): value + for key, value in raw.items() + if getattr(key, "schema", None) is not None + } + validated = {} + for field, value in data.items(): + validator = validators.get(field) + validated[field] = validator(value) if callable(validator) else value + return validated + + async def test_the_schema_accepts_an_omitted_none_and_empty_value(self, tmp_path): + flow = _options_flow( + tmp_path, {CONF_COMMAND_SCRIPTS: {"turn_on": "script.watch"}} + ) + form = await flow.async_step_init(None) + + for value in ("", None): + assert self._validate(form, {f"{SCRIPT_FIELD_PREFIX}turn_on": value}) == { + f"{SCRIPT_FIELD_PREFIX}turn_on": "" + } + # a real entity id is still passed through, and an omitted key stays omitted + validated = self._validate(form, {f"{SCRIPT_FIELD_PREFIX}turn_on": "script.other"}) + assert validated[f"{SCRIPT_FIELD_PREFIX}turn_on"] == "script.other" + assert f"{SCRIPT_FIELD_PREFIX}volume_up" not in validated + + def test_a_plain_entity_selector_rejects_the_same_values(self): + """The trap itself: without the tolerant selector these shapes blow up. + + The stub mirrors real HA here (see entity_stublib.EntitySelectorStub), so + this stays honest without needing a real-HA interpreter. + """ + import homeassistant.helpers.selector as selector_module + + strict = selector_module.EntitySelector( + selector_module.EntitySelectorConfig(domain="script") + ) + + # the stub raises ValueError; real HA raises probatio.Invalid for the same + # values ("Entity is neither a valid entity ID nor a valid UUID", verified) + for value in ("", None): + with pytest.raises(ValueError): + strict(value) + + @pytest.mark.parametrize("value", ["", None]) + async def test_an_explicit_empty_value_clears_the_script(self, tmp_path, value): + flow = _options_flow( + tmp_path, {CONF_COMMAND_SCRIPTS: {"turn_on": "script.watch"}} + ) + form = await flow.async_step_init(None) + + # what HA does: validate the submitted page, then run the step + result = await flow.async_step_init( + self._validate(form, {f"{SCRIPT_FIELD_PREFIX}turn_on": value}) + ) + + assert result["type"] == "create_entry" + assert result["data"][CONF_COMMAND_SCRIPTS] == {} + + @pytest.mark.parametrize("value", ["", None]) + async def test_an_explicit_empty_value_clears_a_preexisting_entity_field( + self, tmp_path, value + ): + """The pattern predates the script selectors — jellyfin had it too.""" + flow = _options_flow( + tmp_path, + {CONF_JELLYFIN_ENTITY: "media_player.jellyfin", CONF_HOST: "10.0.0.9"}, + ) + form = await flow.async_step_init(None) + + result = await flow.async_step_init( + self._validate(form, {CONF_JELLYFIN_ENTITY: value}) + ) + + assert not result["data"][CONF_JELLYFIN_ENTITY] # cleared, not kept + assert result["data"][CONF_HOST] == "10.0.0.9" # untouched fields survive + + async def test_an_omitted_script_field_is_cleared_as_well(self, tmp_path): + """The script dict is rebuilt from the page, so omitting a command clears it.""" + flow = _options_flow( + tmp_path, {CONF_COMMAND_SCRIPTS: {"turn_on": "script.watch"}} + ) + form = await flow.async_step_init(None) + + result = await flow.async_step_init(self._validate(form, {})) + + assert result["data"][CONF_COMMAND_SCRIPTS] == {} + + async def test_an_omitted_entity_field_keeps_its_stored_value(self, tmp_path): + """An omitted key is "not touched" for the patched options, not a clear.""" + flow = _options_flow( + tmp_path, {CONF_JELLYFIN_ENTITY: "media_player.jellyfin"} + ) + form = await flow.async_step_init(None) + + result = await flow.async_step_init(self._validate(form, {CONF_HOST: "10.0.0.9"})) + + assert result["data"][CONF_JELLYFIN_ENTITY] == "media_player.jellyfin" + + async def test_a_prefill_submit_round_trip_changes_nothing(self, tmp_path): + stored = { + CONF_COMMAND_SCRIPTS: {"turn_on": "script.watch"}, + CONF_JELLYFIN_ENTITY: "media_player.jellyfin", + CONF_POWER_ENTITY: "switch.plug", + CONF_HOST: "10.0.0.9", + } + flow = _options_flow(tmp_path, stored) + form = await flow.async_step_init(None) + suggested = form["data_schema"].get("__suggested__", {}) + + # the page comes back carrying the stored values, and the user submits them + assert suggested[f"{SCRIPT_FIELD_PREFIX}turn_on"] == "script.watch" + result = await flow.async_step_init( + self._validate( + form, + { + f"{SCRIPT_FIELD_PREFIX}turn_on": "script.watch", + CONF_JELLYFIN_ENTITY: "media_player.jellyfin", + CONF_POWER_ENTITY: "switch.plug", + CONF_HOST: "10.0.0.9", + }, + ) + ) + + assert result["data"][CONF_COMMAND_SCRIPTS] == {"turn_on": "script.watch"} + assert result["data"][CONF_JELLYFIN_ENTITY] == "media_player.jellyfin" + assert result["data"][CONF_POWER_ENTITY] == "switch.plug" + assert result["data"][CONF_HOST] == "10.0.0.9" + + class TestStringsTranslationsParity: """Both copies of the UI copy must expose the same key tree.""" diff --git a/tests/test_standalone_setup.py b/tests/test_standalone_setup.py index 8e87fb5..a384f48 100644 --- a/tests/test_standalone_setup.py +++ b/tests/test_standalone_setup.py @@ -215,11 +215,7 @@ def _install_stubs() -> None: helpers.device_registry = device_registry sys.modules["homeassistant.helpers.device_registry"] = device_registry selector = types.ModuleType("homeassistant.helpers.selector") - selector.EntitySelector = lambda *a, **k: None - selector.EntitySelectorConfig = lambda *a, **k: "" - selector.SelectSelector = lambda *a, **k: None - selector.SelectSelectorConfig = lambda **k: "" - selector.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") + entity_stublib.install_selector_faces(selector) sys.modules["homeassistant.helpers.selector"] = selector