Conversation
Implemented _make_gql_request on _DataConnectApiClient to execute and handle responses/errors for GraphQL operations. Added corresponding unit tests in tests/test_data_connect.py.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the _make_gql_request method to the _DataConnectApiClient class in firebase_admin/dataconnect.py to handle GraphQL requests, along with corresponding unit tests in tests/test_data_connect.py. Feedback on these changes highlights opportunities to improve error handling robustness. Specifically, it is recommended to ensure resp_dict is a dictionary before checking for errors, to avoid silent failures when the errors field is present but empty or malformed, and to add a unit test verifying that GraphQL errors are correctly parsed and raised as FirebaseError exceptions.
Refactored _parse_graphql_response and added robust recursive type deserialization to _DataConnectApiClient. - Implemented _deserialize_type and _deserialize_dataclass helper methods to support nested dataclasses, generic lists (List[T]), generic dictionaries (Dict[K, V]), Unions (Union[...]), Enums, and primitive casting. - Enhanced _make_gql_request error handling to prevent silent error swallowing when the errors key is present. - Added comprehensive unit test coverage in tests/test_data_connect.py.
mk2023
changed the title
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
Some comments - haven't looked at tests yet
| try: | ||
| from types import UnionType # pylint: disable=no-name-in-module | ||
| except ImportError: | ||
| UnionType = None |
There was a problem hiding this comment.
My first reaction is to be hesitant about this pattern. It looks like UnionType was only introduced in Python 3.10 - if we're running on 3.9, what happens? Does our type checking break?
Is there a way to support legacy and modern union types all at once with something like typing.Union?
| else "GraphQL execution failed." | ||
| ) | ||
| raise exceptions.FirebaseError( | ||
| code="query-error", |
There was a problem hiding this comment.
let's define these codes as an enum so that there's no hard-coded values for them. that way we don't have to update every hardcoded use in the future if a change is needed
There was a problem hiding this comment.
That is a great point, and I completely agree with the reasoning for avoiding hard-coded error strings across the codebase. Looking deeper into the SDK's existing patterns, other service modules (such as db.py with TransactionAbortedError) define dedicated error classes by subclassing FirebaseError rather than using an Enum. Thus, what do you think about doing this for dataconnect as well:
class QueryError(exceptions.FirebaseError):
"""Raised when a GraphQL query or mutation execution fails."""
def __init__(self, message: str, http_response: Any = None) -> None:
super().__init__(
code=_QUERY_ERROR_CODE,
message=message,
http_response=http_response
)
and call it through:
raise QueryError( message=all_messages, http_response=resp )
Let me know if you want to stick to the Enum option!
| @staticmethod | ||
| def _extract_actual_type(field_type: Any) -> Any: | ||
| origin = typing.get_origin(field_type) | ||
| if origin is Union or (UnionType is not None and origin is UnionType): |
There was a problem hiding this comment.
Re: this comment - this line of code seems to imply that something can be either a Union or a UnionType and therefore we have to check for both of them - is that true? If not, I suggest we just use the backwards compatible type to keep things simple
| if data is None or type_hint is Any: | ||
| return data | ||
|
|
||
| actual_type = _DataConnectApiClient._extract_actual_type(type_hint) |
There was a problem hiding this comment.
Re: this comment - could we change this variable name to optional_type?
| return field_type | ||
|
|
||
| @staticmethod | ||
| def _deserialize_type(type_hint: Any, data: Any) -> Any: |
There was a problem hiding this comment.
great job with this function, it makes me appreciate how complex type deserialization is in python lol
QQ: should we just use a 3rd party library for this (something like the TypeAdapter from pydantic)? i think that it would be preferred that we don't have to maintain / verify this complexity ourselves (especially considering in the future we're going to add generated types). Letting a third party library handle it would be ideal IMO
i don't want to bloat the other packages with another library, but thankfully it seems with setup.py we can actually do extras_require so that this is only required for FDC and not other packages -WDYT @lahirumaramba ?
2 participants
feat(fdc): Add request execution and response parsing to
_DataConnectApiClientThis change introduces GraphQL request preparation, HTTP request execution, and recursive response deserialization (supporting dataclasses, generic lists, dicts, unions, and enums) to the
_DataConnectApiClientclass.Testing: All 71 unit tests passed with 100% linter rating in
tests/test_data_connect.py.