-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
jsonpb: check for nil in Marshal and return error to avoid panic. #469
Conversation
jsonpb/jsonpb_test.go
Outdated
var msg *pb.Simple | ||
m := &Marshaler{} | ||
if _, err := m.MarshalToString(msg); err == nil { | ||
t.Errorf("mashalling nil returned no 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.
Nit: It seems the convention is usually one l.
s/marshalling/marshaling/
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.
jsonpb/jsonpb.go
Outdated
@@ -118,6 +118,9 @@ type JSONPBUnmarshaler interface { | |||
|
|||
// Marshal marshals a protocol buffer into JSON. | |||
func (m *Marshaler) Marshal(out io.Writer, pb proto.Message) error { | |||
if pb == nil || reflect.ValueOf(pb).IsNil() { |
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.
This assumes that pb is a pointer, which is not always 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.
Ah yes. Switched to checking for pointer only. I'm guessing I shouldn't worry about nil maps, slices, etc.
Fixed check for nil to pointer types only.
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.
Thanks.
jsonpb/jsonpb.go
Outdated
@@ -118,6 +118,9 @@ type JSONPBUnmarshaler interface { | |||
|
|||
// Marshal marshals a protocol buffer into JSON. | |||
func (m *Marshaler) Marshal(out io.Writer, pb proto.Message) error { | |||
if pb == nil || reflect.ValueOf(pb).IsNil() { |
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.
Ah yes. Switched to checking for pointer only. I'm guessing I shouldn't worry about nil maps, slices, etc.
jsonpb/jsonpb_test.go
Outdated
var msg *pb.Simple | ||
m := &Marshaler{} | ||
if _, err := m.MarshalToString(msg); err == nil { | ||
t.Errorf("mashalling nil returned no 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.
Done.
Add check for nil in Marshal and return error to avoid panic.