feat(generator): gapic generator centralization mtls#17818
Conversation
![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 refactors mTLS configuration and client certificate source logic into a new compatibility template _compat.py.j2 to support older versions of google-api-core, updating client.py.j2 and unit tests accordingly. Feedback suggests using the imported mtls module directly in the test template's hasattr check to avoid a potential NameError, and notes that the generated golden files should be regenerated to ensure they are fully in sync with the updated templates.
1d81ba9 to
3cc6389
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a compatibility module (_compat.py.j2) to support older versions of google-api-core in generated GAPIC clients, refactoring several helper functions out of the main client template and adding corresponding unit tests. Feedback on these changes highlights a backwards-compatibility issue in the fallback implementation of get_client_cert_source where an exception is raised instead of returning None, as well as a minor type mismatch in a test assertion where a string is passed instead of a boolean.
| elif ( | ||
| hasattr(mtls, "has_default_client_cert_source") | ||
| and mtls.has_default_client_cert_source() | ||
| ): | ||
| client_cert_source = mtls.default_client_cert_source() | ||
| else: | ||
| raise ValueError( | ||
| "Client certificate is required for mTLS, but no client certificate source was provided or found." | ||
| ) |
There was a problem hiding this comment.
The fallback implementation of get_client_cert_source raises a ValueError when no client certificate source is found, whereas the historical implementation in client.py.j2 gracefully returned None. To prevent breaking changes for downstream users and maintain backwards compatibility, we should preserve the historical fallback behavior by returning None instead of raising an exception.
elif (
hasattr(mtls, "has_default_client_cert_source")
and mtls.has_default_client_cert_source()
):
client_cert_source = mtls.default_client_cert_source()
References
- Do not replace historical graceful fallback behaviors (such as returning False/falling back to standard TLS) with exceptions if doing so would introduce breaking changes for downstream users and violate backwards compatibility.
| with mock.patch('google.auth.transport.mtls.has_default_client_cert_source', return_value=True): | ||
| with mock.patch('google.auth.transport.mtls.default_client_cert_source', return_value=mock_default_cert_source): | ||
| assert {{ service.client_name }}._get_client_cert_source(None, True) is mock_default_cert_source | ||
| assert {{ service.client_name }}._get_client_cert_source(mock_provided_cert_source, "true") is mock_provided_cert_source |
There was a problem hiding this comment.
The second argument to _get_client_cert_source is annotated as a boolean (use_cert_flag: bool). Passing the string "true" instead of the boolean True is a type mismatch and can cause type-checking failures in downstream libraries. Please use the boolean literal True instead.
assert {{ service.client_name }}._get_client_cert_source(mock_provided_cert_source, True) is mock_provided_cert_source
1 participant
This PR updates the generator templates to rely on the centralized MTLS fallback logic from
google-api-core.Since the MTLS helpers (
use_client_cert_effective,get_client_cert_source,read_environment_variables) are being centralized intogoogle/api_core/gapic_v1/config.pyin the linkedapi-corePR, we no longer need the generator to produce fallback implementations for them. The generated_compat.pywill now import these directly fromconfig.py.Depends on the
api-coreMTLS centralization PR.related pr: #17817