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
| 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!
…helper - Introduced QueryError subclass of FirebaseError for Data Connect GraphQL query/mutation errors and exposed it in __all__. - Extracted _check_graphql_errors helper method on _DataConnectApiClient. - Updated error handling for non-dictionary response payloads in _parse_graphql_response to raise InternalError. - Note: Did not edit parse_graphql_response because we are waiting on whether this will even be a function or not.
…nt instantiation - Removed output deserialization helpers (_extract_actual_type, _deserialize_type, _deserialize_dataclass) to return raw JSON payload dictionaries (ExecuteGraphqlResponse.data), aligning Data Connect with Firestore and Realtime Database patterns for user-defined schemas. - Updated DataConnect.__init__ to immediately instantiate _DataConnectApiClient for consistency with Node.js and other Python Admin SDK services. - Updated test suite in tests/test_data_connect.py to cover raw response parsing and immediate client instantiation.
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.