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