Skip to content

feat(api_core): add universe and endpoint routing logic to gapic_v1 public helpers#17745

Open
hebaalazzeh wants to merge 18 commits into
mainfrom
feat/gapic-centralization-api-core-routing
Open

feat(api_core): add universe and endpoint routing logic to gapic_v1 public helpers#17745
hebaalazzeh wants to merge 18 commits into
mainfrom
feat/gapic-centralization-api-core-routing

Conversation

@hebaalazzeh

@hebaalazzeh hebaalazzeh commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Centralizes generic routing, mTLS, and universe domain resolution helpers under google.api_core.gapic_v1.client_utils.

Exposes get_api_endpoint, get_default_mtls_endpoint, and get_universe_domain as public API helpers to be used by newly generated clients.

The unit tests added in test_client_utils.py are identical to the generator's original routing test cases in test_service.py.j2 (namely test__get_api_endpoint and test__get_universe_domain), modified only to call the public helpers directly:

@hebaalazzeh hebaalazzeh changed the title feat: move universe and endpoint routing logic to gapic_v1 public helpers chore(api_core): move universe and endpoint routing logic to gapic_v1 public helpers Jul 16, 2026
@hebaalazzeh
hebaalazzeh marked this pull request as ready for review July 16, 2026 19:17
@hebaalazzeh
hebaalazzeh requested a review from a team as a code owner July 16, 2026 19:17
@hebaalazzeh
hebaalazzeh marked this pull request as draft July 16, 2026 19:17

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new routing module to handle mTLS endpoint resolution and universe domain configuration, along with corresponding unit tests and a test fixture for mTLS environment isolation. It also includes minor refactoring in noxfile.py and test_bidi.py. The reviewer identified an issue with the regex-based endpoint conversion logic in routing.py and suggested a more robust string-based implementation to prevent potential security vulnerabilities.

Comment thread packages/google-api-core/google/api_core/gapic_v1/routing.py Outdated
@hebaalazzeh
hebaalazzeh marked this pull request as ready for review July 16, 2026 19:42
…public helpers

Introduces routing module containing get_api_endpoint, get_default_mtls_endpoint, and get_universe_domain helpers. Exposes the module as public in gapic_v1.
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-centralization-api-core-routing branch from 3e2d8e2 to 0fe63b5 Compare July 16, 2026 20:00
Comment thread packages/google-api-core/google/api_core/gapic_v1/routing.py Outdated
Comment thread packages/google-api-core/google/api_core/gapic_v1/routing.py Outdated
Comment thread packages/google-api-core/google/api_core/gapic_v1/client_utils.py
Comment thread packages/google-api-core/google/api_core/gapic_v1/routing.py Outdated
Comment thread packages/google-api-core/google/api_core/gapic_v1/routing.py Outdated
@hebaalazzeh hebaalazzeh self-assigned this Jul 16, 2026
@hebaalazzeh hebaalazzeh reopened this Jul 17, 2026
@hebaalazzeh

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new routing module in gapic_v1 to handle API endpoint and universe domain resolution, along with comprehensive unit tests. The feedback suggests improving get_default_mtls_endpoint by making domain suffix checks case-insensitive to correctly handle mixed-case or uppercase endpoints, and adding corresponding test cases to verify this behavior.

I am having trouble creating individual review comments. Click here to see my feedback.

packages/google-api-core/google/api_core/gapic_v1/routing.py (36-47)

medium

Domain names are case-insensitive. Performing case-sensitive checks like .endswith(".googleapis.com") can fail to recognize mixed-case or uppercase endpoints (e.g., foo.GoogleAPIs.com), resulting in a failure to resolve the correct mTLS endpoint. We should convert the endpoint to lowercase before performing suffix checks.

    if not api_endpoint or ".mtls." in api_endpoint.lower():
        return api_endpoint

    lowered_endpoint = api_endpoint.lower()
    if lowered_endpoint.endswith(".sandbox.googleapis.com"):
        # len(".sandbox.googleapis.com") == 23
        return api_endpoint[:-23] + ".mtls.sandbox.googleapis.com"

    if lowered_endpoint.endswith(".googleapis.com"):
        # len(".googleapis.com") == 15
        return api_endpoint[:-15] + ".mtls.googleapis.com"

    return api_endpoint

packages/google-api-core/tests/unit/gapic/test_routing.py (35-52)

medium

Add test cases to verify that get_default_mtls_endpoint correctly handles case-insensitive domain suffixes (e.g., foo.GoogleAPIs.com).

def test_get_default_mtls_endpoint():
    # Test valid API endpoints
    assert get_default_mtls_endpoint("foo.googleapis.com") == "foo.mtls.googleapis.com"
    assert (
        get_default_mtls_endpoint("foo.sandbox.googleapis.com")
        == "foo.mtls.sandbox.googleapis.com"
    )
    # Test case-insensitivity
    assert (
        get_default_mtls_endpoint("foo.GoogleAPIs.com")
        == "foo.mtls.googleapis.com"
    )
    assert (
        get_default_mtls_endpoint("foo.Sandbox.GoogleAPIs.com")
        == "foo.mtls.sandbox.googleapis.com"
    )

    # Test endpoints that shouldn't be converted
    assert (
        get_default_mtls_endpoint("foo.mtls.googleapis.com")
        == "foo.mtls.googleapis.com"
    )
    assert get_default_mtls_endpoint("foo.com") == "foo.com"

    # Test empty/None endpoints
    assert get_default_mtls_endpoint("") == ""
    assert get_default_mtls_endpoint(None) is None

@daniel-sanche daniel-sanche changed the title chore(api_core): move universe and endpoint routing logic to gapic_v1 public helpers feat(api_core): add universe and endpoint routing logic to gapic_v1 public helpers Jul 17, 2026
Comment thread packages/google-api-core/google/api_core/gapic_v1/client_utils.py Outdated
Comment thread packages/google-api-core/google/api_core/gapic_v1/client_utils.py
Comment thread packages/google-api-core/tests/unit/gapic/test_client_utils.py Outdated
Comment thread packages/google-api-core/google/api_core/gapic_v1/client_utils.py
Comment thread packages/google-api-core/google/api_core/gapic_v1/client_utils.py Outdated
Comment thread packages/google-api-core/google/api_core/gapic_v1/client_utils.py Outdated
@nbayati

nbayati commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

I noticed that the logic in get_api_endpoint for handling use_mtls_endpoint closely overlaps with the existing logic in google.auth.transport.mtls.should_use_mtls_endpoint().

To maintain a single source of truth for evaluating the mTLS endpoint configuration and compliance with AIP-4114, consider refactoring get_api_endpoint to call should_use_mtls_endpoint() directly.

This might also allow you to remove the use_mtls_endpoint argument entirely from get_api_endpoint (since should_use_mtls_endpoint handles reading the GOOGLE_API_USE_MTLS_ENDPOINT environment variable natively), which would streamline the method signature and reduce duplicate logic in the generated GAPIC clients.

@daniel-sanche

Copy link
Copy Markdown
Contributor

^ I gave similar feedback for read_enviornment_variables in the other PR. I didn't realize that code was copied in so many places

Comment thread packages/google-api-core/google/api_core/gapic_v1/client_utils.py Outdated
Comment thread packages/google-api-core/google/api_core/gapic_v1/client_utils.py Outdated
Comment thread packages/google-api-core/google/api_core/gapic_v1/client_utils.py Outdated
Comment thread packages/google-api-core/google/api_core/gapic_v1/client_utils.py Outdated
Comment thread packages/google-api-core/tests/unit/gapic/test_client_utils.py Outdated

@nbayati nbayati left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this refactoring! The mTLS logic looks solid. I have a few comments regarding some edge cases (specifically around is_proto3_optional and mock test coverage). I'll be OOO next week and I don't want to delay this PR, so there's no need to wait on my re-review. I'll defer to @daniel-sanche for the final review and sign-off!


1. packages/google-api-core/google/api_core/gapic_v1/requests.py (Line 68)
For proto-plus messages or other objects when is_proto3_optional=True, this fallback logic if not getattr(request, field_name, None): incorrectly overwrites an explicitly set empty string "" with a new UUID. The code should distinguish between missing fields and fields explicitly set to empty values, similar to how dictionaries are handled on line 55.

2. packages/google-api-core/tests/unit/gapic/test_requests.py (Line 30 and Line 47)
The __contains__ methods defined in MockRequest and MockValueErrorRequest are dead code. The setup_request_id function only uses the in operator for dict types, so these methods will never be executed.

3. packages/google-api-core/tests/unit/gapic/test_requests.py (Line 68)
There is a missing test case for is_proto3_optional=True with an explicit empty string for non-dict objects (e.g., (MockRequest(request_id=""), True, "")). Adding this would correctly expose the bug where setup_request_id overwrites explicitly set empty strings for proto-plus messages.

4. packages/google-api-core/tests/unit/gapic/test_requests.py (Line 113)
Using re.match allows trailing characters in the string because it only checks for a match at the beginning of the string. This should be changed to re.fullmatch(UUID_REGEX, value) (or the regex should be anchored with $) to guarantee strict UUID validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants