feat(fdc): Add execute_graphql signatures and integration test suite - #970
feat(fdc): Add execute_graphql signatures and integration test suite#970mk2023 wants to merge 11 commits into
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.
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.
…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.
Added execute_graphql and execute_graphql_read method signatures and docstrings to DataConnect and _DataConnectApiClient. Also introduced a comprehensive integration test suite in integration/test_data_connect.py translated from Node.js Admin SDK integration tests.
![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 execute_graphql and execute_graphql_read methods to the DataConnect client, along with corresponding integration and unit tests. The review feedback highlights that several of these new methods are left unimplemented (raising NotImplementedError) and provides their implementation details. Additionally, the reviewer identifies a critical type-checking bug in the validation logic when variables_type is Any, and points out a mismatch in a test assertion message.
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
in progress, leaving comments early - have to look at test cases still
Added an explicit __init__ constructor to Impersonation to validate parameter configurations at object instantiation time. Enforced choosing either unauthenticated=True or auth_claims, with support for both auth_claims (snake_case) and authClaims (camelCase). Updated class docstring to recommend factory methods. Also added unit test suite TestImpersonation in tests/test_data_connect.py.
…mulator test setup Implemented execute_graphql and execute_graphql_read methods on DataConnect and _DataConnectApiClient. Configured Data Connect emulator schema, connector, queries, mutations, seed script, and GitHub Actions CI workflow step.
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
LGTM with some changes requested - mostly small stuff, only a few blocking comments
| -H "Content-Type: application/json" \ | ||
| -d '{"query": "mutation { email_upsert(data: { id: \"email_id\", subject: \"free bitcoin inside\", date: \"1999-12-31\", text: \"get pranked! LOL!\", fromId: \"fred_id\" }) }"}' > /dev/null | ||
|
|
||
| echo "Successfully seeded initial test data (2 users, 1 email)!" |
There was a problem hiding this comment.
i think this may fail silently for GraphQL errors. IIUC, set -e and using curl -f will fail the script if curl returns a non-OK status code, but if we successfully send this request and receive a proper GQL response from the backend that contains errors, it will still report 200 and the errors won't be caught/reported, and then we'll report a success on the last line
There was a problem hiding this comment.
Oh, I didn't know that! Thanks so much for catching it! To fix this I updated seed.sh with a helper method that captures the response body and explicitly checks for a non-empty "errors" field in the JSON payload. And if there is an error message, the script fails!
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
Accidentally selected Approve but mean Request changes - re-submitting review
LGTM with some changes requested - mostly small stuff, only a few blocking comments
…d streamlined read integration tests
2 participants
Added
execute_graphqlandexecute_graphql_readmethod signatures and docstrings toDataConnectand_DataConnectApiClient. Expanded unit test coverage intests/test_data_connect.pyfor payload formatting, options validation, and error bubbling, and introduced a comprehensive integration test suite inintegration/test_data_connect.pytranslated from Node.js Admin SDK integration tests.