chore(api_core): move universe and endpoint routing logic to gapic_v1 public helpers#17745
chore(api_core): move universe and endpoint routing logic to gapic_v1 public helpers#17745hebaalazzeh wants to merge 5 commits into
Conversation
hebaalazzeh
changed the title
![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 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.
…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.
3e2d8e2 to
0fe63b5
Compare
| # limitations under the License. | ||
| # | ||
|
|
||
| """Helpers for routing and endpoint resolution.""" |
There was a problem hiding this comment.
In #17738, you added a new request.py file. In this PR, you added a routing.py. Is there a way we can group these better, to avoid adding too many new files?
| default_mtls_endpoint: Optional[str], | ||
| default_endpoint_template: Optional[str], | ||
| ) -> Optional[str]: | ||
| """Return the API endpoint used by the client.""" |
There was a problem hiding this comment.
Can you add better docstrings, now that these are public?
There was a problem hiding this comment.
Added detailed, standardized docstrings for the public helpers get_api_endpoint, get_universe_domain, and get_default_mtls_endpoint documenting all parameters, return values, and exceptions.
|
|
||
| Convert "*.sandbox.googleapis.com" and "*.googleapis.com" to | ||
| "*.mtls.sandbox.googleapis.com" and "*.mtls.googleapis.com" respectively. | ||
| Args: |
There was a problem hiding this comment.
IIRC I think you need a blank line above the Args section
There was a problem hiding this comment.
Done. Added blank line and documented the arguments.
| default_universe: str, | ||
| ) -> str: | ||
| """Return the universe domain used by the client.""" | ||
| universe_domain = default_universe |
There was a problem hiding this comment.
why rename the variable?
There was a problem hiding this comment.
Removed the unnecessary variable renaming (universe_domain = default_universe) and simplified the default fallback assignments.
| use_mtls_endpoint: str, | ||
| default_universe: str, | ||
| default_mtls_endpoint: Optional[str], | ||
| default_endpoint_template: Optional[str], |
There was a problem hiding this comment.
These types don't seem to agree with the source. Why is that? This could lead to mypy issues
There was a problem hiding this comment.
fixed type annotations (such as making default_endpoint_template a required str and the return type of get_api_endpoint a non-optional str) to ensure complete alignment with usages and prevent any mypy type check failures.
2 participants
Centralizes generic routing, mTLS, and universe domain resolution helpers under
google.api_core.gapic_v1.routing.Exposes
get_api_endpoint,get_default_mtls_endpoint, andget_universe_domainas public API helpers to be used by newly generated clients.The unit tests added in test_routing.py are identical to the generator's original routing test cases in test_service.py.j2 (namely
test__get_api_endpointandtest__get_universe_domain), modified only to call the public helpers directly:test__get_api_endpointmaps to the newget_api_endpointhelper.test__get_universe_domainmaps to the newget_universe_domainhelper.