rpc: disallow PUT and DELETE on HTTP RPC (#15493)#15501
Conversation
| httpErrorResponseTest(t, "POST", contentType, "", 0) | ||
| } | ||
|
|
||
| func httpErrorResponseTest(t *testing.T, |
There was a problem hiding this comment.
Please call this testHTTPErrorResponse. Lower case won't be executed and it's usually the convention.
There was a problem hiding this comment.
This is just a helper called by all the other tests.
| @@ -0,0 +1,40 @@ | |||
| package rpc | |||
There was a problem hiding this comment.
Please add the LGPL copyright header that we have on each of our source files.
| srv.ServeSingleRequest(codec, OptionMethodInvocation) | ||
| } | ||
|
|
||
| // Returns a non-zero response code and error message if the request is invalid. |
There was a problem hiding this comment.
Method docs in Go start with the name of the method. ie.
// httpErrorResponse returns a non-zero response code and error message if the request is invalid.
There was a problem hiding this comment.
I would also rather call it validateRequest, to make it clearer what it does.
| http.Error(w, | ||
| "invalid content type, only application/json is supported", | ||
| http.StatusUnsupportedMediaType) | ||
| if responseCode, errorMessage := httpErrorResponse(r); responseCode != 0 { |
There was a problem hiding this comment.
Nitpicking, but perhaps use shorted variable names?
responseCode -> code
errorMessage -> err
| } | ||
|
|
||
| // Returns a non-zero response code and error message if the request is invalid. | ||
| func httpErrorResponse(r *http.Request) (int, string) { |
There was a problem hiding this comment.
Instead of returning (int, string), please return (int, error). That makes it a lot cleaner imho, since you don't need to check for 0 equality outside (which is arbitrary), rather can do a clean error check which is the standard in Go.
4 participants
Addresses #15493 .
Extracted HTTP request checks into a separate method.
If more checks like this are added in the future, it might be nice to abstract this even more, perhaps having some notion of a "rule" with a validation function (though this is probably overkill for now).
Also added some basic tests to cover the changes.