fix: JsonObject array/scalar variants now implement standard dict protocol#16496
Open
waiho-gumloop wants to merge 1 commit intogoogleapis:mainfrom
Open
fix: JsonObject array/scalar variants now implement standard dict protocol#16496waiho-gumloop wants to merge 1 commit intogoogleapis:mainfrom
waiho-gumloop wants to merge 1 commit intogoogleapis:mainfrom
Conversation
…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. Fixes googleapis#15870
Contributor
There was a problem hiding this comment.
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) |
Contributor
There was a problem hiding this comment.
The current implementation of __eq__ has several issues:
-
Inconsistency:
JsonObject(None) == {}returnsTrue(line 107), butJsonObject(None) == JsonObject({})returnsFalse(line 101) because their serializations differ ("null"vs"{}"). This violates the transitivity of equality. A JSONnullshould not be equal to an empty object{}. -
Efficiency: Comparing two
JsonObjectinstances by serializing them to strings is very expensive ($O(N)$ time and memory). It is much more efficient to compare their internal values directly. -
Redundancy: The check for
isinstance(other, dict)in line 107 is partially redundant with the finalsuper().__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): | |
| 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) | |
| def __eq__(self, other): | |
| if isinstance(other, JsonObject): | |
| if self._is_null: | |
| return other._is_null | |
| if self._is_array: | |
| return other._is_array and self._array_value == other._array_value | |
| if self._is_scalar_value: | |
| return other._is_scalar_value and self._simple_value == other._simple_value | |
| # Both are dict variants | |
| if other._is_null or other._is_array or other._is_scalar_value: | |
| return False | |
| return super(JsonObject, self).__eq__(other) | |
| 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 | |
| return super(JsonObject, self).__eq__(other) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
JsonObjectsubclassesdictbut when wrapping arrays or scalars,__init__returns early without callingsuper().__init__(). This leaves thedictparent empty, causinglen(),bool(),iter(), indexing,json.dumps(), andinto silently return wrong results -- whileisinstance(obj, dict)returnsTrue.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_valuefor non-dict variants. Dict variant behavior is unchanged (delegates tosuper()).Before (broken)
After (fixed)
Tests
Added
Test_JsonObject_dict_protocoltest class coveringlen,bool,iter,getitem,contains, andeqfor all four variants (array, dict, scalar, null). All tests pass.Fixes #15870