fix: deduplicate x-goog-api-client headers - #17616
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 introduces changes to handle and merge duplicate x-goog-api-client metadata headers in google-api-core and allows suppressing these metrics headers in google-auth's AuthMetadataPlugin. Feedback on the changes includes a recommendation to copy self._arbitrary_metadata to prevent shared state mutation, and a suggestion to simplify the _extract_metrics_header function into a single-pass loop for better readability and compatibility with general iterables.
| default_host=default_host, | ||
| suppress_metrics_header=True, | ||
| ) | ||
| except TypeError: |
There was a problem hiding this comment.
Just double checking it is intended/desirable to catch/handle ALL TypeErrors that could be raised?
There was a problem hiding this comment.
TypeError is what Python raises when an invalid kwarg argument is passed, so this is as specific as we can be here. If something else is causing the error, we would expect to see it on the next line
| Given one or more metadata payload strings, create a combined | ||
| string with deduplicated tokens, while preserving token order. | ||
|
|
||
| Inputs are expected contain a set of metadata tokens separated by spaces |
There was a problem hiding this comment.
nit: "expected to contain" (missing "to").
| return func | ||
|
|
||
|
|
||
| def _deduplicate_metadata_tokens(*headers: str) -> str: |
There was a problem hiding this comment.
Should this be "*headers: Optional[str]" since it seems like None is handled
| Returns: | ||
| A tuple containing: | ||
| - A sequence of remaining metadata tuples. | ||
| - a string representing the header value. |
There was a problem hiding this comment.
This return type tuple doc string seems to be reversed from the actual return tuple which is the string first and then the metadata
There was a problem hiding this comment.
Good catch, fixed
daniel-sanche
changed the title
chalmerlowe
left a comment
chalmerlowe
left a comment
There was a problem hiding this comment.
BLOCKER: minor spelling error.
Otherwise LGTM.
Approved.
| Given one or more metadata payload strings, create a combined | ||
| string with deduplicated tokens, while preserving token order. | ||
|
|
||
| Inputs are expecte to contain a set of metadata tokens separated by spaces |
There was a problem hiding this comment.
| Inputs are expecte to contain a set of metadata tokens separated by spaces | |
| Inputs are expected to contain a set of metadata tokens separated by spaces |
4 participants
We currently populate the x-goog-api-client header multiple times, in different places in the stack (api-core, google-auth, user-provided, etc). While some backend systems are able to handle the duplicate headers, it can cause issues for others
This PR explores a potential fix:
b/477429588