Conversation
6518eca to
29e3585
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces several new BSON types (BSONInt32, BSONBinary, BSONTimestamp, BSONRegex, and BSONDecimal128) to the Firestore Python client, along with their respective decoders, integration tests, and unit tests. Feedback on these changes highlights a violation of Python's hash contract in BSONDecimal128 due to mixed-type equality with decimal.Decimal without matching hashes. Additionally, the reviewer recommended replacing the boolean or fallback logic in decode_dict with an explicit None check to prevent potential bugs with falsy decoded BSON objects.
29e3585 to
b271751
Compare
b271751 to
e598276
Compare
e598276 to
be4c859
Compare
6a402d4 to
2ba70f6
Compare
2ba70f6 to
3630dd6
Compare
0ac138b to
38f629a
Compare
88f98a2 to
4cdbf85
Compare
4cdbf85 to
5ec6976
Compare
| ) -> Union[ | ||
| None, bool, int, float, list, datetime.datetime, str, bytes, dict, GeoPoint, Vector | ||
| ]: | ||
| def decode_value(value, client=None, decode_bson: Optional[bool] = None) -> Any: |
There was a problem hiding this comment.
Can we do this without changing this to Any? Can't we just add BSONType to the output list?
This would remove a lot of the value of the type annotations
| Raises: | ||
| NotImplementedError: If the ``value_type`` is ``reference_value``. | ||
| ValueError: If the ``value_type`` is unknown. | ||
| Any: A native Python value converted from the ``value``. |
There was a problem hiding this comment.
Why did you drop the Raises section?
| def decode_dict( | ||
| value_fields, | ||
| client=None, | ||
| decode_bson: Optional[bool] = None, |
There was a problem hiding this comment.
Why does this need 3 states? Can't it just be a bool that defaults to False?
| Firestore protobuf to be decoded / parsed / converted. | ||
| client (:class:`~google.cloud.firestore_v1.client.Client`): | ||
| A client that has a document factory. | ||
| decode_bson (Optional[bool]): Whether to decode BSON extended types. |
There was a problem hiding this comment.
Can we have this opted in by default? It looks like other languages decode to BSON by default. And we do similar decoding to custom objects for GeoPoint, Vector, etc. Making this opt-in would really lower the value of for the feature
BSON is a new feature, so we wouldn't expect workarounds in existing code. But let me know if you have any specific breaking change concerns, and maybe we can find solutions
| """Decode a single-key wire map dictionary if registered.""" | ||
| if len(data) == 1: | ||
| key, val = next(iter(data.items())) | ||
| decoder = _BSON_DECODERS.get(key) |
There was a problem hiding this comment.
This seems like it should be implemented as a class method:
BSONType._from_dict(data)
or
BSONType._class_for_key(key)(value)
5ec6976 to
c8310e9
Compare
c8310e9 to
8fcabfc
Compare
Adds opt-in BSON read deserialization support to the Google Cloud Firestore Python SDK.
When enabled via
decode_bson=True, document fields containing BSON wire map structures returned by Firestore (such as{"__oid__": "507f191e810c19729de860ea"}) are automatically deserialized into their corresponding Python BSON container instances (BSONObjectId,BSONDecimal128,BSONTimestamp,BSONRegex,BSONBinary,BSONInt32,BSONMinKey,BSONMaxKey).💻 Usage
Default Behavior (
decode_bson=False)Existing applications continue to receive raw map dictionaries by default to preserve 100% backward compatibility:
Opt-in Behavior (
decode_bson=True)🏛️ Design Decisions
Opt-In decode_bson=False Default (Enterprise Backward Safety): Defaulting to decode_bson=False ensures existing production code accessing raw dictionary keys (dict["user_id"]["oid"]) will not break upon upgrading the SDK.
Subtype 0 Binary Deserialization: Wire maps representing Subtype 0 BSON Binary (v[0] == 0) are deserialized into native Python bytes (b"..."), while non-zero subtypes ($1 \le v[0] \le 255$ ) deserialize into BSONBinary(data, subtype=v[0]) objects.
Explicit Non-None Fallback Control: Updated decode_dict() to explicitly check if decoded is not None: rather than relying on Python truthiness (or), preventing false fallback on empty byte payloads (b"") or falsy objects.
Recursive Nested Map & Array Support: Added _decode_bson_dict_recursive() to ensure BSON wire maps inside nested dictionaries and array elements are deserialized properly.
Fixes b/562164140 🦕