Skip to content

Conversation

@Lash-L
Copy link
Collaborator

@Lash-L Lash-L commented Dec 2, 2025

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
},
Suggested change
map_id: CombinedMapInfo.from_dict(map_data)
int(map_id): CombinedMapInfo.from_dict(map_data)

Copilot uses AI. Check for mistakes.
},
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,
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Suggested change
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,

Copilot uses AI. Check for mistakes.
Comment on lines +79 to +88
"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"},
],
}
},
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Comment on lines +55 to +56
home_map_content=data.get("home_map_content", {}),
home_map_content_base64=data.get("home_map_content_base64", {}),
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {},
Suggested change
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 {},

Copilot uses AI. Check for mistakes.
}
},
"home_map_content": {},
"home_map_content_base64": {"0": "fake_bytes"},
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
"home_map_content_base64": {"0": "fake_bytes"},
"home_map_content_base64": {0: "fake_bytes"},

Copilot uses AI. Check for mistakes.
Comment on lines +101 to +105
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)
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
assert not cache_file.exists()


def test_serialize_dictionary_cache() -> None:
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
def test_serialize_dictionary_cache() -> None:
def test_deserialize_dictionary_cache() -> None:

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants