-
Notifications
You must be signed in to change notification settings - Fork 1
Fix error handling for GS errors #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f-necas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not very fluent in python but seems ok.
There was a problem hiding this 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 fixes inconsistent error handling for GS errors by standardizing the serialization of operations data across different response paths. The issue was that operations were being serialized differently in error handling versus success responses, causing some error content to be lost.
- Standardizes operations serialization to use dictionary format consistently
- Removes redundant serialization logic from DetailedResponse model
- Updates get_json_responses method to handle mixed data types
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/maelstro/middleware.py | Removes manual dict() conversion, passes operations directly |
| backend/maelstro/main.py | Removes manual dict() conversion for consistency |
| backend/maelstro/core/operations.py | Updates get_json_responses to handle mixed data types and serialize properly |
| backend/maelstro/common/models.py | Changes DetailedResponse to accept dict operations and removes redundant serializer |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
backend/maelstro/core/operations.py
Outdated
| return [ | ||
| r.model_dump() if isinstance(r, OperationsRecord) else r | ||
| for r in self.responses | ||
| ] |
Copilot
AI
Aug 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mixed type handling with isinstance check suggests inconsistent data types in self.responses. Consider validating that all items in self.responses are either OperationsRecord instances or dictionaries to make the code more predictable and maintainable.
| return [ | |
| r.model_dump() if isinstance(r, OperationsRecord) else r | |
| for r in self.responses | |
| ] | |
| result = [] | |
| for r in self.responses: | |
| if not isinstance(r, OperationsRecord): | |
| raise TypeError(f"Non-OperationsRecord found in responses: {type(r)}") | |
| result.append(r.model_dump()) | |
| return result |
* Fix error handling by harmonizing operations in DetailedResponse and ExceptionDetail * Fix code for CI * Review comment by copilot (cherry picked from commit bc592e6)
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
* Fix error handling by harmonizing operations in DetailedResponse and ExceptionDetail * Fix code for CI * Review comment by copilot (cherry picked from commit bc592e6) Co-authored-by: Moritz Kirmse <[email protected]>
fix for #77
serialization of requests log was handled differently in ExceptionDetail and DetailedResponse
therefore some error content was lost.