fix: handle base64 serializing wrong #643
Conversation
| s = s.lower() | ||
| # Temporary fix to avoid breaking any serialization. | ||
| s = s.replace("base_64", "base64") | ||
| return s |
There was a problem hiding this comment.
Very targetted and not ideal
Issue to track here: #644
There was a problem hiding this comment.
The current set of tests should be exhaustive, so you can break anything that is not tested.
There was a problem hiding this comment.
This regexp was modified to match the names in device features so if we just want to change field names in device features it should not be a problem to change. Agree with this change that making serialization work is more important in the short term.
There was a problem hiding this comment.
Will reference this in issue for tracking. Will merge for now
|
This was the cause of home-assistant/core#158151 having none maps |
There was a problem hiding this comment.
Pull request overview
This PR fixes a serialization bug where base64 in field names was incorrectly transformed to base_64 during deserialization. The issue occurred because the _decamelize function's regex pattern splits characters before numbers (e.g., Base64 → Base_64), which broke field names like home_map_content_base64.
Key Changes:
- Added a targeted string replacement in
_decamelize()to convertbase_64back tobase64after decamelization - Added comprehensive test coverage with a new "all_fields_cache" test case that includes
home_map_content_base64field - Updated test snapshots to reflect the new test case across all serialization methods (default, pickle, and JSON)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| roborock/data/containers.py | Added base_64 → base64 replacement in _decamelize() function to fix the serialization bug |
| tests/devices/test_file_cache.py | Added new "all_fields_cache" test case with home_map_content_base64, device_features, and trait_data to verify the fix works with end-to-end serialization |
| tests/devices/snapshots/test_file_cache.ambr | Updated snapshots with expected outputs for the new test case across all serialization methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| home_map_content_base64={ | ||
| 1: "ZHVtbXlfbWFwX2NvbnRlbnQ=", | ||
| 2: "bW9yZV9kdW1teV9jb250ZW50", | ||
| }, |
There was a problem hiding this comment.
While this test case validates end-to-end serialization with home_map_content_base64, consider adding a direct unit test for the _decamelize and _camelize functions in tests/test_containers.py to explicitly verify the base64 edge case.
For example, add a test case to the existing test_decamelize_function parametrized test:
("homeMapContentBase64", "home_map_content_base64"),This would make the fix more explicit and easier to maintain.
Basically, the serialization and deserialization logic was changing base64 to base_64 because of camelization and decamelization. I could modify the regex, but I was a bit worried that something might be relying on it that I'm not thinking of. I figured it would be best to make a targeted fix and revisit this once everything is stable again.