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 · 6 comments

Comments

@tomocy
Copy link
Contributor

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

@gopherbot gopherbot added the Proposal label 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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented May 14, 2019

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

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

@rsc rsc closed this Aug 20, 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.