Conversation
Added comprehensive unit tests to TestDataConnectApiClient covering client constructor, inputs validation, variables and impersonation serialization, and service URL construction.
![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 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.
mk2023
changed the title
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
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
Partial review - just dropping comments early
| # 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): |
There was a problem hiding this comment.
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>]orDict[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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.Mappingor adataclass _validate_variables_type()should validate that if the variables are notNone, they must either be acollections.abc.Mappingor adataclassvariables_typeshould be type-annotated as an optionaldataclasssince we can't type-check aTypedDict(we may end up removingvariables_typeafter 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)
There was a problem hiding this comment.
Sounds good, I updated _validate_variables_type() and _validate_graphql_options()!
There was a problem hiding this comment.
SGTM - leaving this comment open for Lahiru to see when he reviews
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
partial review of main file - now for tests
…ost utility - Inherited Impersonation from dict to resolve the type-checking mismatch for static methods while maintaining dictionary behavior at runtime. - Updated GraphqlOptions.impersonate type annotation to accept Union[Impersonation, Dict[str, Any]] to support both helper and raw dictionary inputs. - Extracted generic variable and impersonate validation into separate helper methods _validate_variables_type and _validate_impersonation_options. - Shared emulator host extraction and validation logic in _utils.get_emulator_host and reused it across dataconnect and functions. - Removed AuthClaims and FirebaseClaim TypedDicts to keep them as raw dicts for now. - Renamed generic TypeVars to private naming conventions (_Data and _Variables).
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
some minor changes but i think this looks good!
…te variable_type - Updated _validate_variables_type to validate that if variables are provided, they must be either a collections.abc.Mapping or a dataclass. - Type-annotated the variable_type parameter as Optional[Type[Any]] = None in both validation helper signatures. - Refactored variable unit tests to use a realistic CreateUserVariables (nesting a UserProfile dataclass) instead of response-like shapes. - Added a test case confirming that standard dictionaries (Mapping) are accepted as valid variables.
- Fallback to str(variable_type) if variable_type lacks a __name__ attribute to prevent AttributeError crashes. - Added a unit test validating this fallback behavior for a tuple of types.
3 participants
Implemented three helper functions in
_DataConnectApiClientalong 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.