Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http, net/textproto: add a Header method to get multiple values []string, with canonicalized key #34799

Closed
trung opened this issue Oct 9, 2019 · 6 comments

Comments

@trung
Copy link
Contributor

commented Oct 9, 2019

In order to get multiple values from a header, we have to use a standard map expression. However, we need to be aware that keys are canonicalized (header["My-Key"]) otherwise header["my-key"] will not return any value. Or need to use textproto.CanonicalMIMEHeaderKey() explicitly.

header.Get(key) allows to get the first value and internally uses textproto.CanonicalMIMEHeaderKey to canonicalize the key before getting the value.

Is there a reason not to provide, something likeheader.GetValues(key), which returns []string?

@smasher164 smasher164 changed the title http/header: allow to get multiple values with canonicalized key net/textproto: allow to get multiple values with canonicalized key Oct 9, 2019
@smasher164 smasher164 changed the title net/textproto: allow to get multiple values with canonicalized key net/textproto: get multiple values with canonicalized key Oct 9, 2019
@smasher164

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

Both the http.Header and textproto.MIMEHeader types specify that "[t]o access multiple values of a key, or to use non-canonical keys, access the map directly." Also, given how simply this function can be written yourself, why is it necessary to include it in the standard library?

func GetValues(h textproto.MIMEHeader, key string) []string {
	if h == nil {
		return nil
	}
	return h[textproto.CanonicalMIMEHeaderKey(key)]
}
@trung

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

Thanks @smasher164 for the explanation.

When using http.Client to invoke an HTTP API endpoint, all keys in the http.Response.Header are canonicalized automatically although the API server returns a non-canonicalized header key.

I thought it would be useful to provide such method so developers don't have to worry about case sensitivity when providing header key to get multiple values just like getting a single value.

I'm cool with having a utility function to perform the same. Hope no other developers would hit the similar confusion as I did.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

I'd prefer to change the representation of net/http (and probably net/textproto) Header to not be a map and instead be a struct containing an internal map (along with other stuff), and enforce that we only store canonicalized keys.

That's part of #23707.

That said, a new method for now sounds fine. GetValues is a little weird in that the existing method Get isn't GetValue. GetAll? GetMulti? Just Values? (we prefer to avoid Get in general)

@andybons andybons added the NeedsFix label Oct 10, 2019
@andybons andybons added this to the Unplanned milestone Oct 10, 2019
@smasher164

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

I'm in favor of calling it Values, since it reads nicely when called as header.Values(key).

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

I like Values, but the downside is that it doesn't sort next to Get in the godoc. Not the end of the world, I suppose.

@odeke-em odeke-em changed the title net/textproto: get multiple values with canonicalized key net/http, net/textproto: add a Header method to get multiple values []string, with canonicalized key Oct 10, 2019
@gopherbot

This comment has been minimized.

Copy link

commented Oct 11, 2019

Change https://golang.org/cl/200760 mentions this issue: net/http, net/textproto: add a Header method to get multiple values []string, with canonicalized key

@gopherbot gopherbot closed this in 3972f97 Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.