refactor(firestore): address review feedback for BSON read deserialization

- Perform automatic BSON deserialization in decode_dict and DocumentSnapshot.to_dict using _BSONType._from_dict.
- Remove decode_bson configuration parameter across Client, AsyncClient, BaseClient, and DocumentSnapshot.
- Preserve precise return type annotations in decode_dict and restore docstring Raises section.

Towards #18402

Author: ohmayr

packages/google-cloud-firestore/google/cloud/firestore_v1/_helpers.py

@@ -347,18 +347,20 @@ def reference_value_to_document(reference_value, client) -> Any:
     return document
 
 
-def decode_value(value, client=None, decode_bson: Optional[bool] = None) -> Any:
+def decode_value(value, client=None) -> Any:
     """Converts a Firestore protobuf ``Value`` to a native Python value.
 
     Args:
         value (google.cloud.firestore_v1.types.Value): A
             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.
 
     Returns:
         Any: A native Python value converted from the ``value``.
+
+    Raises:
+        ValueError: If ``value_type`` is unknown or unsupported.
     """
     value_pb = getattr(value, "_pb", value)
     value_type = value_pb.WhichOneof("value_type")
@@ -385,32 +387,19 @@ def decode_value(value, client=None, decode_bson: Optional[bool] = None) -> Any:
         )
     elif value_type == "array_value":
         return [
-            decode_value(element, client, decode_bson=decode_bson)
+            decode_value(element, client)
             for element in value_pb.array_value.values
         ]
     elif value_type == "map_value":
-        return decode_dict(value_pb.map_value.fields, client, decode_bson=decode_bson)
+        return decode_dict(value_pb.map_value.fields, client)
     else:
         raise ValueError("Unknown ``value_type``", value_type)
 
 
-def _decode_bson_dict(data: dict) -> Optional[_BSONType]:
-    """Decode a single-key wire map dictionary if registered."""
-    if len(data) == 1:
-        key, val = next(iter(data.items()))
-        decoder = _BSON_DECODERS.get(key)
-        if decoder is not None:
-            try:
-                return decoder(val)
-            except Exception:
-                pass
-    return None
-
-
 def _decode_bson_dict_recursive(data: Any) -> Any:
     """Recursively decodes BSON wire map dictionaries."""
     if isinstance(data, dict):
-        decoded = _decode_bson_dict(data)
+        decoded = _BSONType._from_dict(data)
         if decoded is not None:
             return decoded
         return {k: _decode_bson_dict_recursive(v) for k, v in data.items()}
@@ -422,25 +411,23 @@ def _decode_bson_dict_recursive(data: Any) -> Any:
 def decode_dict(
     value_fields,
     client=None,
-    decode_bson: Optional[bool] = None,
-) -> Union[dict, Vector, _BSONType]:
+) -> Union[dict, Vector, _BSONType, bytes]:
     """Converts a protobuf map of Firestore ``Value``-s.
 
     Args:
         value_fields (google.protobuf.pyext._message.MessageMapContainer): A
             protobuf map of Firestore ``Value``-s.
         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.
 
     Returns:
         Union[dict, ~google.cloud.firestore_v1.vector.Vector, \
-            ~google.cloud.firestore_v1.bson._BSONType]: A dictionary of native \
+            ~google.cloud.firestore_v1.bson._BSONType, bytes]: A dictionary of native \
         Python values, Vector, or BSON object converted from ``value_fields``.
     """
     value_fields_pb = getattr(value_fields, "_pb", value_fields)
     res = {
-        key: decode_value(value, client, decode_bson=decode_bson)
+        key: decode_value(value, client)
         for key, value in value_fields_pb.items()
     }
 
@@ -450,15 +437,9 @@ def decode_dict(
         values = cast(Sequence[float], res["value"])
         return Vector(values)
 
-    should_decode = (
-        decode_bson
-        if decode_bson is not None
-        else getattr(client, "_decode_bson", False)
-    )
-    if should_decode:
-        decoded = _decode_bson_dict(res)
-        if decoded is not None:
-            return decoded
+    decoded = _BSONType._from_dict(res)
+    if decoded is not None:
+        return decoded
 
     return res
 

packages/google-cloud-firestore/google/cloud/firestore_v1/async_client.py

@@ -105,15 +105,13 @@ def __init__(
         database=None,
         client_info=_CLIENT_INFO,
         client_options=None,
-        decode_bson: bool = False,
     ) -> None:
         super(AsyncClient, self).__init__(
             project=project,
             credentials=credentials,
             database=database,
             client_info=client_info,
             client_options=client_options,
-            decode_bson=decode_bson,
         )
 
     def _to_sync_copy(self):

packages/google-cloud-firestore/google/cloud/firestore_v1/base_client.py

@@ -132,7 +132,6 @@ def __init__(
         database=None,
         client_info=_CLIENT_INFO,
         client_options=None,
-        decode_bson: bool = False,
     ) -> None:
         database = database or DEFAULT_DATABASE
         # NOTE: This API has no use for the _http argument, but sending it
@@ -166,7 +165,6 @@ def __init__(
         self._client_options = client_options
 
         self._database = database
-        self._decode_bson: bool = decode_bson
 
     def _firestore_api_helper(self, transport, client_class, client_module) -> Any:
         """Lazy-loading getter GAPIC Firestore API.

packages/google-cloud-firestore/google/cloud/firestore_v1/base_document.py

@@ -512,17 +512,12 @@ def get(self, field_path: str) -> Any:
         nested_data = field_path_module.get_nested_value(field_path, self._data)
         return copy.deepcopy(nested_data)
 
-    def to_dict(
-        self, decode_bson: Optional[bool] = None
-    ) -> Union[Dict[str, Any], None]:
+    def to_dict(self) -> Union[Dict[str, Any], None]:
         """Retrieve the data contained in this snapshot.
 
         A copy is returned since the data may contain mutable values,
         but the data stored in the snapshot must remain immutable.
 
-        Args:
-            decode_bson (Optional[bool]): Whether to decode BSON extended types.
-
         Returns:
             Dict[str, Any] or None:
                 The data in the snapshot.  Returns None if reference
@@ -531,15 +526,7 @@ def to_dict(
         if not self._exists:
             return None
         data = copy.deepcopy(self._data)
-        client = self._reference._client if self._reference is not None else None
-        should_decode = (
-            decode_bson
-            if decode_bson is not None
-            else getattr(client, "_decode_bson", False)
-        )
-        if should_decode:
-            return _helpers._decode_bson_dict_recursive(data)
-        return data
+        return _helpers._decode_bson_dict_recursive(data)
 
     def _to_protobuf(self) -> Optional[Document]:
         return _helpers.document_snapshot_to_protobuf(self)

packages/google-cloud-firestore/google/cloud/firestore_v1/client.py

@@ -94,15 +94,13 @@ def __init__(
         database=None,
         client_info=_CLIENT_INFO,
         client_options=None,
-        decode_bson: bool = False,
     ) -> None:
         super(Client, self).__init__(
             project=project,
             credentials=credentials,
             database=database,
             client_info=client_info,
             client_options=client_options,
-            decode_bson=decode_bson,
         )
 
     @property

packages/google-cloud-firestore/tests/system/test_system.py

@@ -1307,7 +1307,7 @@ def test_bson_document_read_and_write(client, cleanup, database):
 
     snapshot = doc_ref.get()
     assert snapshot.exists
-    assert snapshot.to_dict(decode_bson=True) == bson_payload
+    assert snapshot.to_dict() == bson_payload
 
 
 @pytest.fixture(scope="module")

packages/google-cloud-firestore/tests/system/test_system_async.py

@@ -1280,7 +1280,7 @@ async def test_async_bson_document_read_and_write(client, cleanup, database):
 
     snapshot = await doc_ref.get()
     assert snapshot.exists
-    assert snapshot.to_dict(decode_bson=True) == bson_payload
+    assert snapshot.to_dict() == bson_payload
 
 
 @pytest_asyncio.fixture(scope="module")

packages/google-cloud-firestore/tests/unit/v1/test__helpers.py

@@ -733,13 +733,7 @@ def test_decode_dict_w_bson_types():
     }
 
     pb_fields = encode_dict(original_dict)
-    # Default (decode_bson=False) returns raw dict
-    raw_decoded = decode_dict(pb_fields, mock.sentinel.client)
-    assert raw_decoded != original_dict
-    assert raw_decoded["oid"] == {"__oid__": "507f191e810c19729de860ea"}
-
-    # decode_bson=True returns deserialized BSON objects
-    decoded = decode_dict(pb_fields, mock.sentinel.client, decode_bson=True)
+    decoded = decode_dict(pb_fields, mock.sentinel.client)
     assert decoded == original_dict