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: Elaborate requirement for header key format #53140

Open
minherz opened this issue May 30, 2022 · 9 comments
Open

net/http: Elaborate requirement for header key format #53140

minherz opened this issue May 30, 2022 · 9 comments
Labels
Documentation NeedsFix
Milestone

Comments

@minherz
Copy link

@minherz minherz commented May 30, 2022

What is the URL of the page with the issue?

https://pkg.go.dev/net/http

What is your user agent?

Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/102.0.5005.61 Safari/537.36

What did you expect to see?

An easy way to find information that helps troubleshooting cases when a function Get of the Header type returns nil instead of the header's value.
It happens when the Request object is initialized like in the following example:

&http.Request{
    URL:    "http://acme.com/index",
    Header: http.Header{"x-cloud-trace-context": {"105445aa7843bc8bf206b12000100012/000012345;o=1"}},
}

In this case the header's key does not get converted into the cannonical format and, in result, executing the following code prints <nil>:

req := &http.Request{
    URL:    "http://acme.com/index",
    Header: http.Header{"x-cloud-trace-context": {"105445aa7843bc8bf206b12000100012/000012345;o=1"}},
}
fmt.Println(req.Header.Get(x-cloud-trace-context")

What did you see instead?

The current documentation about Get() function of the Header does not clearly elaborates that if the header's key is stored not in the canonical format then it will not be returned.

@gopherbot gopherbot added this to the Unreleased milestone May 30, 2022
@jimen0
Copy link

@jimen0 jimen0 commented May 30, 2022

The docs of http.Header say:

The keys should be in canonical form, as returned by CanonicalHeaderKey.

In Get it says:

To use non-canonical keys, access the map directly.

And in Set it states you can access the map directly if you want to use non-canonical keys.

To use non-canonical keys, assign to the map directly.

This seems to be working as documented and the documentation seems to be there.

@seankhliao seankhliao changed the title x/pkgsite: Elaborate requirement for header key format net/http: Elaborate requirement for header key format May 30, 2022
@seankhliao seankhliao removed this from the Unreleased milestone May 30, 2022
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented May 30, 2022

See above, seems clear enough.

@minherz
Copy link
Author

@minherz minherz commented May 30, 2022

@seankhliao and @jimen0 thank you for your points. There are a couple of things that should be considered here:

  1. The aforementioned sentences are distributed in the documentation and aren't easy to find and correlate. As someone who tried to figure out why req.Header.Get() returns empty string for a couple of hours, I can attest to it.
  2. The Get() documentation states:

    If there are no values associated with the key, Get returns "". It is case insensitive; textproto.CanonicalMIMEHeaderKey is used to canonicalize the provided key.
    While one can argue that these sentences explain the requirement, I can testify that it is not clear. It is possible to read it as empty string is returned if there is no values associated with the key which is case insensitive. This is what happened to me and few my colleagues (in Yoshi team in Google) whom I asked for assistance.

  3. The behavior of Set method and the http server implementation which implicitly convert the header's keys to the canonical format and do not require extra special handling make it hard to understand that when you create a http.Request object with an explicit initialization of the Header field one has to take care of this conversion by themselves.

I do not say that the documentation does not have this information. What I say is that the current documentation is hard to use in the described situation when the valid behavior of the package is hard to explain or troubleshoot.

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented May 30, 2022

Any specific wording you would like to suggest?

The 2 places I would look, Header and Header.Get have:

The keys should be in canonical form, as returned by CanonicalHeaderKey.

...; textproto.CanonicalMIMEHeaderKey is used to canonicalize the provided key

@minherz
Copy link
Author

@minherz minherz commented May 31, 2022

@seankhliao I would suggest changing Header.Get doc to something like:

If there are no values associated with the key, Get returns "". It is case insensitive; textproto.CanonicalMIMEHeaderKey is used to canonicalize the provided key. Get assumes that all keys are stored in canonical form. To use non-canonical keys, access the map directly.

This way the emphasis about the fact that Header still may has keys in the non-canonical form is easily found. WDYT?

@seankhliao seankhliao added the NeedsInvestigation label May 31, 2022
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented May 31, 2022

cc @neild

@seankhliao seankhliao reopened this May 31, 2022
@neild
Copy link
Contributor

@neild neild commented May 31, 2022

Adding "Get assumes that all keys are stored in canonical form" sounds fine to me.

(Very small style nitpick: "stored in canonical form" matches existing usage in net/http a bit better than "the canonical form".)

@dmitshur dmitshur added NeedsFix and removed NeedsInvestigation labels May 31, 2022
@dmitshur dmitshur added this to the Backlog milestone May 31, 2022
@minherz
Copy link
Author

@minherz minherz commented Jun 2, 2022

I would love to submit the fix if it is OK. I just have a problem with doing it via Gerrit. So, I will probably do it in the core repo.

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Jun 3, 2022

@minherz Thanks. You should be able to send a GitHub PR if you prefer. Please see https://go.dev/doc/contribute#sending_a_change_github for more information on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsFix
Projects
None yet
Development

No branches or pull requests

6 participants