fix: Encode map content bytes as base64#608
fix: Encode map content bytes as base64#608allenporter merged 2 commits intoPython-roborock:mainfrom
Conversation
This is to ensure we can easily persist this in json or other formats.
There was a problem hiding this comment.
Pull request overview
This PR addresses a JSON persistence issue by encoding map content bytes as base64 strings. The change enables the map cache data to be serialized to JSON or other text-based formats that don't handle binary data well.
Key Changes:
- Introduces
home_map_content_base64field to store base64-encoded map content - Maintains backward compatibility by supporting both old (
home_map_content) and new formats - Updates all cache write operations to use the new base64-encoded format while clearing the old format
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| roborock/devices/cache.py | Adds new home_map_content_base64 field to CacheData dataclass and marks old home_map_content field as deprecated |
| roborock/devices/traits/v1/home.py | Updates cache read/write logic to encode map content as base64 on write and decode on read, with fallback support for legacy format |
| tests/devices/traits/v1/test_home.py | Adds parametrized test cases to verify both old and new cache formats work correctly, updates assertions to check for base64 field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if cache_data.home_map_content and not cache_data.home_map_content_base64: | ||
| cache_data.home_map_content_base64 = { |
There was a problem hiding this comment.
The None check for cache_data.home_map_content_base64 is unnecessary. Since the field is defined with field(default_factory=dict) in the CacheData dataclass, it will always be initialized to an empty dict and can never be None. This check should be removed for consistency with the _update_home_cache method (line 228) which directly assigns to home_map_content_base64 without a None check.
| if cache_data.home_map_content_base64: | ||
| self._home_map_content = { | ||
| k: self._map_content.parse_map_content(base64.b64decode(v)) | ||
| for k, v in cache_data.home_map_content_base64.items() | ||
| } |
There was a problem hiding this comment.
While the code handles exceptions from base64 decoding (line 97) and parsing within the try-except block, there's no test coverage for the case where home_map_content_base64 contains invalid base64 data or data that fails to parse. Consider adding a test similar to test_existing_home_cache_invalid_bytes but using the new base64 format to ensure this error path is tested.
| """ | ||
|
|
||
| home_map_content_base64: dict[int, str] = field(default_factory=dict) | ||
| """Home cache content for each map data (encoded base64) indexed by map_flag.""" |
There was a problem hiding this comment.
[nitpick] The docstring could be more descriptive about what data is being base64-encoded. Consider updating it to: "Home cache content for each map data (base64-encoded bytes) indexed by map_flag." This makes it clearer that the bytes from the original home_map_content field are what's being encoded.
| """Home cache content for each map data (encoded base64) indexed by map_flag.""" | |
| """Home cache content for each map data (base64-encoded bytes) indexed by map_flag.""" |
2 participants
This is to ensure we can easily persist this in json or other formats.
For fixing this issue:
home-assistant/core#157461