Skip to content

feat(firestore): add BSONInt32 support - #18388

Merged
ohmayr merged 1 commit into
mainfrom
bson-pr1c-int32
Sep 16, 2026
Merged

ohmayr merged 1 commit into
mainfrom
bson-pr1c-int32

Conversation

@ohmayr

@ohmayr ohmayr commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

This PR adds support for BSONInt32 in the Firestore Python SDK. BSONInt32 allows developers to explicitly store 32-bit signed integer values in Firestore documents according to the official Firestore BSON specification.

Added BSONInt32 class: Introduces a container class that holds 32-bit signed integers between $-2,147,483,648$ and $2,147,483,647$.
Input Validation: Automatically rejects booleans and non-integer types with a TypeError, and raises a ValueError if a number is outside 32-bit signed integer bounds.
Wire Format: Serializes to {"__int__": value} for Firestore BSON document writes.
Package Exports: Exported BSONInt32 in google.cloud.firestore and google.cloud.firestore_v1, and updated post-processing generator rules to preserve exports during regeneration.
Testing: Added unit tests covering validation, equality, hashing, copying, and pickling, plus verified sync and async document writes on Firestore Enterprise DB.

Fixes b/562163919 🦕

@gemini-code-assist gemini-code-assist Bot 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 introduces the BSONInt32 class to represent 32-bit signed integers in Firestore BSON, updating the package exports and adding system and unit tests. The feedback suggests improving the __hash__ method of BSONInt32 to include the class type in the hash tuple, which prevents hash collisions with plain integers since they are not considered equal.

Comment on lines +230 to +231
def __hash__(self) -> int:
return hash(self._value)

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.

medium

The current implementation of __hash__ returns hash(self._value). Since BSONInt32(42) != 42 (as verified in the unit tests), having the same hash value for both BSONInt32(42) and 42 violates the best practice of minimizing hash collisions for unequal objects of different types. If both are stored in the same dictionary or set, it will cause a hash collision and degrade lookup performance.

Consider incorporating the class type into the hash to ensure distinct hash values for BSONInt32 instances compared to plain integers.

Suggested change
def __hash__(self) -> int:
return hash(self._value)
def __hash__(self) -> int:
return hash((BSONInt32, self._value))

@ohmayr
ohmayr marked this pull request as ready for review September 16, 2026 00:48
@ohmayr
ohmayr requested review from a team as code owners September 16, 2026 00:48
@ohmayr
ohmayr added this pull request to stack #18386 September 16, 2026 00:48
@ohmayr
ohmayr force-pushed the bson-pr1c-int32 branch 5 times, most recently from 74a4eb4 to 0f09d53 Compare September 16, 2026 02:31
Base automatically changed from bson-pr1b-minkey-maxkey to main September 16, 2026 08:07

@daniel-sanche daniel-sanche 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.

LGTM

if not (self._MIN_INT32 <= value <= self._MAX_INT32):
raise ValueError(
f"BSONInt32 value must be between {self._MIN_INT32} and {self._MAX_INT32}."
)

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.

nit: we can probably rely on the backend for this kind of validation. But this is fine too

@ohmayr
ohmayr merged commit 136d4b9 into main Sep 16, 2026
48 checks passed
@ohmayr
ohmayr deleted the bson-pr1c-int32 branch September 16, 2026 20:02
Sign up for free to 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.

2 participants