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 7 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
| raise ValueError("connector cannot be empty") | ||
|
|
||
|
|
||
| class Impersonation(dict): |
There was a problem hiding this comment.
just realizing this now but IIUC, because there's no constructor defined here a default one is available for users to call directly
we should make sure that:
- we call out in documentation it's preferred that you use the static factory methods
- if they DO call the constructor, we should strictly enforce that they choose a single configuration - they can either provide
unauthenticated=Trueorauth_claims=auth_claims
There was a problem hiding this comment.
Good catch! _validate_impersonation_options extensively validates raw dictionary inputs passed directly to GraphqlOptions(impersonate={...}), but adding validation in Impersonation.__init__ handles direct class instantiation early.
I've updated Impersonation:
- Updated docstring to highlight static factory methods (
unauthenticated()/authenticated()). - Added an explicit
__init__constructor validatingunauthenticatedvsauth_claims(and handling bothauth_claimsandauthClaims). - Added unit tests in
TestImpersonationcovering all constructor validation scenarios.
|
|
||
| @pytest.fixture(autouse=True) | ||
| def setup_teardown_db(default_app): | ||
| """Sets initial database state before each test and cleans up after.""" |
There was a problem hiding this comment.
ah, this was a bad practice that we did in the admin node SDK - we haven't tested these functions yet, so how could we possibly use them to initialize the database?
instead, WDYT of this - we should seed the database using some already-tested thing... maybe the firebase CLI? i'll look more into this, feel free to as well and drop some ideas
There was a problem hiding this comment.
Yes, I understand why that isn't the most honest testing method!
If we use the Node.js SDK (or Firebase CLI) to seed the database, we would have to introduce external Node.js subprocess dependencies into pytest.
Instead, what do you think about using raw requests.post() calls directly against the emulator URL inside setup_teardown_db? This way we can seed the database via raw HTTP POST requests without invoking any firebase-admin-python SDK methods while keeping the test suite 100% Python-native.
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.
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.