httprequest icon indicating copy to clipboard operation
httprequest copied to clipboard

HTTP 200 status not being set for one handler signature pattern

Open alnvdl opened this issue 3 years ago • 2 comments

In order to match the behavior of handler signature case 2: https://github.com/juju/httprequest/blob/77d36ac4b71a6095506c0617d5881846478558cb/handler.go#L418-L428

Wouldn't it make sense to also set the status to http.StatusOK in case 1? https://github.com/juju/httprequest/blob/77d36ac4b71a6095506c0617d5881846478558cb/handler.go#L413-L416

I know that ultimately it doesn't matter, as 200 would get set anyway, but I found this when logging via a middleware that was logging status as 0 at this point (but just for endpoints with return a single error value).

alnvdl avatar Mar 03 '21 21:03 alnvdl

The expected use case for the single return value functions is that they will generally, or at least potentially, be writing directly to the ResponseWriter. In which case writing our own header is wrong, the go documentation for a ResponseWriter (https://golang.org/pkg/net/http/#ResponseWriter) states that "Only one header may be written".

To be able to write a header we would need to detect if the function wrote one first. This could be done by wrapping the given ResponseWriter in a new one which intercepted the WriteHeader call and logged whether it had been run. This however brings a new problem. ResponseWriters are often used to smuggle other interfaces (c.f. https://golang.org/pkg/net/http/#Hijacker) we would rather not elide those interfaces, but there is no reasonable way to try and preserve them only when they are present.

Also it means we don't have to have a robust discussion about whether to use "200 OK" or "204 No Content".

mhilton avatar Mar 04 '21 09:03 mhilton

The problem/solution you described involving RequestWriter and header handling is the same as used by Go Chi: https://github.com/go-chi/chi/blob/master/middleware/wrap_writer.go

Indeed, this can be a bit awkward to justify, especially considering the 200/204 distinction...

alnvdl avatar Mar 04 '21 13:03 alnvdl