Skip to content

Commit

Permalink
all: refactor extensions, add proto.GetExtension etc.
Browse files Browse the repository at this point in the history 
Change protoiface.ExtensionDescV1 to implement protoreflect.ExtensionType.

ExtensionDescV1's Name field conflicts with the Descriptor Name method,
so change the protoreflect.{Message,Enum,Extension}Type types to no
longer implement the corresponding Descriptor interface. This also leads
to a clearer distinction between the two types.

Introduce a protoreflect.ExtensionTypeDescriptor type which bridges
between ExtensionType and ExtensionDescriptor.

Add extension accessor functions to the proto package:
proto.{Has,Clear,Get,Set}Extension. These functions take a
protoreflect.ExtensionType parameter, which allows writing the
same function call using either the old or new API:

  proto.GetExtension(message, somepb.E_ExtensionFoo)

Fixes golang/protobuf#908

Change-Id: Ibc65d12a46666297849114fd3aefbc4a597d9f08
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/189199
Reviewed-by: Joe Tsai <[email protected]>
  • Loading branch information
@neild
neild committed Aug 8, 2019
1 parent d4f0800 commit 92f7618
Show file tree
Hide file tree
Showing 35 changed files with 388 additions and 247 deletions.
2 changes: 1 addition & 1 deletion cmd/protoc-gen-go-grpc/testdata/go.mod
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module google.golang.org/protobuf/cmd/protoc-gen-go-grpc/testdata

require (
github.com/golang/protobuf v1.2.1-0.20190717234224-b9f5089fb9d4
github.com/golang/protobuf v1.2.1-0.20190806214225-7037721e6de0
google.golang.org/grpc v1.19.0
google.golang.org/protobuf v1.0.0
)
Expand Down
2 changes: 1 addition & 1 deletion cmd/protoc-gen-go/testdata/go.mod
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module google.golang.org/protobuf/cmd/protoc-gen-go/testdata

require (
github.com/golang/protobuf v1.2.1-0.20190717234224-b9f5089fb9d4
github.com/golang/protobuf v1.2.1-0.20190806214225-7037721e6de0
google.golang.org/protobuf v1.0.0
)

Expand Down
4 changes: 3 additions & 1 deletion encoding/protojson/decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,9 @@ func (o UnmarshalOptions) unmarshalFields(m pref.Message, skipTypeURL bool) erro
if err != nil && err != protoregistry.NotFound {
return errors.New("unable to resolve [%v]: %v", extName, err)
}
fd = extType
if extType != nil {
fd = extType.Descriptor()
}
} else {
// The name can either be the JSON name or the proto field name.
fd = fieldDescs.ByJSONName(name)
Expand Down
40 changes: 20 additions & 20 deletions encoding/protojson/decode_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1200,10 +1200,10 @@ func TestUnmarshal(t *testing.T) {
OptBool: proto.Bool(true),
OptInt32: proto.Int32(42),
}
setExtension(m, pb2.E_OptExtBool, true)
setExtension(m, pb2.E_OptExtString, "extension field")
setExtension(m, pb2.E_OptExtEnum, pb2.Enum_TEN)
setExtension(m, pb2.E_OptExtNested, &pb2.Nested{
proto.SetExtension(m, pb2.E_OptExtBool, true)
proto.SetExtension(m, pb2.E_OptExtString, "extension field")
proto.SetExtension(m, pb2.E_OptExtEnum, pb2.Enum_TEN)
proto.SetExtension(m, pb2.E_OptExtNested, &pb2.Nested{
OptString: proto.String("nested in an extension"),
OptNested: &pb2.Nested{
OptString: proto.String("another nested in an extension"),
Expand All @@ -1225,9 +1225,9 @@ func TestUnmarshal(t *testing.T) {
}`,
wantMessage: func() proto.Message {
m := &pb2.Extensions{}
setExtension(m, pb2.E_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
setExtension(m, pb2.E_RptExtFixed32, &[]uint32{42, 47})
setExtension(m, pb2.E_RptExtNested, &[]*pb2.Nested{
proto.SetExtension(m, pb2.E_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
proto.SetExtension(m, pb2.E_RptExtFixed32, &[]uint32{42, 47})
proto.SetExtension(m, pb2.E_RptExtNested, &[]*pb2.Nested{
&pb2.Nested{OptString: proto.String("one")},
&pb2.Nested{OptString: proto.String("two")},
&pb2.Nested{OptString: proto.String("three")},
Expand All @@ -1250,10 +1250,10 @@ func TestUnmarshal(t *testing.T) {
}`,
wantMessage: func() proto.Message {
m := &pb2.Extensions{}
setExtension(m, pb2.E_ExtensionsContainer_OptExtBool, true)
setExtension(m, pb2.E_ExtensionsContainer_OptExtString, "extension field")
setExtension(m, pb2.E_ExtensionsContainer_OptExtEnum, pb2.Enum_TEN)
setExtension(m, pb2.E_ExtensionsContainer_OptExtNested, &pb2.Nested{
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtBool, true)
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtString, "extension field")
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtEnum, pb2.Enum_TEN)
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtNested, &pb2.Nested{
OptString: proto.String("nested in an extension"),
OptNested: &pb2.Nested{
OptString: proto.String("another nested in an extension"),
Expand Down Expand Up @@ -1282,9 +1282,9 @@ func TestUnmarshal(t *testing.T) {
OptBool: proto.Bool(true),
OptInt32: proto.Int32(42),
}
setExtension(m, pb2.E_ExtensionsContainer_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
setExtension(m, pb2.E_ExtensionsContainer_RptExtString, &[]string{"hello", "world"})
setExtension(m, pb2.E_ExtensionsContainer_RptExtNested, &[]*pb2.Nested{
proto.SetExtension(m, pb2.E_ExtensionsContainer_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
proto.SetExtension(m, pb2.E_ExtensionsContainer_RptExtString, &[]string{"hello", "world"})
proto.SetExtension(m, pb2.E_ExtensionsContainer_RptExtNested, &[]*pb2.Nested{
&pb2.Nested{OptString: proto.String("one")},
&pb2.Nested{OptString: proto.String("two")},
&pb2.Nested{OptString: proto.String("three")},
Expand Down Expand Up @@ -1323,13 +1323,13 @@ func TestUnmarshal(t *testing.T) {
}`,
wantMessage: func() proto.Message {
m := &pb2.MessageSet{}
setExtension(m, pb2.E_MessageSetExtension_MessageSetExtension, &pb2.MessageSetExtension{
proto.SetExtension(m, pb2.E_MessageSetExtension_MessageSetExtension, &pb2.MessageSetExtension{
OptString: proto.String("a messageset extension"),
})
setExtension(m, pb2.E_MessageSetExtension_NotMessageSetExtension, &pb2.MessageSetExtension{
proto.SetExtension(m, pb2.E_MessageSetExtension_NotMessageSetExtension, &pb2.MessageSetExtension{
OptString: proto.String("not a messageset extension"),
})
setExtension(m, pb2.E_MessageSetExtension_ExtNested, &pb2.Nested{
proto.SetExtension(m, pb2.E_MessageSetExtension_ExtNested, &pb2.Nested{
OptString: proto.String("just a regular extension"),
})
return m
Expand All @@ -1345,7 +1345,7 @@ func TestUnmarshal(t *testing.T) {
}`,
wantMessage: func() proto.Message {
m := &pb2.FakeMessageSet{}
setExtension(m, pb2.E_FakeMessageSetExtension_MessageSetExtension, &pb2.FakeMessageSetExtension{
proto.SetExtension(m, pb2.E_FakeMessageSetExtension_MessageSetExtension, &pb2.FakeMessageSetExtension{
OptString: proto.String("not a messageset extension"),
})
return m
Expand All @@ -1371,7 +1371,7 @@ func TestUnmarshal(t *testing.T) {
}`,
wantMessage: func() proto.Message {
m := &pb2.MessageSet{}
setExtension(m, pb2.E_MessageSetExtension, &pb2.FakeMessageSetExtension{
proto.SetExtension(m, pb2.E_MessageSetExtension, &pb2.FakeMessageSetExtension{
OptString: proto.String("another not a messageset extension"),
})
return m
Expand Down Expand Up @@ -2402,7 +2402,7 @@ func TestUnmarshal(t *testing.T) {
}`,
wantMessage: func() proto.Message {
m := &pb2.Extensions{}
setExtension(m, pb2.E_OptExtNested, &pb2.Nested{})
proto.SetExtension(m, pb2.E_OptExtNested, &pb2.Nested{})
return m
}(),
}, {
Expand Down
44 changes: 19 additions & 25 deletions encoding/protojson/encode_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
pimpl "google.golang.org/protobuf/internal/impl"
"google.golang.org/protobuf/proto"
preg "google.golang.org/protobuf/reflect/protoregistry"
"google.golang.org/protobuf/runtime/protoiface"

"google.golang.org/protobuf/encoding/testprotos/pb2"
"google.golang.org/protobuf/encoding/testprotos/pb3"
Expand All @@ -29,11 +28,6 @@ import (
"google.golang.org/protobuf/types/known/wrapperspb"
)

// TODO: Replace this with proto.SetExtension.
func setExtension(m proto.Message, xd *protoiface.ExtensionDescV1, val interface{}) {
m.ProtoReflect().Set(xd.Type, xd.Type.ValueOf(val))
}

func TestMarshal(t *testing.T) {
tests := []struct {
desc string
Expand Down Expand Up @@ -886,10 +880,10 @@ func TestMarshal(t *testing.T) {
OptBool: proto.Bool(true),
OptInt32: proto.Int32(42),
}
setExtension(m, pb2.E_OptExtBool, true)
setExtension(m, pb2.E_OptExtString, "extension field")
setExtension(m, pb2.E_OptExtEnum, pb2.Enum_TEN)
setExtension(m, pb2.E_OptExtNested, &pb2.Nested{
proto.SetExtension(m, pb2.E_OptExtBool, true)
proto.SetExtension(m, pb2.E_OptExtString, "extension field")
proto.SetExtension(m, pb2.E_OptExtEnum, pb2.Enum_TEN)
proto.SetExtension(m, pb2.E_OptExtNested, &pb2.Nested{
OptString: proto.String("nested in an extension"),
OptNested: &pb2.Nested{
OptString: proto.String("another nested in an extension"),
Expand All @@ -915,9 +909,9 @@ func TestMarshal(t *testing.T) {
desc: "extensions of repeated fields",
input: func() proto.Message {
m := &pb2.Extensions{}
setExtension(m, pb2.E_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
setExtension(m, pb2.E_RptExtFixed32, &[]uint32{42, 47})
setExtension(m, pb2.E_RptExtNested, &[]*pb2.Nested{
proto.SetExtension(m, pb2.E_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
proto.SetExtension(m, pb2.E_RptExtFixed32, &[]uint32{42, 47})
proto.SetExtension(m, pb2.E_RptExtNested, &[]*pb2.Nested{
&pb2.Nested{OptString: proto.String("one")},
&pb2.Nested{OptString: proto.String("two")},
&pb2.Nested{OptString: proto.String("three")},
Expand Down Expand Up @@ -950,10 +944,10 @@ func TestMarshal(t *testing.T) {
desc: "extensions of non-repeated fields in another message",
input: func() proto.Message {
m := &pb2.Extensions{}
setExtension(m, pb2.E_ExtensionsContainer_OptExtBool, true)
setExtension(m, pb2.E_ExtensionsContainer_OptExtString, "extension field")
setExtension(m, pb2.E_ExtensionsContainer_OptExtEnum, pb2.Enum_TEN)
setExtension(m, pb2.E_ExtensionsContainer_OptExtNested, &pb2.Nested{
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtBool, true)
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtString, "extension field")
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtEnum, pb2.Enum_TEN)
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtNested, &pb2.Nested{
OptString: proto.String("nested in an extension"),
OptNested: &pb2.Nested{
OptString: proto.String("another nested in an extension"),
Expand All @@ -980,9 +974,9 @@ func TestMarshal(t *testing.T) {
OptBool: proto.Bool(true),
OptInt32: proto.Int32(42),
}
setExtension(m, pb2.E_ExtensionsContainer_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
setExtension(m, pb2.E_ExtensionsContainer_RptExtString, &[]string{"hello", "world"})
setExtension(m, pb2.E_ExtensionsContainer_RptExtNested, &[]*pb2.Nested{
proto.SetExtension(m, pb2.E_ExtensionsContainer_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
proto.SetExtension(m, pb2.E_ExtensionsContainer_RptExtString, &[]string{"hello", "world"})
proto.SetExtension(m, pb2.E_ExtensionsContainer_RptExtNested, &[]*pb2.Nested{
&pb2.Nested{OptString: proto.String("one")},
&pb2.Nested{OptString: proto.String("two")},
&pb2.Nested{OptString: proto.String("three")},
Expand Down Expand Up @@ -1018,13 +1012,13 @@ func TestMarshal(t *testing.T) {
desc: "MessageSet",
input: func() proto.Message {
m := &pb2.MessageSet{}
setExtension(m, pb2.E_MessageSetExtension_MessageSetExtension, &pb2.MessageSetExtension{
proto.SetExtension(m, pb2.E_MessageSetExtension_MessageSetExtension, &pb2.MessageSetExtension{
OptString: proto.String("a messageset extension"),
})
setExtension(m, pb2.E_MessageSetExtension_NotMessageSetExtension, &pb2.MessageSetExtension{
proto.SetExtension(m, pb2.E_MessageSetExtension_NotMessageSetExtension, &pb2.MessageSetExtension{
OptString: proto.String("not a messageset extension"),
})
setExtension(m, pb2.E_MessageSetExtension_ExtNested, &pb2.Nested{
proto.SetExtension(m, pb2.E_MessageSetExtension_ExtNested, &pb2.Nested{
OptString: proto.String("just a regular extension"),
})
return m
Expand All @@ -1045,7 +1039,7 @@ func TestMarshal(t *testing.T) {
desc: "not real MessageSet 1",
input: func() proto.Message {
m := &pb2.FakeMessageSet{}
setExtension(m, pb2.E_FakeMessageSetExtension_MessageSetExtension, &pb2.FakeMessageSetExtension{
proto.SetExtension(m, pb2.E_FakeMessageSetExtension_MessageSetExtension, &pb2.FakeMessageSetExtension{
OptString: proto.String("not a messageset extension"),
})
return m
Expand All @@ -1060,7 +1054,7 @@ func TestMarshal(t *testing.T) {
desc: "not real MessageSet 2",
input: func() proto.Message {
m := &pb2.MessageSet{}
setExtension(m, pb2.E_MessageSetExtension, &pb2.FakeMessageSetExtension{
proto.SetExtension(m, pb2.E_MessageSetExtension, &pb2.FakeMessageSetExtension{
OptString: proto.String("another not a messageset extension"),
})
return m
Expand Down
4 changes: 2 additions & 2 deletions encoding/protojson/well_known_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ func (o MarshalOptions) marshalAny(m pref.Message) error {
// If type of value has custom JSON encoding, marshal out a field "value"
// with corresponding custom JSON encoding of the embedded message as a
// field.
if isCustomType(emt.FullName()) {
if isCustomType(emt.Descriptor().FullName()) {
o.encoder.WriteName("value")
return o.marshalCustomType(em)
}
Expand Down Expand Up @@ -235,7 +235,7 @@ func (o UnmarshalOptions) unmarshalAny(m pref.Message) error {

// Create new message for the embedded message type and unmarshal into it.
em := emt.New()
if isCustomType(emt.FullName()) {
if isCustomType(emt.Descriptor().FullName()) {
// If embedded message is a custom type, unmarshal the JSON "value" field
// into it.
if err := o.unmarshalAnyValue(em); err != nil {
Expand Down
4 changes: 3 additions & 1 deletion encoding/prototext/decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,9 @@ func (o UnmarshalOptions) unmarshalMessage(tmsg [][2]text.Value, m pref.Message)
if err != nil && err != protoregistry.NotFound {
return errors.New("unable to resolve [%v]: %v", extName, err)
}
fd = xt
if xt != nil {
fd = xt.Descriptor()
}
}

if fd == nil {
Expand Down
38 changes: 19 additions & 19 deletions encoding/prototext/decode_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1171,10 +1171,10 @@ opt_int32: 42
OptBool: proto.Bool(true),
OptInt32: proto.Int32(42),
}
setExtension(m, pb2.E_OptExtBool, true)
setExtension(m, pb2.E_OptExtString, "extension field")
setExtension(m, pb2.E_OptExtEnum, pb2.Enum_TEN)
setExtension(m, pb2.E_OptExtNested, &pb2.Nested{
proto.SetExtension(m, pb2.E_OptExtBool, true)
proto.SetExtension(m, pb2.E_OptExtString, "extension field")
proto.SetExtension(m, pb2.E_OptExtEnum, pb2.Enum_TEN)
proto.SetExtension(m, pb2.E_OptExtNested, &pb2.Nested{
OptString: proto.String("nested in an extension"),
OptNested: &pb2.Nested{
OptString: proto.String("another nested in an extension"),
Expand Down Expand Up @@ -1207,9 +1207,9 @@ opt_int32: 42
`,
wantMessage: func() proto.Message {
m := &pb2.Extensions{}
setExtension(m, pb2.E_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
setExtension(m, pb2.E_RptExtFixed32, &[]uint32{42, 47})
setExtension(m, pb2.E_RptExtNested, &[]*pb2.Nested{
proto.SetExtension(m, pb2.E_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
proto.SetExtension(m, pb2.E_RptExtFixed32, &[]uint32{42, 47})
proto.SetExtension(m, pb2.E_RptExtNested, &[]*pb2.Nested{
&pb2.Nested{OptString: proto.String("one")},
&pb2.Nested{OptString: proto.String("two")},
&pb2.Nested{OptString: proto.String("three")},
Expand All @@ -1231,10 +1231,10 @@ opt_int32: 42
`,
wantMessage: func() proto.Message {
m := &pb2.Extensions{}
setExtension(m, pb2.E_ExtensionsContainer_OptExtBool, true)
setExtension(m, pb2.E_ExtensionsContainer_OptExtString, "extension field")
setExtension(m, pb2.E_ExtensionsContainer_OptExtEnum, pb2.Enum_TEN)
setExtension(m, pb2.E_ExtensionsContainer_OptExtNested, &pb2.Nested{
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtBool, true)
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtString, "extension field")
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtEnum, pb2.Enum_TEN)
proto.SetExtension(m, pb2.E_ExtensionsContainer_OptExtNested, &pb2.Nested{
OptString: proto.String("nested in an extension"),
OptNested: &pb2.Nested{
OptString: proto.String("another nested in an extension"),
Expand Down Expand Up @@ -1269,9 +1269,9 @@ opt_int32: 42
OptBool: proto.Bool(true),
OptInt32: proto.Int32(42),
}
setExtension(m, pb2.E_ExtensionsContainer_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
setExtension(m, pb2.E_ExtensionsContainer_RptExtString, &[]string{"hello", "world"})
setExtension(m, pb2.E_ExtensionsContainer_RptExtNested, &[]*pb2.Nested{
proto.SetExtension(m, pb2.E_ExtensionsContainer_RptExtEnum, &[]pb2.Enum{pb2.Enum_TEN, 101, pb2.Enum_ONE})
proto.SetExtension(m, pb2.E_ExtensionsContainer_RptExtString, &[]string{"hello", "world"})
proto.SetExtension(m, pb2.E_ExtensionsContainer_RptExtNested, &[]*pb2.Nested{
&pb2.Nested{OptString: proto.String("one")},
&pb2.Nested{OptString: proto.String("two")},
&pb2.Nested{OptString: proto.String("three")},
Expand Down Expand Up @@ -1299,13 +1299,13 @@ opt_int32: 42
`,
wantMessage: func() proto.Message {
m := &pb2.MessageSet{}
setExtension(m, pb2.E_MessageSetExtension_MessageSetExtension, &pb2.MessageSetExtension{
proto.SetExtension(m, pb2.E_MessageSetExtension_MessageSetExtension, &pb2.MessageSetExtension{
OptString: proto.String("a messageset extension"),
})
setExtension(m, pb2.E_MessageSetExtension_NotMessageSetExtension, &pb2.MessageSetExtension{
proto.SetExtension(m, pb2.E_MessageSetExtension_NotMessageSetExtension, &pb2.MessageSetExtension{
OptString: proto.String("not a messageset extension"),
})
setExtension(m, pb2.E_MessageSetExtension_ExtNested, &pb2.Nested{
proto.SetExtension(m, pb2.E_MessageSetExtension_ExtNested, &pb2.Nested{
OptString: proto.String("just a regular extension"),
})
return m
Expand All @@ -1321,7 +1321,7 @@ opt_int32: 42
`,
wantMessage: func() proto.Message {
m := &pb2.FakeMessageSet{}
setExtension(m, pb2.E_FakeMessageSetExtension_MessageSetExtension, &pb2.FakeMessageSetExtension{
proto.SetExtension(m, pb2.E_FakeMessageSetExtension_MessageSetExtension, &pb2.FakeMessageSetExtension{
OptString: proto.String("not a messageset extension"),
})
return m
Expand All @@ -1346,7 +1346,7 @@ opt_int32: 42
}`,
wantMessage: func() proto.Message {
m := &pb2.MessageSet{}
setExtension(m, pb2.E_MessageSetExtension, &pb2.FakeMessageSetExtension{
proto.SetExtension(m, pb2.E_MessageSetExtension, &pb2.FakeMessageSetExtension{
OptString: proto.String("another not a messageset extension"),
})
return m
Expand Down
Loading

0 comments on commit 92f7618

Please sign in to comment.