-
Notifications
You must be signed in to change notification settings - Fork 538
Bugfix v2: Return proper error content type. #1516
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
Conversation
| buf, err := json.Marshal(sr) | ||
| if err != nil { | ||
| logger.Errorw("error sending response", "error", err) | ||
| setContentType(w, r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some ideas, but instead of setContentType could we simplify by changing sendJSON and sendPlain to take statusCode int and then set the content type inside each one. Maybe we could also change sendJSON to take an interface{} instead and then use that both in v1 and v2.
Then, instead of having a sendStatus in common_handlers.go which is only used in v1 (AFAICT), we could perhaps create one that we can use from both v1 and v2:
func sendResponse(w http.ResponseWriter, r *http.Request, statusCode int, response interface{}) {
if acceptsJSON(r) {
sendJSON(w, r, statusCode, response)
return
}
sendPlain(w, r, statusCode, response)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of differences between v1 and v2 (in v2 Content-Type is only set on errors, counter for errors is increased in different places, the returned error messages are not the same). So I could align that and change some behaviour in v1, but I wasn't aiming for any change in v1, especially not within a bugfix.
I think we should refactor that when we remove v1, but if you feel strongly about it I can come up with something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure @simitt 👍
fixes #1514
V2 Endpoints now return proper content-type and content in proper format. On Success no body is returned so no content-type is set. On Failure content type is set to
application/jsonif the Accept header specifically requests json or it allows any type. Otherwise it is set totext/plain; charset=utf-8.Details to the bugfix: The
WriteHeaderneeds to be called after all other headers have been set, but before the actualWritemethod is called. According to the WriteHeader docs there are two points to consider: