fix(ConfigurableMessageHandler): Fix quota header on retry#3151
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 modifies the ConfigurableMessageHandler to ensure that the quota project header is removed from the request before a retry is attempted. This allows the header to be re-applied fresh for each retry attempt. There are no review comments to evaluate, and I have no additional feedback to provide.
robertvoinescu-work
force-pushed
the
fix/configurableMessageHandler
branch
4 times, most recently
from
7cf1f4c to
afd6c72
Compare
robertvoinescu-work
changed the title
robertvoinescu-work
force-pushed
the
fix/configurableMessageHandler
branch
5 times, most recently
from
bb4ba17 to
c699f2d
Compare
robertvoinescu-work
changed the title
robertvoinescu-work
force-pushed
the
fix/configurableMessageHandler
branch
from
c699f2d to
96ad649
Compare
amanda-tarafa
left a comment
amanda-tarafa
left a comment
There was a problem hiding this comment.
I have an alternative proposal that I believe it's both flexible and future proof.
Also, we don't go to the level of the type on the commit name prefixes. Here is fine to say just fix: ... without more granularity.
robertvoinescu-work
force-pushed
the
fix/configurableMessageHandler
branch
2 times, most recently
from
60f0954 to
c794d4d
Compare
amanda-tarafa
left a comment
amanda-tarafa
left a comment
There was a problem hiding this comment.
The existing test for this is now failing, which is right. I think we need to add more tests for AccessTOkenWithHeaders and can remove the failing test for the request.
| requestHeaders.Add(header.Key, header.Value); | ||
| // In the case it's a single value header we will not add it if already present, just validate we match | ||
| // what's already there. | ||
| if (IsSingleValueHeader(header.Key) && requestHeaders.TryGetValues(header.Key, out var existingValue)) |
There was a problem hiding this comment.
tiny nit:
| if (IsSingleValueHeader(header.Key) && requestHeaders.TryGetValues(header.Key, out var existingValue)) | |
| if (IsSingleValueHeader(header.Key) && requestHeaders.TryGetValues(header.Key, out var existingValues)) |
|
|
||
| void ValidateMatch(string key, IEnumerable<string> existing, IEnumerable<string> incoming) | ||
| { | ||
| if (!existing.SequenceEqual(incoming)) |
There was a problem hiding this comment.
As this check is being done for single value headers only I think we can skip the part where we compare collections. If either side has more than one value, we can throw. And if both sides have a single value each we only throw if those values are different.
The exception message needs to read something like "Only a single value may be specified for {key}".
robertvoinescu-work
force-pushed
the
fix/configurableMessageHandler
branch
3 times, most recently
from
604cca1 to
b751429
Compare
robertvoinescu-work
force-pushed
the
fix/configurableMessageHandler
branch
from
b751429 to
b3df97f
Compare
2 participants
b/414979515