diff --git a/jsonpb/jsonpb.go b/jsonpb/jsonpb.go index 89b80655..4be2a068 100644 --- a/jsonpb/jsonpb.go +++ b/jsonpb/jsonpb.go @@ -829,6 +829,25 @@ func UnmarshalString(str string, pb proto.Message) error { return new(Unmarshaler).Unmarshal(strings.NewReader(str), pb) } +// Oneof members use a wrapper pointer as their presence bit, so explicit JSON +// null must be filtered before allocating that wrapper for members that would +// otherwise behave like unset fields. +func nullIsUnsetForOneofValue(target reflect.Value) bool { + targetType := target.Type() + if targetType.Kind() == reflect.Ptr { + _, isJSONPBUnmarshaler := reflect.Zero(targetType).Interface().(JSONPBUnmarshaler) + return targetType != reflect.TypeOf(&types.Value{}) && !isJSONPBUnmarshaler + } + + if target.CanAddr() { + if _, ok := target.Addr().Interface().(JSONPBUnmarshaler); ok { + return false + } + } + + return true +} + // unmarshalValue converts/copies a value into the target. // prop may be nil. func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMessage, prop *proto.Properties) error { @@ -1103,13 +1122,29 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe } // Check for any oneof fields. if len(jsonFields) > 0 { - for _, oop := range sprops.OneofTypes { + oneofKeys := make([]string, 0, len(sprops.OneofTypes)) + for k := range sprops.OneofTypes { + oneofKeys = append(oneofKeys, k) + } + slices.Sort(oneofKeys) + + setOneofFields := make(map[int]bool) + for _, key := range oneofKeys { + oop := sprops.OneofTypes[key] raw, ok := consumeField(oop.Prop) if !ok { continue } nv := reflect.New(oop.Type.Elem()) + if bytes.Equal(bytes.TrimSpace(raw), []byte("null")) && nullIsUnsetForOneofValue(nv.Elem().Field(0)) { + continue + } + if setOneofFields[oop.Field] { + return fmt.Errorf("field %q would overwrite already-set oneof %q in %v", + oop.Prop.OrigName, targetType.Field(oop.Field).Name, targetType) + } target.Field(oop.Field).Set(nv) + setOneofFields[oop.Field] = true if err := u.unmarshalValue(nv.Elem().Field(0), raw, oop.Prop); err != nil { return err } @@ -1118,7 +1153,14 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe // Handle proto2 extensions. if len(jsonFields) > 0 { if ep, ok := target.Addr().Interface().(proto.Message); ok { - for _, ext := range proto.RegisteredExtensions(ep) { + extensions := proto.RegisteredExtensions(ep) + extIDs := make([]int32, 0, len(extensions)) + for id := range extensions { + extIDs = append(extIDs, id) + } + sort.Sort(int32Slice(extIDs)) + for _, id := range extIDs { + ext := extensions[id] name := fmt.Sprintf("[%s]", ext.Name) raw, ok := jsonFields[name] if !ok { diff --git a/jsonpb/jsonpb_test.go b/jsonpb/jsonpb_test.go index fa1e5714..ad4643e4 100644 --- a/jsonpb/jsonpb_test.go +++ b/jsonpb/jsonpb_test.go @@ -961,6 +961,8 @@ var unmarshalingShouldError = []struct { {"StringValue containing invalid character", `{"str": "\U00004E16\U0000754C"}`, &pb.KnownTypes{}}, {"StructValue containing invalid character", `{"str": "\U00004E16\U0000754C"}`, &types.Struct{}}, {"repeated proto3 enum with non array input", `{"rFunny":"PUNS"}`, &proto3pb.Message{RFunny: []proto3pb.Message_Humour{}}}, + {"oneof conflict: two fields from the same group", `{"title":"foo","salary":31000}`, new(pb.MsgWithOneof)}, + {"oneof conflict: camelCase and orig_name from the same group", `{"Country":"Australia","homeAddress":"Brisbane"}`, new(pb.MsgWithOneof)}, } func TestUnmarshalingBadInput(t *testing.T) { @@ -972,6 +974,66 @@ func TestUnmarshalingBadInput(t *testing.T) { } } +func TestUnmarshalOneofNullIsIgnored(t *testing.T) { + tests := []struct { + desc string + in string + want *pb.MsgWithOneof + }{ + { + desc: "single null member", + in: `{"title":null}`, + want: &pb.MsgWithOneof{}, + }, + { + desc: "null member with concrete sibling", + in: `{"title":null,"salary":31000}`, + want: &pb.MsgWithOneof{Union: &pb.MsgWithOneof_Salary{Salary: 31000}}, + }, + { + desc: "concrete sibling with null member", + in: `{"salary":31000,"title":null}`, + want: &pb.MsgWithOneof{Union: &pb.MsgWithOneof_Salary{Salary: 31000}}, + }, + } + + for _, tt := range tests { + var msg pb.MsgWithOneof + if err := UnmarshalString(tt.in, &msg); err != nil { + t.Fatalf("%s: %v", tt.desc, err) + } + + got := proto.MarshalTextString(&msg) + want := proto.MarshalTextString(tt.want) + if got != want { + t.Errorf("%s: got [%s] want [%s]", tt.desc, got, want) + } + } +} + +// TestUnmarshalOneofConflictDeterminism verifies that unmarshalling a JSON +// object containing multiple keys from the same oneof group always produces a +// consistent result. Before the fix the outcome depended on random map +// iteration order, so different runs could decode different oneof variants. +func TestUnmarshalOneofConflictDeterminism(t *testing.T) { + const runs = 100 + seen := make(map[string]struct{}) + for range runs { + var msg pb.MsgWithOneof + err := UnmarshalString(`{"title":"foo","salary":31000}`, &msg) + var key string + if err != nil { + key = "error:" + err.Error() + } else { + key = proto.MarshalTextString(&msg) + } + seen[key] = struct{}{} + } + if len(seen) > 1 { + t.Errorf("non-deterministic unmarshal: got %d different outcomes over %d runs", len(seen), runs) + } +} + type funcResolver func(turl string) (proto.Message, error) func (fn funcResolver) Resolve(turl string) (proto.Message, error) {