From d47ea19d2fa7c1255b49a6365876797d79ad8a5b Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Tue, 9 Jul 2019 22:38:15 -0700 Subject: [PATCH] encoding: support weak fields in text and JSON unmarshaling If the message for a weak field is linked in, we treat it as if it were identical to a normal known field. However, if the weak field is not linked in, we treat it as if the field were not known. Change-Id: I576d911deec98e13211304024a6353734d055465 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/185457 Reviewed-by: Herbie Ong --- encoding/protojson/decode.go | 3 +++ encoding/protojson/decode_test.go | 27 +++++++++++++++++-- encoding/prototext/decode.go | 18 ++++++------- encoding/prototext/decode_test.go | 23 ++++++++++++++++ .../testprotos/test/weak2/test_weak.pb.go | 4 +-- .../testprotos/test/weak2/test_weak.proto | 2 +- 6 files changed, 63 insertions(+), 14 deletions(-) diff --git a/encoding/protojson/decode.go b/encoding/protojson/decode.go index 8dad1d58..d97a60c3 100644 --- a/encoding/protojson/decode.go +++ b/encoding/protojson/decode.go @@ -186,6 +186,9 @@ Loop: if fd == nil { fd = fieldDescs.ByName(pref.Name(name)) } + if fd != nil && fd.IsWeak() && fd.Message().IsPlaceholder() { + fd = nil // reset since the weak reference is not linked in + } } if fd == nil { diff --git a/encoding/protojson/decode_test.go b/encoding/protojson/decode_test.go index 00b1a546..03bd10a7 100644 --- a/encoding/protojson/decode_test.go +++ b/encoding/protojson/decode_test.go @@ -9,12 +9,15 @@ import ( "testing" "google.golang.org/protobuf/encoding/protojson" - "google.golang.org/protobuf/encoding/testprotos/pb2" - "google.golang.org/protobuf/encoding/testprotos/pb3" + "google.golang.org/protobuf/internal/flags" pimpl "google.golang.org/protobuf/internal/impl" "google.golang.org/protobuf/proto" preg "google.golang.org/protobuf/reflect/protoregistry" + "google.golang.org/protobuf/encoding/testprotos/pb2" + "google.golang.org/protobuf/encoding/testprotos/pb3" + testpb "google.golang.org/protobuf/internal/testprotos/test" + weakpb "google.golang.org/protobuf/internal/testprotos/test/weak1" "google.golang.org/protobuf/types/known/anypb" "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/protobuf/types/known/emptypb" @@ -33,6 +36,7 @@ func TestUnmarshal(t *testing.T) { wantMessage proto.Message // TODO: verify expected error message substring. wantErr bool + skip bool }{{ desc: "proto2 empty message", inputMessage: &pb2.Scalars{}, @@ -2440,10 +2444,29 @@ func TestUnmarshal(t *testing.T) { wantMessage: &anypb.Any{ TypeUrl: "type.googleapis.com/google.protobuf.Empty", }, + }, { + desc: "weak fields", + inputMessage: &testpb.TestWeak{}, + inputText: `{"weak_message1":{"a":1}}`, + wantMessage: func() *testpb.TestWeak { + m := new(testpb.TestWeak) + m.SetWeakMessage1(&weakpb.WeakImportMessage1{A: proto.Int32(1)}) + return m + }(), + skip: !flags.Proto1Legacy, + }, { + desc: "weak fields; unknown field", + inputMessage: &testpb.TestWeak{}, + inputText: `{"weak_message1":{"a":1}, "weak_message2":{"a":1}}`, + wantErr: true, // weak_message2 is unknown since the package containing it is not imported + skip: !flags.Proto1Legacy, }} for _, tt := range tests { tt := tt + if tt.skip { + continue + } t.Run(tt.desc, func(t *testing.T) { err := tt.umo.Unmarshal([]byte(tt.inputText), tt.inputMessage) if err != nil && !tt.wantErr { diff --git a/encoding/prototext/decode.go b/encoding/prototext/decode.go index df05f950..78744104 100644 --- a/encoding/prototext/decode.go +++ b/encoding/prototext/decode.go @@ -95,19 +95,19 @@ func (o UnmarshalOptions) unmarshalMessage(tmsg [][2]text.Value, m pref.Message) case text.Name: name, _ = tkey.Name() fd = fieldDescs.ByName(name) - // The proto name of a group field is in all lowercase. However, the - // textproto field name is the type name. Check to make sure that - // group name is correct. - if fd == nil { + switch { + case fd == nil: + // The proto name of a group field is in all lowercase, + // while the textproto field name is the group message name. + // Check to make sure that group name is correct. gd := fieldDescs.ByName(pref.Name(strings.ToLower(string(name)))) if gd != nil && gd.Kind() == pref.GroupKind && gd.Message().Name() == name { fd = gd } - } else { - if fd.Kind() == pref.GroupKind && fd.Message().Name() != name { - // Reset fd to nil because name does not match. - fd = nil - } + case fd.Kind() == pref.GroupKind && fd.Message().Name() != name: + fd = nil // reset since field name is actually the message name + case fd.IsWeak() && fd.Message().IsPlaceholder(): + fd = nil // reset since the weak reference is not linked in } case text.String: // Handle extensions only. This code path is not for Any. diff --git a/encoding/prototext/decode_test.go b/encoding/prototext/decode_test.go index 66c77f31..31de4d7c 100644 --- a/encoding/prototext/decode_test.go +++ b/encoding/prototext/decode_test.go @@ -9,12 +9,15 @@ import ( "testing" "google.golang.org/protobuf/encoding/prototext" + "google.golang.org/protobuf/internal/flags" pimpl "google.golang.org/protobuf/internal/impl" "google.golang.org/protobuf/proto" preg "google.golang.org/protobuf/reflect/protoregistry" "google.golang.org/protobuf/encoding/testprotos/pb2" "google.golang.org/protobuf/encoding/testprotos/pb3" + testpb "google.golang.org/protobuf/internal/testprotos/test" + weakpb "google.golang.org/protobuf/internal/testprotos/test/weak1" "google.golang.org/protobuf/types/known/anypb" ) @@ -26,6 +29,7 @@ func TestUnmarshal(t *testing.T) { inputText string wantMessage proto.Message wantErr bool // TODO: Verify error message content. + skip bool }{{ desc: "proto2 empty message", inputMessage: &pb2.Scalars{}, @@ -1484,10 +1488,29 @@ unknown: "" type_url: "pb2.Nested" `, wantErr: true, + }, { + desc: "weak fields", + inputMessage: &testpb.TestWeak{}, + inputText: `weak_message1:{a:1}`, + wantMessage: func() *testpb.TestWeak { + m := new(testpb.TestWeak) + m.SetWeakMessage1(&weakpb.WeakImportMessage1{A: proto.Int32(1)}) + return m + }(), + skip: !flags.Proto1Legacy, + }, { + desc: "weak fields; unknown field", + inputMessage: &testpb.TestWeak{}, + inputText: `weak_message1:{a:1} weak_message2:{a:1}`, + wantErr: true, // weak_message2 is unknown since the package containing it is not imported + skip: !flags.Proto1Legacy, }} for _, tt := range tests { tt := tt + if tt.skip { + continue + } t.Run(tt.desc, func(t *testing.T) { err := tt.umo.Unmarshal([]byte(tt.inputText), tt.inputMessage) if err != nil && !tt.wantErr { diff --git a/internal/testprotos/test/weak2/test_weak.pb.go b/internal/testprotos/test/weak2/test_weak.pb.go index 0ce64191..bc6f9df2 100644 --- a/internal/testprotos/test/weak2/test_weak.pb.go +++ b/internal/testprotos/test/weak2/test_weak.pb.go @@ -1,7 +1,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // source: test/weak2/test_weak.proto -package weak1 +package weak2 import ( protoreflect "google.golang.org/protobuf/reflect/protoreflect" @@ -69,7 +69,7 @@ var file_test_weak2_test_weak_proto_rawDesc = []byte{ 0x67, 0x6c, 0x65, 0x2e, 0x67, 0x6f, 0x6c, 0x61, 0x6e, 0x67, 0x2e, 0x6f, 0x72, 0x67, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x75, 0x66, 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, 0x74, 0x65, 0x73, 0x74, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x73, 0x2f, 0x74, 0x65, 0x73, 0x74, - 0x2f, 0x77, 0x65, 0x61, 0x6b, 0x31, + 0x2f, 0x77, 0x65, 0x61, 0x6b, 0x32, } var ( diff --git a/internal/testprotos/test/weak2/test_weak.proto b/internal/testprotos/test/weak2/test_weak.proto index 456af8af..eb4d0ff5 100644 --- a/internal/testprotos/test/weak2/test_weak.proto +++ b/internal/testprotos/test/weak2/test_weak.proto @@ -6,7 +6,7 @@ syntax = "proto2"; package goproto.proto.test.weak; -option go_package = "google.golang.org/protobuf/internal/testprotos/test/weak1"; +option go_package = "google.golang.org/protobuf/internal/testprotos/test/weak2"; message WeakImportMessage2 { optional int32 a = 1;