Skip to content

unmarshal null wrapper types to nil instead of empty values #394

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 26, 2017
Merged

unmarshal null wrapper types to nil instead of empty values #394

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5e0b73
unmarshal null wrapper types to nil instead of empty values
cspahr-tulipretail Jul 19, 2017
ee59be8
convert null wrappers to nil, along with Duration, Timestamp, ListVal…
cspahr-tulipretail Jul 25, 2017
01f5de7
made changes suggested in comments on pull request
cspahr-tulipretail Jul 26, 2017
8403eee
fix typo in comment
cspahr-tulipretail Jul 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 12 additions & 25 deletions jsonpb/jsonpb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,22 +640,23 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
// Allocate memory for pointer fields.
if targetType.Kind() == reflect.Ptr {
target.Set(reflect.New(targetType.Elem()))
if string(inputValue) == "null" && targetType != reflect.TypeOf(&stpb.Value{}) {
target.Set(reflect.Zero(targetType))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the value is going to be nil, then there should be no point in constructing the value and then setting the pointer to nil.

This if block should be before target.Set(reflect.new(targetType.Elem())) and hence no need to set value to nil as that should be the default.

Also, please do add comment for this if block. While it's easy to know why a "null" should be nil for pointer to messages, the if condition excludes Value. So, I'd suggest adding comment ...

// If input JSON value is "null" and target is a pointer type, it should be treated as the field is NOT
// set EXCEPT for a pointer to structpb.Value, which should be set to NullValue.

return nil
}

return u.unmarshalValue(target.Elem(), inputValue, prop)
}

if jsu, ok := target.Addr().Interface().(JSONPBUnmarshaler); ok {
return jsu.UnmarshalJSONPB(u, []byte(inputValue))
}

// Handle well-known types.
// Handle well-known types that are not null => Go nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/not null => Go nil/not pointers./

if w, ok := target.Addr().Interface().(wkt); ok {
switch w.XXX_WellKnownType() {
case "DoubleValue", "FloatValue", "Int64Value", "UInt64Value",
"Int32Value", "UInt32Value", "BoolValue", "StringValue", "BytesValue":
// "Wrappers use the same representation in JSON
// as the wrapped primitive type, except that null is allowed."
// encoding/json will turn JSON `null` into Go `nil`,
// so we don't have to do any extra work.
return u.unmarshalValue(target.Field(0), inputValue, prop)
case "Any":
// Use json.RawMessage pointer type instead of value to support pre-1.8 version.
Expand Down Expand Up @@ -717,20 +718,16 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
return nil
case "Duration":
ivStr := string(inputValue)
if ivStr == "null" {
target.Field(0).SetInt(0)
target.Field(1).SetInt(0)
return nil
}

unq, err := strconv.Unquote(ivStr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor. May as well inline string(inputValue) here.

if err != nil {
return err
}

d, err := time.ParseDuration(unq)
if err != nil {
return fmt.Errorf("bad Duration: %v", err)
}

ns := d.Nanoseconds()
s := ns / 1e9
ns %= 1e9
Expand All @@ -739,32 +736,25 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
return nil
case "Timestamp":
ivStr := string(inputValue)
if ivStr == "null" {
target.Field(0).SetInt(0)
target.Field(1).SetInt(0)
return nil
}

unq, err := strconv.Unquote(ivStr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor. May as well inline string(inputValue) here.

if err != nil {
return err
}

t, err := time.Parse(time.RFC3339Nano, unq)
if err != nil {
return fmt.Errorf("bad Timestamp: %v", err)
}

target.Field(0).SetInt(t.Unix())
target.Field(1).SetInt(int64(t.Nanosecond()))
return nil
case "Struct":
if string(inputValue) == "null" {
// Interpret a null struct as empty.
return nil
}
var m map[string]json.RawMessage
if err := json.Unmarshal(inputValue, &m); err != nil {
return fmt.Errorf("bad StructValue: %v", err)
}

target.Field(0).Set(reflect.ValueOf(map[string]*stpb.Value{}))
for k, jv := range m {
pv := &stpb.Value{}
Expand All @@ -775,14 +765,11 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
}
return nil
case "ListValue":
if string(inputValue) == "null" {
// Interpret a null ListValue as empty.
return nil
}
var s []json.RawMessage
if err := json.Unmarshal(inputValue, &s); err != nil {
return fmt.Errorf("bad ListValue: %v", err)
}

target.Field(0).Set(reflect.ValueOf(make([]*stpb.Value, len(s), len(s))))
for i, sv := range s {
if err := u.unmarshalValue(target.Field(0).Index(i), sv, prop); err != nil {
Expand Down
23 changes: 16 additions & 7 deletions jsonpb/jsonpb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,12 +553,12 @@ var unmarshalingTests = []struct {
{"camelName input", Unmarshaler{}, `{"oBool":true}`, &pb.Simple{OBool: proto.Bool(true)}},

{"Duration", Unmarshaler{}, `{"dur":"3.000s"}`, &pb.KnownTypes{Dur: &durpb.Duration{Seconds: 3}}},
{"null Duration", Unmarshaler{}, `{"dur":null}`, &pb.KnownTypes{Dur: &durpb.Duration{Seconds: 0}}},
{"null Duration", Unmarshaler{}, `{"dur":null}`, &pb.KnownTypes{Dur: nil}},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&pb.KnownTypes{} should be good enough. Same for ones below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you have particular strong feelings about it, I prefer to leave it with the explicit field name/nil, since it makes the intent of the tests more readable. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I think the intent is clear, i.e. the field is not being set. Also, I fear that some linter tool may flag/change this to simplify the notation. I've seen some PRs to simplify code.

Anyways, I don't have a strong opinion on this and am ok if you think it should be there. Something that I wish to do if I get a chance is to clean up the structure of these tests, i.e. it'll be good to specify field names for all these test cases and hence also have more line breaks to make it more readable, e.g.
{
desc: "null Duration",
unmarshaler: ...,
...
},

{"Timestamp", Unmarshaler{}, `{"ts":"2014-05-13T16:53:20.021Z"}`, &pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: 14e8, Nanos: 21e6}}},
{"PreEpochTimestamp", Unmarshaler{}, `{"ts":"1969-12-31T23:59:58.999999995Z"}`, &pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: -2, Nanos: 999999995}}},
{"ZeroTimeTimestamp", Unmarshaler{}, `{"ts":"0001-01-01T00:00:00Z"}`, &pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: -62135596800, Nanos: 0}}},
{"null Timestamp", Unmarshaler{}, `{"ts":null}`, &pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: 0, Nanos: 0}}},
{"null Struct", Unmarshaler{}, `{"st": null}`, &pb.KnownTypes{St: &stpb.Struct{}}},
{"null Timestamp", Unmarshaler{}, `{"ts":null}`, &pb.KnownTypes{Ts: nil}},
{"null Struct", Unmarshaler{}, `{"st": null}`, &pb.KnownTypes{St: nil}},
{"empty Struct", Unmarshaler{}, `{"st": {}}`, &pb.KnownTypes{St: &stpb.Struct{}}},
{"basic Struct", Unmarshaler{}, `{"st": {"a": "x", "b": null, "c": 3, "d": true}}`, &pb.KnownTypes{St: &stpb.Struct{Fields: map[string]*stpb.Value{
"a": {Kind: &stpb.Value_StringValue{"x"}},
Expand All @@ -575,7 +575,7 @@ var unmarshalingTests = []struct {
}}}},
}}}},
}}}},
{"null ListValue", Unmarshaler{}, `{"lv": null}`, &pb.KnownTypes{Lv: &stpb.ListValue{}}},
{"null ListValue", Unmarshaler{}, `{"lv": null}`, &pb.KnownTypes{Lv: nil}},
{"empty ListValue", Unmarshaler{}, `{"lv": []}`, &pb.KnownTypes{Lv: &stpb.ListValue{}}},
{"basic ListValue", Unmarshaler{}, `{"lv": ["x", null, 3, true]}`, &pb.KnownTypes{Lv: &stpb.ListValue{Values: []*stpb.Value{
{Kind: &stpb.Value_StringValue{"x"}},
Expand Down Expand Up @@ -612,8 +612,17 @@ var unmarshalingTests = []struct {
{"BoolValue", Unmarshaler{}, `{"bool":true}`, &pb.KnownTypes{Bool: &wpb.BoolValue{Value: true}}},
{"StringValue", Unmarshaler{}, `{"str":"plush"}`, &pb.KnownTypes{Str: &wpb.StringValue{Value: "plush"}}},
{"BytesValue", Unmarshaler{}, `{"bytes":"d293"}`, &pb.KnownTypes{Bytes: &wpb.BytesValue{Value: []byte("wow")}}},
// `null` is also a permissible value. Let's just test one.
{"null DoubleValue", Unmarshaler{}, `{"dbl":null}`, &pb.KnownTypes{Dbl: &wpb.DoubleValue{}}},

// ensure that `null` as a value ends up with a nil pointer instead of a [type]Value struct
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor. Use full statement for comments, i.e. capitalized first word if possible and end with period .

{"null DoubleValue", Unmarshaler{}, `{"dbl":null}`, &pb.KnownTypes{Dbl: nil}},
{"null FloatValue", Unmarshaler{}, `{"flt":null}`, &pb.KnownTypes{Flt: nil}},
{"null Int64Value", Unmarshaler{}, `{"i64":null}`, &pb.KnownTypes{I64: nil}},
{"null UInt64Value", Unmarshaler{}, `{"u64":null}`, &pb.KnownTypes{U64: nil}},
{"null Int32Value", Unmarshaler{}, `{"i32":null}`, &pb.KnownTypes{I32: nil}},
{"null UInt32Value", Unmarshaler{}, `{"u32":null}`, &pb.KnownTypes{U32: nil}},
{"null BoolValue", Unmarshaler{}, `{"bool":null}`, &pb.KnownTypes{Bool: nil}},
{"null StringValue", Unmarshaler{}, `{"str":null}`, &pb.KnownTypes{Str: nil}},
{"null BytesValue", Unmarshaler{}, `{"bytes":null}`, &pb.KnownTypes{Bytes: nil}},
}

func TestUnmarshaling(t *testing.T) {
Expand Down Expand Up @@ -756,4 +765,4 @@ func (m *dynamicMessage) MarshalJSONPB(jm *Marshaler) ([]byte, error) {
func (m *dynamicMessage) UnmarshalJSONPB(jum *Unmarshaler, js []byte) error {
m.rawJson = string(js)
return nil
}
}