Skip to content

Ensure protodesc.FileDescriptor references are sourced from the global proto registry #411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 29, 2021
Merged

Ensure protodesc.FileDescriptor references are sourced from the global proto registry #411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions common/types/pb/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
],
)
17 changes: 17 additions & 0 deletions common/types/pb/pb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Copy link
Contributor

@JimLarson JimLarson Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document somewhere the internal constraint that the cel library must not register anything in protoregistry.GlobalFiles as doing so might violate the isolation needed by ExprService.

Copy link
Collaborator Author

@TristonianJones TristonianJones Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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
Expand Down
47 changes: 47 additions & 0 deletions common/types/pb/pb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
}
}