Change MarshalResponse and UnmarshalRequest to gracefully handle a proto type check error #470
Conversation
|
We should probably just fix this in MarshalResponse: add a |
|
Sounds good! Will do. |
kkaneda
force-pushed
the
kkaneda/permission_empty_key
branch
2 times, most recently
from
0c5deb2 to
d3cf42b
Compare
kkaneda
changed the title
|
Updated the PR. @bdarnell , could you take another look? I can also change the code to fall back to JSON instead of returning an error. |
| err = Errorf("unable to marshal %+v to protobuf: %s", value, err) | ||
| msg, ok := value.(gogoproto.Message) | ||
| if !ok { | ||
| err = Errorf("unable to convert %+v to protobuf", value) |
There was a problem hiding this comment.
We shouldn't return an error if someone sends Accept: protobuf when we can't support it (we don't reject requests that have completely unknown values in the accept header). Move the gogoproto.Message conversion earlier and if it fails set protoIdx to MaxInt32 (just as if protobuf weren't in the accept header at all).
There was a problem hiding this comment.
Sure, updated the PR. Does this look better now?
kkaneda
force-pushed
the
kkaneda/permission_empty_key
branch
from
d3cf42b to
9898a0e
Compare
|
LGTM |
Change MarshalResponse and UnmarshalRequest to gracefully handle a proto type check error
I'm still learning Go, and the main motivation of this PR is to add a test. I'm not totally sure the change to permission.go makes sense. Falling back to JSON would be better than panic, but partially supporting protobuf encoding (i.e., protobuf encoding is fine when a key is given) doesn't look very clear.