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

proposal: net/http: add constant for content type #31572

Closed
tomocy opened this issue Apr 19, 2019 · 10 comments
Closed

proposal: net/http: add constant for content type #31572

tomocy opened this issue Apr 19, 2019 · 10 comments

Comments

@tomocy
Copy link
Contributor

tomocy commented Apr 19, 2019

Is it a good idea to add constants for content type of HTTP body like http.MethodPost in HTTP methods?

It might be

const (
    ContentTypeFormURLEncoded = "application/x-www-form-urlencoded"
)

This changes will help us not typo, and get support from editors.

@gopherbot gopherbot added this to the Proposal milestone Apr 19, 2019
@rsc rsc changed the title proposal: net/http add constant for content type proposal: net/http: add constant for content type Apr 24, 2019
@rsc
Copy link
Contributor

rsc commented Apr 24, 2019

"ContentTypeForm" would be a shorter, better name. Should there be a few of the most common ones here?

const (
    ContentTypeBinary = "application/octet-stream"
    ContentTypeForm = "application/x-www-form-urlencoded"
    ContentTypeJSON = "application/json"
    ContentTypeHTML = "text/html; charset=utf-8"
    ContentTypeText = "text/plain; charset=utf-8"
)

It would be unfortunate if this leads to more requests to add more, like MethodPost etc have.
It would also be unfortunate if people start writing r.FormValue("Content-Type") == ContentTypeText, since that kind of comparison still needs to parse the string.

Thoughts?

@cespare
Copy link
Contributor

cespare commented Apr 24, 2019

It would also be unfortunate if people start writing r.FormValue("Content-Type") == ContentTypeText, since that kind of comparison still needs to parse the string.

This is already an all-too-common mistake and I think that adding the constants would, if anything, make it a little worse.

Before, the issue with code like

r.Header.Get("Content-Type") == "text/plain"

was that the left side needed to be parsed to get the media type value out. With

r.Header.Get("Content-Type") == http.ContentTypeText

both sides need to be parsed. Or worse, with

r.Header.Get("Content-Type") == http.ContentTypeJSON

it happens to work now, but depends on ContentTypeJSON being a media type value without optional parameters.

Because these constants can only be effectively used for setting the Content-Type and not retrieving it, I don't think we should add them. If we do add API for common Content-Types, I think it ought to be opaque types that can be used for both setting and retrieving.

@bradfitz Have you thought about Content-Type usability? I don't see any references to Content-Type in the discussion around http(client) v2.

@bradfitz
Copy link
Contributor

I'd like to see a new (string-backed) type for media types. Then we could do:

r.SetContentType(http.TypeForm)

And because http.ContentTypeForm would be a typed constant, you couldn't write r.Header.Get("Content-Type") == http.ContentTypeJSON and get bitten by mismatched parameters.

A nice place for this type might be the mime package, but https://golang.org/pkg/mime/#ParseMediaType already exists and just returns a string, which is unfortunate.

I'd rather not duplicate it with a newer version in net/http.

@rsc
Copy link
Contributor

rsc commented Aug 13, 2019

@bradfitz suggested something quite different from the original proposal, and no one has replied. It sounds like we should decline this proposal as too little impact / not clear enough solution.

Will leave this open for a week for any final comments; if not, will decline.

@cespare
Copy link
Contributor

cespare commented Aug 13, 2019

I think @bradfitz's proposal would be an improvement over the status quo, and I would use it in a bunch of places.

However, I'd still prefer an API which helps with both common content-type operations: setting a response's content-type and checking a request's content-type. The ideas in this issue only address the former.

That should probably be a different proposal, though, if someone comes up with a concrete suggestion.

@rsc
Copy link
Contributor

rsc commented Aug 20, 2019

Marked this last week as likely decline w/ call for last comments (#31572 (comment)).
Declining now.

@rsc rsc closed this as completed Aug 20, 2019
@ghostsquad
Copy link

Sorry to post on a dead proposal, but I'd like understand more about why this is potentially an issue:

r.Header.Get("Content-Type") == http.ContentTypeText

@cespare mentioned that the right hand side has to be parsed. How is a const string different from a string literal in this regard?

@cespare
Copy link
Contributor

cespare commented May 31, 2020

@ghostsquad it's not different in that regard. Whether the RHS is using a constant or a string literal, that code is a little broken.

@ghostsquad
Copy link

what's broken about it?

@bradfitz
Copy link
Contributor

bradfitz commented Jun 1, 2020

what's broken about it?

"text/plain" != "text/plain; charset=utf-8"

@golang golang locked and limited conversation to collaborators Jun 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants