feat(fdc): Add execute_graphql and execute_graphql_read support with Pythonic impersonation - #970
feat(fdc): Add execute_graphql and execute_graphql_read support with Pythonic impersonation#970mk2023 wants to merge 19 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
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
…leanup - Impersonation API: Updated Impersonation to use auth_claims (snake_case) in Python land, while translating it to authClaims (camelCase) in _prepare_graphql_payload during JSON wire serialization. - Impersonation Validation: Updated _validate_impersonation_options to validate auth_claims in Python land. - Integration Tests (integration/test_data_connect.py): Added UPDATED_FRED_EMAIL mutation test fixtures with real state changes, restored initial state via UPSERT_FRED_EMAIL cleanup at the end of mutation tests, reordered query tests before mutations, and removed redundant read impersonation test cases.
mk2023
changed the title
jonathanedey
left a comment
jonathanedey
left a comment
There was a problem hiding this comment.
Thanks @mk2023! This overall looks great, with a few comments mainly on testing! I've also added the integration test tag so those tests should run from your next commit.
| - name: Run Functions emulator tests | ||
| run: firebase emulators:exec --config integration/emulators/firebase.json --only tasks,functions --project fake-project-id 'CLOUD_TASKS_EMULATOR_HOST=localhost:9499 pytest integration/test_functions.py' | ||
| - name: Run Data Connect emulator tests | ||
| run: firebase emulators:exec --config integration/emulators/firebase.json --only dataconnect --project fake-project-id './integration/emulators/seed.sh && DATA_CONNECT_EMULATOR_HOST=localhost:9399 pytest integration/test_data_connect.py --cert=tests/data/service_account.json' |
There was a problem hiding this comment.
We shouldn't need to pass credentials here since this is only running against emulator but i'm guessing the test suite was complaining for one.
To get around that we override that check in the integration test file if the emulator host is set. For reference see:
firebase-admin-python/integration/test_functions.py
Lines 27 to 44 in b40e738
There was a problem hiding this comment.
Thanks for letting me know!
I removed the credentials flag from ci.yml! Unlike test_functions.py which explicitly passes a named app instance to every call, test_data_connect.py relies on dataconnect.client() resolving the default app, so default_app initializes the default app directly instead of using pass. Let me know if this isn't optimal!
There was a problem hiding this comment.
Yeah, that sounds like it should be fine as long as we define the default_app(request) fixture to use the same integration_conf(request) fixture override.
There was a problem hiding this comment.
Awesome, thanks! That's how I defined default_app(request) in integration/test_data_connect.py!
There was a problem hiding this comment.
Update: Defining default_app in test_data_connect.py caused session app collisions (ValueError: The default Firebase app already exists) during full integration test runs.
Thus, I aligned the code with existing codebase conventions to use a named app fixture and set default_app to pass. I also had to add a dc_client(app) fixture to inject the client into tests rather than instantiating dataconnect.client() in every method. Now, my code adheres to established project conventions and significantly reduces boilerplate code!
Additionally, the stage_release check revealed tests were attempting to run against live GCP, so I added a check_emulator fixture to safely skip execution when DATA_CONNECT_EMULATOR_HOST is absent.
There was a problem hiding this comment.
Update: stage_release check is supposed to fail because we do not have a prod project set up to run tests against. Deleted check_emulator!
| ) | ||
|
|
||
|
|
||
| class TestExecuteGraphql: |
There was a problem hiding this comment.
Since these integration tests are ran against the same project and config that we use for our Admin Node tests we should ensure that they are compatible. The tests can use the same data but we should ensure we populate and clean up data so that they are idempotent similar to node
https://github.com/firebase/firebase-admin-node/blob/f9bff6b2db209edc182fbae27701e127ec5d71b6/test/integration/data-connect.spec.ts#L162-L176
There was a problem hiding this comment.
Done! I added a setup_and_cleanup_database pytest fixture (autouse=True) to match Node's beforeEach / afterEach lifecycle. To avoid calling execute_graphql() during setup and teardown before tests run, the fixture executes raw HTTP bash scripts (seed.sh and cleanup.sh via curl) before and after each test. This populates initial state (fred_id, jeff_id, email_id) before each test and wipes the database clean (email_deleteMany, user_deleteMany) afterwards, ensuring 100% test idempotency! Let me know if you think this is okay!
There was a problem hiding this comment.
oops i missed this comment - this changes what i originally said in this comment - if both SDKs are using the same project, we shouldn't have to set up any fdc service or deploy anything - they should already be set up from the node SDK!
| def __init__( | ||
| self, | ||
| *, | ||
| unauthenticated: Optional[bool] = None, |
There was a problem hiding this comment.
No action item here just thinking aloud, i know we are discouraging the constructor uses here but should we have a default state of unauthenticated=True here instead?
There was a problem hiding this comment.
I'm not sure if that's a good idea--defaulting unauthenticated=True in Impersonation.__init__ creates a risky API because calling Impersonation() without arguments would silently strip authentication credentials without explicit developer intent. Technically, if unauthenticated defaulted to True, calling Impersonation(auth_claims={...}) would also implicitly trigger a validation error for providing mutually exclusive parameters (both unauthenticated and auth_claims)!
There was a problem hiding this comment.
Sounds good, lets leave it as is then.
…t suite - Type Annotations: Added from __future__ import annotations to dataconnect.py for clean return type hints. - Integration Test Fixtures: Added setup_and_cleanup_database fixture in integration/test_data_connect.py using raw HTTP bash scripts (seed.sh and cleanup.sh) before/after every test without relying on the SDK under test. - Emulator Cleanup: Added integration/emulators/cleanup.sh script executing raw HTTP POST deleteMany mutations via curl. - Test Naming & CI: Updated TestImpersonation test method names to start with test_impersonation_ and removed unnecessary credentials flag from ci.yml.
… CI collision Updated default_app fixture in integration/test_data_connect.py to safely delete pre-existing default app before initializing with EmulatorAdminCredentials, allowing dataconnect.client() to implicitly use default_app.
…ta_connect.py
Aligned integration/test_data_connect.py with test_functions.py and test_db.py by using a named app fixture ('integration-dataconnect'), overriding default_app with pass, and injecting dc_client fixture into test methods.
…ot set Added a check_emulator autouse module fixture in integration/test_data_connect.py. Unlike services with dynamic resource creation (e.g., Realtime Database), Data Connect requires a pre-deployed Cloud SQL Postgres schema and connector. Since live integration projects do not host these resources, Data Connect integration tests are strictly emulator-only and are safely skipped when DATA_CONNECT_EMULATOR_HOST is absent.
…ta_connect.py Removed check_emulator fixture from integration/test_data_connect.py. Note that stage_release integration test runs are expected to fail until the expected GraphQL schema and connector are deployed to the FDC service within the GCP project tied to the service key.
| if [ -z "${response}" ]; then | ||
| echo "Failed to receive response from Data Connect emulator at ${ENDPOINT}" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
this one should also have an errors check just like the other one
actually - we shouldn't duplicate this code at all. how about we instead make a single setup_teardown.sh file, so the code can be shared between each use case?
There was a problem hiding this comment.
Done! I consolidated seed.sh and cleanup.sh into integration/emulators/dataconnect/setup_teardown.sh. It shares the send_gql_mutation helper with full error checking for both setup and teardown actions.
|
|
||
| @pytest.fixture(scope='module', autouse=True) | ||
| def check_emulator(): | ||
| """Skips Data Connect integration tests if emulator host is not set.""" |
There was a problem hiding this comment.
we shouldn't be skipping the integration tests if the emulator host isn't set - in that case, we should be running against production!
for this, i think we'll need to initialize the FDC service in the project for the service key we use for github actions test runs - and then deploy the schema + connectors we have in this repo to that project. i believe only Lahiru and @jonathanedey can do this
i think we should either remove the stage release tag or get that project set up
EDIT: according to this comment, the project used here and in the node admin SDK are actually the same - so there's no need to set anything up!
There was a problem hiding this comment.
for now, we can test against production manually using the same project we use for manual testing in the admin node SDK - i'll share the cert files with you
There was a problem hiding this comment.
Good point, sorry about that folks, I'll remove the tag to unblock this until we can get that setup correctly.
…sh inside dataconnect/ Combined seed.sh and cleanup.sh into a unified setup_teardown.sh script located in integration/emulators/dataconnect/ to clarify its exclusive use for FDC, eliminate code duplication, and add support for both emulator and live GCP testing.
4 participants
Overview
✨ Adds public
execute_graphqlandexecute_graphql_readmethods toDataConnect, refactorsImpersonationto use Pythonicauth_claims(PEP 8 snake_case) in Python land while translating toauthClaimsduring JSON payload serialization, and includes comprehensive unit and emulator integration test suites.Highlights
✨ Key changes:
firebase_admin.dataconnect): Addedexecute_graphqlfor query/mutation execution andexecute_graphql_readfor read-only queries with mutation validation.Impersonationto storeauth_claimsin Python land, with automaticauth_claims->authClaimskey translation in_prepare_graphql_payloadfor network serialization._validate_impersonation_optionsto validateauth_claimsin Python land.tests/test_data_connect.py): Added 82 unit tests covering method execution, payload serialization, dataclass variables, impersonation options, and error parsing.integration/): Added 20 integration tests against the Data Connect emulator, includingseed.shdata seeding (fred_id,jeff_id,email_id) and state cleanup (UPSERT_FRED_EMAIL)..github/workflows/ci.yml): Added--certflag and emulator seeding step for CI integration testing.Detailed Changelog
Core SDK Changes
firebase_admin/dataconnect.py:execute_graphqlandexecute_graphql_readonDataConnect.Impersonationclass withunauthenticated()andauthenticated(auth_claims=...)._prepare_graphql_payloadto translateauth_claimstoauthClaimsvia dictionary key popping._validate_impersonation_optionsandExecuteGraphqlResponsedataclass.Integration & Emulator Changes
integration/test_data_connect.py: Added 20 integration tests covering queries, mutations, multi-operation documents, impersonated requests, and mutation state cleanup.integration/emulators/seed.sh: Added script to seed initial test data (fred_id,jeff_id,email_id) into the emulator..github/workflows/ci.yml: Added--certflag and emulator seeding to integration test workflow.Unit Test Changes
tests/test_data_connect.py: Added 82 unit tests, refactoring shared constants (TEST_URL,TEST_HEADERS,TEST_PAYLOAD,TEST_AUTH_CLAIMS,TEST_VARIABLES) and dataclasses (UserProfile,CreateUserVariables,User).Testing Strategy
pytest tests/test_data_connect.py(82/82 PASSED)DATA_CONNECT_EMULATOR_HOST=127.0.0.1:9399 pytest integration/test_data_connect.py --cert tests/data/service_account.json(20/20 PASSED)./lint.shContext Sources Used: