Model the REST fields the cloud has started returning - #124
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Smart-union validation currently leaves known error codes as integers, and the documented example fails for intentionally supported unknown codes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extends REST models for newly observed device metadata, fault summaries, and descaling data.
Changes:
- Adds and exports REST error/descaling models.
- Models new
DeviceInfofields. - Updates tests, OpenAPI documentation, and changelog.
Validation: PR reports 716 passing tests.
File summaries
| File | Description |
|---|---|
src/nwp500/models/device.py |
Adds REST fields and models. |
src/nwp500/models/__init__.py |
Exports new models. |
src/nwp500/__init__.py |
Adds top-level exports. |
tests/test_device_rest_models.py |
Tests REST payload parsing. |
docs/reference/python_api/models.rst |
Documents new models. |
docs/openapi.yaml |
Updates REST schemas. |
CHANGELOG.rst |
Records the additions. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| device = Device.model_validate(payload) | ||
|
|
||
| assert device.error.error_code == ErrorCode(code) |
There was a problem hiding this comment.
Right — the assertion passed from either union branch, so it could not have caught the bug above. Fixed in ae3f1c0: the test now asserts isinstance(error_code, ErrorCode) and identity against the member, and the unknown-code test asserts not isinstance(..., ErrorCode). Also extended the parametrization to cover 0/NO_ERROR.
| for device in devices: | ||
| if device.error and device.error.error_code != ErrorCode.NO_ERROR: | ||
| print(f"{device.device_info.device_name}: {device.error.error_code.name}" | ||
| f" at {device.error.error_occurred_time}") |
There was a problem hiding this comment.
Fixed in ae3f1c0 — the example now branches on isinstance(code, ErrorCode) and prints code {n} for an unknown value instead of reaching for .name. With the union fix a known code is an enum member, but the int fallback is the whole point of the type, so the example should show handling it.
| #: ``NO_ERROR`` when the device has no recorded fault. Typed to accept a | ||
| #: bare int as well, following ``device_type``, so a code the enum does | ||
| #: not know cannot make a whole ``/device/list`` response unparseable. | ||
| error_code: ErrorCode | int = ErrorCode.NO_ERROR |
There was a problem hiding this comment.
Confirmed and fixed in ae3f1c0. Verified directly:
smart union -> 96 stays int
left_to_right -> 96 becomes ErrorCode.E096_UPPER_HEATER, 9999 stays int
error_code now uses Field(union_mode="left_to_right"), with a comment recording why, so a known code becomes an enum member and only an unknown one falls through to int.
Worth noting the same class of bug exists on the pre-existing device_type: DeviceType | int (and FirmwareInfo.device_type) — those also stay plain ints today. Out of scope here since changing them alters the type callers already receive, but it should probably be a follow-up.
A live /device/list response carries more than it did when these models were written. Comparing today's responses against the HAR captures in reference/ (September 2025), the cloud has added: - an `error` block: errorCode, errorOccuredTime - `descaling`, previously only on /device/info - deviceInfo.modelTypeCode and deviceInfo.installerId NavienBaseModel sets extra="ignore", so every one of these was being discarded without a trace. The `error` block is the useful one: it makes the device's last recorded fault readable without an MQTT connection, including while the device is offline. Device gains optional `error` and `descaling` sections and DeviceInfo gains `model_type_code`/`installer_id`. All are optional, so a response that omits them - /device/info returns no `error` block - parses exactly as before. `error_code` is typed `ErrorCode | int`, following `device_type`, so a code the enum does not know degrades to a plain int instead of making a whole device listing unparseable. docs/openapi.yaml and the model reference are updated to match. Claude-Session: https://claude.ai/code/session_01XVj9BYvuLj7Th3iFUVoeCn
Pydantic's default smart union matches an incoming int against the int branch exactly and never reaches the enum, so every code - known or not - stayed a plain int, and the .name access in the documented example would have raised AttributeError on a code the enum does know. union_mode="left_to_right" makes a known code an ErrorCode member while an unknown one still falls back to int. The test asserted only equality, which an IntEnum satisfies from either branch; it now asserts the type. The doc example handles the int fallback. Claude-Session: https://claude.ai/code/session_01XVj9BYvuLj7Th3iFUVoeCn
ae3f1c0 to
1fde090
Compare
What changed upstream
Comparing today's live REST responses against the HAR captures in
reference/(September 2025),/device/listhas grown:error: {errorCode, errorOccuredTime}openapi.yamldescaling: {descalingStartTime, descalingEndTime}/device/infoonly/device/listdeviceInfo.modelTypeCodedeviceInfo.installerIdNavienBaseModelsetsextra="ignore", so all four were being discarded silently. Theerrorblock is the useful one: it makes the device's last recorded fault readable without an MQTT connection, and it stays readable while the device is offline.Found while diffing live MQTT and REST traffic against the models after a controller firmware update (11.1.0.0 → 11.5.0.0).
Changes
Devicegains optionalerror(DeviceErrorSummary) anddescaling(DescalingInfo) sections.DeviceInfogainsmodel_type_codeandinstaller_id./device/inforeturns noerrorblock, and that path is covered by a test.error_codeis typedErrorCode | int, following the existingdevice_typepattern, so a code the enum doesn't know degrades to a plain int rather than making an entire device listing unparseable.ErrorCodeis a bareIntEnumwith no_missing_, so without this an unrecognised code would raise out oflist_devices().error_occurred_timeis spelled correctly in Python; the API'serrorOccuredTimeis handled by the alias.docs/openapi.yamlanddocs/reference/python_api/models.rstupdated to match.Not included:
/device/infoalso returnsinstallerandalarmInfosections that no model declares. Those are visible in the 2025 HARs too, so they aren't new, and I left them out of this change.Tests
tests/test_device_rest_models.pyvalidates trimmed real payloads from both endpoints: the error and descaling blocks parse, the newdeviceInfofields parse, known error codes become enum members, an unknown code stays an int, and a/device/infopayload with noerrorblock still parses. Full suite: 716 passed.https://claude.ai/code/session_01XVj9BYvuLj7Th3iFUVoeCn