From bee38a6bc7c0b82a4074229ef390e64d21f22448 Mon Sep 17 00:00:00 2001 From: rhammen <75572839+rhammen@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:20:18 +0200 Subject: [PATCH 1/8] fix: relax Installation validation to fields api.py actually uses Active, CurrentUserRoles, InstallationType and NetworkType are only ever read through set_attributes()'s optional ATTR_TYPES conversion, never hard-indexed. Requiring them meant a User-role-only account's reduced installation object (already known to drop AuthenticationType, see #357) would fail validation on whichever of these Zaptec's backend also happens to omit for that role. Only Id is genuinely required -- it's the one field Zaptec.build() indexes directly. Part of #359. Co-Authored-By: Claude Haiku 4.5 --- custom_components/zaptec/zaptec/validate.py | 10 +++++----- tests/zaptec/test_validate.py | 16 ++++++++++++++-- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/custom_components/zaptec/zaptec/validate.py b/custom_components/zaptec/zaptec/validate.py index efa5be57..bbd3bc36 100644 --- a/custom_components/zaptec/zaptec/validate.py +++ b/custom_components/zaptec/zaptec/validate.py @@ -16,10 +16,10 @@ class Installation(BaseModel): model_config = ConfigDict(extra="allow") Id: str - Active: bool - CurrentUserRoles: int - InstallationType: int - NetworkType: int + Active: bool | None = None + CurrentUserRoles: int | None = None + InstallationType: int | None = None + NetworkType: int | None = None class Installations(BaseModel): @@ -27,7 +27,7 @@ class Installations(BaseModel): model_config = ConfigDict(extra="allow") Data: list[Installation] - Pages: int + Pages: int | None = None class Charger(BaseModel): diff --git a/tests/zaptec/test_validate.py b/tests/zaptec/test_validate.py index 91e86caa..0afeb816 100644 --- a/tests/zaptec/test_validate.py +++ b/tests/zaptec/test_validate.py @@ -90,9 +90,21 @@ def test_installation_validation() -> None: with pytest.raises(ValidationError): validate(invalid_installation_list, installation_list_url) - # check that an installation missing NetworkType fails validation + # Users without the Owner/Service role get a reduced installation object + # missing Active/CurrentUserRoles/InstallationType/NetworkType (see #357). + # api.py only ever indexes Id directly, so this must still validate. + limited_installation = {"Id": valid_installation["Id"]} + validate(limited_installation, single_installation_url) + + limited_installation_list = { + "Pages": 1, + "Data": [limited_installation], + } + validate(limited_installation_list, installation_list_url) + + # Id is required: Zaptec.build() indexes inst_item["Id"] directly. invalid_installation = valid_installation.copy() - invalid_installation.pop("NetworkType") + invalid_installation.pop("Id") with pytest.raises(ValidationError): validate(invalid_installation, single_installation_url) From 7369f23f2f4a69fb8480c79b3b64f8a95db8bd78 Mon Sep 17 00:00:00 2001 From: rhammen <75572839+rhammen@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:27:08 +0200 Subject: [PATCH 2/8] fix: add missing Circuit.MaxCurrent validation, split hierarchy charger model Circuit.MaxCurrent is hard-indexed in Installation.build() (circuit["MaxCurrent"]) but was never declared on the Circuit model, so a response missing it would pass validation and then crash with a raw KeyError deep in build(). Also splits a minimal HierarchyCharger model out of Charger for the hierarchy endpoint's embedded charger stubs, since only Id is read there -- Name/Active/DeviceType requirements on that path were validating data the code never uses at that point. Part of #359. --- custom_components/zaptec/zaptec/validate.py | 29 +++++-- tests/zaptec/test_validate.py | 86 +++++++++++++++++++++ 2 files changed, 107 insertions(+), 8 deletions(-) diff --git a/custom_components/zaptec/zaptec/validate.py b/custom_components/zaptec/zaptec/validate.py index bbd3bc36..9d2c7369 100644 --- a/custom_components/zaptec/zaptec/validate.py +++ b/custom_components/zaptec/zaptec/validate.py @@ -31,15 +31,27 @@ class Installations(BaseModel): class Charger(BaseModel): - """Pydantic model for a Zaptec charger.""" + """Pydantic model for a Zaptec charger, as returned by /chargers and /chargers/{id}.""" model_config = ConfigDict(extra="allow") Id: str - Name: str - Active: bool + Name: str | None = None + Active: bool | None = None DeviceType: int +class HierarchyCharger(BaseModel): + """Pydantic model for the minimal charger stub embedded in a hierarchy Circuit. + + This is a distinct, smaller shape than Charger: at parse time in + Installation.build() only Id is read from it -- the rest of a charger's + data is filled in later from the /chargers list response. + """ + + model_config = ConfigDict(extra="allow") + Id: str + + class ChargerState(BaseModel): """Pydantic model for a single state of a Zaptec charger.""" @@ -61,17 +73,18 @@ class Circuit(BaseModel): model_config = ConfigDict(extra="allow") Id: str - Name: str - Chargers: list[Charger] + Name: str | None = None + MaxCurrent: float + Chargers: list[HierarchyCharger] | None = None class Hierarchy(BaseModel): """Pydantic model for the hierarchy of Zaptec objects in an installation.""" model_config = ConfigDict(extra="allow") - Id: str - Name: str - NetworkType: int + Id: str | None = None + Name: str | None = None + NetworkType: int | None = None Circuits: list[Circuit] diff --git a/tests/zaptec/test_validate.py b/tests/zaptec/test_validate.py index 0afeb816..5aae35ce 100644 --- a/tests/zaptec/test_validate.py +++ b/tests/zaptec/test_validate.py @@ -116,6 +116,92 @@ def test_installation_validation() -> None: validate(invalid_installation_list2, installation_list_url) +def test_charger_validation() -> None: + """Check validation of /chargers and /chargers/{id} responses.""" + + chargers_list_url = "chargers" + single_charger_url = "chargers/12345678-90ab-cdef-1234567890ab" + + valid_charger = { + "Id": "12345678-90ab-cdef-1234567890ab", + "Name": "Garage", + "Active": True, + "DeviceType": 4, + } + validate(valid_charger, single_charger_url) + validate({"Pages": 1, "Data": [valid_charger]}, chargers_list_url) + + # Users without the Owner role get a reduced charger object missing + # Name/Active; only Id and DeviceType are consumed directly by api.py. + limited_charger = {"Id": valid_charger["Id"], "DeviceType": 4} + validate(limited_charger, single_charger_url) + validate({"Pages": 1, "Data": [limited_charger]}, chargers_list_url) + + # DeviceType is required: Zaptec.build() indexes chg["DeviceType"] on + # every registered charger once merged from the /chargers list. + missing_device_type = {"Id": valid_charger["Id"]} + with pytest.raises(ValidationError): + validate(missing_device_type, single_charger_url) + + # Id is required: Zaptec.build() indexes charger_item["Id"] directly. + missing_id = {"DeviceType": 4} + with pytest.raises(ValidationError): + validate(missing_id, single_charger_url) + + +def test_hierarchy_validation() -> None: + """Check validation of installation/{id}/hierarchy responses.""" + + hierarchy_url = "installation/abcdef01-2345-6789-abcd-ef0123456789/hierarchy" + + valid_hierarchy = { + "Id": "abcdef01-2345-6789-abcd-ef0123456789", + "Name": "Main hierarchy", + "NetworkType": 2, + "Circuits": [ + { + "Id": "11111111-1111-1111-1111-111111111111", + "Name": "Circuit 1", + "MaxCurrent": 32.0, + "Chargers": [ + {"Id": "12345678-90ab-cdef-1234567890ab"}, + ], + }, + ], + } + validate(valid_hierarchy, hierarchy_url) + + # Id/Name/NetworkType on the hierarchy itself aren't read by api.py, and + # per the Zaptec API docs a circuit's Name/Chargers may be null -- all + # of this must still validate. + minimal_hierarchy = { + "Circuits": [ + { + "Id": "11111111-1111-1111-1111-111111111111", + "MaxCurrent": 32.0, + "Chargers": None, + }, + ], + } + validate(minimal_hierarchy, hierarchy_url) + + # MaxCurrent is required: Installation.build() indexes + # circuit["MaxCurrent"] directly with no validation coverage today -- + # exactly the class of bug #359 asks to close. + missing_max_current = { + "Circuits": [{"Id": "11111111-1111-1111-1111-111111111111"}], + } + with pytest.raises(ValidationError): + validate(missing_max_current, hierarchy_url) + + # A circuit's Id is required: Installation.build() indexes circuit["Id"]. + missing_circuit_id = { + "Circuits": [{"MaxCurrent": 32.0}], + } + with pytest.raises(ValidationError): + validate(missing_circuit_id, hierarchy_url) + + def test_missing_and_skipped_validation() -> None: """Check that unknown urls and urls setup with None as the Validation model pass.""" From cf2018772cbd96ec79347e155b609cc1c037117d Mon Sep 17 00:00:00 2001 From: rhammen <75572839+rhammen@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:44:19 +0200 Subject: [PATCH 3/8] docs: disclose Circuit.Name's Task-3 api.py dependency Circuit.Name was relaxed to optional in the same commit as Circuit.Chargers, for the same reason (api.py still hard-subscripts it, hardened in the next task) -- but only Chargers' rationale was written down. Add the same disclosure for Name so the two-task dependency is visible in the code, not just in the plan doc. --- custom_components/zaptec/zaptec/validate.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/custom_components/zaptec/zaptec/validate.py b/custom_components/zaptec/zaptec/validate.py index 9d2c7369..9035a93a 100644 --- a/custom_components/zaptec/zaptec/validate.py +++ b/custom_components/zaptec/zaptec/validate.py @@ -73,7 +73,9 @@ class Circuit(BaseModel): model_config = ConfigDict(extra="allow") Id: str - Name: str | None = None + Name: str | None = ( + None # Relaxed to optional here; api.py still hard-subscripts it (hardened in Task 3) + ) MaxCurrent: float Chargers: list[HierarchyCharger] | None = None From 8c5693675745b96e3c2be12d980e78b13739b7fd Mon Sep 17 00:00:00 2001 From: rhammen <75572839+rhammen@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:54:19 +0200 Subject: [PATCH 4/8] fix: tolerate a null Chargers/Name on a hierarchy Circuit Circuit.Chargers and Circuit.Name are documented nullable by the Zaptec API and are now validated as optional (see the preceding validate.py commit); Installation.build() was still hard-indexing both, which would crash with a TypeError the moment either one is actually null instead of absent. Part of #359. --- custom_components/zaptec/zaptec/api.py | 5 +++-- tests/zaptec/test_api.py | 27 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/custom_components/zaptec/zaptec/api.py b/custom_components/zaptec/zaptec/api.py index cd818dd4..292e576f 100644 --- a/custom_components/zaptec/zaptec/api.py +++ b/custom_components/zaptec/zaptec/api.py @@ -274,14 +274,15 @@ async def build(self) -> None: redact.add_uid(ctid, "Circuit") _LOGGER.debug(" Circuit %s", redact(ctid)) - for charger_item in circuit["Chargers"]: + # Chargers and Name are nullable per the Zaptec API docs. + for charger_item in circuit.get("Chargers") or []: chgid = charger_item["Id"] redact.add_uid(chgid, "Charger") # Inject additional attributes charger_item["InstallationId"] = self.id charger_item["CircuitId"] = ctid - charger_item["CircuitName"] = circuit["Name"] + charger_item["CircuitName"] = circuit.get("Name") charger_item["CircuitMaxCurrent"] = circuit["MaxCurrent"] # Add or update the charger diff --git a/tests/zaptec/test_api.py b/tests/zaptec/test_api.py index 825b05d9..e409391d 100644 --- a/tests/zaptec/test_api.py +++ b/tests/zaptec/test_api.py @@ -968,6 +968,33 @@ def test_zaptec_collections_and_accessors() -> None: assert set(zap) == {"i1"} +@pytest.mark.asyncio +async def test_build_hierarchy_handles_null_circuit_chargers() -> None: + """A circuit with a null Chargers list must not crash Installation.build(). + + A circuit with a null Chargers list (nullable per the Zaptec API docs) + must not crash Installation.build(); it should contribute no chargers + rather than raising a TypeError. + """ + + hierarchy_payload = { + "Circuits": [ + { + "Id": "11111111-1111-1111-1111-111111111111", + "MaxCurrent": 32.0, + "Chargers": None, + }, + ], + } + zap, _ = _make_zaptec([FakeResponse(HTTPStatus.OK, json_data=hierarchy_payload)]) + inst = Installation({"Id": "abcdef01-2345-6789-abcd-ef0123456789"}, zap) + zap.register(inst.id, inst) + + await inst.build() + + assert inst.chargers == [] + + @pytest.mark.asyncio async def test_zaptec_async_context_manager_closes_internal_client() -> None: """Entering/exiting the context manager works with an internally-created client.""" From d3da4d57010b6d1c911b04cc215d3db42750f2fd Mon Sep 17 00:00:00 2001 From: rhammen <75572839+rhammen@users.noreply.github.com> Date: Sun, 12 Jul 2026 16:00:50 +0200 Subject: [PATCH 5/8] fix: relax ChargerFirmware validation to match its own defensive call site poll_firmware_info() already treats CurrentVersion/AvailableVersion/ IsUpToDate as optional (a charger added but not yet initialized omits them) and skips the charger with a warning -- but validate() ran first and rejected the response outright, so that defensive branch was unreachable in practice. The Zaptec API docs confirm all of IsOnline/ CurrentVersion/AvailableVersion/IsUpToDate/DeviceType are nullable; only ChargerId is genuinely required. Fixes #359. --- custom_components/zaptec/zaptec/validate.py | 10 +++---- tests/zaptec/test_validate.py | 32 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/custom_components/zaptec/zaptec/validate.py b/custom_components/zaptec/zaptec/validate.py index 9035a93a..684bcbd0 100644 --- a/custom_components/zaptec/zaptec/validate.py +++ b/custom_components/zaptec/zaptec/validate.py @@ -95,11 +95,11 @@ class ChargerFirmware(BaseModel): model_config = ConfigDict(extra="allow") ChargerId: str - DeviceType: int - IsOnline: bool - CurrentVersion: str - AvailableVersion: str - IsUpToDate: bool + DeviceType: int | None = None + IsOnline: bool | None = None + CurrentVersion: str | None = None + AvailableVersion: str | None = None + IsUpToDate: bool | None = None class InstallationConnectionDetails(BaseModel): diff --git a/tests/zaptec/test_validate.py b/tests/zaptec/test_validate.py index 5aae35ce..8c47d5b8 100644 --- a/tests/zaptec/test_validate.py +++ b/tests/zaptec/test_validate.py @@ -202,6 +202,38 @@ def test_hierarchy_validation() -> None: validate(missing_circuit_id, hierarchy_url) +def test_charger_firmware_validation() -> None: + """Check validation of chargerFirmware/installation/{id} responses.""" + + firmware_url = "chargerFirmware/installation/abcdef01-2345-6789-abcd-ef0123456789" + + valid_firmware = [ + { + "ChargerId": "12345678-90ab-cdef-1234567890ab", + "DeviceType": 4, + "IsOnline": True, + "CurrentVersion": "1.2.3", + "AvailableVersion": "1.2.4", + "IsUpToDate": False, + }, + ] + validate(valid_firmware, firmware_url) + + # A charger added to the platform but not yet initialized reports only + # ChargerId, per api.py's poll_firmware_info(), which treats + # CurrentVersion/AvailableVersion/IsUpToDate as optional and skips the + # charger if any are missing. Per the Zaptec API docs, all fields + # except ChargerId are nullable, so validation must not reject this + # before that defensive code ever gets to run. + uninitialized_firmware = [{"ChargerId": "12345678-90ab-cdef-1234567890ab"}] + validate(uninitialized_firmware, firmware_url) + + # ChargerId is required: poll_firmware_info() indexes fm["ChargerId"] directly. + missing_charger_id = [{"DeviceType": 4}] + with pytest.raises(ValidationError): + validate(missing_charger_id, firmware_url) + + def test_missing_and_skipped_validation() -> None: """Check that unknown urls and urls setup with None as the Validation model pass.""" From ac759fd288b82f2719258dc10f4aa22a32f589e2 Mon Sep 17 00:00:00 2001 From: rhammen <75572839+rhammen@users.noreply.github.com> Date: Sun, 12 Jul 2026 16:56:40 +0200 Subject: [PATCH 6/8] fix: require DeviceType on hierarchy-sourced chargers, fix stale comment HierarchyCharger dropped DeviceType when it was split out in the prior commit, reasoning that only Id is read from a hierarchy charger stub at parse time. That missed that api.py's standalone-charger merge loop skips re-merging any charger already found via the installation hierarchy (the `if chgid in installation_chargers: continue` guard), so a hierarchy-sourced charger's attributes come only from its hierarchy stub -- never from the fuller /chargers list. Zaptec.build() later hard- subscripts chg["DeviceType"] on every registered charger, so a hierarchy response missing it now crashes with a raw KeyError instead of failing validation cleanly, as it did before this branch. Also fixes a stale comment on Circuit.Name that referenced "Task 3" as still pending, when that task is now merged into this same branch. Found in final whole-branch review. --- custom_components/zaptec/zaptec/validate.py | 10 ++-- tests/zaptec/test_api.py | 54 +++++++++++++++++++++ tests/zaptec/test_validate.py | 18 ++++++- 3 files changed, 78 insertions(+), 4 deletions(-) diff --git a/custom_components/zaptec/zaptec/validate.py b/custom_components/zaptec/zaptec/validate.py index 684bcbd0..69defc82 100644 --- a/custom_components/zaptec/zaptec/validate.py +++ b/custom_components/zaptec/zaptec/validate.py @@ -44,12 +44,16 @@ class HierarchyCharger(BaseModel): """Pydantic model for the minimal charger stub embedded in a hierarchy Circuit. This is a distinct, smaller shape than Charger: at parse time in - Installation.build() only Id is read from it -- the rest of a charger's - data is filled in later from the /chargers list response. + Installation.build() only Id is read from it. But Zaptec.build()'s + standalone-charger merge loop skips re-merging any charger already found + via the installation hierarchy, so a hierarchy-sourced charger's + attributes come only from this stub -- including DeviceType, which is + later hard-subscripted for every registered charger. """ model_config = ConfigDict(extra="allow") Id: str + DeviceType: int class ChargerState(BaseModel): @@ -74,7 +78,7 @@ class Circuit(BaseModel): model_config = ConfigDict(extra="allow") Id: str Name: str | None = ( - None # Relaxed to optional here; api.py still hard-subscripts it (hardened in Task 3) + None # Nullable per the Zaptec API docs; api.py reads it defensively via .get(). ) MaxCurrent: float Chargers: list[HierarchyCharger] | None = None diff --git a/tests/zaptec/test_api.py b/tests/zaptec/test_api.py index e409391d..28619edf 100644 --- a/tests/zaptec/test_api.py +++ b/tests/zaptec/test_api.py @@ -995,6 +995,60 @@ async def test_build_hierarchy_handles_null_circuit_chargers() -> None: assert inst.chargers == [] +@pytest.mark.asyncio +async def test_build_hierarchy_charger_with_device_type_survives_full_build() -> None: + """A hierarchy-sourced charger stub with DeviceType must not crash Zaptec.build(). + + Chargers found only via the installation hierarchy are never re-merged + with the fuller /chargers list data -- Zaptec.build()'s standalone-charger + loop skips any charger id already present via the hierarchy -- so such a + charger's DeviceType comes solely from its hierarchy stub. This drives + the full Zaptec.build() sequence (constants -> installation list -> + hierarchy -> chargers list) to confirm the chg["DeviceType"] hard + subscript at the end of build() no longer raises a raw KeyError now that + HierarchyCharger requires DeviceType (see test_validate.py's + missing_device_type_in_hierarchy case for the validation-layer half of + this fix). + """ + inst_id = "abcdef01-2345-6789-abcd-ef0123456789" + charger_id = "12345678-90ab-cdef-1234567890ab" + + outcomes = [ + FakeResponse(HTTPStatus.OK, json_data={}), # constants + FakeResponse( # installation list + HTTPStatus.OK, json_data={"Pages": 1, "Data": [{"Id": inst_id}]} + ), + FakeResponse( # installation/{id}/hierarchy + HTTPStatus.OK, + json_data={ + "Circuits": [ + { + "Id": "11111111-1111-1111-1111-111111111111", + "MaxCurrent": 32.0, + "Chargers": [{"Id": charger_id, "DeviceType": 4}], + }, + ], + }, + ), + FakeResponse( # chargers list + HTTPStatus.OK, + json_data={"Pages": 1, "Data": [{"Id": charger_id, "DeviceType": 4}]}, + ), + ] + zap, _ = _make_zaptec(outcomes) + + await zap.build() + + assert zap.is_built + charger: Charger = zap[charger_id] + # DeviceType is stored via the type_device_type converter, which falls + # back to str(val) when (as here) the constants schema has no matching + # entry -- the point of this assertion is simply that build() populated + # the attribute at all, proving the chg["DeviceType"] subscript in + # Zaptec.build() succeeded instead of raising KeyError. + assert charger["DeviceType"] == "4" + + @pytest.mark.asyncio async def test_zaptec_async_context_manager_closes_internal_client() -> None: """Entering/exiting the context manager works with an internally-created client.""" diff --git a/tests/zaptec/test_validate.py b/tests/zaptec/test_validate.py index 8c47d5b8..add2b91f 100644 --- a/tests/zaptec/test_validate.py +++ b/tests/zaptec/test_validate.py @@ -164,7 +164,7 @@ def test_hierarchy_validation() -> None: "Name": "Circuit 1", "MaxCurrent": 32.0, "Chargers": [ - {"Id": "12345678-90ab-cdef-1234567890ab"}, + {"Id": "12345678-90ab-cdef-1234567890ab", "DeviceType": 4}, ], }, ], @@ -201,6 +201,22 @@ def test_hierarchy_validation() -> None: with pytest.raises(ValidationError): validate(missing_circuit_id, hierarchy_url) + # DeviceType is required: Zaptec.build() indexes chg["DeviceType"] on every + # registered charger once merged, including hierarchy-only chargers that are + # never re-merged with the /chargers list (see api.py's installation_chargers + # skip in the standalone-charger loop). + missing_device_type_in_hierarchy = { + "Circuits": [ + { + "Id": "11111111-1111-1111-1111-111111111111", + "MaxCurrent": 32.0, + "Chargers": [{"Id": "12345678-90ab-cdef-1234567890ab"}], + }, + ], + } + with pytest.raises(ValidationError): + validate(missing_device_type_in_hierarchy, hierarchy_url) + def test_charger_firmware_validation() -> None: """Check validation of chargerFirmware/installation/{id} responses.""" From 4f704a4d8c68e808d2d09172b8ba6747ff18037e Mon Sep 17 00:00:00 2001 From: rhammen <75572839+rhammen@users.noreply.github.com> Date: Sun, 26 Jul 2026 23:47:56 +0200 Subject: [PATCH 7/8] review: drop HierarchyCharger model, require Hierarchy.Id Address steinmn's review on #397: - HierarchyCharger added unnecessary complexity now that Charger.Name and Charger.Active are optional -- Charger already requires only Id+DeviceType, the exact shape a hierarchy Circuit's Chargers need. Reuse Charger for Circuit.Chargers and drop the separate model. DeviceType stays required, so the hierarchy-only-charger fix is preserved. - Hierarchy.Id is always present in the response, so revert it from optional back to required. Tests updated: a dedicated missing-Id negative case, and the required Id added to the other hierarchy fixtures so each negative test still asserts exactly one violation. Also trim the new tests' docstrings and the surrounding validate.py/test comments to their key point (no behavior change). Co-Authored-By: Claude Opus 4.8 --- custom_components/zaptec/zaptec/validate.py | 27 ++++------------ tests/zaptec/test_api.py | 33 ++++++------------- tests/zaptec/test_validate.py | 36 +++++++++++++-------- 3 files changed, 39 insertions(+), 57 deletions(-) diff --git a/custom_components/zaptec/zaptec/validate.py b/custom_components/zaptec/zaptec/validate.py index 69defc82..b7d8f429 100644 --- a/custom_components/zaptec/zaptec/validate.py +++ b/custom_components/zaptec/zaptec/validate.py @@ -40,22 +40,6 @@ class Charger(BaseModel): DeviceType: int -class HierarchyCharger(BaseModel): - """Pydantic model for the minimal charger stub embedded in a hierarchy Circuit. - - This is a distinct, smaller shape than Charger: at parse time in - Installation.build() only Id is read from it. But Zaptec.build()'s - standalone-charger merge loop skips re-merging any charger already found - via the installation hierarchy, so a hierarchy-sourced charger's - attributes come only from this stub -- including DeviceType, which is - later hard-subscripted for every registered charger. - """ - - model_config = ConfigDict(extra="allow") - Id: str - DeviceType: int - - class ChargerState(BaseModel): """Pydantic model for a single state of a Zaptec charger.""" @@ -77,18 +61,19 @@ class Circuit(BaseModel): model_config = ConfigDict(extra="allow") Id: str - Name: str | None = ( - None # Nullable per the Zaptec API docs; api.py reads it defensively via .get(). - ) + # Nullable per the Zaptec API docs; api.py reads it defensively via .get(). + Name: str | None = None MaxCurrent: float - Chargers: list[HierarchyCharger] | None = None + # Reuses Charger: a hierarchy-only charger is never re-merged with /chargers, + # so its required DeviceType (hard-subscripted later in build()) comes only from here. + Chargers: list[Charger] | None = None class Hierarchy(BaseModel): """Pydantic model for the hierarchy of Zaptec objects in an installation.""" model_config = ConfigDict(extra="allow") - Id: str | None = None + Id: str Name: str | None = None NetworkType: int | None = None Circuits: list[Circuit] diff --git a/tests/zaptec/test_api.py b/tests/zaptec/test_api.py index 28619edf..df1c55ac 100644 --- a/tests/zaptec/test_api.py +++ b/tests/zaptec/test_api.py @@ -970,14 +970,10 @@ def test_zaptec_collections_and_accessors() -> None: @pytest.mark.asyncio async def test_build_hierarchy_handles_null_circuit_chargers() -> None: - """A circuit with a null Chargers list must not crash Installation.build(). - - A circuit with a null Chargers list (nullable per the Zaptec API docs) - must not crash Installation.build(); it should contribute no chargers - rather than raising a TypeError. - """ + """A null Circuit.Chargers (nullable per the API docs) yields no chargers, not a TypeError.""" hierarchy_payload = { + "Id": "abcdef01-2345-6789-abcd-ef0123456789", "Circuits": [ { "Id": "11111111-1111-1111-1111-111111111111", @@ -997,18 +993,11 @@ async def test_build_hierarchy_handles_null_circuit_chargers() -> None: @pytest.mark.asyncio async def test_build_hierarchy_charger_with_device_type_survives_full_build() -> None: - """A hierarchy-sourced charger stub with DeviceType must not crash Zaptec.build(). - - Chargers found only via the installation hierarchy are never re-merged - with the fuller /chargers list data -- Zaptec.build()'s standalone-charger - loop skips any charger id already present via the hierarchy -- so such a - charger's DeviceType comes solely from its hierarchy stub. This drives - the full Zaptec.build() sequence (constants -> installation list -> - hierarchy -> chargers list) to confirm the chg["DeviceType"] hard - subscript at the end of build() no longer raises a raw KeyError now that - HierarchyCharger requires DeviceType (see test_validate.py's - missing_device_type_in_hierarchy case for the validation-layer half of - this fix). + """A hierarchy-only charger survives Zaptec.build()'s chg["DeviceType"] subscript. + + A charger seen only via the hierarchy is never re-merged with the /chargers + list, so its DeviceType comes solely from the hierarchy stub -- which the + Charger model now requires. Drives the full build() to prove no KeyError. """ inst_id = "abcdef01-2345-6789-abcd-ef0123456789" charger_id = "12345678-90ab-cdef-1234567890ab" @@ -1021,6 +1010,7 @@ async def test_build_hierarchy_charger_with_device_type_survives_full_build() -> FakeResponse( # installation/{id}/hierarchy HTTPStatus.OK, json_data={ + "Id": inst_id, "Circuits": [ { "Id": "11111111-1111-1111-1111-111111111111", @@ -1041,11 +1031,8 @@ async def test_build_hierarchy_charger_with_device_type_survives_full_build() -> assert zap.is_built charger: Charger = zap[charger_id] - # DeviceType is stored via the type_device_type converter, which falls - # back to str(val) when (as here) the constants schema has no matching - # entry -- the point of this assertion is simply that build() populated - # the attribute at all, proving the chg["DeviceType"] subscript in - # Zaptec.build() succeeded instead of raising KeyError. + # str(val) fallback: the constants schema has no DeviceType entry here. The + # point is only that build() populated it at all (no KeyError on subscript). assert charger["DeviceType"] == "4" diff --git a/tests/zaptec/test_validate.py b/tests/zaptec/test_validate.py index add2b91f..56a11ddd 100644 --- a/tests/zaptec/test_validate.py +++ b/tests/zaptec/test_validate.py @@ -171,10 +171,13 @@ def test_hierarchy_validation() -> None: } validate(valid_hierarchy, hierarchy_url) - # Id/Name/NetworkType on the hierarchy itself aren't read by api.py, and - # per the Zaptec API docs a circuit's Name/Chargers may be null -- all - # of this must still validate. + hierarchy_id = "abcdef01-2345-6789-abcd-ef0123456789" + + # Name/NetworkType on the hierarchy itself aren't read by api.py, and per + # the Zaptec API docs a circuit's Name/Chargers may be null -- all of this + # must still validate. The hierarchy Id, however, is always present. minimal_hierarchy = { + "Id": hierarchy_id, "Circuits": [ { "Id": "11111111-1111-1111-1111-111111111111", @@ -185,10 +188,20 @@ def test_hierarchy_validation() -> None: } validate(minimal_hierarchy, hierarchy_url) + # The hierarchy Id is required: the response always carries it. + missing_hierarchy_id = { + "Circuits": [ + {"Id": "11111111-1111-1111-1111-111111111111", "MaxCurrent": 32.0}, + ], + } + with pytest.raises(ValidationError): + validate(missing_hierarchy_id, hierarchy_url) + # MaxCurrent is required: Installation.build() indexes # circuit["MaxCurrent"] directly with no validation coverage today -- # exactly the class of bug #359 asks to close. missing_max_current = { + "Id": hierarchy_id, "Circuits": [{"Id": "11111111-1111-1111-1111-111111111111"}], } with pytest.raises(ValidationError): @@ -196,16 +209,16 @@ def test_hierarchy_validation() -> None: # A circuit's Id is required: Installation.build() indexes circuit["Id"]. missing_circuit_id = { + "Id": hierarchy_id, "Circuits": [{"MaxCurrent": 32.0}], } with pytest.raises(ValidationError): validate(missing_circuit_id, hierarchy_url) - # DeviceType is required: Zaptec.build() indexes chg["DeviceType"] on every - # registered charger once merged, including hierarchy-only chargers that are - # never re-merged with the /chargers list (see api.py's installation_chargers - # skip in the standalone-charger loop). + # DeviceType is required: Zaptec.build() hard-subscripts chg["DeviceType"], + # including hierarchy-only chargers never re-merged with the /chargers list. missing_device_type_in_hierarchy = { + "Id": hierarchy_id, "Circuits": [ { "Id": "11111111-1111-1111-1111-111111111111", @@ -235,12 +248,9 @@ def test_charger_firmware_validation() -> None: ] validate(valid_firmware, firmware_url) - # A charger added to the platform but not yet initialized reports only - # ChargerId, per api.py's poll_firmware_info(), which treats - # CurrentVersion/AvailableVersion/IsUpToDate as optional and skips the - # charger if any are missing. Per the Zaptec API docs, all fields - # except ChargerId are nullable, so validation must not reject this - # before that defensive code ever gets to run. + # A charger not yet initialized reports only ChargerId; poll_firmware_info() + # treats the rest as optional. All fields except ChargerId are nullable, so + # validation must not reject this before that defensive code runs. uninitialized_firmware = [{"ChargerId": "12345678-90ab-cdef-1234567890ab"}] validate(uninitialized_firmware, firmware_url) From 235933744d4f10d1f9628bb9001674c4c69f204c Mon Sep 17 00:00:00 2001 From: rhammen <75572839+rhammen@users.noreply.github.com> Date: Tue, 28 Jul 2026 01:15:43 +0200 Subject: [PATCH 8/8] review: warn on empty Circuits/Chargers, drop stale comments Address steinmn's PR #415 review: silently defaulting missing Circuits/Chargers hid a misconfigured installation, so log and skip/return instead. Also trims a docstring addition and two comments left over from an earlier iteration. --- custom_components/zaptec/zaptec/api.py | 15 +++++++++++++-- custom_components/zaptec/zaptec/validate.py | 5 +---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/custom_components/zaptec/zaptec/api.py b/custom_components/zaptec/zaptec/api.py index 292e576f..b906c87a 100644 --- a/custom_components/zaptec/zaptec/api.py +++ b/custom_components/zaptec/zaptec/api.py @@ -269,13 +269,24 @@ async def build(self) -> None: redact = self.zaptec.redact self.chargers = [] + if not hierarchy.get("Circuits"): + _LOGGER.warning("Installation %s contains no circuits.", self.qual_id) + return + for circuit in hierarchy["Circuits"]: ctid = circuit["Id"] redact.add_uid(ctid, "Circuit") _LOGGER.debug(" Circuit %s", redact(ctid)) - # Chargers and Name are nullable per the Zaptec API docs. - for charger_item in circuit.get("Chargers") or []: + if not circuit.get("Chargers"): + _LOGGER.warning( + "Circuit %s of installation %s contains no chargers.", + redact(ctid), + self.qual_id, + ) + continue + + for charger_item in circuit["Chargers"]: chgid = charger_item["Id"] redact.add_uid(chgid, "Charger") diff --git a/custom_components/zaptec/zaptec/validate.py b/custom_components/zaptec/zaptec/validate.py index b7d8f429..31f0254d 100644 --- a/custom_components/zaptec/zaptec/validate.py +++ b/custom_components/zaptec/zaptec/validate.py @@ -31,7 +31,7 @@ class Installations(BaseModel): class Charger(BaseModel): - """Pydantic model for a Zaptec charger, as returned by /chargers and /chargers/{id}.""" + """Pydantic model for a Zaptec charger.""" model_config = ConfigDict(extra="allow") Id: str @@ -61,11 +61,8 @@ class Circuit(BaseModel): model_config = ConfigDict(extra="allow") Id: str - # Nullable per the Zaptec API docs; api.py reads it defensively via .get(). Name: str | None = None MaxCurrent: float - # Reuses Charger: a hierarchy-only charger is never re-merged with /chargers, - # so its required DeviceType (hard-subscripted later in build()) comes only from here. Chargers: list[Charger] | None = None