Skip to content

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

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

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

Conversation

@waiho-gumloop
Copy link

@waiho-gumloop waiho-gumloop commented Feb 27, 2026

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 (#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)  # True
len(val)               # 0  ← wrong
bool(val)              # False  ← wrong
list(val)              # []  ← wrong
json.dumps(val)        # "{}"  ← wrong

After (fixed)

val = JsonObject([{"id": "m1", "content": "hello"}])
isinstance(val, dict)  # True
len(val)               # 1  ← correct
bool(val)              # True  ← correct
list(val)              # [{"id": "m1", "content": "hello"}]  ← correct
val[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 31 tests pass.

Fixes #1505

…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
@waiho-gumloop waiho-gumloop requested review from a team as code owners February 27, 2026 19:00
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/python-spanner API. labels Feb 27, 2026
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, 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 JsonObject class, despite subclassing dict, exhibited incorrect behavior for standard Python operations like len(), bool(), and iteration when it wrapped non-dictionary types (arrays or scalars). The changes ensure that JsonObject now consistently adheres to the dict protocol across all its internal data representations, providing predictable and correct results for common Python operations.

Highlights

  • JsonObject Protocol Compliance: The JsonObject class now correctly implements the standard Python dictionary protocol (len(), bool(), iter(), __getitem__, __contains__, __eq__) for its array and scalar variants, resolving previous silent incorrect behaviors.
  • Method Overrides: New __len__, __bool__, __iter__, __getitem__, __contains__, and __eq__ methods were added to JsonObject to delegate operations to the internal _array_value or _simple_value when the object wraps an array or scalar, respectively.
  • Comprehensive Testing: A new test class, Test_JsonObject_dict_protocol, was introduced with 31 tests covering all four JsonObject variants (array, dict, scalar, null) to ensure proper behavior of the overridden methods.
Changelog
  • google/cloud/spanner_v1/data_types.py
    • Implemented __len__ to return the length of the internal array or 1 for scalar values, 0 for null, and delegate to super() for dicts.
    • Implemented __bool__ to return the boolean value of the internal array or True for scalar values, False for null, and delegate to len() for dicts.
    • Implemented __iter__ to iterate over the internal array, raise TypeError for scalar values, and delegate to super() for dicts.
    • Implemented __getitem__ to access elements of the internal array, raise TypeError for scalar values, and delegate to super() for dicts.
    • Implemented __contains__ to check for item presence in the internal array, raise TypeError for scalar values, and delegate to super() for dicts.
    • Implemented __eq__ to compare based on serialized value for JsonObject instances, or directly compare with internal array/scalar values, and handle nulls.
  • tests/unit/test_datatypes.py
    • Added Test_JsonObject_dict_protocol class to test len, bool, iter, getitem, contains, and eq for JsonObject instances wrapping arrays, dictionaries, scalars, and nulls.
    • Included specific tests for array length, boolean evaluation (truthy/empty), iteration, item access, containment, equality, and json.dumps behavior.
    • Added tests for dict length, boolean evaluation, iteration, and item access.
    • Added tests for null length and boolean evaluation.
    • Added tests for scalar length, boolean evaluation, and TypeError for iteration and subscripting.
Activity
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

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 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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.

Suggested change
return other is None or (isinstance(other, dict) and len(other) == 0)
return other is None

Comment on lines +139 to +143
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

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.

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

Labels

api: spanner Issues related to the googleapis/python-spanner API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants