fix: JsonObject array/scalar variants now implement standard dict protocol#1506
fix: JsonObject array/scalar variants now implement standard dict protocol#1506waiho-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, and json.dumps() to silently return wrong results while isinstance(obj, dict) is True. Add __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. Fixes googleapis#1505 Made-with: Cursor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a long-standing issue where the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses a significant issue where JsonObject instances wrapping arrays or scalars did not behave correctly with standard Python protocols like len(), bool(), and iteration. By implementing the appropriate dunder methods (__len__, __bool__, __iter__, etc.), you've made the class behave more intuitively and fixed the silent failures. The addition of a comprehensive test suite for these protocol methods is also a great improvement.
I've found a couple of areas for improvement. The most important one is in the __eq__ implementation, which currently breaks the symmetry of the equality operator. I've also noted a test case that seems redundant and has a misleading name. Please see my detailed comments below.
| 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) |
There was a problem hiding this comment.
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.
| return other is None or (isinstance(other, dict) and len(other) == 0) | |
| return other is None |
| 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) |
There was a problem hiding this comment.
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.
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 (#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 31 tests pass.Fixes #1505