Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions google/cloud/spanner_v1/data_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,22 +80,41 @@ def from_str(cls, str_repr):

return cls(json.loads(str_repr))

def serialize(self):
def serialize(self, default=None):
"""Return the object text representation.

Args:
default (callable, optional): A function that is called for
objects that are not JSON serializable. It should return a
JSON-encodable version of the object or raise a
:class:`TypeError`. This is passed directly to
:func:`json.dumps` as the ``default`` parameter.
For example, to support ``datetime`` objects::

obj.serialize(default=lambda o: o.isoformat()
if hasattr(o, 'isoformat') else str(o))
Comment on lines +94 to +95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)


Returns:
str: JSON object text representation.
str: JSON object text representation, or None if the object
is null.
"""
if self._is_null:
return None

if self._is_scalar_value:
return json.dumps(self._simple_value)
return json.dumps(self._simple_value, default=default)

if self._is_array:
return json.dumps(self._array_value, sort_keys=True, separators=(",", ":"))
return json.dumps(
self._array_value,
sort_keys=True,
separators=(",", ":"),
default=default,
)

return json.dumps(self, sort_keys=True, separators=(",", ":"))
return json.dumps(
self, sort_keys=True, separators=(",", ":"), default=default
)


@dataclass
Expand Down
48 changes: 48 additions & 0 deletions tests/unit/test_datatypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,51 @@ def test_w_JsonObject_of_list_of_simple_JsonData(self):
expected = json.dumps(data, sort_keys=True, separators=(",", ":"))
data_jsonobject = JsonObject(JsonObject(data))
self.assertEqual(data_jsonobject.serialize(), expected)


class Test_JsonObject_serialize_default(unittest.TestCase):
"""Tests for the ``default`` parameter of ``JsonObject.serialize()``."""

def test_dict_with_custom_type_and_default(self):
from datetime import datetime

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)
        # ...

parsed = json.loads(result)
self.assertEqual(parsed["ts"], "2023-06-15T09:30:00")
self.assertEqual(parsed["name"], "test")

def test_array_with_custom_type_and_default(self):
from datetime import datetime

dt = datetime(2023, 1, 1)
data = [dt, "hello"]
obj = JsonObject(data)
result = obj.serialize(default=lambda o: o.isoformat() if isinstance(o, datetime) else str(o))
parsed = json.loads(result)
self.assertEqual(parsed[0], "2023-01-01T00:00:00")
self.assertEqual(parsed[1], "hello")

def test_without_default_raises_on_custom_type(self):
from datetime import datetime

data = {"ts": datetime(2023, 1, 1)}
obj = JsonObject(data)
with self.assertRaises(TypeError):
obj.serialize()

def test_default_none_preserves_existing_behavior(self):
data = {"foo": "bar"}
expected = json.dumps(data, sort_keys=True, separators=(",", ":"))
obj = JsonObject(data)
self.assertEqual(obj.serialize(default=None), expected)

def test_scalar_with_default(self):
from datetime import datetime

dt = datetime(2023, 6, 15)
obj = JsonObject(dt)
result = obj.serialize(default=lambda o: o.isoformat() if isinstance(o, datetime) else str(o))
self.assertEqual(json.loads(result), "2023-06-15T00:00:00")
Loading