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

jsonpb/jsonpb.go

@@ -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)
+			}
 		}
 
 		if !m.EmitDefaults {
@@ -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)
 		}
@@ -644,7 +649,19 @@ 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
+	}
+
+	// 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
+	// marshal.
+	m := &Marshaler{AnyResolver: u.AnyResolver}
+	if _, err := m.MarshalToString(pb); err != nil {
+		return err
+	}
+
+	return nil
 }
 
 // UnmarshalNext unmarshals the next protocol buffer from a JSON object stream.

jsonpb/jsonpb_test.go

@@ -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.MsgWithRequired1{Str: proto.String("hello")}, `{"str":"hello"}`},
 }
 
 func TestMarshaling(t *testing.T) {
@@ -500,6 +502,53 @@ func TestMarshalAnyJSONPBMarshaler(t *testing.T) {
 	}
 }
 
+// Test marshaling unset required fields should produce error.
+func TestMarshalingUnsetRequiredFields(t *testing.T) {
+	tests := []struct {
+		desc      string
+		marshaler *Marshaler
+		pb        proto.Message
+	}{
+		{
+			desc:      "direct required field",
+			marshaler: &Marshaler{},
+			pb:        &pb.MsgWithRequired1{},
+		},
+		{
+			desc:      "direct required field + emit defaults",
+			marshaler: &Marshaler{EmitDefaults: true},
+			pb:        &pb.MsgWithRequired1{},
+		},
+		{
+			desc:      "indirect required field",
+			marshaler: &Marshaler{},
+			pb:        &pb.MsgWithRequired2{Subm: &pb.MsgWithRequired1{}},
+		},
+		{
+			desc:      "indirect required field + emit defaults",
+			marshaler: &Marshaler{EmitDefaults: true},
+			pb:        &pb.MsgWithRequired2{Subm: &pb.MsgWithRequired1{}},
+		},
+		{
+			desc:      "direct required wkt field",
+			marshaler: &Marshaler{},
+			pb:        &pb.MsgWithRequired3{},
+		},
+		{
+			desc:      "direct required wkt field + emit defaults",
+			marshaler: &Marshaler{EmitDefaults: true},
+			pb:        &pb.MsgWithRequired3{},
+		},
+	}
+
+	for _, tc := range tests {
+		_, err := tc.marshaler.MarshalToString(tc.pb)
+		if 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
@@ -631,6 +680,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.MsgWithRequired1{Str: proto.String("hello")}},
 }
 
 func TestUnmarshaling(t *testing.T) {
@@ -762,7 +813,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")
@@ -771,16 +822,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) {
@@ -902,3 +954,49 @@ 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.MsgWithRequired1{},
+			json: `{}`,
+		},
+		{
+			desc: "direct required field set to null",
+			pb:   &pb.MsgWithRequired1{},
+			json: `{"str": null}`,
+		},
+		{
+			desc: "indirect required field missing",
+			pb:   &pb.MsgWithRequired2{},
+			json: `{"subm": {}}`,
+		},
+		{
+			desc: "indirect required field set to null",
+			pb:   &pb.MsgWithRequired2{},
+			json: `{"subm": {"str": null}}`,
+		},
+		{
+			desc: "direct required wkt field missing",
+			pb:   &pb.MsgWithRequired3{},
+			json: `{}`,
+		},
+		{
+			desc: "direct required wkt field set to null",
+			pb:   &pb.MsgWithRequired3{},
+			json: `{"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)
+		}
+	}
+}