Skip to content

Commit cc89149

Browse files
committed
Handle null sys.status values from old firmware
Gen2 devices whose clock never synced report "unixtime": null in sys.status. SystemComponent.from_raw_data passed that None to a non-optional int field, Pydantic raised, the gateway fell back to the Gen1 path, and the device became a 404 on the detail page. Local firmware updates failed on the same read. Null-guard the numeric and dict fields in SystemComponent the way InputComponent already does, and make ComponentFactory fall back to the base Component with a warning when a typed model rejects its data, so one malformed component can no longer hide a reachable device. Closes #88
1 parent b55f917 commit cc89149

5 files changed

Lines changed: 399 additions & 9 deletions

File tree

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
# Issue 88: Null values in sys.status break device detail and local updates
2+
3+
GitHub issue: https://github.com/jfmlima/shelly-manager/issues/88
4+
5+
## Problem
6+
7+
Gen2 devices on firmware 1.4.0 show two symptoms:
8+
9+
1. The device detail page fails with "Device not found or unreachable" even though the device answers RPC calls.
10+
2. Firmware updates through the manager fail for the same devices.
11+
12+
Devices on firmware 1.7.5 work. The firmware version is an indirect signal. The real trigger is that these devices report literal nulls in `sys.status` when their clock never synced (common on isolated IoT VLANs without NTP or internet access):
13+
14+
```json
15+
"sys": {
16+
"status": { "time": null, "unixtime": null, "uptime": 3539695, ... }
17+
}
18+
```
19+
20+
## Root cause (verified with the payload attached to the issue)
21+
22+
`SystemComponent.from_raw_data` uses `status.get("unixtime", 0)`. When the key is present with a null value, `get` returns `None`, not the default. The field is declared `unixtime: int` (non optional), so Pydantic raises `ValidationError`.
23+
24+
- Crash site: `packages/core/src/core/domain/entities/components/system.py:45` (field declared at line 21).
25+
- The same pattern applies to `uptime`, `ram_size`, `ram_free`, `fs_size`, `fs_free`, `restart_required`, and `available_updates` at lines 38 to 44. Any of them can be null on old firmware.
26+
- `InputComponent.from_raw_data` already guards against this with `or <default>` (`packages/core/src/core/domain/entities/components/input.py:23-27`). `SystemComponent` does not.
27+
28+
### Failure chain
29+
30+
1. `DeviceStatus.from_raw_response` calls `ComponentFactory.create_component` per component and propagates the `ValidationError` (`packages/core/src/core/domain/entities/device_status.py:69`).
31+
2. `ShellyDeviceGateway.get_device_status` catches the generic exception and falls back to the legacy Gen1 gateway (`packages/core/src/core/gateways/device/shelly_device_gateway.py:166-181`). A Gen2 device does not serve the Gen1 `/status` endpoint, so the fallback returns `None`.
32+
3. The devices controller raises `DeviceNotFoundError`, which the API maps to a 404 with the message "Device not found or unreachable" (`packages/api/src/api/presentation/handlers.py`). The web detail page renders that message verbatim.
33+
4. The update path fails through the same read: `UpdateDeviceFromLocal.execute` raises `DeviceNotFoundError` when `get_device_status` returns `None` (`packages/core/src/core/use_cases/update_device_from_local.py:62-64`), and `GetLocalFirmwareReleases` fails the same way (`packages/core/src/core/use_cases/get_local_firmware_releases.py:41-43`).
34+
35+
One bug therefore explains both symptoms.
36+
37+
## Fix
38+
39+
Both changes are in the core domain layer.
40+
41+
### 1. Null-guard the fields in `SystemComponent.from_raw_data`
42+
43+
In `packages/core/src/core/domain/entities/components/system.py`, apply the same guard `InputComponent` uses:
44+
45+
```python
46+
uptime=status.get("uptime", 0) or 0,
47+
restart_required=status.get("restart_required", False) or False,
48+
ram_total=status.get("ram_size", 0) or 0,
49+
ram_free=status.get("ram_free", 0) or 0,
50+
fs_total=status.get("fs_size", 0) or 0,
51+
fs_free=status.get("fs_free", 0) or 0,
52+
available_updates=status.get("available_updates", {}) or {},
53+
unixtime=status.get("unixtime", 0) or 0,
54+
```
55+
56+
### 2. Make `ComponentFactory.create_component` degrade instead of raise
57+
58+
In `packages/core/src/core/domain/entities/components/factory.py:13`, catch `pydantic.ValidationError` from the typed model and fall back to `Component.from_raw_data(component_data)`, with a warning log. One malformed component must never turn a reachable device into a 404 and a dead update path. Without this, the next firmware quirk reproduces the whole issue.
59+
60+
Note: `Component.from_raw_data` itself can raise if `key` is missing, but the factory is only called with dicts that carry a `key`, so guarding the typed-model call is enough.
61+
62+
## Out of scope (file separately)
63+
64+
- `Shelly.GetComponents` pagination: the gateway always requests `{"offset": 0}` and never loops to `total` (`packages/core/src/core/gateways/device/shelly_device_gateway.py:375-380` and `get_component_keys` at 184-198). Masked today because `DeviceStatus.from_raw_response` backfills missing components from `Shelly.GetStatus` keys, but backfilled components have empty config and `get_component_keys` silently misses page-two components. Separate issue.
65+
- Device-side internet updates failing on these devices: their own `available_updates` is empty because they cannot reach the Shelly cloud. Environmental, not a manager bug. The via-manager local update path is the designated answer, and this fix restores it.
66+
67+
## Tests
68+
69+
All in `packages/core/tests/unit/domain/entities/`:
70+
71+
1. `test_components.py`: `SystemComponent.from_raw_data` with the exact `sys` component payload from the issue (`"time": null, "unixtime": null`) parses and yields `unixtime == 0`. Add a variant with nulls in the other guarded fields.
72+
2. `test_device_status.py`: `DeviceStatus.from_raw_response` with the issue's full `GetComponents` capture returns a `DeviceStatus` instead of raising. This is the regression test for the 404 chain.
73+
3. `test_components.py` (factory): a component dict whose status payload makes the typed model raise `ValidationError` returns a base `Component` with the right `key` instead of raising.
74+
75+
## Verification
76+
77+
- `make test-core`
78+
- `make lint`
79+
- Manual, if a 1.4.0 device is available: open the device detail page and run a via-manager update.

packages/core/src/core/domain/entities/components/factory.py

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,29 @@
1+
import logging
12
from typing import Any
23

4+
from pydantic import ValidationError
5+
36
from .base import Component
47
from .registry import model_for
58

9+
logger = logging.getLogger(__name__)
10+
611

712
class ComponentFactory:
813
@staticmethod
914
def create_component(component_data: dict[str, Any]) -> Component:
1015
key = component_data.get("key", "")
1116
component_type = key.split(":")[0] if ":" in key else key
1217

13-
return model_for(component_type).from_raw_data(component_data)
18+
try:
19+
return model_for(component_type).from_raw_data(component_data)
20+
except ValidationError as e:
21+
logger.warning(
22+
"Component %s did not match its typed model, keeping raw data: %s",
23+
key,
24+
e,
25+
)
26+
return Component.from_raw_data(component_data)
1427

1528
@staticmethod
1629
def create_component_from_status(

packages/core/src/core/domain/entities/components/system.py

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,13 @@ def from_raw_data(cls, component_data: dict[str, Any]) -> "SystemComponent":
3535
device_name=device_config.get("name"),
3636
mac_address=status.get("mac"),
3737
firmware_version=device_config.get("fw_id"),
38-
uptime=status.get("uptime", 0),
39-
restart_required=status.get("restart_required", False),
40-
ram_total=status.get("ram_size", 0),
41-
ram_free=status.get("ram_free", 0),
42-
fs_total=status.get("fs_size", 0),
43-
fs_free=status.get("fs_free", 0),
44-
available_updates=status.get("available_updates", {}),
45-
unixtime=status.get("unixtime", 0),
38+
uptime=status.get("uptime", 0) or 0,
39+
restart_required=status.get("restart_required", False) or False,
40+
ram_total=status.get("ram_size", 0) or 0,
41+
ram_free=status.get("ram_free", 0) or 0,
42+
fs_total=status.get("fs_size", 0) or 0,
43+
fs_free=status.get("fs_free", 0) or 0,
44+
available_updates=status.get("available_updates", {}) or {},
45+
unixtime=status.get("unixtime", 0) or 0,
4646
timezone=location_config.get("tz"),
4747
)

packages/core/tests/unit/domain/entities/test_components.py

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
EMDataComponent,
88
InputComponent,
99
SwitchComponent,
10+
SystemComponent,
1011
WebSocketComponent,
1112
WifiComponent,
1213
ZigbeeComponent,
@@ -856,3 +857,100 @@ def test_it_creates_em_component_from_status_only(self):
856857
assert isinstance(component, EMComponent)
857858
assert component.total_act_power == 500.0
858859
assert component.a_act_power == 200.0
860+
861+
862+
class TestSystemComponent:
863+
def test_it_parses_sys_status_with_null_time_fields(self):
864+
raw_data = {
865+
"key": "sys",
866+
"status": {
867+
"mac": "E86BEAE5F208",
868+
"restart_required": False,
869+
"time": None,
870+
"unixtime": None,
871+
"uptime": 3539695,
872+
"ram_size": 252308,
873+
"ram_free": 132132,
874+
"fs_size": 393216,
875+
"fs_free": 98304,
876+
"cfg_rev": 11,
877+
"kvs_rev": 0,
878+
"schedule_rev": 1,
879+
"webhook_rev": 1,
880+
"available_updates": {},
881+
"reset_reason": 1,
882+
},
883+
"config": {
884+
"device": {
885+
"name": "Flur SZ Rollo links",
886+
"mac": "xxxx",
887+
"fw_id": "20240726-114505/1.4.0-gb2aeadb",
888+
"discoverable": True,
889+
"eco_mode": False,
890+
"profile": "cover",
891+
"addon_type": None,
892+
},
893+
"location": {"tz": None, "lat": None, "lon": None},
894+
"sntp": {"server": "xxxx"},
895+
"cfg_rev": 11,
896+
},
897+
"attrs": {},
898+
}
899+
900+
component = SystemComponent.from_raw_data(raw_data)
901+
902+
assert component.unixtime == 0
903+
assert component.uptime == 3539695
904+
assert component.mac_address == "E86BEAE5F208"
905+
assert component.device_name == "Flur SZ Rollo links"
906+
assert component.firmware_version == "20240726-114505/1.4.0-gb2aeadb"
907+
assert component.timezone is None
908+
909+
def test_it_parses_sys_status_with_nulls_in_every_guarded_field(self):
910+
raw_data = {
911+
"key": "sys",
912+
"status": {
913+
"restart_required": None,
914+
"unixtime": None,
915+
"uptime": None,
916+
"ram_size": None,
917+
"ram_free": None,
918+
"fs_size": None,
919+
"fs_free": None,
920+
"available_updates": None,
921+
},
922+
"config": {},
923+
"attrs": {},
924+
}
925+
926+
component = SystemComponent.from_raw_data(raw_data)
927+
928+
assert component.unixtime == 0
929+
assert component.uptime == 0
930+
assert component.restart_required is False
931+
assert component.ram_total == 0
932+
assert component.ram_free == 0
933+
assert component.fs_total == 0
934+
assert component.fs_free == 0
935+
assert component.available_updates == {}
936+
937+
938+
class TestComponentFactoryFallback:
939+
def test_it_falls_back_to_base_component_when_typed_model_rejects_data(
940+
self, caplog
941+
):
942+
raw_data = {
943+
"key": "sys",
944+
"status": {"uptime": "not-a-number"},
945+
"config": {},
946+
"attrs": {},
947+
}
948+
949+
with caplog.at_level("WARNING"):
950+
component = ComponentFactory.create_component(raw_data)
951+
952+
assert type(component) is Component
953+
assert component.key == "sys"
954+
assert component.component_type == "sys"
955+
assert component.status == {"uptime": "not-a-number"}
956+
assert any("sys" in record.message for record in caplog.records)

0 commit comments

Comments
 (0)