feat: add default parameter to JsonObject.serialize()#1508
feat: add default parameter to JsonObject.serialize()#1508waiho-gumloop wants to merge 1 commit intogoogleapis:mainfrom
Conversation
Add an optional `default` parameter to `JsonObject.serialize()` that is passed through to `json.dumps()`. This allows callers to handle custom types (e.g., `datetime`) that are not natively JSON-serializable, using the standard `json.dumps` extension mechanism. Backward-compatible: `default=None` preserves existing behavior. Made-with: Cursor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a default parameter to JsonObject.serialize() for custom JSON serialization, which is a useful enhancement. The implementation correctly passes the parameter to json.dumps(), and the new unit tests provide good coverage for various scenarios. My feedback focuses on improving the API documentation to promote safer usage patterns and on enhancing the test code's maintainability by reducing duplication.
| obj.serialize(default=lambda o: o.isoformat() | ||
| if hasattr(o, 'isoformat') else str(o)) |
There was a problem hiding this comment.
The example default function can lead to subtle bugs. The else str(o) clause will convert any unhandled non-serializable object into its string representation (e.g., '<MyObject object at 0x...>'), which might not be the desired behavior and can mask serialization errors. A more robust approach is to handle only the expected types and let other types raise a TypeError, which is the standard behavior of json.dumps.
Consider providing a more explicit example that promotes this safer pattern, even if it's more verbose:
# In a scope where `datetime` is imported
def custom_serializer(obj):
if isinstance(obj, (datetime.date, datetime.datetime)):
return obj.isoformat()
raise TypeError(f'Object of type {type(obj).__name__} is not JSON serializable')
obj.serialize(default=custom_serializer)| dt = datetime(2023, 6, 15, 9, 30, 0) | ||
| data = {"ts": dt, "name": "test"} | ||
| obj = JsonObject(data) | ||
| result = obj.serialize(default=lambda o: o.isoformat() if isinstance(o, datetime) else str(o)) |
There was a problem hiding this comment.
This lambda function is repeated in test_array_with_custom_type_and_default and test_scalar_with_default. To improve maintainability and readability, consider defining it once at the test class level and reusing it across these tests.
For example:
class Test_JsonObject_serialize_default(unittest.TestCase):
_DATETIME_SERIALIZER = lambda o: o.isoformat() if isinstance(o, datetime) else str(o)
def test_dict_with_custom_type_and_default(self):
# ...
result = obj.serialize(default=self._DATETIME_SERIALIZER)
# ...|
Closing this PR — after further analysis, we found a cleaner approach for the SQLAlchemy dialect that doesn't require modifying Instead of injecting a This removes the dependency on any core library changes and keeps The |
Summary
Add an optional
defaultparameter toJsonObject.serialize()that is passed through tojson.dumps(). This allows callers to handle custom types (e.g.,datetime) that are not natively JSON-serializable, using the standardjson.dumpsextension mechanism.Fixes #1507
Changes
google/cloud/spanner_v1/data_types.py: Adddefault=Noneparameter toJsonObject.serialize(), passed to alljson.dumps()call sites (dict, array, and scalar branches).tests/unit/test_datatypes.py: Add 5 unit tests covering:datetimevalues + customdefaultdatetimevalues + customdefaultdatetimevalue + customdefaultTypeErrorraised withoutdefault(existing behavior preserved)default=Noneproduces identical output to current behaviorBackward Compatibility
Fully backward-compatible.
default=Noneis the default, which matches the existingjson.dumps()behavior. No existing callers are affected.Motivation
JsonObject.serialize()currently cannot handle non-standard types in JSON columns. This is a common need — for example,datetimeobjects in JSON columns raiseTypeError. The standard Python way to extend JSON serialization is viajson.dumps(obj, default=fn), and this PR simply plumbs that parameter through.This also unblocks the
python-spanner-sqlalchemydialect from supporting the standard SQLAlchemycreate_engine(json_serializer=...)parameter (see context in #1507).Made with Cursor