julienschmidt / httprouter

A high performance HTTP request router that scales well
https://pkg.go.dev/github.com/julienschmidt/httprouter
BSD 3-Clause "New" or "Revised" License
16.64k stars 1.47k forks source link

OPTIONS returns "200 OK" even though there is no body #156

Open cd1 opened 8 years ago

cd1 commented 8 years ago

Isn't "204 No Content" a more appropriate return status for OPTIONS requests, as they don't return anything in their bodies?

AFAIU, "200 OK" should be used when the request is successful and there is some data to return, and "204 No Content" when the request is successful but there isn't data to return. "200 OK" is used currently because in https://github.com/julienschmidt/httprouter/blob/master/router.go#L382, no status is set.

When I send an OPTIONS request to my server, I get:

HTTP/1.1 200 OK
Allow: GET, HEAD, POST, OPTIONS
Date: Sat, 10 Sep 2016 23:08:40 GMT
Content-Length: 0
Content-Type: text/plain; charset=utf-8

IMO, even the Content-Type is misleading because there's no content at all.

I applied the following patch to my local code:

diff --git a/router.go b/router.go
index bb17330..bcc69bf 100644
--- a/router.go
+++ b/router.go
@@ -381,6 +381,7 @@ func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) {
                if r.HandleOPTIONS {
                        if allow := r.allowed(path, req.Method); len(allow) > 0 {
                                w.Header().Set("Allow", allow)
+                               w.WriteHeader(http.StatusNoContent)
                                return
                        }
                }

And it seemed to do the trick. If you agree with this, I can send a PR.

sagikazarmark commented 7 years ago

According to https://tools.ietf.org/html/rfc7231#section-6.3.1 200 status code is perfectly valid for an OPTIONS request. As far as I know 204 is usually used when there is some kind of content involved (in the request for example), but the response does not have any.

I don't see any improvement in semantics by returning 204 (and in the end this is a semantical question IMO), but I am not against it either. This could be a breaking change though.

cd1 commented 7 years ago

According to that same link, returning 200 OK for an OPTIONS request is valid only when it succeeded and it has data to return:

The payload sent in a 200 response depends on the request method. For the methods defined by this specification, the intended meaning of the payload can be summarized as: ... OPTIONS a representation of the communications options;

The server isn't returning any data for the OPTIONS request; the "communications options" are indicated via the response header Allow.

It's also more explicit in the following section:

Aside from responses to CONNECT, a 200 response always has a payload, though an origin server MAY generate a payload body of zero length. If no payload is desired, an origin server ought to send 204 (No Content) instead.

Returning 204 No Content isn't necessarily related to a request with some content; for example, DELETE /resource/1 is a typical example of a request which should return 204 No Content on success and it has no content in the request as well.

I opened this issue because I'm writing some test cases for a server and I'm validating every data returned by 200 OK responses; in the case of the OPTIONS response here, the status is 200 OK but there's no data to validate - which is exactly the purpose of the status 204 No Content, IMO.

According to the RFC, a server may generate a payload body of zero length for a 200 OK response, but it seems much more appropriate to use a status which indicates just that.

sagikazarmark commented 7 years ago

According to that same link, returning 200 OK for an OPTIONS request is valid only when it succeeded and it has data to return

Although that's what the spec suggests, I wouldn't be too strict on that one. That's an option you can choose. It recommends to use 204 when there is no content, but it's not a strict rule.

I would say relying on the status code to determine whether there is content is not really a good choice.

My particular issue with using multiple success status codes is that clients might not be ready to handle them all, this doing this change is definitely breaking. I would rather use 200 always instead of changing from 204 to 200 if necessary. (That of course does not apply to error status codes as a client MUST handle every non-success status and have some kind of fallback logic for unknown statuses)

plroebuck commented 6 years ago

I would say relying on the status code to determine whether there is content is not really a good choice.

Odd. RFC7230-section3.3.3 would seem to disagree with your assertion, concerning using 204.

My particular issue with using multiple success status codes is that clients might not be ready to handle them all, this doing this change is definitely breaking. I would rather use 200 always instead of changing from 204 to 200 if necessary. (That of course does not apply to error status codes as a client MUST handle every non-success status and have some kind of fallback logic for unknown statuses)

Although I'm certain someone hardcoded if status == 200, I disagree with the rest of your assertion. One would generally check for success (200<=status<300), correctable failure (400<=status<500), and uncorrectable failure (500<=status) to determine how to proceed.

Would kinda wonder about a portion of the logic in "router.go" though; wouldn't there always be an allow length? At minimum, wouldn't Allow: OPTIONS always be possible to qualify for a successful return?

sagikazarmark commented 6 years ago

Odd. RFC7230-section3.3.3 would seem to disagree with your assertion, concerning using 204.

I think I wasn't totally clear. I agree that 204 should mean there is actually no content. On the other hand, 200 with an empty body should be fine too. Maybe not for an OPTIONS request, but I think clients/servers with this level of RFC conformity is rare.

Ultimately my main argument is backwards compatibility which is no longer valid if we are talking about a new major version.

julienschmidt commented 5 years ago

Changing the status code is now possible via a global OPTIONS handler. See Automatic OPTIONS responses and CORS.

I will leave this issue open, since 204 should be considered as the default response code in v2.