Skip to content
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

Jump to bottom

jsonpb: change Marshal/Unmarshal to return error if any required field is not set #472

Merged
merged 7 commits into from
Jan 5, 2018
Merged

jsonpb: change Marshal/Unmarshal to return error if any required field is not set #472

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f269205
jsonpb: change Marshal/Unmarshal to return error if any required field
cybrcodr Dec 13, 2017
49ce6df
jsonpb: improve Unmarshal check for required fields.
cybrcodr Dec 20, 2017
c24ad06
jsonpb: improve Unmarshal check for required fields.
cybrcodr Dec 21, 2017
f899cde
jsonpb: fix Marshal/Unmarshal logic in checking required fields.
cybrcodr Dec 22, 2017
5f2e3b1
jsonpb: revert accidental changes in jsonpb_test.go in PR#472.
cybrcodr Dec 22, 2017
1f41e39
jsonpb: add safety check on OneOf wrapper structs to contain a field.
cybrcodr Jan 4, 2018
a7bdcd1
jsonpb: minor fix in check for OneOf struct fields.
cybrcodr Jan 5, 2018
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
107 changes: 105 additions & 2 deletions jsonpb/jsonpb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,12 +257,17 @@ func (m *Marshaler) marshalObject(out *errWriter, v proto.Message, indent, typeU
continue
}

prop := jsonProperties(valueField, m.OrigName)
// IsNil will panic on most value kinds.
switch value.Kind() {
case reflect.Chan, reflect.Func, reflect.Interface:
if value.IsNil() {
continue
}
case reflect.Ptr:
if prop.Required && value.IsNil() {
return fmt.Errorf("required field %q is not set", prop.Name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious, for error messages like this, should I use Name or OrigName? Or is it dependent on whether message is for marshaling or unmarshaling?

Copy link
Member

Choose a reason for hiding this comment

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

Properties.Name is fine. It's even documented as such: "name of the field, for error messages"

}
}

if !m.EmitDefaults {
Expand Down Expand Up @@ -300,8 +305,8 @@ func (m *Marshaler) marshalObject(out *errWriter, v proto.Message, indent, typeU
sv := value.Elem().Elem() // interface -> *T -> T
value = sv.Field(0)
valueField = sv.Type().Field(0)
prop = jsonProperties(valueField, m.OrigName)
}
prop := jsonProperties(valueField, m.OrigName)
if !firstField {
m.writeSep(out)
}
Expand Down Expand Up @@ -644,7 +649,105 @@ func (u *Unmarshaler) UnmarshalNext(dec *json.Decoder, pb proto.Message) error {
// permutations of the related Marshaler.
func (u *Unmarshaler) Unmarshal(r io.Reader, pb proto.Message) error {
dec := json.NewDecoder(r)
return u.UnmarshalNext(dec, pb)
if err := u.UnmarshalNext(dec, pb); err != nil {
return err
}

return checkRequiredFields(pb)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to work the code into UnmarshalNext, rather than iterate over all the fields a second time? (albeit, yes, fairly fast code.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly. The current unmarshaling logic does rely on json.Unmarshal to populate fields. To embed the check for required in there means that it will still need to check for all fields that have been unmarshaled.

But you are correct that doing the check w/in the unmarshaling code may be more efficient. The unmarshaling code is quite complicated though that I prefer to avoid mixing this logic in right now. A refactor/rewrite of the unmarshaling code should account for this logic.

Copy link
Member

Choose a reason for hiding this comment

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

The jsonpb logic is pretty non-performant to begin with. We should aim for correctness in this PR.
This package is in need a major re-factoring (really, the entire protobuf repo). Let's revisit this when that refactoring occurs.

Copy link
Member

Choose a reason for hiding this comment

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

Even though UnmarshalNext is used by almost nobody, why is this check in Unmarshal as opposed to UnmarshalNext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thanks for catching. Fixed.

}

// checkRequiredFields returns an error if any required field in the given proto message is not set.
// This function is called from Unmarshal and assumes certain actions are done ahead.
func checkRequiredFields(pb proto.Message) error {
// Most well-known type messages do not contain any required field. The "Any" type may have
// required fields, however the Unmarshaler should have invoked proto.Marshal on the value field
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean proto.Unmarshal here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. During unmarshaling of an "Any" JSON to a Go proto message, after it resolves and constructs the embedded message, it will actual do a proto marshal to a serialized form and store the bytes into Any.Value field. See https://github.com/golang/protobuf/blob/dev/jsonpb/jsonpb.go#L751.

I've updated the comment to explain this better.

// during unmarshaling and that would have returned an error if a required field is not set.
if _, ok := pb.(wkt); ok {
return nil
}

v := reflect.ValueOf(pb).Elem()
Copy link
Member

Choose a reason for hiding this comment

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

You will need to verify that pb is a pointer to a struct. If it isn't, then return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I'm guessing we'll simply ignore message if not pointer to struct.

for i := 0; i < v.NumField(); i++ {
field := v.Field(i)
sfield := v.Type().Field(i)
if strings.HasPrefix(sfield.Name, "XXX_") {
continue
}

// Oneof fields need special handling.
if sfield.Tag.Get("protobuf_oneof") != "" {
// field is an interface containing &T{real_value}.
v := field.Elem().Elem() // interface -> *T -> T
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you considered the case where a oneof contains a simple scalar (string, bytes, varint), which in proto3 is not a pointer?

What will this code do in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current proto compiler will always produce an interface field for a oneof field. It will also produce wrapper structs, one for each oneof field. And hence, this block will grab the oneof field inside the wrapper struct which can be any type.

Copy link
Member

Choose a reason for hiding this comment

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

Need to check:

  1. for nil on both the interface.
  2. that the interface element is a pointer to a struct
  3. that the pointer is not nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

field = v.Field(0)
sfield = v.Type().Field(0)
}

var prop proto.Properties
prop.Init(sfield.Type, sfield.Name, sfield.Tag.Get("protobuf"), &sfield)

// IsNil will panic on most value kinds.
switch field.Kind() {
case reflect.Map:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we discussed this before, and you told me how a proto3 could contain a proto2 message, but maps are exclusive to proto3, so do they really need to be checked?

Consider adding some code comments explaining the rational of checking maps despite their appearance being expected to be mutually exclusive with required fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, maps are allowed in proto2. I have a test case for this.

if field.IsNil() {
continue
}
// Check each map value.
keys := field.MapKeys()
for _, k := range keys {
v := field.MapIndex(k)
if err := checkRequiredFieldsInValue(v); err != nil {
return err
}
}
case reflect.Slice:
Copy link
Member

Choose a reason for hiding this comment

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

The "bytes" scalar for proto will fall under this case and will need to be specially handled.
IIRC, []byte{} means that a zero-length bytes is set, while []byte(nil) means that it was never set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I forgot about that. Added handling of non-repeated slice.

Yes, I'm going with []byte{} to mean it is set.

if field.IsNil() {
continue
}
// Check each slice item.
for i := 0; i < field.Len(); i++ {
v := field.Index(i)
if err := checkRequiredFieldsInValue(v); err != nil {
return err
}
}
case reflect.Ptr:
if field.IsNil() {
if prop.Required {
return fmt.Errorf("required field %q is not set", prop.Name)
}
continue
}
if err := checkRequiredFieldsInValue(field); err != nil {
return err
}
}
}

// Handle proto2 extensions.
for _, ext := range proto.RegisteredExtensions(pb) {
if !proto.HasExtension(pb, ext) {
continue
}
ep, err := proto.GetExtension(pb, ext)
if err != nil {
return err
}
err = checkRequiredFieldsInValue(reflect.ValueOf(ep))
if err != nil {
return err
}
}

return nil
}

func checkRequiredFieldsInValue(v reflect.Value) error {
if pm, ok := v.Interface().(proto.Message); ok {
if err := checkRequiredFields(pm); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this isn’t in a loop, then it’s not necessary to check the error value before returning it. i.e. you can just use return checkRequiredFields(pm)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I previously have this block inlined above multiple times and then pulled it out to this helper func but forgot that this added check is no longer required. Fixed.

return err
}
}
return nil
}

// UnmarshalNext unmarshals the next protocol buffer from a JSON object stream.
Expand Down
187 changes: 184 additions & 3 deletions jsonpb/jsonpb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,8 @@ var marshalingTests = []struct {
{"BoolValue", marshaler, &pb.KnownTypes{Bool: &wpb.BoolValue{Value: true}}, `{"bool":true}`},
{"StringValue", marshaler, &pb.KnownTypes{Str: &wpb.StringValue{Value: "plush"}}, `{"str":"plush"}`},
{"BytesValue", marshaler, &pb.KnownTypes{Bytes: &wpb.BytesValue{Value: []byte("wow")}}, `{"bytes":"d293"}`},

{"required", marshaler, &pb.MsgWithRequired{Str: proto.String("hello")}, `{"str":"hello"}`},
}

func TestMarshaling(t *testing.T) {
Expand Down Expand Up @@ -500,6 +502,86 @@ func TestMarshalAnyJSONPBMarshaler(t *testing.T) {
}
}

// Test marshaling unset required fields should produce error.
func TestMarshalingUnsetRequiredFields(t *testing.T) {
msgExt := &pb.Real{}
proto.SetExtension(msgExt, pb.E_Extm, &pb.MsgWithRequired{})

tests := []struct {
desc string
marshaler *Marshaler
pb proto.Message
}{
{
desc: "direct required field",
marshaler: &Marshaler{},
pb: &pb.MsgWithRequired{},
},
{
desc: "direct required field + emit defaults",
marshaler: &Marshaler{EmitDefaults: true},
pb: &pb.MsgWithRequired{},
},
{
desc: "indirect required field",
marshaler: &Marshaler{},
pb: &pb.MsgWithIndirectRequired{Subm: &pb.MsgWithRequired{}},
},
{
desc: "indirect required field + emit defaults",
marshaler: &Marshaler{EmitDefaults: true},
pb: &pb.MsgWithIndirectRequired{Subm: &pb.MsgWithRequired{}},
},
{
desc: "direct required wkt field",
marshaler: &Marshaler{},
pb: &pb.MsgWithRequiredWKT{},
},
{
desc: "direct required wkt field + emit defaults",
marshaler: &Marshaler{EmitDefaults: true},
pb: &pb.MsgWithRequiredWKT{},
},
{
desc: "required in map value",
marshaler: &Marshaler{},
pb: &pb.MsgWithIndirectRequired{
MapField: map[string]*pb.MsgWithRequired{
"key": {},
},
},
},
{
desc: "required in slice item",
marshaler: &Marshaler{},
pb: &pb.MsgWithIndirectRequired{
SliceField: []*pb.MsgWithRequired{
{Str: proto.String("hello")},
{},
},
},
},
{
desc: "required inside oneof",
marshaler: &Marshaler{},
pb: &pb.MsgWithOneof{
Union: &pb.MsgWithOneof_MsgWithRequired{&pb.MsgWithRequired{}},
},
},
{
desc: "required inside extension",
marshaler: &Marshaler{},
pb: msgExt,
},
}

for _, tc := range tests {
if _, err := tc.marshaler.MarshalToString(tc.pb); err == nil {
t.Errorf("%s: expecting error in marshaling with unset required fields %+v", tc.desc, tc.pb)
}
}
}

var unmarshalingTests = []struct {
desc string
unmarshaler Unmarshaler
Expand Down Expand Up @@ -631,6 +713,8 @@ var unmarshalingTests = []struct {
{"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}},

{"required", Unmarshaler{}, `{"str":"hello"}`, &pb.MsgWithRequired{Str: proto.String("hello")}},
}

func TestUnmarshaling(t *testing.T) {
Expand Down Expand Up @@ -762,7 +846,7 @@ func TestAnyWithCustomResolver(t *testing.T) {
if err != nil {
t.Errorf("an unexpected error occurred when marshaling any to JSON: %v", err)
}
if len(resolvedTypeUrls) != 1 {
if len(resolvedTypeUrls) == 0 {
t.Errorf("custom resolver was not invoked during marshaling")
} else if resolvedTypeUrls[0] != "https://foobar.com/some.random.MessageKind" {
t.Errorf("custom resolver was invoked with wrong URL: got %q, wanted %q", resolvedTypeUrls[0], "https://foobar.com/some.random.MessageKind")
Expand All @@ -771,16 +855,17 @@ func TestAnyWithCustomResolver(t *testing.T) {
if js != wanted {
t.Errorf("marshalling JSON produced incorrect output: got %s, wanted %s", js, wanted)
}
resolvedTypeUrls = nil

u := Unmarshaler{AnyResolver: resolver}
roundTrip := &anypb.Any{}
err = u.Unmarshal(bytes.NewReader([]byte(js)), roundTrip)
if err != nil {
t.Errorf("an unexpected error occurred when unmarshaling any from JSON: %v", err)
}
if len(resolvedTypeUrls) != 2 {
if len(resolvedTypeUrls) == 0 {
t.Errorf("custom resolver was not invoked during marshaling")
} else if resolvedTypeUrls[1] != "https://foobar.com/some.random.MessageKind" {
} else if resolvedTypeUrls[0] != "https://foobar.com/some.random.MessageKind" {
t.Errorf("custom resolver was invoked with wrong URL: got %q, wanted %q", resolvedTypeUrls[1], "https://foobar.com/some.random.MessageKind")
}
if !proto.Equal(any, roundTrip) {
Expand Down Expand Up @@ -902,3 +987,99 @@ func (m *dynamicMessage) UnmarshalJSONPB(jum *Unmarshaler, js []byte) error {
m.rawJson = string(js)
return nil
}

// Test unmarshaling unset required fields should produce error.
func TestUnmarshalingUnsetRequiredFields(t *testing.T) {
tests := []struct {
desc string
pb proto.Message
json string
}{
{
desc: "direct required field missing",
pb: &pb.MsgWithRequired{},
json: `{}`,
},
{
desc: "direct required field set to null",
pb: &pb.MsgWithRequired{},
json: `{"str": null}`,
},
{
desc: "indirect required field missing",
pb: &pb.MsgWithIndirectRequired{},
json: `{"subm": {}}`,
},
{
desc: "indirect required field set to null",
pb: &pb.MsgWithIndirectRequired{},
json: `{"subm": {"str": null}}`,
},
{
desc: "direct required wkt field missing",
pb: &pb.MsgWithRequiredWKT{},
json: `{}`,
},
{
desc: "direct required wkt field set to null",
pb: &pb.MsgWithRequiredWKT{},
json: `{"str": null}`,
},
{
desc: "any containing message with required field set to null",
pb: &pb.KnownTypes{},
json: `{"an": {"@type": "example.com/jsonpb.MsgWithRequired", "str": null}}`,
},
{
desc: "any containing message with missing required field",
pb: &pb.KnownTypes{},
json: `{"an": {"@type": "example.com/jsonpb.MsgWithRequired"}}`,
},
{
desc: "missing required in map value",
pb: &pb.MsgWithIndirectRequired{},
json: `{"map_field": {"a": {}, "b": {"str": "hi"}}}`,
},
{
desc: "required in map value set to null",
pb: &pb.MsgWithIndirectRequired{},
json: `{"map_field": {"a": {"str": "hello"}, "b": {"str": null}}}`,
},
{
desc: "missing required in slice item",
pb: &pb.MsgWithIndirectRequired{},
json: `{"slice_field": [{}, {"str": "hi"}]}`,
},
{
desc: "required in slice item set to null",
pb: &pb.MsgWithIndirectRequired{},
json: `{"slice_field": [{"str": "hello"}, {"str": null}]}`,
},
{
desc: "required inside oneof missing",
pb: &pb.MsgWithOneof{},
json: `{"msgWithRequired": {}}`,
},
{
desc: "required inside oneof set to null",
pb: &pb.MsgWithOneof{},
json: `{"msgWithRequired": {"str": null}}`,
},
{
desc: "required field in extension missing",
pb: &pb.Real{},
json: `{"[jsonpb.extm]":{}}`,
},
{
desc: "required field in extension set to null",
pb: &pb.Real{},
json: `{"[jsonpb.extm]":{"str": null}}`,
},
}

for _, tc := range tests {
if err := UnmarshalString(tc.json, tc.pb); err == nil {
t.Errorf("%s: expecting error in unmarshaling with unset required fields %s", tc.desc, tc.json)
}
}
}
3 changes: 3 additions & 0 deletions jsonpb/jsonpb_test_proto/more_test_objects.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading