fix: handle different query types #650
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the A01 device handling where query values can be received as stringified JSON instead of native Python lists. The fix adds JSON deserialization logic to handle this case gracefully.
- Adds JSON parsing to handle stringified query values in
send_decoded_command - Includes comprehensive test coverage for the new string-to-list conversion functionality
- Logs a warning and returns empty dict when JSON parsing fails
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| roborock/devices/a01_channel.py | Adds string type check and JSON parsing logic to handle stringified query values before processing |
| tests/test_a01_api.py | Adds test case to verify correct handling of stringified ID_QUERY values and adds necessary mock imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isinstance(query_values, str): | ||
| try: | ||
| query_values = json.loads(query_values) | ||
| except ValueError: |
There was a problem hiding this comment.
Consider catching json.JSONDecodeError instead of ValueError for consistency with the rest of the codebase. While ValueError works (since JSONDecodeError is a subclass), other similar code in a01_protocol.py, b01_protocol.py, and v1_protocol.py all use json.JSONDecodeError for better clarity and specificity.
| except ValueError: | |
| except json.JSONDecodeError: |
| _LOGGER.warning("Failed to parse query values: %s", query_values) | ||
| return {} | ||
|
|
||
| # Merge any results together than contain the requested data. This |
There was a problem hiding this comment.
Typo in comment: "than" should be "that".
| # Merge any results together than contain the requested data. This | |
| # Merge any results together that contain the requested data. This |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
|
||
| if isinstance(query_values, str): | ||
| try: | ||
| query_values = json.loads(query_values) |
There was a problem hiding this comment.
So this is then undoing the str from ZeoApi.query_values. That means we probably need to (1) revert that change in the trait, pass in the raw values here, then go back to encoding that inside ofencode_mqtt_payload and (2) update the tests to verify at the mqtt channel level, decoding the Roborockmessage itself? The other caller of encode_mqtt_payload should probably push its logic down or we need to make it a flag or something to have that behavior.
| assert data.get(RoborockZeoProtocol.STATE) is None | ||
|
|
||
|
|
||
| async def test_send_decoded_command_handles_stringified_query() -> None: |
There was a problem hiding this comment.
This should be somewhere under tests/devices/ instead?
|
I sent #651 which is partial rollback of the PR that introduced the bug, doing it another way |
This fixes A01 devices. Open to tweaks as needed. I believe it will always be a string now? But a check is probably the safest thing rn.