-
Notifications
You must be signed in to change notification settings - Fork 23
http: http-utils added after extraction #33
Conversation
/cc @ethanfrey @ebuchman |
2ec05a4
to
537d94c
Compare
// request. It closes the body of the request after parsing. | ||
// Since it uses json.Unmarshal, save must be of a pointer type | ||
// or compatible with json.Unmarshal. | ||
func ParseRequestJSON(r *http.Request, save interface{}) error { |
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.
Please replace *http.Request with io.ReadCloser and r.Body with r
Makes it more reusable
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.
I have cases where I would like to parse the Body two times into two different structs. Like first one set of keys, and use those to select the other set I need.
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.
Or maybe leave as is. And add another function that just reads the bytes safely. Anyway, i do want to parse that data twice sometimes
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.
Thank you for the request for updates. Sure, I can change it and that might be a less broader API for the io.Reader instead of a io.ReadCloser or *http.Request. Coming up in a few.
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.
I like the FparseRequest format.
common/http.go
Outdated
return nil | ||
} | ||
|
||
func ParseAndValidateRequestJSON(r *http.Request, save interface{}) error { |
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.
Same as above
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.
Roger that, thanks.
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.
Done, thanks.
common/http.go
Outdated
|
||
func WriteError(w http.ResponseWriter, err error) { | ||
resp := &ErrorResponse{ | ||
Code: http.StatusBadRequest, |
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.
We do want a way to set this, right?
How about allowing some special interface with a method HTTPCode() int
and if err implements that, use it, otherwise BadRequest
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.
Oh, and a helper method/struct to add this info
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.
Or just a third argument to the function which is probably easier
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.
Yeah before, was operating on the premise that all the users just dumped out an http.StatusBadRequest, but sure great idea to make it changeable. I'll make it so that we type assert to check if it implements the HTTPCode interface then switch up the status code
common/http_test.go
Outdated
w := httptest.NewRecorder() | ||
common.WriteSuccess(w, "foo") | ||
got, want := w.Code, 200 | ||
if got != want { |
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.
We have decided to use testify for all test cases. Can you port this... And the below checks
assert.Equal(want,got) instead of the if.
Use require instead of assert if you want t.Fatal
} | ||
|
||
func TestParseRequestJSON(t *testing.T) { | ||
tests := [...]struct { |
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.
Why not just use a slice here? Please enlighten me
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.
Good question. So using a slice is usually nice, but doesn't allow you to seek back to the index easily in case of any error, for example
given an error
sample_test.go:174 "#5: unknown string"
Slice
tests := []string{"a","b","c","d","e","f","g"}
Indexed slice
tests := [...]string{0:"a", 1:"b", 2:"c", 3:"d", 4:"e", 5:"f", 6:"g"}
in the case of the slice, you'll have to manually go find the element then edit it, and in many cases since we are humans, might change the wrong element(testing becomes harder to fix)
in the case of indexed slice, you just search /5: and that's it. It's a personal style that I've adopted from dealing with many confusing tests cases.
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.
Interesting
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.
I definitely do find myself counting through the slices often, and it is confusing sometimes.
@ethanfrey thanks for the first round, I've updated the PR as per your comments, PTAL. |
FYI: Please don't rebase during a review (until it is approved). This makes it much harder for me to track what is changed. I know this is common practice and we had this discussion recently internally as well. It's nice to see a few changes and track the evolution. |
common/http.go
Outdated
return errNilBody | ||
} | ||
|
||
slurp, err := ioutil.ReadAll(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.
Actually now that I think of it... json.NewDecoder()
will give you a Decoder that can parse a stream rather than loading all bytes first.
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.
Good train of thought, I had briefly considered that but thought ioutil.ReadAll sufficed. I'll update it to use json.NewDecoder, thanks.
// FparseJSON unmarshals into save, the body of the provided reader. | ||
// Since it uses json.Unmarshal, save must be of a pointer type | ||
// or compatible with json.Unmarshal. | ||
func FparseJSON(r io.Reader, save interface{}) error { |
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.
Nice job with the more generic Fparse functions and keeping the more generic Parse... from request wrappers.
// set code to be http.StatusBadRequest | ||
func WriteError(w http.ResponseWriter, err error) { | ||
code := http.StatusBadRequest | ||
if httpC, ok := err.(httpCoder); ok { |
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.
Nice, more flexible.
} | ||
} | ||
|
||
type httpCoder interface { |
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.
Can you add a simple WithCode(error, int) error
function that wraps the error and adds a HTTPCode method to it? It would make this easier to use....
WriteError(w, WithCode(err, 406))
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.
Implemented with ErrorWithCode
common/http_test.go
Outdated
common.WriteError(w, errors.New(msg)) | ||
assert.Equal(t, w.Code, http.StatusBadRequest, "should get a 400") | ||
blob, err := ioutil.ReadAll(w.Body) | ||
require.Equal(t, err, nil, "expecting a successful ioutil.ReadAll") |
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 is also require.Nil
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.
Nice, thanks, I'll use that.
|
||
var blankErrResponse = new(common.ErrorResponse) | ||
|
||
func TestWriteError(t *testing.T) { |
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.
Please add a test here that checks what happens when it implements httpCoder
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.
Done.
"github.com/tendermint/tmlibs/common" | ||
) | ||
|
||
func TestWriteSuccess(t *testing.T) { |
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.
also interesting to see what happens when the object returns an error on json.Marshal, that the error code is set properly.
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.
That's done in TestWriteCode, thanks.
) | ||
|
||
type ErrorResponse struct { | ||
Success bool `json:"success,omitempty"` |
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.
Since success
is always set to false
, omitempty
will make it disappear.
Maybe we should remove it, but then from the code below as well.
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.
So ErrorResponse.Success is used in tendermint/basecoin/cmd/baseserver and tendermint/go-crypto/keys so IMO we still need it.
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.
In particular it is used in tendermint/client/rest/handlers.go#DeleteKey to indicate the response to the caller, on the wire.
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.
ugh, i was abusing that type, huh? okay, leave it, but maybe remove the omitempty
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.
So I am keeping the omitempty so that in cases where we do
success
and leave the rest out, only success
is sent on the wire and the opposite, when err
and code
are defined, we only send err
and code
on the wire.
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.
okay
common/http.go
Outdated
// or compatible with json.Unmarshal. | ||
func FparseJSON(r io.Reader, save interface{}) error { | ||
if r == nil { | ||
return errNilBody |
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.
Noted other errors are errors.Wrap
d but this not. Is there a reason?
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.
Not really, just a migration without too much thought plus the other errors are wrapped because we received an error and wanted to add context; here r == nil
as opposed to err != nil
but the two cases could be made into the same thing; Seems like you are leaning towards errors.Wrap right?
common/http_test.go
Outdated
body string | ||
want interface{} | ||
wantErr bool | ||
}{ |
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.
want is always nil? shouldn't you provide a new(saver)
or something like that. Otherwise, of course they all error. It would be good to see some positive results as well. Feel these tests were 80% finished.
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.
No, want is never used in the test, it was just an artifact of a changed test, I'll remove it.
And there are some positive results, please see the usage of wantErr:true and not, that tests the different scenarios.
common/http_test.go
Outdated
want interface{} | ||
wantErr bool | ||
}{ | ||
0: {wantErr: true, body: ``}, |
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.
Same comment as above:
want is always nil? shouldn't you provide a new(saver)
or something like that. Otherwise, of course they all error. It would be good to see some positive results as well. Feel these tests were 80% finished.
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.
No, want is never used in the test, it was just an artifact of a changed test, I'll remove it.
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.
Yeah as mentioned in #33 (comment), there are also positive results which is what wantErr: true
helps define the wanted errors from non wanted errors, please see the test body in the for loop.
Thank you @ethanfrey for the second round of review, please see the latest PR. Oh so it is the style at Tendermint to keep all revisions of updates in history? My bad I didn't know about that and was always rebasing all my commits so that I'd have a single commit at the end so anyways now I won't anymore. |
Rebasing at the end is not bad. I usually rebase together minor fixups, bugfixes, but allow multiple commits per pr. If they are adding separate pieces of code (our prs are often rather large, >500 lines and touch a few different aspects), it makes it easier to follow. This PR would make sense as one commit though, as it is just different revisions of the same file. The point was only to wait until it was all approved, then collapse commits and rebase on the latest develop to get a clean history. |
common/http.go
Outdated
// Error is the error message if Success is false | ||
Error string `json:"error,omitempty"` | ||
// Err is the error message if Success is false | ||
Err string `json:"error,omitempty"` |
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.
Yeah, I think that is a nicer name
// If err implements the interface HTTPCode() int, | ||
// it will use that status code otherwise, it will | ||
// set code to be http.StatusBadRequest | ||
func WriteError(w http.ResponseWriter, err error) { |
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.
I like it very much now, with the ErrorWithCode reuse.
// ErrorWithCode makes an ErrorResponse with the | ||
// provided err's Error() content, and status code. | ||
// It panics if err is nil. | ||
func ErrorWithCode(err error, code int) *ErrorResponse { |
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.
Crazy, so this is both a struct to serialize, and an enhanced error to add the http code in WriteError?
Minimal API and I think this is a good design, I just had to scroll up again to check my eyes.
common/http_test.go
Outdated
w = httptest.NewRecorder() | ||
|
||
common.WriteError(w, common.ErrorWithCode(errors.New("foo"), tt.code)) | ||
assert.Equal(t, w.Code, tt.code, fmt.Sprintf("#%d: got=%d want=%d", i, w.Code, tt.code)) |
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.
Good test, but you can write the check shorter.
- assert.Equal always prints the two values if they don't equal, as a default.
- The arguments after the comparison values are interpreter by fmt.Sprintf already
This is effectively the same:
assert.Equal(t, w.Code, tt.code, "#%d", i)
It will fmt.Sprintf("#%d", i)
as the message for you, no need to write it out.
And want and got are always in the top of the error message.
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.
Nice, thanks for letting know. Yeah:
- The signature for assert.Equal kinda tripped me out and I couldn't intuitively guess what it did
http://godoc.org/github.com/stretchr/testify/assert#Equal
VS
https://golang.org/pkg/fmt/#Printf
Notice the lack of: format string, args ...interface{}
which in the case of assert.Equal are msgAndArgs ...interface{}
I'll update that in a sec.
common/http_test.go
Outdated
common.WriteCode(w, &marshalFailer{}, code) | ||
wantCode := http.StatusBadRequest | ||
assert.Equal(t, w.Code, wantCode, fmt.Sprintf("#%d", i)) | ||
assert.Equal(t, strings.Contains(string(w.Body.Bytes()), errFooFailed.Error()), true, |
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 is also assert.True
for this case.
common/http_test.go
Outdated
w = httptest.NewRecorder() | ||
common.WriteCode(w, &marshalFailer{}, code) | ||
wantCode := http.StatusBadRequest | ||
assert.Equal(t, w.Code, wantCode, fmt.Sprintf("#%d", i)) |
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.
Nitpick: assert.Equal(t, w.Code, wantCode, "#%d", i)
wantErr bool | ||
useNil bool | ||
}{ | ||
0: {wantErr: true, body: ``}, |
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.
look good the cases
wantErr bool | ||
useNil bool | ||
}{ | ||
0: {wantErr: true, body: ``}, |
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.
please add at least one test that passes, eg. {"foo": 20}
or there is no proof that anything will validate successfully.
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.
Oops, thought I had one before, but not anymore.
common/http_test.go
Outdated
assert.NotEqual(t, err, nil, fmt.Sprintf("#%d: want non-nil error", i)) | ||
continue | ||
} | ||
assert.Equal(t, err, nil, fmt.Sprintf("#%d: want nil error", i)) |
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.
We never check the value was stored properly on success.... maybe nitpicky but at least one check on foo.
Also, simplifying the assert checks... shorter and I get the full stack trace if there is an error.
assert.Nil(t, err, "#%d: %+v", i, err)
Okay, I am happy with how it has shaped up. Nice code I am happy to use :) Some minor feedback to add a check or two in the Parse tests, and to simplify the assert calls. I have basically three patterns:
They let you know the reason (a is not b, err is not nil, ...) and no need for fmt.Sprintf, it is used by the library internally. If you do a little cleanup, you can squash the commits and rebase on the latest unstable, and I will merge it in. (I'll be online til like noon your time today). |
Found common http utils that were being multiply duplicated across many libraries and since am moving things in basecoin/unstable to add for more functionality, it's better to put them in one place. Utilities and tests added: - [X] FparseJSON - [X] FparseAndValidateJSON - [X] ParseRequestJSON - [X] ParseAndValidateRequestJSON - [X] WriteCode - [X] WriteError - [X] WriteSuccess - [X] ErrorResponse During review from @ethanfrey, made updates: * Removed tt.want since it was a distraction/artifact that made the reviewer think the tests weren't testing for both failed and passed results. * Added ErrorWithCode as WithCode is a common options pattern in Go that could cause confusion: ErrorWithCode(error, int) ErrorResponse * Using json.NewDecoder(io.Reader) error instead of ioutil.ReadAll(io.Reader) to slurp all the bytes. * Added more test scenarios to achieve 100% coverage of http.go
Thank you @ethanfrey for the other round of review, I've updated the PR with your comments(thank you), and also squashed my commits into 1. PTAL. |
Found common http utils that were being multiply duplicated across
many libraries and since am moving things in basecoin/unstable to
add for more functionality, it's better to put them in one
place.
Utilities and tests added: