jsonpb: don't allocate memory for null-value reference types #404
Conversation
There was a problem hiding this comment.
Thanks for sending in a fix.
Please also add tests for both map and slice. Sadly current unmarshal test approach is flawed and won't expose the bug that this PR fixes. I filed issue #405 for that and we can have a separate PR to fix the tests.
jsonpb/jsonpb.go
Outdated
| // Allocate memory for pointer fields. | ||
| if targetType.Kind() == reflect.Ptr { | ||
| switch targetType.Kind() { | ||
| case reflect.Map, reflect.Ptr, reflect.Slice: |
There was a problem hiding this comment.
Let's not combine the logic for map and slice here. It's better to just leave this for pointer type alone.
Suggest adding the following logic instead ...
if string(inputValue) == "null" {
return nil
}
after original file lines 917 for handling slice
if targetType.Kind() == reflect.Slice...
and after line 933
if targetType.Kind() == reflect.Map {
There was a problem hiding this comment.
Done, and added tests. Thanks for making me do that; this implementation turned out not to be correct.
jsonpb/jsonpb.go
Outdated
| for i := 0; i < len; i++ { | ||
| if err := u.unmarshalValue(target.Index(i), slc[i], prop); err != nil { | ||
| return err | ||
| if l := len(slc); l > 0 { |
There was a problem hiding this comment.
I think if user explicitly specifies an empty JSON list/map then the deserialized Go struct field should also be an empty slice/map. If JSON field is not specified or if explicitly set to "null", then leave Go field as nil.
jsonpb/jsonpb_test.go
Outdated
| reflect.TypeOf(pb.Maps{}), | ||
| } { | ||
| in := reflect.New(ty).Interface().(proto.Message) | ||
| if err := (&Marshaler{EmitDefaults: true}).Marshal(&b, in); err != nil { |
There was a problem hiding this comment.
I prefer to test unmarshaler by itself and not rely on the Marshaler at all. Basically, provide JSON strings to unmarshal and compare with expected result.
I also just realized that proto.Equal will not properly compare nil vs empty on these, so I reflect.DeepEqual is ok.
| if err := u.unmarshalValue(k, json.RawMessage(ks), keyprop); err != nil { | ||
| return err | ||
| } | ||
| if mp != nil { |
There was a problem hiding this comment.
Optional. Do early return for mp == nil, that way there's no need to indent block below.
if mp == nil {
return nil
}
target.Set(reflect.MakeMap(targetType))
...
Same above if you're going to make this change.
There was a problem hiding this comment.
I'd rather leave it, avoiding the repetition of return nil. 6 of one, half-dozen of the other.
This was missed in 748d386.
@cybrcodr another small issue following on from discussion in #338.