Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for deserializing cached device data from dictionary format, which is necessary because Home Assistant serializes cache data to JSON/dict format instead of using pickle. The new from_dict static method in CacheData reconstructs the proper object types from deserialized dictionaries.
- Added
CacheData.from_dict()static method to handle dict-to-object conversion - Added comprehensive test to verify dictionary deserialization works correctly
- Handles conversion of nested objects like HomeData, NetworkInfo, CombinedMapInfo, and DeviceFeatures
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| roborock/devices/cache.py | Adds from_dict static method to CacheData class to reconstruct objects from dictionaries, handling HomeData, NetworkInfo, CombinedMapInfo, DeviceFeatures, and other cached data |
| tests/devices/test_file_cache.py | Adds test case to validate dictionary-based cache deserialization, including imports for testing data structures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| duid: NetworkInfo.from_dict(duid_data) for duid, duid_data in data.get("network_info", {}).items() | ||
| }, | ||
| home_map_info={ | ||
| map_id: CombinedMapInfo.from_dict(map_data) |
There was a problem hiding this comment.
The home_map_info dictionary keys should be converted to integers to match the type annotation dict[int, CombinedMapInfo]. When data is deserialized from JSON (as Home Assistant does), dictionary keys become strings. This should be:
home_map_info={
int(map_id): CombinedMapInfo.from_dict(map_data)
for map_id, map_data in data.get("home_map_info", {}).items()
},| map_id: CombinedMapInfo.from_dict(map_data) | |
| int(map_id): CombinedMapInfo.from_dict(map_data) |
| }, | ||
| home_map_content=data.get("home_map_content", {}), | ||
| home_map_content_base64=data.get("home_map_content_base64", {}), | ||
| device_features=DeviceFeatures(**data.get("device_features", {})) if data.get("device_features") else None, |
There was a problem hiding this comment.
For consistency with the other fields and to handle edge cases properly, consider using DeviceFeatures.from_dict() instead of direct unpacking. While unpacking works for simple boolean fields, from_dict provides additional robustness such as handling camelCase/snake_case conversion and None values:
device_features=DeviceFeatures.from_dict(data.get("device_features", {})) if data.get("device_features") else None,| device_features=DeviceFeatures(**data.get("device_features", {})) if data.get("device_features") else None, | |
| device_features=DeviceFeatures.from_dict(data.get("device_features", {})) if data.get("device_features") else None, |
| "home_map_info": { | ||
| "0": { | ||
| "map_flag": 0, | ||
| "name": "", | ||
| "rooms": [ | ||
| {"segment_id": 16, "iot_id": "2537178", "name": "Living room"}, | ||
| {"segment_id": 17, "iot_id": "2537175", "name": "Kitchen"}, | ||
| ], | ||
| } | ||
| }, |
There was a problem hiding this comment.
The test uses a string key "0" for home_map_info, but CacheData.home_map_info is typed as dict[int, CombinedMapInfo] with integer keys. This test should use an integer key to properly validate the deserialization behavior:
"home_map_info": {
0: {
"map_flag": 0,
"name": "",
"rooms": [
{"segment_id": 16, "iot_id": "2537178", "name": "Living room"},
{"segment_id": 17, "iot_id": "2537175", "name": "Kitchen"},
],
}
},Note: If Home Assistant serializes this to JSON and back, the keys will become strings. The from_dict method should handle this conversion (see related comment in cache.py).
| home_map_content=data.get("home_map_content", {}), | ||
| home_map_content_base64=data.get("home_map_content_base64", {}), |
There was a problem hiding this comment.
The home_map_content and home_map_content_base64 dictionary keys should be converted to integers to match the type annotations dict[int, bytes] and dict[int, str] respectively. When data is deserialized from JSON, dictionary keys become strings. This should be:
home_map_content={int(k): v for k, v in data.get("home_map_content", {}).items()} if data.get("home_map_content") else {},
home_map_content_base64={int(k): v for k, v in data.get("home_map_content_base64", {}).items()} if data.get("home_map_content_base64") else {},| home_map_content=data.get("home_map_content", {}), | |
| home_map_content_base64=data.get("home_map_content_base64", {}), | |
| home_map_content={int(k): v for k, v in data.get("home_map_content", {}).items()} if data.get("home_map_content") else {}, | |
| home_map_content_base64={int(k): v for k, v in data.get("home_map_content_base64", {}).items()} if data.get("home_map_content_base64") else {}, |
| } | ||
| }, | ||
| "home_map_content": {}, | ||
| "home_map_content_base64": {"0": "fake_bytes"}, |
There was a problem hiding this comment.
The test uses a string key "0" for home_map_content_base64, but CacheData.home_map_content_base64 is typed as dict[int, str] with integer keys. This should use an integer key to properly validate the deserialization:
"home_map_content_base64": {0: "fake_bytes"},Note: If Home Assistant serializes this to JSON and back, the keys will become strings. The from_dict method should handle this conversion (see related comment in cache.py).
| "home_map_content_base64": {"0": "fake_bytes"}, | |
| "home_map_content_base64": {0: "fake_bytes"}, |
| cache_data = CacheData.from_dict(data) | ||
| assert isinstance(cache_data, CacheData) | ||
| assert isinstance(cache_data.device_features, DeviceFeatures) | ||
| assert isinstance(cache_data.network_info["fake_duid"], NetworkInfo) | ||
| assert isinstance(cache_data.home_data, HomeData) |
There was a problem hiding this comment.
The test should also validate that home_map_info was correctly deserialized and converted to the proper types. Consider adding assertions to verify the map data:
assert 0 in cache_data.home_map_info # or int("0") depending on fix
assert isinstance(cache_data.home_map_info[0], CombinedMapInfo)
assert len(cache_data.home_map_info[0].rooms) == 2| assert not cache_file.exists() | ||
|
|
||
|
|
||
| def test_serialize_dictionary_cache() -> None: |
There was a problem hiding this comment.
The test name test_serialize_dictionary_cache is misleading as it actually tests deserialization (converting from dict to CacheData). Consider renaming to test_deserialize_dictionary_cache or test_from_dict_cache_data to better reflect what the test does.
| def test_serialize_dictionary_cache() -> None: | |
| def test_deserialize_dictionary_cache() -> None: |
HA is turning the data into a dict when it dumps it. When we load it, we have to turn it into the correct objects.
home-assistant/core#157663