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
requested changes
Jul 10, 2026
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
Partial review - just dropping comments early
stephenarosaj
requested changes
Jul 11, 2026
itsrakhil
reviewed
Jul 13, 2026
itsrakhil
reviewed
Jul 13, 2026
itsrakhil
reviewed
Jul 13, 2026
stephenarosaj
requested changes
Jul 13, 2026
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
requested changes
Jul 14, 2026
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.
itsrakhil
reviewed
Jul 14, 2026
- 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.
lahirumaramba
requested changes
Jul 17, 2026
lahirumaramba
left a comment
lahirumaramba
left a comment
Member
There was a problem hiding this comment.
Thank you! Overall LGTM!
Added a few comments/questions.
…in emulator hostnames Refactored GraphqlOptions operationName validation and serialization to avoid mutating the original options object. Updated emulator hostname regex validation to support underscores in hostnames, and added/updated corresponding unit tests.
Updated _validate_variables_type to use typing.get_origin() to unwrap subscripted generics (e.g. Dict, Mapping) for runtime type checks. Added corresponding unit tests to verify generic type validation and mismatch scenarios.
mk2023
added a commit
that referenced
this pull request
Jul 20, 2026
…ataConnectApiClient class (#965) *Core API Client Helpers* - Implemented `_validate_inputs` to enforce strict validation on query structures, options, variables, and custom impersonation claims. - Implemented `_prepare_graphql_payload` to construct the GraphQL JSON payload, seamlessly serializing variables (including nested dataclasses) and optional extensions. - Implemented `_get_firebase_dataconnect_service_url` to dynamically build endpoint URLs for both production and local emulator hosts. - Implemented `_get_headers` to inject standard telemetry metrics (X-Firebase-Client and x-goog-api-client). *Validation & Serialization Refinements* - Enforced that GraphQL variables must be either a Mapping (like standard dicts) or a dataclass. - Added support for runtime validation of subscripted generic types (e.g., Dict, Mapping) using typing.get_origin(). - Refactored `Impersonation` to inherit from `dict`, resolving type-checking mismatches while maintaining runtime dictionary behavior. Extracted variable and impersonation validation into dedicated helper methods. *Testing & Code Health* - Added comprehensive unit test coverage for the client constructor, input validation, headers, and payload serialization (using realistic nested dataclasses).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.