[wip] firestore bson comprehensive

[ohmayr]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

[gemini-code-assist]

Code Review

This pull request introduces native BSON data type support for the Firestore Python SDK, including classes for ObjectID, Decimal128, Timestamp, Regex, Binary, Int32, MinKey, and MaxKey, along with corresponding encoding, decoding, and ordering logic. The review feedback suggests several key improvements: wrapping legacy BSON map parsing in a try-except block to handle malformed data gracefully, reverting an over-engineered lambda-based handler dispatch table to a simpler if-elif chain, simplifying defensive attribute checks on guaranteed properties, optimizing canonical value extraction by including common native types in the fast path, and replacing strict type checks with isinstance to correctly support integer subclasses.

[gemini-code-assist]

Constructing BSON objects from legacy map dictionaries in _extract_canonical_value is not wrapped in a try...except block. If there is any malformed legacy BSON data in the database (e.g., an invalid decimal string or non-integer timestamp seconds), it will raise ValueError or TypeError and crash the query comparison. Wrapping this block in a try...except (ValueError, TypeError): block ensures robust and graceful fallback behavior.

            v = val[key]
            try:
                if key == "__oid__" and isinstance(v, str):
                    return BSONObjectID(v)
                elif key == "__decimal128__" and isinstance(v, str):
                    return BSONDecimal128(v)
                elif key == "__int__" and isinstance(v, int):
                    return BSONInt32(v)
                elif key == "__minkey__":
                    return BSONMinKey()
                elif key == "__maxkey__":
                    return BSONMaxKey()
                elif key == "__timestamp__" and isinstance(v, (dict, collections.abc.Mapping)):
                    return BSONTimestamp(v.get("seconds", 0), v.get("increment", 0))
                elif key == "__regex__" and isinstance(v, (dict, collections.abc.Mapping)):
                    return BSONRegex(v.get("pattern", ""), v.get("options", ""))
                elif key == "__binary__" and isinstance(v, (dict, collections.abc.Mapping)):
                    return BSONBinary(v.get("bytes", b""), subtype=v.get("sub_type", 0))
            except (ValueError, TypeError):
                pass

[gemini-code-assist]

The lambda-based handler dispatch table introduced here is over-engineered and introduces unnecessary performance overhead on every DocumentExtractor instantiation. Additionally, using .update({fp: v.value}) creates temporary dictionaries and is slower than direct assignment. Reverting to a clean if/elif chain improves both readability and performance.

        for field_path, value in iterator:
            if field_path == prefix_path and value is _EmptyDict:
                self.empty_document = True
                continue

            if value is transforms.DELETE_FIELD:
                self.deleted_fields.append(field_path)
            elif value is transforms.SERVER_TIMESTAMP:
                self.server_timestamps.append(field_path)
            elif isinstance(value, transforms.ArrayRemove):
                self.array_removes[field_path] = value.values
            elif isinstance(value, transforms.ArrayUnion):
                self.array_unions[field_path] = value.values
            elif isinstance(value, transforms.Increment):
                self.increments[field_path] = value.value
            elif isinstance(value, transforms.Maximum):
                self.maximums[field_path] = value.value
            elif isinstance(value, transforms.Minimum):
                self.minimums[field_path] = value.value
            else:
                self.field_paths.append(field_path)
                set_field_value(self.set_fields, field_path, value)

[gemini-code-assist]

The defensive checks and fallback logic for self._decode_bson are unnecessarily complex. Since self._decode_bson is guaranteed to be initialized as a boolean in the class's __init__ method, we can simplify this expression directly.

        effective_decode = (
            decode_bson
            if decode_bson is not None
            else self._decode_bson
        )

References

1. Do not use defensive getattr(self, 'attribute', None) checks for attributes that are guaranteed to be initialized in the class's init method, as it adds unnecessary complexity.

[gemini-code-assist]

To optimize the fast path for canonical value extraction, we should include the most common native Python types (int, float, str, bytes) in the initial isinstance check. This avoids executing multiple slow hasattr and isinstance checks on every native type comparison during ordering.

    if val is None or isinstance(
        val,
        (
            bool,
            int,
            float,
            str,
            bytes,
            BSONMinKey,
            BSONMaxKey,
            BSONObjectID,
            BSONDecimal128,
            BSONTimestamp,
            BSONRegex,
            BSONBinary,
            BSONInt32,
            GeoPoint,
            Vector,
            _RefValue,
            BaseDocumentReference,
        ),
    ):

[gemini-code-assist]

Using strict type(seconds) is not int checks rejects integer subclasses such as BSONInt32. To allow compatible integer subclasses while still rejecting booleans, use isinstance(seconds, int) and not isinstance(seconds, bool).

Suggested change

	if type(seconds) is not int or type(increment) is not int:
	raise TypeError("seconds and increment must be ints.")
	if (not isinstance(seconds, int) or isinstance(seconds, bool) or
	not isinstance(increment, int) or isinstance(increment, bool)):
	raise TypeError("seconds and increment must be ints.")

[gemini-code-assist]

Using strict type(subtype) is not int checks rejects integer subclasses such as BSONInt32. To allow compatible integer subclasses, use isinstance(subtype, int).

Suggested change

	if isinstance(subtype, bool) or type(subtype) is not int:
	if isinstance(subtype, bool) or not isinstance(subtype, int):