jsonpb: Fix handling of repeated enums.

Enums used in maps is still broken, but commented-out test data
is now there, together with TODOs.

Fixes #164.

Author: dsymonds

jsonpb/jsonpb.go

@@ -436,7 +436,7 @@ func UnmarshalNext(dec *json.Decoder, pb proto.Message) error {
 	if err := dec.Decode(&inputValue); err != nil {
 		return err
 	}
-	return unmarshalValue(reflect.ValueOf(pb).Elem(), inputValue)
+	return unmarshalValue(reflect.ValueOf(pb).Elem(), inputValue, nil)
 }
 
 // Unmarshal unmarshals a JSON object stream into a protocol
@@ -455,13 +455,14 @@ func UnmarshalString(str string, pb proto.Message) error {
 }
 
 // unmarshalValue converts/copies a value into the target.
-func unmarshalValue(target reflect.Value, inputValue json.RawMessage) error {
+// prop may be nil.
+func unmarshalValue(target reflect.Value, inputValue json.RawMessage, prop *proto.Properties) error {
 	targetType := target.Type()
 
 	// Allocate memory for pointer fields.
 	if targetType.Kind() == reflect.Ptr {
 		target.Set(reflect.New(targetType.Elem()))
-		return unmarshalValue(target.Elem(), inputValue)
+		return unmarshalValue(target.Elem(), inputValue, prop)
 	}
 
 	// Handle well-known types.
@@ -476,7 +477,7 @@ func unmarshalValue(target reflect.Value, inputValue json.RawMessage) error {
 			//  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 unmarshalValue(target.Field(0), inputValue)
+			return unmarshalValue(target.Field(0), inputValue, prop)
 		case "Duration":
 			unq, err := strconv.Unquote(string(inputValue))
 			if err != nil {
@@ -510,6 +511,27 @@ func unmarshalValue(target reflect.Value, inputValue json.RawMessage) error {
 		}
 	}
 
+	// Handle enums, which have an underlying type of int32,
+	// and may appear as strings.
+	// The case of an enum appearing as a number is handled
+	// at the bottom of this function.
+	if inputValue[0] == '"' && prop != nil && prop.Enum != "" {
+		vmap := proto.EnumValueMap(prop.Enum)
+		// Don't need to do unquoting; valid enum names
+		// are from a limited character set.
+		s := inputValue[1 : len(inputValue)-1]
+		n, ok := vmap[string(s)]
+		if !ok {
+			return fmt.Errorf("unknown value %q for enum %s", s, prop.Enum)
+		}
+		if target.Kind() == reflect.Ptr { // proto2
+			target.Set(reflect.New(targetType.Elem()))
+			target = target.Elem()
+		}
+		target.SetInt(int64(n))
+		return nil
+	}
+
 	// Handle nested messages.
 	if targetType.Kind() == reflect.Struct {
 		var jsonFields map[string]json.RawMessage
@@ -551,30 +573,7 @@ func unmarshalValue(target reflect.Value, inputValue json.RawMessage) error {
 				continue
 			}
 
-			// Handle enums, which have an underlying type of int32,
-			// and may appear as strings. We do this while handling
-			// the struct so we have access to the enum info.
-			// The case of an enum appearing as a number is handled
-			// by the recursive call to unmarshalValue.
-			if enum := sprops.Prop[i].Enum; valueForField[0] == '"' && enum != "" {
-				vmap := proto.EnumValueMap(enum)
-				// Don't need to do unquoting; valid enum names
-				// are from a limited character set.
-				s := valueForField[1 : len(valueForField)-1]
-				n, ok := vmap[string(s)]
-				if !ok {
-					return fmt.Errorf("unknown value %q for enum %s", s, enum)
-				}
-				f := target.Field(i)
-				if f.Kind() == reflect.Ptr { // proto2
-					f.Set(reflect.New(f.Type().Elem()))
-					f = f.Elem()
-				}
-				f.SetInt(int64(n))
-				continue
-			}
-
-			if err := unmarshalValue(target.Field(i), valueForField); err != nil {
+			if err := unmarshalValue(target.Field(i), valueForField, sprops.Prop[i]); err != nil {
 				return err
 			}
 		}
@@ -587,7 +586,7 @@ func unmarshalValue(target reflect.Value, inputValue json.RawMessage) error {
 				}
 				nv := reflect.New(oop.Type.Elem())
 				target.Field(oop.Field).Set(nv)
-				if err := unmarshalValue(nv.Elem().Field(0), raw); err != nil {
+				if err := unmarshalValue(nv.Elem().Field(0), raw, oop.Prop); err != nil {
 					return err
 				}
 			}
@@ -613,7 +612,7 @@ func unmarshalValue(target reflect.Value, inputValue json.RawMessage) error {
 		len := len(slc)
 		target.Set(reflect.MakeSlice(targetType, len, len))
 		for i := 0; i < len; i++ {
-			if err := unmarshalValue(target.Index(i), slc[i]); err != nil {
+			if err := unmarshalValue(target.Index(i), slc[i], prop); err != nil {
 				return err
 			}
 		}
@@ -627,6 +626,13 @@ func unmarshalValue(target reflect.Value, inputValue json.RawMessage) error {
 			return err
 		}
 		target.Set(reflect.MakeMap(targetType))
+		var keyprop, valprop *proto.Properties
+		if prop != nil {
+			// These could still be nil if the protobuf metadata is broken somehow.
+			// TODO: This won't work because the fields are unexported.
+			// We should probably just reparse them.
+			//keyprop, valprop = prop.mkeyprop, prop.mvalprop
+		}
 		for ks, raw := range mp {
 			// Unmarshal map key. The core json library already decoded the key into a
 			// string, so we handle that specially. Other types were quoted post-serialization.
@@ -635,14 +641,14 @@ func unmarshalValue(target reflect.Value, inputValue json.RawMessage) error {
 				k = reflect.ValueOf(ks)
 			} else {
 				k = reflect.New(targetType.Key()).Elem()
-				if err := unmarshalValue(k, json.RawMessage(ks)); err != nil {
+				if err := unmarshalValue(k, json.RawMessage(ks), keyprop); err != nil {
 					return err
 				}
 			}
 
 			// Unmarshal map value.
 			v := reflect.New(targetType.Elem()).Elem()
-			if err := unmarshalValue(v, raw); err != nil {
+			if err := unmarshalValue(v, raw, valprop); err != nil {
 				return err
 			}
 			target.SetMapIndex(k, v)

jsonpb/jsonpb_test.go

@@ -299,6 +299,18 @@ var marshalingTests = []struct {
 		&pb.Widget{Color: pb.Widget_BLUE.Enum()}, colorPrettyJSON},
 	{"unknown enum value object", marshalerAllOptions,
 		&pb.Widget{Color: pb.Widget_Color(1000).Enum(), RColor: []pb.Widget_Color{pb.Widget_RED}}, colorListPrettyJSON},
+	{"repeated proto3 enum", Marshaler{},
+		&proto3pb.Message{RFunny: []proto3pb.Message_Humour{
+			proto3pb.Message_PUNS,
+			proto3pb.Message_SLAPSTICK,
+		}},
+		`{"rFunny":["PUNS","SLAPSTICK"]}`},
+	{"repeated proto3 enum as int", Marshaler{EnumsAsInts: true},
+		&proto3pb.Message{RFunny: []proto3pb.Message_Humour{
+			proto3pb.Message_PUNS,
+			proto3pb.Message_SLAPSTICK,
+		}},
+		`{"rFunny":[1,2]}`},
 	{"empty value", marshaler, &pb.Simple3{}, `{}`},
 	{"empty value emitted", Marshaler{EmitDefaults: true}, &pb.Simple3{}, `{"dub":0}`},
 	{"map<int64, int32>", marshaler, &pb.Mappy{Nummy: map[int64]int32{1: 2, 3: 4}}, `{"nummy":{"1":2,"3":4}}`},
@@ -313,6 +325,9 @@ var marshalingTests = []struct {
 	{"map<int64, string>", marshaler, &pb.Mappy{Buggy: map[int64]string{1234: "yup"}},
 		`{"buggy":{"1234":"yup"}}`},
 	{"map<bool, bool>", marshaler, &pb.Mappy{Booly: map[bool]bool{false: true}}, `{"booly":{"false":true}}`},
+	// TODO: This is broken.
+	//{"map<string, enum>", marshaler, &pb.Mappy{Enumy: map[string]pb.Numeral{"XIV": pb.Numeral_ROMAN}}, `{"enumy":{"XIV":"ROMAN"}`},
+	{"map<string, enum as int>", Marshaler{EnumsAsInts: true}, &pb.Mappy{Enumy: map[string]pb.Numeral{"XIV": pb.Numeral_ROMAN}}, `{"enumy":{"XIV":2}}`},
 	{"proto2 map<int64, string>", marshaler, &pb.Maps{MInt64Str: map[int64]string{213: "cat"}},
 		`{"mInt64Str":{"213":"cat"}}`},
 	{"proto2 map<bool, Object>", marshaler,
@@ -373,11 +388,29 @@ var unmarshalingTests = []struct {
 	{"unknown enum value object",
 		"{\n  \"color\": 1000,\n  \"r_color\": [\n    \"RED\"\n  ]\n}",
 		&pb.Widget{Color: pb.Widget_Color(1000).Enum(), RColor: []pb.Widget_Color{pb.Widget_RED}}},
+	{"repeated proto3 enum", `{"rFunny":["PUNS","SLAPSTICK"]}`,
+		&proto3pb.Message{RFunny: []proto3pb.Message_Humour{
+			proto3pb.Message_PUNS,
+			proto3pb.Message_SLAPSTICK,
+		}}},
+	{"repeated proto3 enum as int", `{"rFunny":[1,2]}`,
+		&proto3pb.Message{RFunny: []proto3pb.Message_Humour{
+			proto3pb.Message_PUNS,
+			proto3pb.Message_SLAPSTICK,
+		}}},
+	{"repeated proto3 enum as mix of strings and ints", `{"rFunny":["PUNS",2]}`,
+		&proto3pb.Message{RFunny: []proto3pb.Message_Humour{
+			proto3pb.Message_PUNS,
+			proto3pb.Message_SLAPSTICK,
+		}}},
 	{"unquoted int64 object", `{"oInt64":-314}`, &pb.Simple{OInt64: proto.Int64(-314)}},
 	{"unquoted uint64 object", `{"oUint64":123}`, &pb.Simple{OUint64: proto.Uint64(123)}},
 	{"map<int64, int32>", `{"nummy":{"1":2,"3":4}}`, &pb.Mappy{Nummy: map[int64]int32{1: 2, 3: 4}}},
 	{"map<string, string>", `{"strry":{"\"one\"":"two","three":"four"}}`, &pb.Mappy{Strry: map[string]string{`"one"`: "two", "three": "four"}}},
 	{"map<int32, Object>", `{"objjy":{"1":{"dub":1}}}`, &pb.Mappy{Objjy: map[int32]*pb.Simple3{1: &pb.Simple3{Dub: 1}}}},
+	// TODO: This is broken.
+	//{"map<string, enum>", `{"enumy":{"XIV":"ROMAN"}`, &pb.Mappy{Enumy: map[string]pb.Numeral{"XIV": pb.Numeral_ROMAN}}},
+	{"map<string, enum as int>", `{"enumy":{"XIV":2}}`, &pb.Mappy{Enumy: map[string]pb.Numeral{"XIV": pb.Numeral_ROMAN}}},
 	{"oneof", `{"salary":31000}`, &pb.MsgWithOneof{Union: &pb.MsgWithOneof_Salary{31000}}},
 	{"oneof spec name", `{"country":"Australia"}`, &pb.MsgWithOneof{Union: &pb.MsgWithOneof_Country{"Australia"}}},
 	{"oneof orig_name", `{"Country":"Australia"}`, &pb.MsgWithOneof{Union: &pb.MsgWithOneof_Country{"Australia"}}},
@@ -421,14 +454,17 @@ func TestUnmarshaling(t *testing.T) {
 }
 
 func TestUnmarshalNext(t *testing.T) {
+	// We only need to check against a few, not all of them.
+	tests := unmarshalingTests[:5]
+
 	// Create a buffer with many concatenated JSON objects.
 	var b bytes.Buffer
-	for _, tt := range unmarshalingTests {
+	for _, tt := range tests {
 		b.WriteString(tt.json)
 	}
 
 	dec := json.NewDecoder(&b)
-	for _, tt := range unmarshalingTests {
+	for _, tt := range tests {
 		// Make a new instance of the type of our expected object.
 		p := reflect.New(reflect.TypeOf(tt.pb).Elem()).Interface().(proto.Message)
 

jsonpb/jsonpb_test_proto/more_test_objects.pb.go

@@ -37,10 +37,17 @@ message Simple3 {
   double dub = 1;
 }
 
+enum Numeral {
+  UNKNOWN = 0;
+  ARABIC = 1;
+  ROMAN = 2;
+}
+
 message Mappy {
   map<int64, int32> nummy = 1;
   map<string, string> strry = 2;
   map<int32, Simple3> objjy = 3;
   map<int64, string> buggy = 4;
   map<bool, bool> booly = 5;
+  map<string, Numeral> enumy = 6;
 }