-
Notifications
You must be signed in to change notification settings - Fork 653
Fix: handle empty response_message after storage operation #516
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
Fix: handle empty response_message after storage operation #516
Conversation
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.
Thank you for the pull request. Could you consider making the following revisions?
Syntax/indentation: In the diff the line after elif result is None: appears unindented; that would produce an IndentationError / SyntaxError. Make sure result_str = ... is indented under the elif.
Fragile empty-list handling: You convert lists to a string and then test result_str == "[]". That can be brittle (and non-JSON list formatting uses single quotes). Better to detect empty lists/tuples before converting.
Inconsistent serialization: You json.dumps dicts but fall back to str() for lists/tuples. Consider using json.dumps for lists/tuples too (with a try/except fallback if not JSON-serializable).
The elif result == "" check works but could mis-handle other falsy values (0, False). Keep it explicit as result == "" if that’s intended.
No handling for bytes: if result can be bytes, you may want to decode before comparing/serializing.
Suggested corrected implementations:
Import json at top.
Handle dict/list/tuple with json.dumps and explicit empty-list check before string conversion.
Fallback to str(result) if json.dumps fails.
Example corrected snippet:
import json
def address_request(self, agent_request):
result = self.filesystem.address_request(agent_request)
# Normalize result to string format for StorageResponse
if isinstance(result, (dict, list, tuple)):
try:
result_str = json.dumps(result)
except (TypeError, ValueError):
result_str = str(result)
# Empty list/tuple should show a meaningful message
if isinstance(result, (list, tuple)) and len(result) == 0:
result_str = "No documents found"
elif result is None:
result_str = "Operation completed with no result"
elif result == "":
result_str = "Operation completed with empty result"
else:
# Ensure we always return a string
result_str = str(result)
return StorageResponse(
response_message=result_str,
finished=True
)
Extra recommendations:
Add unit tests verifying behavior for: dict, list (non-empty and empty), None, "", bytes, int, and custom objects.
Consider whether StorageResponse could accept other types (and do the conversion there) to centralize normalization.
If preserving JSON semantics is important, always use json.dumps for serializable objects so clients get consistent formatting.
|
Thank you for the detailed review! I've understood all the points you mentioned:
I'll update the PR with these revisions and include the improved normalization logic you suggested. Thanks again for the thorough feedback — very helpful! |
evison
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.
if isinstance(result, (list, tuple)) and len(result) == 0:
The above line will never execute since it's within the if isinstance(result, dict) condition. Need to change this line "if isinstance(result, dict):" to "if isinstance(result, (dict, list, tuple)):"
Description
Fixes ValidationError in
StorageManager.address_requestwhen storage operations return lists or edge case values (None, empty string, empty list). The issue occurs becauseStorageResponse.response_messagerequires a string, but lists were passed directly.Problem
When
sto_retrievereturns an empty list[], it causes a ValidationError sinceresponse_messageexpects a string. See issue #515.Solution
Changes
aios/storage/storage.py: Updatedaddress_requestto handle all result types and edge cases