Skip to content
Open
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
50 changes: 50 additions & 0 deletions google/cloud/spanner_v1/data_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,56 @@ def __init__(self, *args, **kwargs):
if not self._is_null:
super(JsonObject, self).__init__(*args, **kwargs)

def __len__(self):
if self._is_null:
return 0
if self._is_array:
return len(self._array_value)
if self._is_scalar_value:
return 1
return super(JsonObject, self).__len__()

def __bool__(self):
if self._is_null:
return False
if self._is_array:
return bool(self._array_value)
if self._is_scalar_value:
return True
return len(self) > 0

def __iter__(self):
if self._is_array:
return iter(self._array_value)
if self._is_scalar_value:
raise TypeError(f"'{type(self._simple_value).__name__}' object is not iterable")
return super(JsonObject, self).__iter__()

def __getitem__(self, key):
if self._is_array:
return self._array_value[key]
if self._is_scalar_value:
raise TypeError(f"'{type(self._simple_value).__name__}' object is not subscriptable")
return super(JsonObject, self).__getitem__(key)

def __contains__(self, item):
if self._is_array:
return item in self._array_value
if self._is_scalar_value:
raise TypeError(f"argument of type '{type(self._simple_value).__name__}' is not iterable")
return super(JsonObject, self).__contains__(item)

def __eq__(self, other):
if isinstance(other, JsonObject):
return self.serialize() == other.serialize()
if self._is_array:
return self._array_value == other
if self._is_scalar_value:
return self._simple_value == other
if self._is_null:
return other is None or (isinstance(other, dict) and len(other) == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The implementation of __eq__ for a null JsonObject breaks the symmetry of the equality operator. Specifically, JsonObject(None) == {} evaluates to True, while the reverse, {} == JsonObject(None), evaluates to False. This can lead to subtle and hard-to-debug issues.

Furthermore, from a conceptual standpoint, a JSON null value is distinct from an empty JSON object ({}). Equating them could lead to incorrect assumptions in consumer code.

To ensure symmetry and maintain a clear distinction between null and an empty object, I recommend that a null JsonObject should only be equal to None or another null JsonObject.

Suggested change
return other is None or (isinstance(other, dict) and len(other) == 0)
return other is None

return super(JsonObject, self).__eq__(other)

def __repr__(self):
if self._is_array:
return str(self._array_value)
Expand Down
87 changes: 87 additions & 0 deletions tests/unit/test_datatypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,90 @@ 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_dict_protocol(unittest.TestCase):
"""Verify that JsonObject behaves correctly with standard Python
operations (len, bool, iteration, indexing) for all JSON variants."""

def test_array_len(self):
obj = JsonObject([{"id": 1}, {"id": 2}])
self.assertEqual(len(obj), 2)

def test_array_bool_truthy(self):
obj = JsonObject([{"id": 1}])
self.assertTrue(obj)

def test_array_bool_empty(self):
obj = JsonObject([])
self.assertFalse(obj)

def test_array_iter(self):
data = [{"a": 1}, {"b": 2}]
obj = JsonObject(data)
self.assertEqual(list(obj), data)

def test_array_getitem(self):
data = [{"a": 1}, {"b": 2}]
obj = JsonObject(data)
self.assertEqual(obj[0], {"a": 1})
self.assertEqual(obj[1], {"b": 2})

def test_array_contains(self):
data = [1, 2, 3]
obj = JsonObject(data)
self.assertIn(2, obj)
self.assertNotIn(4, obj)

def test_array_eq(self):
data = [{"id": 1}]
obj = JsonObject(data)
self.assertEqual(obj, data)

def test_array_json_dumps(self):
data = [{"id": "m1", "content": "hello"}]
obj = JsonObject(data)
result = json.loads(json.dumps(list(obj)))
self.assertEqual(result, data)
Comment on lines +139 to +143
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 name of this test, test_array_json_dumps, is misleading. It implies that it's testing json.dumps(obj), but it's actually testing json.dumps(list(obj)). This doesn't verify that json.dumps works directly on an array-like JsonObject.

In fact, json.dumps(obj) would still produce "{}" for an array-like JsonObject because json.dumps treats dict subclasses by accessing their items, and the underlying dictionary is empty in this case.

The core functionality this test verifies—that list(obj) returns the correct list—is already covered by test_array_iter. The additional json round-trip doesn't add significant value.

Given the misleading name and redundancy, I recommend removing this test to avoid confusion.


def test_dict_len(self):
obj = JsonObject({"a": 1, "b": 2})
self.assertEqual(len(obj), 2)

def test_dict_bool(self):
obj = JsonObject({"a": 1})
self.assertTrue(obj)

def test_dict_iter(self):
obj = JsonObject({"a": 1, "b": 2})
self.assertEqual(sorted(obj), ["a", "b"])

def test_dict_getitem(self):
obj = JsonObject({"key": "value"})
self.assertEqual(obj["key"], "value")

def test_null_len(self):
obj = JsonObject(None)
self.assertEqual(len(obj), 0)

def test_null_bool(self):
obj = JsonObject(None)
self.assertFalse(obj)

def test_scalar_len(self):
obj = JsonObject(42)
self.assertEqual(len(obj), 1)

def test_scalar_bool(self):
obj = JsonObject(42)
self.assertTrue(obj)

def test_scalar_not_iterable(self):
obj = JsonObject(42)
with self.assertRaises(TypeError):
iter(obj)

def test_scalar_not_subscriptable(self):
obj = JsonObject(42)
with self.assertRaises(TypeError):
obj[0]
Loading