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.

Sign up for GitHub

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

Jump to bottom

proposal: net/http: add helper funcs for HTTP status codes #29652

Closed
vkuzmin-uber opened this issue Jan 10, 2019 · 13 comments
Closed

proposal: net/http: add helper funcs for HTTP status codes #29652

vkuzmin-uber opened this issue Jan 10, 2019 · 13 comments
Labels
FrozenDueToAge Proposal
Milestone
Proposal

Comments

@vkuzmin-uber
Copy link
Contributor

What version of Go are you using (go version)?

go version go1.11.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

darwin, OS X

What did you do?

I needed functions to check if http StatusCode is in the following ranges https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml

1xx: Informational - Request received, continuing process
2xx: Success - The action was successfully received, understood, and accepted
3xx: Redirection - Further action must be taken in order to complete the request
4xx: Client Error - The request contains bad syntax or cannot be fulfilled
5xx: Server Error - The server failed to fulfill an apparently valid request

This is pretty common task when Server may return StatusOK, StatusCreated and other 2xx. Usually in this case developers has several options:

  • check available Status* at http package (cons: it may miss other 2xx if custom server implemented it)
  • check 'if code>=200 && code<=299' (cons: magic numbers)
  • check 'if code / 100 == 2' (cons: magic numbers and expression that is not obvious)

What did you expect to see?

I'd expect that http package provides appropriate interface/methods:

package http
....
func IsInformational(code int) bool
func IsSuccess(code int) bool
func IsRedirection(code int) bool
func IsClientError(code int) bool
func IsServerError(code int) bool

Such methods, regardless of the way they are implemented, provides a standard way to do such check and so developers don't need to spend time on implementation, review and discussion of better way. (I am talking now about my own experience)

I also considered having interface for http.Response but this is not flexible, because in some cases I would like to check status code without having Response object.

What did you see instead?

It has only 1 helper:

func StatusText(code int) string
@vkuzmin-uber
Copy link
Contributor Author

I have a POC and test that I want to publish to review.

@gopherbot
Copy link

Change https://golang.org/cl/157337 mentions this issue: net/http: Status Codes range checks

@vkuzmin-uber vkuzmin-uber changed the title net/http: Status Codes need range checks to make interface complete proposal: net/http: Status Codes need range checks to make interface complete Jan 10, 2019
@gopherbot gopherbot added this to the Proposal milestone Jan 10, 2019
@bradfitz
Copy link
Contributor

Sorry, I think this is too many top-level functions to add to net/http.

I do want to add this later (like I did in https://godoc.org/inet.af/http#Status) but I don't think we should add this to today's net/http.

@vkuzmin-uber
Copy link
Contributor Author

What if leave IsSuccess only? The real life problem is that there are several options:

  1. if code == http.StatusOK || code == http.StatusCerated...
  2. if code >= http.StatusOK && code <=http.StatusOK+99
  3. if code >= 200 && code <=299
  4. if code /100 == 2
  5. if code/100 == http.StatusOK/100

2-5 introduce magic numbers that looks weird consider we have http constants. It forces dev to have local helper, covered with Unit Test.

IsSuccess would be enough for real life tasks.

@ainar-g
Copy link
Contributor

ainar-g commented Jan 10, 2019

As a random thought, net/http/httputil, maybe?

Package httputil provides HTTP utility functions, complementing the more common ones in the net/http package.

@vkuzmin-uber
Copy link
Contributor Author

As a random thought, net/http/httputil, maybe?

Package httputil provides HTTP utility functions, complementing the more common ones in the net/http package.

Well, as an option, we can put it into net/http/httputil/status.go or so. @bradfitz what do you think?

@vkuzmin-uber
Copy link
Contributor Author

Sorry, I think this is too many top-level functions to add to net/http.

I do want to add this later (like I did in https://godoc.org/inet.af/http#Status) but I don't think we should add this to today's net/http.

Making it a part of Status interface almost the same as making it a part of http.Reponse. When I have "int" code, I don't want to allocate Status object first. This is an aexample when IsSuccess, Is... need to be external helpers that covers all cases.

@bradfitz
Copy link
Contributor

When I have "int" code, I don't want to allocate Status object first.

There's no allocation involved. It's also not an interface.

@vkuzmin-uber
Copy link
Contributor Author

vkuzmin-uber commented Jan 10, 2019 •
edited

When I have "int" code, I don't want to allocate Status object first.

There's no allocation involved. It's also not an interface.

Hmm, I mean case:

Foo(code int) bool {
   status := NewStatus(code, "something")
   return  status.IsClientError()
}

there will be always some case, when code is just int (read from file and etc)

@bradfitz
Copy link
Contributor

That still doesn't allocate.

In any case, I don't want to discuss the v2 http packages in this bug.

I say no to adding more helpers to net/http. I'm on the fence for net/http/httputil. I'm fine with it if others in the @golang/proposal-review group are.

@bradfitz bradfitz changed the title proposal: net/http: Status Codes need range checks to make interface complete proposal: net/http: add helper funcs for HTTP status code Jan 11, 2019
@bradfitz bradfitz changed the title proposal: net/http: add helper funcs for HTTP status code proposal: net/http: add helper funcs for HTTP status codes Jan 11, 2019
@vkuzmin-uber
Copy link
Contributor Author

Updated review. Now helpers are part of net/http/httputil as @ainar-g suggested.

Some unrelated question: C++ had/has BOOST library that had many libs that could not be a part of STL but were generic and useful. Many of them became a part of standard later. Anyway, many companies/developers used BOOST with all appropriate contracts/risks. If you know, what is Go's team/community vision on having Go's "boost" ?

@bradfitz
Copy link
Contributor

Your unrelated question is better suited to a mailing list probably. I'd rather not distract this issue.

@rsc
Copy link
Contributor

rsc commented Jan 16, 2019

Let's hold off on adding anything new in net/http and net/http/httputil until we know where we want to end up with a "V2" of net/http. If we're going to add stuff here, we should only add things that will make a transition to an eventual v2 smoother. (See #5465 and #23707.)

@rsc rsc closed this as completed Jan 16, 2019
@kzys kzys mentioned this issue Aug 30, 2019
Closed
@golang golang locked and limited conversation to collaborators Jan 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
5 participants
@bradfitz @rsc @ainar-g @gopherbot @vkuzmin-uber