fix: parse and validate read_console types #565
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideUpdates read_console to parse and validate the types parameter when passed as a JSON string or list, normalizing entries to a restricted set of allowed values and adding integration tests for JSON handling and validation failures. Flow diagram for read_console types parsing and validationflowchart TD
A[Start read_console] --> B{types is None?}
B -- Yes --> C[Set types to default list error, warning, log]
B -- No --> D{types is string?}
D -- Yes --> E[types = parse_json_payload types]
D -- No --> F[Keep types as provided]
E --> G{types is list?}
F --> G
G -- No --> H[Return error: success False, message 'types must be a list']
G -- Yes --> I{types is not None?}
I -- No --> C
I -- Yes --> J[Initialize allowed_types and normalized_types]
J --> K[Iterate over each entry in types]
K --> L{entry is string?}
L -- No --> M[Return error: success False, message 'types entries must be strings']
L -- Yes --> N[normalized = entry stripped and lowercased]
N --> O{normalized in allowed_types?}
O -- No --> P[Return error: success False, message 'invalid types entry']
O -- Yes --> Q[Append normalized to normalized_types]
Q --> R{More entries?}
R -- Yes --> K
R -- No --> S[Set types = normalized_types]
C --> T[Continue with read_console using types]
S --> T
T --> U[End types handling]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe change updates Changes
Sequence Diagram(s)(omitted — change is validation/normalization logic within a single component) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider handling or surfacing parse_json_payload failures explicitly (e.g., catching JSON errors and returning a clear error message) so malformed JSON in types doesn’t bubble up as an unhandled exception.
- The default list of types ['error', 'warning', 'log'] is now duplicated in both the validation block and the else branch; extracting this into a shared constant would reduce the chance of drift.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider handling or surfacing parse_json_payload failures explicitly (e.g., catching JSON errors and returning a clear error message) so malformed JSON in types doesn’t bubble up as an unhandled exception.
- The default list of types ['error', 'warning', 'log'] is now duplicated in both the validation block and the else branch; extracting this into a shared constant would reduce the chance of drift.
## Individual Comments
### Comment 1
<location> `Server/tests/integration/test_read_console_truncate.py:244-249` </location>
<code_context>
+ fake_send_with_unity_instance,
+ )
+
+ # Invalid entry in list should return a clear error and not send.
+ captured.clear()
+ resp = await read_console(ctx=DummyContext(), action="get", types='["error", "nope"]')
+ assert resp["success"] is False
+ assert "invalid types entry" in resp["message"]
+ assert captured == {}
+
+ # Non-string entry should return a clear error and not send.
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for JSON `types` that parse to non-list or are malformed JSON to cover the new validation branch.
Now that `types` can be a JSON string that isn’t a list, add a test where the JSON parses but yields a non-list value (e.g. `"error"`, `123`, `{}`) to assert the specific "types must be a list" error. If `parse_json_payload` propagates JSON parsing failures, also add a test for malformed JSON so the new error path is fully covered and protected against regressions.
</issue_to_address>
### Comment 2
<location> `Server/tests/integration/test_read_console_truncate.py:210-222` </location>
<code_context>
+ fake_send_with_unity_instance,
+ )
+
+ # Test with types as JSON string (the problematic case from issue #561)
+ resp = await read_console(ctx=DummyContext(), action="get", types='["error", "warning", "all"]')
+ assert resp["success"] is True
+ # Verify types was parsed correctly and sent as a list
+ assert isinstance(captured["params"]["types"], list)
+ assert captured["params"]["types"] == ["error", "warning", "all"]
+
+ # Test with types as actual list (should still work)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test that verifies normalization of `types` entries (case and surrounding whitespace).
Since `types` values are normalized with `strip().lower()` before validation against `{"error", "warning", "log", "all"}`, it would be useful to cover that explicitly. For example, add a case like `types='[" Error ", "WARNING"]'` and assert that `captured["params"]["types"] == ["error", "warning"]` to document and lock in this behavior.
```suggestion
# Test with types as JSON string (the problematic case from issue #561)
resp = await read_console(ctx=DummyContext(), action="get", types='["error", "warning", "all"]')
assert resp["success"] is True
# Verify types was parsed correctly and sent as a list
assert isinstance(captured["params"]["types"], list)
assert captured["params"]["types"] == ["error", "warning", "all"]
# Test normalization of types entries (whitespace and case)
captured.clear()
resp = await read_console(
ctx=DummyContext(),
action="get",
types='[" Error ", "WARNING"]',
)
assert resp["success"] is True
assert isinstance(captured["params"]["types"], list)
assert captured["params"]["types"] == ["error", "warning"]
# Test with types as actual list (should still work)
captured.clear()
resp = await read_console(ctx=DummyContext(), action="get", types=["error", "warning"])
assert resp["success"] is True
assert isinstance(captured["params"]["types"], list)
assert captured["params"]["types"] == ["error", "warning"]
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+244
to
+255
| # Invalid entry in list should return a clear error and not send. | ||
| captured.clear() | ||
| resp = await read_console(ctx=DummyContext(), action="get", types='["error", "nope"]') | ||
| assert resp["success"] is False | ||
| assert "invalid types entry" in resp["message"] | ||
| assert captured == {} |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for JSON types that parse to non-list or are malformed JSON to cover the new validation branch.
Now that types can be a JSON string that isn’t a list, add a test where the JSON parses but yields a non-list value (e.g. "error", 123, {}) to assert the specific "types must be a list" error. If parse_json_payload propagates JSON parsing failures, also add a test for malformed JSON so the new error path is fully covered and protected against regressions.
Comment on lines
210
to
228
| # Test with types as JSON string (the problematic case from issue #561) | ||
| resp = await read_console(ctx=DummyContext(), action="get", types='["error", "warning", "all"]') | ||
| assert resp["success"] is True | ||
| # Verify types was parsed correctly and sent as a list | ||
| assert isinstance(captured["params"]["types"], list) | ||
| assert captured["params"]["types"] == ["error", "warning", "all"] | ||
|
|
||
| # Test with types as actual list (should still work) | ||
| captured.clear() | ||
| resp = await read_console(ctx=DummyContext(), action="get", types=["error", "warning"]) | ||
| assert resp["success"] is True | ||
| assert isinstance(captured["params"]["types"], list) | ||
| assert captured["params"]["types"] == ["error", "warning"] |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a test that verifies normalization of types entries (case and surrounding whitespace).
Since types values are normalized with strip().lower() before validation against {"error", "warning", "log", "all"}, it would be useful to cover that explicitly. For example, add a case like types='[" Error ", "WARNING"]' and assert that captured["params"]["types"] == ["error", "warning"] to document and lock in this behavior.
Suggested change
| # Test with types as JSON string (the problematic case from issue #561) | |
| resp = await read_console(ctx=DummyContext(), action="get", types='["error", "warning", "all"]') | |
| assert resp["success"] is True | |
| # Verify types was parsed correctly and sent as a list | |
| assert isinstance(captured["params"]["types"], list) | |
| assert captured["params"]["types"] == ["error", "warning", "all"] | |
| # Test with types as actual list (should still work) | |
| captured.clear() | |
| resp = await read_console(ctx=DummyContext(), action="get", types=["error", "warning"]) | |
| assert resp["success"] is True | |
| assert isinstance(captured["params"]["types"], list) | |
| assert captured["params"]["types"] == ["error", "warning"] | |
| # Test with types as JSON string (the problematic case from issue #561) | |
| resp = await read_console(ctx=DummyContext(), action="get", types='["error", "warning", "all"]') | |
| assert resp["success"] is True | |
| # Verify types was parsed correctly and sent as a list | |
| assert isinstance(captured["params"]["types"], list) | |
| assert captured["params"]["types"] == ["error", "warning", "all"] | |
| # Test normalization of types entries (whitespace and case) | |
| captured.clear() | |
| resp = await read_console( | |
| ctx=DummyContext(), | |
| action="get", | |
| types='[" Error ", "WARNING"]', | |
| ) | |
| assert resp["success"] is True | |
| assert isinstance(captured["params"]["types"], list) | |
| assert captured["params"]["types"] == ["error", "warning"] | |
| # Test with types as actual list (should still work) | |
| captured.clear() | |
| resp = await read_console(ctx=DummyContext(), action="get", types=["error", "warning"]) | |
| assert resp["success"] is True | |
| assert isinstance(captured["params"]["types"], list) | |
| assert captured["params"]["types"] == ["error", "warning"] |
dsarno
force-pushed
the
fix/read-console-types
branch
from
b554546 to
9b3ba61
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Parse
typeswhen provided as JSON string forread_console(fixes #561).Summary by Sourcery
Handle JSON string inputs and validation for the read_console types parameter.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.