reflect/protoregistry: assume unique proto file path
Previously, we liberally permitted mutiple files to be registered that
have the same path. However, doing so causes complexity in various places
that need to assume that file paths are unique. Since unique paths are
the intention of the proto language, we strictly enforce that now.
Change-Id: Ie8fdd57c824c9809a51859cf20c4bc477b6871be
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/182497
Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/reflect/protodesc/protodesc.go b/reflect/protodesc/protodesc.go
index 2ee40ca..4135ade 100644
--- a/reflect/protodesc/protodesc.go
+++ b/reflect/protodesc/protodesc.go
@@ -73,19 +73,12 @@
f.Imports[i].IsWeak = true
}
for i, path := range fd.GetDependency() {
- var n int
imp := &f.Imports[i]
- r.RangeFilesByPath(path, func(fd protoreflect.FileDescriptor) bool {
- imp.FileDescriptor = fd
- n++
- return true
- })
- if n > 1 {
- return nil, errors.New("duplicate files for import %q", path)
+ fd, err := r.FindFileByPath(path)
+ if err != nil {
+ fd = prototype.PlaceholderFile(path, "")
}
- if imp.IsWeak || imp.FileDescriptor == nil {
- imp.FileDescriptor = prototype.PlaceholderFile(path, "")
- }
+ imp.FileDescriptor = fd
}
imps := importedFiles(f.Imports)
diff --git a/reflect/protoregistry/registry.go b/reflect/protoregistry/registry.go
index b778cd2..61b7ac5 100644
--- a/reflect/protoregistry/registry.go
+++ b/reflect/protoregistry/registry.go
@@ -53,7 +53,7 @@
// Note that files are stored as a slice, since a package may contain
// multiple files.
descs map[protoreflect.FullName]interface{}
- filesByPath map[string][]protoreflect.FileDescriptor
+ filesByPath map[string]protoreflect.FileDescriptor
}
type packageDescriptor struct {
@@ -88,7 +88,7 @@
r.descs = map[protoreflect.FullName]interface{}{
"": &packageDescriptor{},
}
- r.filesByPath = make(map[string][]protoreflect.FileDescriptor)
+ r.filesByPath = make(map[string]protoreflect.FileDescriptor)
}
var firstErr error
for _, file := range files {
@@ -99,6 +99,11 @@
return firstErr
}
func (r *Files) registerFile(file protoreflect.FileDescriptor) error {
+ path := file.Path()
+ if r.filesByPath[path] != nil {
+ return errors.New("file %q is already registered", file.Path())
+ }
+
for name := file.Package(); name != ""; name = name.Parent() {
switch r.descs[name].(type) {
case nil, *packageDescriptor:
@@ -116,9 +121,6 @@
return err
}
- path := file.Path()
- r.filesByPath[path] = append(r.filesByPath[path], file)
-
for name := file.Package(); name != ""; name = name.Parent() {
if r.descs[name] == nil {
r.descs[name] = &packageDescriptor{}
@@ -129,6 +131,7 @@
rangeRegisteredDescriptors(file, func(desc protoreflect.Descriptor) {
r.descs[desc.FullName()] = desc
})
+ r.filesByPath[path] = file
return nil
}
@@ -187,6 +190,19 @@
return nil, NotFound
}
+// FindFileByPath looks up a file by the path.
+//
+// This returns (nil, NotFound) if not found.
+func (r *Files) FindFileByPath(path string) (protoreflect.FileDescriptor, error) {
+ if r == nil {
+ return nil, NotFound
+ }
+ if fd, ok := r.filesByPath[path]; ok {
+ return fd, nil
+ }
+ return nil, NotFound
+}
+
// RangeFiles iterates over all registered files.
// The iteration order is undefined.
func (r *Files) RangeFiles(f func(protoreflect.FileDescriptor) bool) {
@@ -206,14 +222,14 @@
// RangeFilesByPath iterates over all registered files filtered by
// the given proto path. The iteration order is undefined.
+//
+// Deprecated: Use FindFileByPath instead.
func (r *Files) RangeFilesByPath(path string, f func(protoreflect.FileDescriptor) bool) {
if r == nil {
return
}
- for _, file := range r.filesByPath[path] {
- if !f(file) {
- return
- }
+ if fd, ok := r.filesByPath[path]; ok {
+ f(fd)
}
}
diff --git a/reflect/protoregistry/registry_test.go b/reflect/protoregistry/registry_test.go
index 055879a..de0c4a6 100644
--- a/reflect/protoregistry/registry_test.go
+++ b/reflect/protoregistry/registry_test.go
@@ -35,23 +35,23 @@
inPkg pref.FullName
wantFiles []file
}
- testRangePath struct {
+ testFindPath struct {
inPath string
wantFiles []file
}
)
tests := []struct {
- files []testFile
- rangePkgs []testRangePkg
- rangePaths []testRangePath
+ files []testFile
+ rangePkgs []testRangePkg
+ findPaths []testFindPath
}{{
// Test that overlapping packages and files are permitted.
files: []testFile{
- {inFile: &ptype.File{Syntax: pref.Proto2, Package: "foo.bar"}},
+ {inFile: &ptype.File{Syntax: pref.Proto2, Path: "test1.proto", Package: "foo.bar"}},
{inFile: &ptype.File{Syntax: pref.Proto2, Path: "foo/bar/test.proto", Package: "my.test"}},
- {inFile: &ptype.File{Syntax: pref.Proto2, Path: "foo/bar/test.proto", Package: "foo.bar.baz"}},
- {inFile: &ptype.File{Syntax: pref.Proto2, Package: "my.test.package"}},
+ {inFile: &ptype.File{Syntax: pref.Proto2, Path: "foo/bar/test.proto", Package: "foo.bar.baz"}, wantErr: "already registered"},
+ {inFile: &ptype.File{Syntax: pref.Proto2, Path: "test2.proto", Package: "my.test.package"}},
{inFile: &ptype.File{Syntax: pref.Proto2, Package: "foo.bar"}},
{inFile: &ptype.File{Syntax: pref.Proto2, Path: "foo/bar/baz/../test.proto", Package: "my.test"}},
},
@@ -71,31 +71,29 @@
}, {
inPkg: "foo.bar",
wantFiles: []file{
- {"", "foo.bar"},
+ {"test1.proto", "foo.bar"},
{"", "foo.bar"},
},
}, {
- inPkg: "foo.bar.baz",
+ inPkg: "my.test",
wantFiles: []file{
- {"foo/bar/test.proto", "foo.bar.baz"},
+ {"foo/bar/baz/../test.proto", "my.test"},
+ {"foo/bar/test.proto", "my.test"},
},
}, {
inPkg: "fo",
}},
- rangePaths: []testRangePath{{
+ findPaths: []testFindPath{{
inPath: "nothing",
}, {
inPath: "",
wantFiles: []file{
{"", "foo.bar"},
- {"", "foo.bar"},
- {"", "my.test.package"},
},
}, {
inPath: "foo/bar/test.proto",
wantFiles: []file{
- {"foo/bar/test.proto", "foo.bar.baz"},
{"foo/bar/test.proto", "my.test"},
},
}},
@@ -127,6 +125,7 @@
files: []testFile{{
inFile: &ptype.File{
Syntax: pref.Proto2,
+ Path: "test1.proto",
Package: "fizz.buzz",
Messages: []ptype.Message{{
Name: "Message",
@@ -165,6 +164,7 @@
}, {
inFile: &ptype.File{
Syntax: pref.Proto2,
+ Path: "test2.proto",
Package: "fizz.buzz.gazz",
Enums: []ptype.Enum{{
Name: "Enum",
@@ -172,9 +172,9 @@
}},
},
}, {
- // Previously failed registration should not pollute the namespace.
inFile: &ptype.File{
Syntax: pref.Proto2,
+ Path: "test3.proto",
Package: "fizz.buzz",
Enums: []ptype.Enum{{
Name: "Enum1",
@@ -213,7 +213,7 @@
t.Fatalf("file %d, prototype.NewFile() error: %v", i, err)
}
gotErr := files.Register(fd)
- if (gotErr == nil && tc.wantErr != "") || !strings.Contains(fmt.Sprint(gotErr), tc.wantErr) {
+ if ((gotErr == nil) != (tc.wantErr == "")) || !strings.Contains(fmt.Sprint(gotErr), tc.wantErr) {
t.Errorf("file %d, Register() = %v, want %v", i, gotErr, tc.wantErr)
}
}
@@ -229,14 +229,13 @@
}
}
- for _, tc := range tt.rangePaths {
+ for _, tc := range tt.findPaths {
var gotFiles []file
- files.RangeFilesByPath(tc.inPath, func(fd pref.FileDescriptor) bool {
+ if fd, err := files.FindFileByPath(tc.inPath); err == nil {
gotFiles = append(gotFiles, file{fd.Path(), fd.Package()})
- return true
- })
+ }
if diff := cmp.Diff(tc.wantFiles, gotFiles, sortFiles); diff != "" {
- t.Errorf("RangeFilesByPath(%v) mismatch (-want +got):\n%v", tc.inPath, diff)
+ t.Errorf("FindFileByPath(%v) mismatch (-want +got):\n%v", tc.inPath, diff)
}
}
})
