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

cybrcodr · 2017-12-13T01:27:28Z

Change Marshal/Unmarshal to return error if any required field is not set.

For Unmarshal, this means JSON is either missing any required field or has required field set to null.

This is an initial attempt at fixing #440. While I've modified Marshal to produce an error for required fields that are not set, I simply reuse this in Unmarshal to check for required fields which is probably not as efficient.

Anyways, I won't have time to improve on this this week and am simply putting this PR out in case someone would like to try it out and/or feel free to pick it up from here.

I've also yet to test the impact of this change on our existing codebase.

is not set. For Unmarshal, this means JSON is either missing any required field or has required field set to null.

puellanivis

The Marshaling code looks good, but the Unmarshal is a no-go. :(

puellanivis · 2017-12-13T15:31:47Z

jsonpb/jsonpb.go

+	}
+
+	// Marshal it back to check for missing required field error.
+	// TODO: Parse through the message and check for any missing required field w/o having to


This TODO will need to be resolved before this could be considered for merging. Marshaling the JSON just after Unmarshaling is going to build in a lot of overhead that wouldn’t be a good idea to pass around. (Especially for everyone using proto3, where required fields have been entirely removed.)

Agree that this needs to be improved. Sorry if I wasn't clear in my initial posting. I didn't intend to merge this yet. Since I didn't have time this week to continue doing this, I decided to post this PR for others to try it out and see if anyone may be interested in improving it further.

Note though that a proto3 message can contain a proto2 message and vice-versa. So, as far as I know, the code will still need to traverse all the fields and subfields of the message looking for required ones that are not set.

puellanivis · 2017-12-14T10:57:45Z

Hm… you are correct, the best kind of correct.

Though, I think anyone using proto3 might have entirely forgotten that fields were even allowed to be required… or maybe just those of us who got the Google “required considered harmful” meme drilled forcefully into our head by SREs…

This change is to improve upon PR #472. Instead of simply calling Marshal within Unmarshal to check for required fields, traverse the proto message instead looking only for required fields.

cybrcodr

I've updated Unmarshal's logic for checking required fields to traverse the message looking for those instead of simply calling Marshal as latter is most likely more inefficient.

The code assumes the use of field tags in proto messages as with most of the existing codebase. I'm not sure if this is good enough though.

I've added more tests as well for the use cases that I'm aware of.

cybrcodr · 2017-12-20T23:17:29Z

jsonpb/jsonpb.go

 		// 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)


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

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

puellanivis · 2017-12-21T09:46:27Z

jsonpb/jsonpb.go

+// 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


Do you mean proto.Unmarshal here?

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.

puellanivis

Much better, but I still have concerns… they maybe just require a code comment or two explaining things though.

puellanivis · 2017-12-21T09:52:34Z

jsonpb/jsonpb.go

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


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)

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.

puellanivis · 2017-12-21T09:57:37Z

jsonpb/jsonpb.go

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


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.

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

puellanivis · 2017-12-21T09:59:34Z

jsonpb/jsonpb.go

+		return err
+	}
+
+	return checkRequiredFields(pb)


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.)

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.

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.

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

Oops, thanks for catching. Fixed.

puellanivis · 2017-12-21T10:02:33Z

jsonpb/jsonpb.go

+		// 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


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?

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.

Added more comments and clean up based on feedback from PR#472.

cybrcodr

Thanks for the review. I've updated the comments and did the clean up you suggested.

cybrcodr · 2017-12-21T22:25:27Z

jsonpb/jsonpb.go

+		// 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


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.

cybrcodr · 2017-12-21T22:26:19Z

jsonpb/jsonpb.go

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


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

cybrcodr · 2017-12-21T22:32:17Z

jsonpb/jsonpb.go

+		return err
+	}
+
+	return checkRequiredFields(pb)


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.

cybrcodr · 2017-12-21T22:35:50Z

jsonpb/jsonpb.go

+// 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


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.

cybrcodr · 2017-12-21T22:37:53Z

jsonpb/jsonpb.go

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


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.

dsnet · 2017-12-21T22:41:02Z

jsonpb/jsonpb.go

 		// 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)


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

dsnet · 2017-12-21T22:48:26Z

jsonpb/jsonpb.go

+		return err
+	}
+
+	return checkRequiredFields(pb)


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.

dsnet · 2017-12-21T23:22:10Z

jsonpb/jsonpb.go

+		return err
+	}
+
+	return checkRequiredFields(pb)


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

dsnet · 2017-12-21T23:27:00Z

jsonpb/jsonpb.go

+		return nil
+	}
+
+	v := reflect.ValueOf(pb).Elem()


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

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

dsnet · 2017-12-21T23:29:21Z

jsonpb/jsonpb.go

+		// A oneof field is an interface implemented by wrapper structs containing the actual oneof
+		// fields.  Field is an interface containing &T{real_value}.
+		if sfield.Tag.Get("protobuf_oneof") != "" {
+			v := field.Elem().Elem() // interface -> *T -> T


Need to check:

for nil on both the interface.

that the interface element is a pointer to a struct

that the pointer is not nil

dsnet · 2017-12-21T23:32:48Z

jsonpb/jsonpb.go

+					return err
+				}
+			}
+		case reflect.Slice:


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.

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

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

Make checking in Marshal to be done up front in order to account for custom marshalers, i.e. do not rely on custom marshalers to do the checking. Reuse the same checkRequiredFields func on both Marshal and Unmarshal. Added more check logic per feedback from PR#472.

cybrcodr

Thanks for the review.

Per conversation with @dsnet, I've also changed Marshal to call checkRequiredFields before it marshals the given message. This is for consistency in ensuring that the required field checking logic is done by jsonpb and not rely on any custom marshaler/unmarshaler.

cybrcodr · 2017-12-22T03:09:58Z

jsonpb/jsonpb.go

+		return err
+	}
+
+	return checkRequiredFields(pb)


Oops, thanks for catching. Fixed.

cybrcodr · 2017-12-22T03:10:45Z

jsonpb/jsonpb.go

+		return nil
+	}
+
+	v := reflect.ValueOf(pb).Elem()


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

cybrcodr · 2017-12-22T03:12:06Z

jsonpb/jsonpb.go

+					return err
+				}
+			}
+		case reflect.Slice:


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

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

cybrcodr · 2017-12-22T03:13:40Z

jsonpb/jsonpb.go

+		// A oneof field is an interface implemented by wrapper structs containing the actual oneof
+		// fields.  Field is an interface containing &T{real_value}.
+		if sfield.Tag.Get("protobuf_oneof") != "" {
+			v := field.Elem().Elem() // interface -> *T -> T


cybrcodr · 2018-01-04T18:27:41Z

Thanks for the review, @dsnet.

@puellanivis do you have more feedback/comments on this PR before I merged it in? Thanks.

puellanivis

Entirely superficial/nitpicky concerns.

puellanivis · 2018-01-04T18:42:21Z

jsonpb/jsonpb.go

+				continue
+			}
+			v = v.Elem()
+			if v.Kind() != reflect.Struct {


&& v.NumFields() > 0 maybe, just to be sure?

I think you meant to check for NumField() == 0 so that Field(0) below won't panic. That would be a bug in protoc-gen-go, but I went ahead and added the check.

Right, but any value of NumFields() less than 0 would be an error state. And checking for greater than zero, and “is not zero” is either: identical with unsigned ints, or just extra CYA for signed ints.

Sure NumField shouldn’t ever return a negative number, but iI find it to generally be a good idea to remember that technically it might… even if only erroneously. So, I typically prefer to avoid weak/narrow tests, when a stronger/broader test is available for the same cost. (I also usually test if len(array) < 1 rather than just len(array) == 0 for the same reasons.)

It could be viewed as an example of “overengineering” but I prefer to cover all load-scenarios rather than only the specific load-scenarios that one thinks might cause a problem.

I mean, if you can order 100 nuts for $100 in either 3 lbs load-hanlding, or 1 lb load-handling, even if you expect that you will only need at most 1 lb of load-handling, it doesn’t cost anything extra to get the higher load handling ones.

cybrcodr

Thanks!

cybrcodr · 2018-01-04T22:50:41Z

jsonpb/jsonpb.go

+				continue
+			}
+			v = v.Elem()
+			if v.Kind() != reflect.Struct {


I think you meant to check for NumField() == 0 so that Field(0) below won't panic. That would be a bug in protoc-gen-go, but I went ahead and added the check.

cybrcodr

Thanks. I'm going to merge this now. If there are other concerns, we can always create another PR to fix.

cybrcodr · 2018-01-05T18:44:02Z

jsonpb/jsonpb.go

+				continue
+			}
+			v = v.Elem()
+			if v.Kind() != reflect.Struct {


jhump · 2018-02-02T17:15:10Z

FYI, this change effectively breaks the support for dynamic/custom messages that was added in #325. The way in which required fields are checked herein assumes that the message is normal generated message, without providing a way to override/customize.

In particular, if a given struct has any unexported field that contains a pointer value (including a slice or map with an element that contains a pointer value) and that pointer value is not nil, then this will cause a panic here:

panic: reflect.Value.Interface: cannot return value obtained from unexported field or method

I'll open an issue for it.

As mentioned in #509, jsonpb panics when marshaling/unmarshaling a non-generated message that has an unexported pointer field. This change will restore dynamic messages to working with jsonpb (they work fine in master; just broken in this dev branch as of #472).

jsonpb: change Marshal/Unmarshal to return error if any required field

f269205

is not set. For Unmarshal, this means JSON is either missing any required field or has required field set to null.

puellanivis suggested changes Dec 13, 2017

View reviewed changes

jsonpb: improve Unmarshal check for required fields.

49ce6df

This change is to improve upon PR #472. Instead of simply calling Marshal within Unmarshal to check for required fields, traverse the proto message instead looking only for required fields.

cybrcodr commented Dec 20, 2017

View reviewed changes

cybrcodr requested a review from dsnet December 20, 2017 23:22

puellanivis reviewed Dec 21, 2017

View reviewed changes

jsonpb: improve Unmarshal check for required fields.

c24ad06

Added more comments and clean up based on feedback from PR#472.

cybrcodr commented Dec 21, 2017

View reviewed changes

dsnet reviewed Dec 21, 2017

View reviewed changes

cybrcodr added 2 commits December 21, 2017 18:54

jsonpb: revert accidental changes in jsonpb_test.go in PR#472.

5f2e3b1

cybrcodr commented Dec 22, 2017

View reviewed changes

dsnet approved these changes Jan 4, 2018

View reviewed changes

puellanivis approved these changes Jan 4, 2018

View reviewed changes

jsonpb: add safety check on OneOf wrapper structs to contain a field.

1f41e39

cybrcodr commented Jan 4, 2018

View reviewed changes

jsonpb: minor fix in check for OneOf struct fields.

a7bdcd1

cybrcodr commented Jan 5, 2018

View reviewed changes

jsonpb/jsonpb.go Outdated

continue

}

v = v.Elem()

if v.Kind() != reflect.Struct {

Copy link

Contributor Author

cybrcodr Jan 5, 2018

Choose a reason for hiding this comment

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

Done.

cybrcodr merged commit 2bc5431 into golang:dev Jan 5, 2018

cybrcodr mentioned this pull request Jan 5, 2018

jsonpb Marshal and Unmarshal should return error on missing required field #440

Closed

cybrcodr deleted the jsonpb branch January 5, 2018 19:38

This was referenced Feb 2, 2018

support custom message validation in JSONPB when checking for required fields #509

Closed

don't panic when trying to check for required fields #510

Merged

golang locked and limited conversation to collaborators Jun 26, 2020

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

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

Conversation

cybrcodr commented Dec 13, 2017

puellanivis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puellanivis commented Dec 14, 2017

cybrcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puellanivis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybrcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybrcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybrcodr commented Jan 4, 2018

puellanivis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybrcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybrcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhump commented Feb 2, 2018