Relax over-strict validation models to fields api.py uses (fixes #359, #400) - #415
Open
rhammen wants to merge 8 commits into
Open
Relax over-strict validation models to fields api.py uses (fixes #359, #400)#415rhammen wants to merge 8 commits into
rhammen wants to merge 8 commits into
Conversation
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 custom-components#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 custom-components#359. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…er 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 custom-components#359.
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.
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 custom-components#359.
… 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 custom-components#359.
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.
Address steinmn's review on custom-components#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 <noreply@anthropic.com>
4 tasks
steinmn
suggested changes
Jul 27, 2026
Address steinmn's PR custom-components#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.
rhammen
added a commit
to rhammen/zaptec
that referenced
this pull request
Jul 28, 2026
Per this repo's demonstrated review bar (PR custom-components#415, commit 2359337): maintainers cut explanatory comments even when accurate, not just stale/duplicative ones. The warn-once-per-outage paragraph explained a behavior detail already covered by the inline comment at the reset condition -- kept the return/raise contract and the CancelledError note since those are safety-critical to not breaking the function. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #397 (un-stacked: this branch is off
master, no test-PR ancestry). Independent of #414.Several validation models required response fields
api.pynever hard-reads, so a reduced or slimmed-down API response failed setup instead of degrading. Rule applied: a field is required only ifapi.pyreads it via a hard subscript (d["Key"]); everything else is optional.ChargerFirmware— onlyChargerIdrequired;CurrentVersion/IsUpToDate/etc. now optional. Fixes Setup fails because ChargerFirmware requires fields no longer returned by the API #400.Circuit.MaxCurrent— was hard-indexed inInstallation.build()but never declared, so a response missing it crashed with a rawKeyError. Now validated;build()also tolerates nullChargers/Name.Installation/Charger/Hierarchy— role-dependent fields relaxed to optional; only the hard-read keys (Id,DeviceType) stay required.Incorporates review feedback from #397: reuses
Chargerfor a Circuit's chargers instead of a separate model, keepsHierarchy.Idrequired.Tested against a real installation across four account roles (no
ValidationError), plus offline unit tests.I reviewed this code myself and believe it complies with the AI policy.
Fixes #359
Fixes #400