feat(dataconnect): Implementation and Testing of the _DataConnectApiClient class

[mk2023]

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.

[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.

[stephenarosaj]

Partial review - just dropping comments early

[stephenarosaj]

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>;

[mk2023]

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}`

[mk2023]

Unlike the Node.js Admin SDK, which exports DecodedIdToken from auth/token-verifier.ts, the Python Admin SDK does not define or export any typed dictionary or class representation for verified ID tokens (the auth module returns raw dicts). Thus, for right now, we just left AuthClaimsas a raw dictionary.

[stephenarosaj]

Further partial review

[stephenarosaj]

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

[mk2023]

Great catch! You're completely right; passing something like list[User] or Dict[str, Any] will crash isinstance() with a TypeError because of type erasure in Python. I've been seeing a bunch of solutions as well, like utilizing typing.get_origin() to extract the base class from a generic type, and then using typing.get_args()to complete a deep-check of the inner types. I think it's a great idea to ask Lahiru and/or Denver for advice on how to go about this matter.

[stephenarosaj]

Note: this is a point of active discussion and there may be some API changes - for now, let's continue with the rest of the PR and project, and we'll make adjustments later as needed

[stephenarosaj]

re: our discussion at lunch today - let's update the code to the following:

• the generic variables type should be limited to being either collections.abc.Mapping or a dataclass
• _validate_variables_type() should validate that if the variables are not None, they must either be a collections.abc.Mapping or a dataclass
• variables_type should be type-annotated as an optional dataclass since we can't type-check a TypedDict (we may end up removing variables_type after further design - for now let's keep it in to follow the API proposal, and because it's easier to remove than to add :P)

[mk2023]

Sounds good, I updated _validate_variables_type() and _validate_graphql_options()!

[itsrakhil]

Left some comments.

[stephenarosaj]

partial review of main file - now for tests

[stephenarosaj]

is it GraphqlOptions[Any]? shouldn't it be GraphqlOptions[_Variables]?

[mk2023]

I don't think we actually need _Variables here. Since _prepare_graphql_payload just serializes the options to a standard Dict[str, Any] and doesn't propagate the type to the return type, the generic type variable doesn't have an effect. I'm totally happy to switch it back if you'd prefer, though!"

[stephenarosaj]

some minor changes but i think this looks good!

[itsrakhil]

If a user passes something like a tuple of types (e.g., (list, dict)) or certain generics from the typing module, Python will crash with an AttributeError when trying to construct this string, hiding the actual ValueError.
To make the error reporting completely safe, we can use getattr with a fallback to str():
raise ValueError(f"variables must be of type {getattr(variable_type, '__name__', str(variable_type))}")