send back HTTP 400 where there is a client error #1131
Conversation
|
|
server/api/util_test.go
Outdated
|
|
||
| type testUtilSuite struct{} | ||
|
|
||
| type nopCloser struct { |
There was a problem hiding this comment.
I think you can use ioutil.NopCloser.
| @@ -0,0 +1,73 @@ | |||
| // Copyright 2016 PingCAP, Inc. | |||
server/api/util.go
Outdated
| r.JSON(w, http.StatusBadRequest, err.Error()) | ||
| default: | ||
| r.JSON(w, http.StatusInternalServerError, err.Error()) | ||
|
|
There was a problem hiding this comment.
Please remove the extra empty line.
server/api/util.go
Outdated
| ) | ||
|
|
||
| func jsonRespondError(r *render.Render, w http.ResponseWriter, body io.ReadCloser, data interface{}) (err error) { |
There was a problem hiding this comment.
The function name jsonRespondError conceals the fact that it reads json from body. Maybe something like readJSONRespondError?
server/api/util.go
Outdated
|
|
||
| } | ||
| } | ||
| return |
There was a problem hiding this comment.
It seems the function will never return an error.
There was a problem hiding this comment.
It will in fact return the error: it has a named return parameter. I am beginning to think though that these generally only have potential to create confusion and will get rid of it.
We often send a 500 error where it should be 400 or 404. It is important to get these right. A client will assume it did nothing wrong when a 500 is received and perhaps automatically retry the request. This is a conservative change: there are many more places that should be changed from 500. A helper function jsonRespondError is added
gregwebs
force-pushed
the
gregwebs/http-400
branch
from
53438ca
to
e549647
Compare
|
@disksing PTAL |
|
You don't have to update the year in all files, only new files need to use '2018'. The rest LGTM. |
| @@ -56,11 +57,23 @@ func (h *tableNamespaceHandler) Get(w http.ResponseWriter, r *http.Request) { | |||
| h.rd.JSON(w, http.StatusOK, nsInfo) | |||
| } | |||
|
|
|||
| func readJSONRespondError(r *render.Render, w http.ResponseWriter, body io.ReadCloser, data interface{}) (err error) { | |||
There was a problem hiding this comment.
I see this function twice times, can we move it to apiutil?
There was a problem hiding this comment.
I didn't want to start creating new imports. I would appreciate the advise of the maintainers here if that is what is desired. I also didn't know why this handler is under /table instead of /server and why it uses the apiutil package instead of api.
There was a problem hiding this comment.
I think the duplicate code here is ok. The package table was originally designed to be independent of the other modules of the project (like a plugin for pd-server), so it is not dependent on other modules of pd as much as possible.
gregwebs
force-pushed
the
gregwebs/http-400
branch
from
f14b2dd
to
7d49f45
Compare
gregwebs
force-pushed
the
gregwebs/http-400
branch
from
7d49f45
to
f5727d1
Compare
* server: use HTTP 400 where there is a client error We often send a 500 error where it should be 400 or 404. It is important to get these right. A client will assume it did nothing wrong when a 500 is received and perhaps automatically retry the request. This is a conservative change: there are many more places that should be changed from 500. A helper function jsonRespondError is added
What have you changed? (required)
pd server
We often send a 500 error where it should be 400 or 404.
It is important to get these right.
A client will assume it did nothing wrong when a 500 is received
and perhaps automatically retry the request.
This is a conservative change in that
there are many more places that should be changed from 500 to 400 or 404.
Mostly this is due to not investing more time to understand the entire code base.
In some cases, functions are called which don't differentiate error types properly.
A forthcoming PR will provide some help in this case.
A helper function
jsonRespondErroris added.What are the type of the changes (required)?
I can only assume that most client code is not differentiating between 400 and 500 error codes. In that case there will not be any breakage.
How has this PR been tested (required)?
There is a new test suite for the jsonRespondError helper.
Otherwise the test suite was ran and I was surprised that no tests broke.
Does this PR affect documentation (docs/docs-cn) update? (optional)
There is a PR to add an API specification. That would need to be updated to add more 400 entries.