Skip to content

fix: JsonObject array/scalar variants now implement standard dict protocol - #16496

Open
waiho-gumloop wants to merge 1 commit into
googleapis:mainfrom
waiho-gumloop:fix/jsonobject-dict-protocol
Open

fix: JsonObject array/scalar variants now implement standard dict protocol#16496
waiho-gumloop wants to merge 1 commit into
googleapis:mainfrom
waiho-gumloop:fix/jsonobject-dict-protocol

Conversation

@waiho-gumloop

Copy link
Copy Markdown
Contributor

Description

JsonObject subclasses dict but when wrapping arrays or scalars, __init__ returns early without calling super().__init__(). This leaves the dict parent empty, causing len(), bool(), iter(), indexing, json.dumps(), and in to silently return wrong results -- while isinstance(obj, dict) returns True.

This has been present since the array support was added in Aug 2022 (googleapis/python-spanner#782).

What this PR does

Adds __len__, __bool__, __iter__, __getitem__, __contains__, and __eq__ overrides that delegate to the internal _array_value/_simple_value for non-dict variants. Dict variant behavior is unchanged (delegates to super()).

Before (broken)

val=JsonObject([{"id": "m1", "content": "hello"}])
isinstance(val, dict) # Truelen(val) # 0 <-- wrongbool(val) # False <-- wronglist(val) # [] <-- wrongjson.dumps(val) # "{}" <-- wrong

After (fixed)

val=JsonObject([{"id": "m1", "content": "hello"}])
isinstance(val, dict) # Truelen(val) # 1 <-- correctbool(val) # True <-- correctlist(val) # [{"id": "m1", "content": "hello"}] <-- correctval[0] # {"id": "m1", "content": "hello"} <-- correct

Tests

Added Test_JsonObject_dict_protocol test class covering len, bool, iter, getitem, contains, and eq for all four variants (array, dict, scalar, null). All tests pass.

Fixes#15870

…tocol
JsonObject subclasses dict but when wrapping arrays or scalars,
__init__ returns early without calling super().__init__(). This
leaves the dict parent empty, causing len(), bool(), iter(),
indexing, json.dumps(), and in to silently return wrong results.
Adds __len__, __bool__, __iter__, __getitem__, __contains__, and
__eq__ overrides that delegate to _array_value/_simple_value for
non-dict variants. Dict variant behavior is unchanged.
Fixesgoogleapis#15870
@waiho-gumloop
waiho-gumloop requested review from a team as code ownersApril 1, 2026 00:13

@gemini-code-assistgemini-code-assistBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements standard Python protocols for the JsonObject class, including __len__, __bool__, __iter__, __getitem__, __contains__, and __eq__, to allow the class to behave more like native Python collections. Unit tests were also added to verify these behaviors across array, dictionary, scalar, and null variants. The review feedback identifies a logic error in the __eq__ method where a null JsonObject incorrectly equates to an empty dictionary, as well as a performance issue caused by using serialization for equality checks between JsonObject instances.

Comment on lines +99 to +108
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)
return super(JsonObject, self).__eq__(other)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The current implementation of __eq__ has several issues:

  1. Inconsistency: JsonObject(None) == {} returns True (line 107), but JsonObject(None) == JsonObject({}) returns False (line 101) because their serializations differ ("null" vs "{}"). This violates the transitivity of equality. A JSON null should not be equal to an empty object {}.
  2. Efficiency: Comparing two JsonObject instances by serializing them to strings is very expensive ($O(N)$ time and memory). It is much more efficient to compare their internal values directly.
  3. Redundancy: The check for isinstance(other, dict) in line 107 is partially redundant with the final super().__eq__ call, but it currently leads to the incorrect equality with empty dicts for null values.

I recommend refactoring the method to handle each variant explicitly and optimize the comparison between JsonObject instances.

Suggested change
def__eq__(self, other):
ifisinstance(other, JsonObject):
returnself.serialize() ==other.serialize()
ifself._is_array:
returnself._array_value==other
ifself._is_scalar_value:
returnself._simple_value==other
ifself._is_null:
returnotherisNoneor (isinstance(other, dict) andlen(other) ==0)
returnsuper(JsonObject, self).__eq__(other)
def__eq__(self, other):
ifisinstance(other, JsonObject):
ifself._is_null:
returnother._is_null
ifself._is_array:
returnother._is_arrayandself._array_value==other._array_value
ifself._is_scalar_value:
returnother._is_scalar_valueandself._simple_value==other._simple_value
# Both are dict variants
ifother._is_nullorother._is_arrayorother._is_scalar_value:
returnFalse
returnsuper(JsonObject, self).__eq__(other)
ifself._is_array:
returnself._array_value==other
ifself._is_scalar_value:
returnself._simple_value==other
ifself._is_null:
returnotherisNone
returnsuper(JsonObject, self).__eq__(other)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JsonObject array/scalar variants break standard dict protocol (len, bool, iteration)

1 participant

@waiho-gumloop