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 helper funcs for HTTP status codes #29652

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

Comments

Projects
None yet
5 participants
@vkuzmin-uber
Copy link
Contributor

commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

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

@gopherbot

This comment has been minimized.

Copy link

commented Jan 10, 2019

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

@gopherbot gopherbot added the Proposal label Jan 10, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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.

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

@vkuzmin-uber

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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

@rsc

This comment has been minimized.

Copy link
Contributor

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 Jan 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.