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

Clarify case for keys in metadata.FromIncomingContext and metadata.FromOutgoingContext #4323

Closed
AberDerBart opened this issue Apr 6, 2021 · 3 comments · Fixed by #4416
Closed
Assignees
Labels
fixit P2 Type: Documentation Documentation or examples

Comments

@AberDerBart
Copy link

The documentation does not state, whether the keys in metadata.FromIncomingContext and metadata.FromOutgoingContext are converted to lowercase (as in metadata.New) or not.
As far as I can see, the keys are not converted to lowercase.
It would be helpful to have this information in the documentation, as the documentation on metadata.New, along with the implementations of Get and Append can lead to the assumption that the keys are always lowercase.

@easwars
Copy link
Contributor

easwars commented Apr 6, 2021

This is what I see:

  • We have two functions to create a metadata.MD. These are New() and Pairs(), and these convert the keys to lowercase, and this is documented.
  • We have a bunch of read-write methods on the metadata.MD type: Get(), Set() and Append() and these also convert the keys to lowercase, but this is not documented.
  • We have a couple of functions NewIncomingContext() and FromIncomingContext() to operate on incoming contexts. The former puts a metadata.MD into a context, while the latter gets a metadata.MD from a context. These do not modify the key in anyway. The user does not deal with keys here, they deal directly with metadata.MD. Therefore, nothing is documented here about changing to lower-case, and this is fine I believe.
  • We have some functions which operate on outgoing contexts.
    • AppendToOutgoingContext(): this clearly takes a bunch of key-value pairs and adds it to the metadata.MD in the context. But this does not convert the keys to lower-case.
    • FromOutgoingContextRaw() and FromOutgoingContext() retrieve the metadata.MD stored in the context.
      • The former does not modify the keys in anyway. But it's documentation says that callers should convert the keys to lower case. This is so because this is reserved for internal use, and the onus is put on the caller.
      • The latter does convert the keys to lower case, but it's documentation does not say so.

There are a few inconsistencies here that would be good to fix:

  • Document that keys will be converted to lower-case on methods Get(), Set() and Append() of metadata.MD
  • AppendToOutgoingContext() should document that keys are not modified.
    • Is there any specific reason the current implementation does not modify the keys to lower case here? We could possibly avoid some confusion if we did so. @dfawley
  • FromOutgoingContext() should document that the keys returned are converted to lower case.

@easwars easwars added the fixit label Apr 8, 2021
@AberDerBart
Copy link
Author

My main concern is, when I get a context from a grpc server and use FromIncomingContext() to access the metadata, are all keys lowercase? If so, the documentation should state this (as it the most frequent use case I assume) - otherwise I would have to convert all keys to lowercase again in order to properly access them.

As MD is a map[string][]string, it is still possible to assign values to non-lowercase keys like this:

md["FOO"] = []string{"bar"}

@easwars easwars self-assigned this Apr 12, 2021
@dfawley
Copy link
Member

dfawley commented Apr 12, 2021

My main concern is, when I get a context from a grpc server and use FromIncomingContext() to access the metadata, are all keys lowercase?

Yes, all incoming keys will be lowercase. Per the HTTP/2 spec, https://httpwg.org/specs/rfc7540.html#rfc.section.8.1.2:

header field names MUST be converted to lowercase prior to their encoding in HTTP/2.

@easwars easwars added the P2 label Apr 13, 2021
@dfawley dfawley added Type: Documentation Documentation or examples and removed Type: Question labels May 4, 2021
@menghanl menghanl self-assigned this May 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fixit P2 Type: Documentation Documentation or examples
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants