client: WithHTTPHeaders: improve doc, add test coverage, and error on duplicate headers#52204
Conversation
thaJeztah
added
status/2-code-review
kind/refactor
| c.customHTTPHeaders = make(map[string]string) | ||
| for k, v := range headers { | ||
| k = http.CanonicalHeaderKey(k) | ||
| _, ok := c.customHTTPHeaders[k] | ||
| if ok { | ||
| // TODO(thaJeztah): consider returning an error on duplicates | ||
| continue | ||
| } | ||
| c.customHTTPHeaders[k] = v | ||
| } |
There was a problem hiding this comment.
This is preserving the current behavior, but removing duplicates early; before this patch, duplicates would be silently discarded when we set the headers (headers.Set normalises the header-name, so overwriting earlier values);
Lines 310 to 314 in 086a6eb
thaJeztah
force-pushed
the
stable_customheaders
branch
from
d733957 to
61e54bb
Compare
| k = http.CanonicalHeaderKey(k) | ||
| _, ok := c.customHTTPHeaders[k] | ||
| if ok { | ||
| // TODO(thaJeztah): consider returning an error on duplicates |
There was a problem hiding this comment.
So this doesn't change the current behavior and they are still discarded silently right?
So
Accept: application/json
Accept: text/plain
would silently still produce unexpected results.
I think we should just bite the bullet and return an error in this PR already.
There was a problem hiding this comment.
Yeah, didn't want to do it if it's a patch release; can do a quick follow-up as well to make it more visible
There was a problem hiding this comment.
do your example; it would be random when using mixed casing; ACCEPT and Accept (eg) in the same map provided here. Probably very corner case if someone would set that
There was a problem hiding this comment.
Updated; added an error if duplicates are detected.
The behavior of this option was under-documented, and there's an issue with duplicate headers leading to unexpected behavior; changing that is probably not suitable for a patch release, so let's start with adding test-coverage, and adding a TODO for a future release. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Previously, WithHTTPHeaders would silently discard duplicate headers (after normalizing). Produce an error instead to prevent unexpected behavior. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
stable_customheaders
branch
from
61e54bb to
28aeae2
Compare
thaJeztah
added
impact/changelog
area/go-sdk
impact/go-sdk
thaJeztah
changed the title
2 participants
The behavior of this option was under-documented, and there's an issue with duplicate headers leading to unexpected behavior; changing that is probably not suitable for a patch release, so let's start with adding test-coverage, and adding a TODO for a future release.
- What I did
- How I did it
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)