From ec83087fa187a55d5ebfe3cab2e166eee362bd6c Mon Sep 17 00:00:00 2001 From: Tristan Swadell Date: Fri, 29 Jan 2021 10:44:06 -0800 Subject: [PATCH] Ensure protodesc.FileDescriptor references are sourced from the global proto registry when possible (#411) --- common/types/pb/BUILD.bazel | 2 ++ common/types/pb/pb.go | 17 ++++++++++++++ common/types/pb/pb_test.go | 47 +++++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/common/types/pb/BUILD.bazel b/common/types/pb/BUILD.bazel index 6c505891..50d8f4d2 100644 --- a/common/types/pb/BUILD.bazel +++ b/common/types/pb/BUILD.bazel @@ -19,6 +19,7 @@ go_library( "@org_golang_google_genproto//googleapis/api/expr/v1alpha1:go_default_library", "@org_golang_google_protobuf//proto:go_default_library", "@org_golang_google_protobuf//reflect/protoreflect:go_default_library", + "@org_golang_google_protobuf//reflect/protoregistry:go_default_library", "@org_golang_google_protobuf//types/dynamicpb:go_default_library", "@org_golang_google_protobuf//types/known/anypb:go_default_library", "@org_golang_google_protobuf//types/known/durationpb:go_default_library", @@ -43,6 +44,7 @@ go_test( "//test/proto2pb:test_all_types_go_proto", "//test/proto3pb:test_all_types_go_proto", "@org_golang_google_protobuf//reflect/protodesc:go_default_library", + "@org_golang_google_protobuf//reflect/protoreflect:go_default_library", "@org_golang_google_protobuf//types/descriptorpb:go_default_library", ], ) diff --git a/common/types/pb/pb.go b/common/types/pb/pb.go index 894e605a..d02fbe0c 100644 --- a/common/types/pb/pb.go +++ b/common/types/pb/pb.go @@ -19,6 +19,7 @@ package pb import ( "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/reflect/protoregistry" anypb "google.golang.org/protobuf/types/known/anypb" durpb "google.golang.org/protobuf/types/known/durationpb" @@ -29,6 +30,10 @@ import ( ) // Db maps from file / message / enum name to file description. +// +// Each Db is isolated from each other, and while information about protobuf descriptors may be +// fetched from the global protobuf registry, no descriptors are added to this registry, else +// the isolation guarantees of the Db object would be violated. type Db struct { revFileDescriptorMap map[string]*FileDescription // files contains the deduped set of FileDescriptions whose types are contained in the pb.Db. @@ -93,6 +98,18 @@ func (pbdb *Db) RegisterDescriptor(fileDesc protoreflect.FileDescriptor) (*FileD if found { return fd, nil } + // Make sure to search the global registry to see if a protoreflect.FileDescriptor for + // the file specified has been linked into the binary. If so, use the copy of the descriptor + // from the global cache. + // + // Note: Proto reflection relies on descriptor values being object equal rather than object + // equivalence. This choice means that a FieldDescriptor generated from a FileDescriptorProto + // will be incompatible with the FieldDescriptor in the global registry and any message created + // from that global registry. + globalFD, err := protoregistry.GlobalFiles.FindFileByPath(fileDesc.Path()) + if err == nil { + fileDesc = globalFD + } fd = NewFileDescription(fileDesc, pbdb) for _, enumValName := range fd.GetEnumNames() { pbdb.revFileDescriptorMap[enumValName] = fd diff --git a/common/types/pb/pb_test.go b/common/types/pb/pb_test.go index a958a36b..549048f9 100644 --- a/common/types/pb/pb_test.go +++ b/common/types/pb/pb_test.go @@ -20,7 +20,11 @@ import ( "reflect" "testing" + "google.golang.org/protobuf/reflect/protodesc" + "google.golang.org/protobuf/reflect/protoreflect" + proto3pb "github.com/google/cel-go/test/proto3pb" + descpb "google.golang.org/protobuf/types/descriptorpb" ) func TestDbCopy(t *testing.T) { @@ -40,3 +44,46 @@ func TestDbCopy(t *testing.T) { t.Error("db.Copy() did not result in eqivalent objects.") } } + +func TestProtoReflectRoundTrip(t *testing.T) { + msg := &proto3pb.TestAllTypes{SingleBool: true} + fdMap := CollectFileDescriptorSet(msg) + files := []*descpb.FileDescriptorProto{} + for _, fd := range fdMap { + files = append(files, protodesc.ToFileDescriptorProto(fd)) + } + // Round-tripping from a protoreflect.FileDescriptor to a FileDescriptorSet and back + // will result in completely independent copies of the protoreflect.FileDescriptor + // whose values are incompatible with each other. + // + // This test showcases what happens when the protoregistry.GlobalFiles values are + // used when a given protoreflect.FileDescriptor is linked into the binary. + fds := &descpb.FileDescriptorSet{File: files} + pbReg, err := protodesc.NewFiles(fds) + if err != nil { + t.Fatalf("protodesc.NewFiles() failed: %v", err) + } + pbdb := NewDb() + pbReg.RangeFiles(func(fd protoreflect.FileDescriptor) bool { + _, err := pbdb.RegisterDescriptor(fd) + if err != nil { + t.Fatalf("pbdb.RegisterDecriptor(%v) failed: %v", fd, err) + } + return true + }) + msgType, found := pbdb.DescribeType("google.expr.proto3.test.TestAllTypes") + if !found { + t.Fatal("pbdb.DescribeType(google.expr.proto3.test.TestAllTypes) failed") + } + boolField, found := msgType.FieldByName("single_bool") + if !found { + t.Fatal("msgType.FieldByName(single_bool) failed") + } + val, err := boolField.GetFrom(msg) + if err != nil { + t.Errorf("boolField.GetFrom(msg) failed: %v", err) + } + if val != true { + t.Errorf("got TestAllTypes.single_bool %v, wanted true", val) + } +}