fix(spanner): implement dict protocol and nested unwrapping for JsonObject

[sakthivelmanii]

Fixes JsonObject array/scalar/null variants breaking standard Python container protocols (len, bool, iter, getitem, contains, eq). Adds _unwrap_for_json helper to safely serialize nested JsonObject instances without data erasure.

Fixes #15870

[gemini-code-assist]

Code Review

This pull request implements standard Python container protocols (such as length, truthiness, iteration, indexing, and equality) for JsonObject to support array, scalar, and null variants, and introduces a recursive unwrapping helper for serialization. The feedback recommends correcting the truthiness of scalar values to reflect their actual boolean value, simplifying equality checks by removing defensive getattr calls, preventing null objects from equating to empty dictionaries, and expanding unit tests to cover falsy scalars.

[olavloite]

Code Review: PR #17915 (fix(spanner): implement dict protocol and nested unwrapping for JsonObject)

---

Executive Summary

PR #17915 aims to fix issue #15870 by allowing JsonObject (which wraps Spanner JSON types) to support standard Python container protocols (len, bool, iter, __getitem__, __contains__, __eq__) for array, scalar, and null JSON variants, as well as fixing nested JsonObject serialization via _unwrap_for_json.

While the PR makes good progress towards fixing nested serialization and basic index/loop access, the current implementation is incomplete, contains critical equality and initialization bugs, and breaks fundamental Python protocol expectations.

---

1. Implementation Assessment

Completeness: Incomplete Dict Protocol

JsonObject inherits directly from dict (class JsonObject(dict)). However, the PR only overrides standard magic methods (__len__, __bool__, __iter__, __getitem__, __contains__), leaving core dict methods unhandled for array, scalar, and null variants:

• .get(key[, default]) is broken: arr = JsonObject([10, 20]) allows arr[0] (returns 10), but arr.get(0) returns None.
• .copy() causes silent data loss: Calling arr.copy() on JsonObject([10, 20]) calls dict.copy(), which returns an empty dictionary {}—erasing all array data!
• .keys(), .values(), .items() diverge from iter(): list(arr) yields [10, 20], but list(arr.keys()) returns [].
• dict(arr) erases data: dict(JsonObject([10, 20])) returns {}.

Reasonableness: High Architectural Tension

Subclassing dict for an object that can hold a JSON list, int, str, bool, or None creates an architectural mismatch. Because isinstance(obj, dict) evaluates to True for all JsonObject instances:

• Passing JsonObject([10, 20]) to standard Python libraries or built-ins that check isinstance(val, dict) (e.g., standard json.dumps(obj)) will result in silent serialization as {} (empty dict) rather than [10, 20].

Efficiency: Good

_unwrap_for_json uses a recursive tree traversal. It is $O(N)$ with respect to the number of nodes in the JSON structure, which is optimal for preparing nested data for json.dumps.

---

2. Bugs & Critical Flaws Found

Bug A: Asymmetric Equality (a == b vs b == a)

Symmetry is a strict requirement for Python equality (a == b MUST equal b == a). In PR 17915, comparing JsonObject() and JsonObject(None) violates this:

>>> JsonObject() == JsonObject(None)
True
>>> JsonObject(None) == JsonObject()
False  # VIOLATION: Equality symmetry is broken!

Root Cause: In JsonObject.__init__:

self._is_scalar_value = len(args) == 1 and not isinstance(args[0], (list, dict))

Because isinstance(None, (list, dict)) is False, JsonObject(None) sets _is_scalar_value = True and stores _simple_value = None.
When evaluating JsonObject(None) == JsonObject():

1. JsonObject(None) checks if self._is_scalar_value:
2. It returns other._is_scalar_value and self._simple_value == other._simple_value
3. JsonObject()._is_scalar_value is False, so it returns False!

Bug B: Inconsistent Invocations for JSON null

JsonObject() and JsonObject(None) behave completely differently when operated on:

• iter(JsonObject(None)) -> Raises TypeError: 'NoneType' object is not iterable (due to _is_scalar_value = True).
• iter(JsonObject()) -> Returns iter(["__json_null__"]) (leaking internal dict key __json_null__).
• "__json_null__" in JsonObject() -> Returns True.
• "__json_null__" in JsonObject(None) -> Raises TypeError.

Bug C: Tuple vs List Array Inequality

>>> JsonObject((1, 2)) == [1, 2]
False
>>> JsonObject((1, 2)) == JsonObject([1, 2])
False

Since JsonObject supports initializing arrays with tuples (isinstance(args[0], (list, tuple))), comparing a tuple-backed array to a list-backed array should evaluate to True for JSON semantics.

---

3. Test Coverage & Missing Tests

While unit tests were added for standard use cases, key edge cases and regression scenarios are missing:

1. Equality Symmetry Tests: Missing test assertions verifying a == b and b == a across all null/scalar/array combinations.
2. Dict Method Tests: Missing unit tests verifying .get(), .copy(), .keys(), .values(), and .items() on non-dict variants.
3. Third-Party / Dict Interop Tests: Missing tests checking behavior when dict() or copy.deepcopy() is called on a JsonObject.

---

4. Breaking Changes Analysis

Category 1: Enhancements & Fixes ("This did not work -> Now works")

• Array / Scalar Indexing & Iteration: Operations like len(JsonObject([1, 2])), JsonObject([1, 2])[0], for item in JsonObject([1, 2]), and 42 in JsonObject([42]) previously raised errors or returned incorrect dict lengths (0). These now work as expected.
• Nested Serialization: Serializing JsonObject({"a": JsonObject([1, 2])}) previously produced {"a": {}} (data loss). It now produces {"a": [1, 2]}.

Category 2: Behavioral Changes ("Works differently than before")

• Truthiness & Length of Null Objects:
  • Previously, bool(JsonObject()) returned True (because the underlying dict held {"__json_null__": True}). Now, bool(JsonObject()) returns False.
  • len(JsonObject()) now returns 0 (previously returned 1).
  • Should we worry about this change? No. Returning False for bool(null) and 0 for len(null) aligns with standard Python semantics where null/empty values are falsy.

---

Recommended Fixes for the PR Author

To make this PR robust and safe to merge, the following updates are recommended:

1. Fix Null / Scalar Detection in __init__:
   Exclude None explicitly from _is_scalar_value:

   self._is_scalar_value = len(args) == 1 and args[0] is not None and not isinstance(args[0], (list, dict))

2. Normalize Tuple Arrays to Lists:
   Convert input tuples to lists during initialization to ensure equality comparisons work consistently:

   if self._is_array:
       self._array_value = list(args[0])
       return

3. Override .get() and .copy() on JsonObject:

   def get(self, key, default=None):
       if self._is_array:
           try:
               return self._array_value[key]
           except (IndexError, TypeError):
               return default
       if self._is_scalar_value or self._is_null:
           return default
       return super().get(key, default)
   
   def copy(self):
       if self._is_array:
           return JsonObject(list(self._array_value))
       if self._is_scalar_value:
           return JsonObject(self._simple_value)
       if self._is_null:
           return JsonObject(None)
       return JsonObject(super().copy())

[olavloite]

Please undo this change. It does not appear to be related to the actual change, and the new comment does not make sense.

[sakthivelmanii]

Done

[olavloite]

Can we undo these unrelated changes? (unless it would otherwise cause CI failures due to wrong formatting)

[sakthivelmanii]

It would cause formatting issues. ruff automatically formatted this.

[olavloite]

This also seems unrelated (and wrong?)

[sakthivelmanii]

Done

[olavloite]

nit: remove, unless it causes a formatting error

[sakthivelmanii]

Done

[olavloite]

Can we add these unit tests:

import unittest
from google.cloud.spanner_v1.data_types import JsonObject, _unwrap_for_json


class Test_JsonObject_edge_cases_and_invariants(unittest.TestCase):
    """Unit tests catching remaining bugs in JsonObject rewrapping and dict invariants."""

    def test_rewrapping_null_object_preserves_null_state(self):
        """Verify that wrapping a null JsonObject preserves _is_null and serialize()."""
        # Test default JsonObject() null
        null_obj = JsonObject()
        rewrapped = JsonObject(null_obj)
        self.assertTrue(rewrapped._is_null, "Rewrapped JsonObject lost _is_null=True flag")
        self.assertIsNone(rewrapped.serialize(), "Rewrapped JsonObject() should serialize to None")
        self.assertFalse(bool(rewrapped), "Rewrapped JsonObject() should be falsy")
        self.assertEqual(len(rewrapped), 0, "Rewrapped JsonObject() length should be 0")

        # Test JsonObject(None) null
        null_none_obj = JsonObject(None)
        rewrapped_none = JsonObject(null_none_obj)
        self.assertTrue(rewrapped_none._is_null, "Rewrapped JsonObject(None) lost _is_null=True flag")
        self.assertIsNone(rewrapped_none.serialize(), "Rewrapped JsonObject(None) should serialize to None")

    def test_keys_values_items_dict_invariant_for_arrays(self):
        """Verify that dict.keys(), dict.values(), and dict.items() are mutually consistent for array JsonObjects."""
        arr = JsonObject([10, 20])
        keys = list(arr.keys())
        values = list(arr.values())
        items = list(arr.items())

        # Standard Python dict invariant: list(keys) MUST match [k for k, v in items]
        self.assertEqual(keys, [0, 1], "keys() should return integer indices matching items() keys")
        self.assertEqual(values, [10, 20], "values() should return array elements")
        self.assertEqual(items, [(0, 10), (1, 20)])
        self.assertEqual(keys, [k for k, v in items], "d.keys() must match [k for k, v in d.items()]")
        
        # Verify zip(keys, values) reconstructs items
        self.assertEqual(dict(zip(arr.keys(), arr.values())), {0: 10, 1: 20})

    def test_nested_rewrapped_null_serialization(self):
        """Verify that nested rewrapped null JsonObjects serialize correctly inside parent containers."""
        nested = JsonObject({"a": JsonObject(JsonObject(None))})
        self.assertEqual(nested.serialize(), '{"a":null}')

        raw_unwrapped = _unwrap_for_json({"a": JsonObject(JsonObject(None))})
        self.assertEqual(raw_unwrapped, {"a": None})

[olavloite]

I think that we need to add an assignement for _is_null as well:

Suggested change

	self._is_array = args[0]._is_array
	self._is_scalar_value = args[0]._is_scalar_value
	self._is_null = args[0]._is_null
	self._is_array = args[0]._is_array
	self._is_scalar_value = args[0]._is_scalar_value

[olavloite]

For an array, the keys should be equal to the indexes of the array:

Suggested change

	def keys(self):
	if self._is_array or self._is_scalar_value or self._is_null:
	return {}.keys()
	return super().keys()
	def keys(self):
	if self._is_array:
	return dict(enumerate(self._array_value)).keys()
	if self._is_scalar_value or self._is_null:
	return {}.keys()
	return super().keys()