feat(dataconnect): Implementation and Testing of the _DataConnectApiClient class by mk2023 · Pull Request #965 · firebase/firebase-admin-python

mk2023 · 2026-07-09T21:13:04Z

Implemented three helper functions in _DataConnectApiClient along with their corresponding implementations:

_validate_inputs: Validates the query structure, options, variables types, and custom impersonation claims.
_prepare_graphql_payload: Prepares the JSON payload for GraphQL calls, serializing variables (including nested dataclasses) and configuring optional extensions like impersonation.
_get_firebase_dataconnect_service_url: Formats the endpoint URL for execution, supporting both production host formats and local emulator host formats.
Added comprehensive unit tests covering standard cases, invalid options, missing configurations, and emulator environments for each of the helper functions.

Added comprehensive unit tests to TestDataConnectApiClient covering client constructor, inputs validation, variables and impersonation serialization, and service URL construction.

gemini-code-assist

Code Review

This pull request introduces a comprehensive suite of unit tests for the _DataConnectApiClient class, covering its constructor, input validation, payload preparation, and service URL generation. The review feedback identifies critical issues in the tests, including a missing import for the dataclass decorator and references to undefined classes and attributes in the dataconnect module, both of which will cause runtime errors during test execution.

Implemented _validate_inputs to validate queries, options, variables, and impersonation, and _prepare_graphql_payload to construct GraphQL JSON payloads. Implemented _get_firebase_dataconnect_service_url to build production and emulator service endpoint URLs. Added full unit test coverage for input validation, payload construction, and URL formatting.

…uilder Implemented three helper functions in _DataConnectApiClient along with their corresponding implementations: - _validate_inputs: Validates the query structure, options, variables types, and custom impersonation claims. - _prepare_graphql_payload: Prepares the JSON payload for GraphQL calls, serializing variables (including nested dataclasses) and configuring optional extensions like impersonation. - _get_firebase_dataconnect_service_url: Formats the endpoint URL for execution, supporting both production host formats and local emulator host formats. Added comprehensive unit tests covering standard cases, invalid options, missing configurations, and emulator environments for each of the helper functions. This completes the first half of milestone 3.

Implemented `_get_headers` inside `_DataConnectApiClient` to build standard telemetry headers for outgoing HTTP requests. Specifically, it populates: - X-Firebase-Client: The current Python SDK version header. - x-goog-api-client: The Google telemetry metrics header. Added the `TestDataConnectApiClientGetHeaders` unit test suite to verify the return type and values.

Fixed formatting and style issues highlighted by pylint: - Removed trailing whitespace in TestDataConnectApiClientGetHeaders test class. - Resolved missing final newline warning at the end of the test file.

stephenarosaj

Partial review - just dropping comments early

stephenarosaj · 2026-07-10T17:58:15Z

+        return {"unauthenticated": True}
+
+    @staticmethod
+    def authenticated(auth_claims: Dict[str, Any]) -> Dict[str, Any]:


is there a way to more strongly type this? In node we use this type:

/** * Type representing the partial claims of a Firebase Auth token used to evaluate the * Data Connect auth policy. */ export type AuthClaims = Partial<DecodedIdToken>;

That's a good point! After taking a look at AuthClaims and DecodedIdToken, I used typing.TypedDict with total=False to mirror Partial<DecodedIdToken>, along with a nested FirebaseClaim type structure. Additionally, I annotated Impersonation.authenticated with Union[AuthClaims, Dict[str, Any]] so that type checkers provide full auto-completion for standard claims while still supporting arbitrary custom claim dictionaries:

class FirebaseClaim(TypedDict, total=False):
"""Provider-specific identity details and sign-in info within a decoded ID token."""
identities: Dict[str, Any]
sign_in_provider: str
sign_in_second_factor: str
second_factor_identifier: str
tenant: str

class AuthClaims(TypedDict, total=False):
"""Partial claims of a Firebase Auth token used for Data Connect policy evaluation."""
aud: str
auth_time: int
email: str
email_verified: bool
exp: int
firebase: FirebaseClaim
iat: int
iss: str
phone_number: str
picture: str
sub: str
uid: str

class Impersonation:
"""Represents impersonation configuration for DataConnect requests."""

@staticmethod def unauthenticated() -> Dict[str, bool]: """Returns impersonation configuration for unauthenticated requests.""" return {"unauthenticated": True} @staticmethod def authenticated(auth_claims: Union[AuthClaims, Dict[str, Any]]) -> Dict[str, Any]: """Returns impersonation configuration for authenticated requests.""" return {"authClaims": auth_claims}`

stephenarosaj · 2026-07-10T18:00:46Z

+)
+
+# Generic Type Parameters
+_T = TypeVar("_T")


let's make these type names more descriptive. how about _Data and _Variables?

Actually, looking at the API proposal, we said T_Variables and T_Returned_Data - but let's just use T_Data instead.

Unfortunately, if I change it to T_Data and T_Variables, the linter says:
firebase_admin/dataconnect.py:54:0: C0103: Type variable name "T_Variables" doesn't conform to predefined naming style (invalid-name)

Should I leave it as _Data and _Variables?

stephenarosaj · 2026-07-10T18:05:58Z

+def _get_emulator_host() -> Optional[str]:
+    emulator_host = os.environ.get("DATA_CONNECT_EMULATOR_HOST")
+    if emulator_host:
+        if "//" in emulator_host:


we don't make this check in the admin SDK - but maybe we should :P

if we're going to enforce that it's not using "//", could we just use a regex to ensure it's not doing anything else we don't expect? i believe we expect either localhost:<port> or <IP address>:<port>

That's a great point I should have considered. One thing I'm worried about though is in firebase_admin/functions.py, they also use the same _get_emulator_host() so should I make a custom implementation specifically for dataconnect.py, or just drop it entirely to avoid the inconsistency?

stephenarosaj

Further partial review

stephenarosaj · 2026-07-10T23:35:38Z

+
+        self._http_client = _http_client.JsonHttpClient(credential=self._credential)
+
+    def _validate_inputs(


this function does validate inputs - but only for execute_graphql() since it expects graphql_options - but in the future there will be other methods whose inputs need to be validated that don't use graphql_options - so the name _validate_inputs will probably have to be more specific

thinking a little deeper on this, it seems the majority of this function deals with validating graphql_options - and string validation is only a few lines - perhaps instead we should make this just _validate_graphql_options, and leave the string validation in-line in the funciton. WDYT?

stephenarosaj · 2026-07-10T23:41:33Z

+    ) -> None:
+        """Validates query and GraphqlOptions inputs at runtime."""
+        # Validate the Query
+        if not isinstance(graphql_query, str) or not graphql_query.strip():


seems like the or not graphql_queryl.strip() is checking if it's an empty string - but it would be simpler to just check or not graphql_query

if your goal was to also remove the leading/trailing whitespace, heads up that .strip() doesn't remove that in place, it returns the edited string (documentation)

stephenarosaj · 2026-07-10T23:45:41Z

+            # Validate Operation Name (if it exists)
+            operation_name = graphql_options.operation_name
+            if operation_name is not None:
+                if not isinstance(operation_name, str) or not operation_name.strip():


see above comment about .strip()

stephenarosaj · 2026-07-10T23:49:45Z

+)
+
+# Generic Type Parameters
+_T = TypeVar("_T")


Actually, looking at the API proposal, we said T_Variables and T_Returned_Data - but let's just use T_Data instead.

stephenarosaj · 2026-07-11T00:15:43Z

+            # Validate Variables against expected variable_type
+            variables = graphql_options.variables
+            if variables is not None and variable_type is not None:
+                if not isinstance(variables, variable_type):


isinstsance() can only be used to check that an object is an instance of a class (documentation)

Is it possible that a user passes in a type that's not a class or that will otherwise cause a false / throw an error in isinstance() even it really is the desired type?? after doing some research, it seems so:

PEP 484 talks about runtime type erasure for generics

there's also reddit and stackoverflow posts about for example it not working with list[<some type>] or Dict[str, any]

There's a bunch of solutions floating around on the internet / with AI, but I propose we ask Lahiru and/or Denver for advice

feat(dataconnect): Add unit tests for DataConnect API Client helpers

db66f2d

Added comprehensive unit tests to TestDataConnectApiClient covering client constructor, inputs validation, variables and impersonation serialization, and service URL construction.

gemini-code-assist Bot reviewed Jul 9, 2026

View reviewed changes

Comment thread tests/test_data_connect.py

Comment thread tests/test_data_connect.py

mk2023 added 2 commits July 9, 2026 14:54

mk2023 changed the title ~~feat(dataconnect): Added unit tests for DataConnect API Client helpers~~ feat(dataconnect): Implementation and Testing of the _DataConnectApiClient class Jul 10, 2026

mk2023 requested review from itsrakhil and stephenarosaj July 10, 2026 03:11

mk2023 added 2 commits July 10, 2026 10:22

chore(fdc): Resolve linter warnings in Data Connect module and tests

2bc4b1b

Fixed formatting and style issues highlighted by pylint: - Removed trailing whitespace in TestDataConnectApiClientGetHeaders test class. - Resolved missing final newline warning at the end of the test file.

mk2023 force-pushed the champagne branch from a1bc473 to 2bc4b1b Compare July 10, 2026 17:23

stephenarosaj requested changes Jul 10, 2026

View reviewed changes

stephenarosaj requested changes Jul 11, 2026

View reviewed changes


		self._http_client = _http_client.JsonHttpClient(credential=self._credential)

		def _validate_inputs(

Uh oh!

Conversation

mk2023 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

stephenarosaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mk2023 Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenarosaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mk2023 commented Jul 9, 2026 •

edited

Loading

mk2023 Jul 11, 2026 •

edited

Loading