reflect/protodesc: robustify dependency resolution implementation
Overview of changes:
* Add an option that specifies whether to replace unresolvable references
with a placeholder instead of producing an error. Since the prior behavior
produced placeholders (not always), we default to that behavior for now,
but will enable strict resolving in a future CL.
* The option is not yet exported because there is concern about what the
public API should look like. This will be exposed in a future CL.
* Unlike before, we now permit placeholders for unresolvable enum values.
* We implement relative name resolution logic.
* We handle the case where the type is unknown, but type_name is specified.
In such a case, we populate both FieldDescriptor.{Enum,Message} and leave
the FieldDescriptor.Kind with the zero value. If the type_name happened
to resolve, we use that to determine the type.
* If a placeholder is used to represent a relative name,
the FullName reports an invalid full name with a "*." prefix.
Change-Id: Ifa8c750423c488fb9324eec4d033a2f251505fda
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/184317
Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/internal/encoding/defval/default.go b/internal/encoding/defval/default.go
index f03acdf..7cde945 100644
--- a/internal/encoding/defval/default.go
+++ b/internal/encoding/defval/default.go
@@ -112,7 +112,7 @@
return pref.ValueOf(b), nil, nil
}
}
- return pref.Value{}, nil, errors.New("invalid default value for %v: %q", k, s)
+ return pref.Value{}, nil, errors.New("could not parse value for %v: %q", k, s)
}
// Marshal serializes v as the default string according to the given kind k.
@@ -168,7 +168,7 @@
return s, nil
}
}
- return "", errors.New("invalid default value for %v: %v", k, v)
+ return "", errors.New("could not format value for %v: %v", k, v)
}
// unmarshalBytes deserializes bytes by applying C unescaping.
diff --git a/reflect/protodesc/desc.go b/reflect/protodesc/desc.go
index 4c39996..012748b 100644
--- a/reflect/protodesc/desc.go
+++ b/reflect/protodesc/desc.go
@@ -9,6 +9,7 @@
import (
"google.golang.org/protobuf/internal/errors"
"google.golang.org/protobuf/internal/filedesc"
+ "google.golang.org/protobuf/internal/pragma"
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/reflect/protoregistry"
@@ -22,6 +23,36 @@
FindDescriptorByName(protoreflect.FullName) (protoreflect.Descriptor, error)
}
+// option is an optional argument that may be provided to NewFile.
+type option struct {
+ pragma.DoNotCompare
+ allowUnresolvable bool
+}
+
+// allowUnresolvable configures NewFile to permissively allow unresolvable
+// file, enum, or message dependencies. Unresolved dependencies are replaced by
+// placeholder equivalents.
+//
+// The following dependencies may be left unresolved:
+// • Resolving an imported file.
+// • Resolving the type for a message field or extension field.
+// If the kind of the field is unknown, then a placeholder is used for both
+// protoreflect.FieldDescriptor.Enum and protoreflect.FieldDescriptor.Message.
+// • Resolving the enum value set as the default for an optional enum field.
+// If unresolvable, the protoreflect.FieldDescriptor.Default is set to the
+// first enum value in the associated enum (or zero if the also enum dependency
+// is also unresolvable). The protoreflect.FieldDescriptor.DefaultEnumValue
+// is set as a placeholder.
+// • Resolving the extended message type for an extension field.
+// • Resolving the input or output message type for a service method.
+//
+// If the unresolved dependency uses a relative name, then the placeholder will
+// contain an invalid FullName with a "*." prefix, indicating that the starting
+// prefix of the full name is unknown.
+func allowUnresolvable() option {
+ return option{allowUnresolvable: true}
+}
+
// NewFile creates a new protoreflect.FileDescriptor from the provided
// file descriptor message. The file must represent a valid proto file according
// to protobuf semantics. The returned descriptor is a deep copy of the input.
@@ -31,9 +62,20 @@
// the path must be unique. The newly created file descriptor is not registered
// back into the provided file registry.
func NewFile(fd *descriptorpb.FileDescriptorProto, r Resolver) (protoreflect.FileDescriptor, error) {
+ // TODO: remove setting allowUnresolvable once naughty users are migrated.
+ return newFile(fd, r, allowUnresolvable())
+}
+func newFile(fd *descriptorpb.FileDescriptorProto, r Resolver, opts ...option) (protoreflect.FileDescriptor, error) {
+ // Process the options.
+ var allowUnresolvable bool
+ for _, o := range opts {
+ allowUnresolvable = allowUnresolvable || o.allowUnresolvable
+ }
if r == nil {
r = (*protoregistry.Files)(nil) // empty resolver
}
+
+ // Handle the file descriptor content.
f := &filedesc.File{L2: &filedesc.FileL2{}}
switch fd.GetSyntax() {
case "proto2", "":
@@ -67,9 +109,10 @@
for i, path := range fd.GetDependency() {
imp := &f.L2.Imports[i]
f, err := r.FindFileByPath(path)
- if err != nil {
- // TODO: This should be an error.
+ if err == protoregistry.NotFound && (allowUnresolvable || imp.IsWeak) {
f = filedesc.PlaceholderFile(path)
+ } else if err != nil {
+ return nil, errors.New("could not resolve import %q: %v", path, err)
}
imp.FileDescriptor = f
@@ -101,7 +144,7 @@
}
// Step 2: Resolve every dependency reference not handled by step 1.
- r2 := &resolver{local: r1, remote: r, imports: imps}
+ r2 := &resolver{local: r1, remote: r, imports: imps, allowUnresolvable: allowUnresolvable}
if err := r2.resolveMessageDependencies(f.L1.Messages.List, fd.GetMessageType()); err != nil {
return nil, err
}
@@ -136,11 +179,3 @@
}
}
}
-
-// check returns an error if d does not belong to a currently imported file.
-func (is importSet) check(d protoreflect.Descriptor) error {
- if !is[d.ParentFile().Path()] {
- return errors.New("reference to type %v without import of %v", d.FullName(), d.ParentFile().Path())
- }
- return nil
-}
diff --git a/reflect/protodesc/desc_resolve.go b/reflect/protodesc/desc_resolve.go
index 4372154..25e2c3b 100644
--- a/reflect/protodesc/desc_resolve.go
+++ b/reflect/protodesc/desc_resolve.go
@@ -5,8 +5,6 @@
package protodesc
import (
- "strings"
-
"google.golang.org/protobuf/internal/encoding/defval"
"google.golang.org/protobuf/internal/errors"
"google.golang.org/protobuf/internal/filedesc"
@@ -16,10 +14,15 @@
"google.golang.org/protobuf/types/descriptorpb"
)
+// resolver is a wrapper around a local registry of declarations within the file
+// and the remote resolver. The remote resolver is restricted to only return
+// descriptors that have been imported.
type resolver struct {
local descsByName
remote Resolver
imports importSet
+
+ allowUnresolvable bool
}
func (r *resolver) resolveMessageDependencies(ms []filedesc.Message, mds []*descriptorpb.DescriptorProto) (err error) {
@@ -32,41 +35,21 @@
}
if fd.OneofIndex != nil {
k := int(fd.GetOneofIndex())
- if k >= len(md.GetOneofDecl()) {
- return errors.New("invalid oneof index: %d", k)
+ if !(0 <= k && k < len(md.GetOneofDecl())) {
+ return errors.New("message field %q has an invalid oneof index: %d", f.FullName(), k)
}
o := &m.L2.Oneofs.List[k]
f.L1.ContainingOneof = o
o.L1.Fields.List = append(o.L1.Fields.List, f)
}
- switch f.L1.Kind {
- case protoreflect.EnumKind:
- ed, err := findEnumDescriptor(fd.GetTypeName(), f.L1.IsWeak, r)
- if err != nil {
- return err
- }
- f.L1.Enum = ed
- case protoreflect.MessageKind, protoreflect.GroupKind:
- md, err := findMessageDescriptor(fd.GetTypeName(), f.L1.IsWeak, r)
- if err != nil {
- return err
- }
- f.L1.Message = md
- default:
- if fd.GetTypeName() != "" {
- return errors.New("field of kind %v has type_name set", f.L1.Kind)
- }
+
+ if f.L1.Kind, f.L1.Enum, f.L1.Message, err = r.findTarget(f.Kind(), f.Parent().FullName(), partialName(fd.GetTypeName()), f.IsWeak()); err != nil {
+ return errors.New("message field %q cannot resolve type: %v", f.FullName(), err)
}
if fd.DefaultValue != nil {
- // Handle default value after resolving the enum since the
- // list of enum values is needed to resolve enums by name.
- var evs protoreflect.EnumValueDescriptors
- if f.L1.Kind == protoreflect.EnumKind {
- evs = f.L1.Enum.Values()
- }
- v, ev, err := defval.Unmarshal(fd.GetDefaultValue(), f.L1.Kind, evs, defval.Descriptor)
+ v, ev, err := unmarshalDefault(fd.GetDefaultValue(), f, r.allowUnresolvable)
if err != nil {
- return err
+ return errors.New("message field %q has invalid default: %v", f.FullName(), err)
}
f.L1.Default = filedesc.DefaultValue(v, ev)
}
@@ -82,42 +65,19 @@
return nil
}
-func (r *resolver) resolveExtensionDependencies(xs []filedesc.Extension, xds []*descriptorpb.FieldDescriptorProto) error {
+func (r *resolver) resolveExtensionDependencies(xs []filedesc.Extension, xds []*descriptorpb.FieldDescriptorProto) (err error) {
for i, xd := range xds {
x := &xs[i]
- md, err := findMessageDescriptor(xd.GetExtendee(), false, r)
- if err != nil {
- return err
+ if x.L1.Extendee, err = r.findMessageDescriptor(x.Parent().FullName(), partialName(xd.GetExtendee()), false); err != nil {
+ return errors.New("extension field %q cannot resolve extendee: %v", x.FullName(), err)
}
- x.L1.Extendee = md
- switch x.L1.Kind {
- case protoreflect.EnumKind:
- ed, err := findEnumDescriptor(xd.GetTypeName(), false, r)
- if err != nil {
- return err
- }
- x.L2.Enum = ed
- case protoreflect.MessageKind, protoreflect.GroupKind:
- md, err := findMessageDescriptor(xd.GetTypeName(), false, r)
- if err != nil {
- return err
- }
- x.L2.Message = md
- default:
- if xd.GetTypeName() != "" {
- return errors.New("field of kind %v has type_name set", x.L1.Kind)
- }
+ if x.L1.Kind, x.L2.Enum, x.L2.Message, err = r.findTarget(x.Kind(), x.Parent().FullName(), partialName(xd.GetTypeName()), false); err != nil {
+ return errors.New("extension field %q cannot resolve type: %v", x.FullName(), err)
}
if xd.DefaultValue != nil {
- // Handle default value after resolving the enum since the
- // list of enum values is needed to resolve enums by name.
- var evs protoreflect.EnumValueDescriptors
- if x.L1.Kind == protoreflect.EnumKind {
- evs = x.L2.Enum.Values()
- }
- v, ev, err := defval.Unmarshal(xd.GetDefaultValue(), x.L1.Kind, evs, defval.Descriptor)
+ v, ev, err := unmarshalDefault(xd.GetDefaultValue(), x, r.allowUnresolvable)
if err != nil {
- return err
+ return errors.New("extension field %q has invalid default: %v", x.FullName(), err)
}
x.L2.Default = filedesc.DefaultValue(v, ev)
}
@@ -130,84 +90,193 @@
s := &ss[i]
for j, md := range sd.GetMethod() {
m := &s.L2.Methods.List[j]
- m.L1.Input, err = findMessageDescriptor(md.GetInputType(), false, r)
+ m.L1.Input, err = r.findMessageDescriptor(m.Parent().FullName(), partialName(md.GetInputType()), false)
if err != nil {
- return err
+ return errors.New("service method %q cannot resolve input: %v", m.FullName(), err)
}
- m.L1.Output, err = findMessageDescriptor(md.GetOutputType(), false, r)
+ m.L1.Output, err = r.findMessageDescriptor(s.FullName(), partialName(md.GetOutputType()), false)
if err != nil {
- return err
+ return errors.New("service method %q cannot resolve output: %v", m.FullName(), err)
}
}
}
return nil
}
-// TODO: Should we allow relative names? The protoc compiler has emitted
-// absolute names for some time now. Requiring absolute names as an input
-// simplifies our implementation as we won't need to implement C++'s namespace
-// scoping rules.
-
-func (r resolver) FindFileByPath(s string) (protoreflect.FileDescriptor, error) {
- return r.remote.FindFileByPath(s)
-}
-
-func (r resolver) FindDescriptorByName(s protoreflect.FullName) (protoreflect.Descriptor, error) {
- if d, ok := r.local[s]; ok {
- return d, nil
- }
- return r.remote.FindDescriptorByName(s)
-}
-
-func findEnumDescriptor(s string, isWeak bool, r *resolver) (protoreflect.EnumDescriptor, error) {
- d, err := findDescriptor(s, isWeak, r)
- if err != nil {
- return nil, err
- }
- if ed, ok := d.(protoreflect.EnumDescriptor); ok {
- if err == protoregistry.NotFound {
- if isWeak {
- return filedesc.PlaceholderEnum(protoreflect.FullName(s[1:])), nil
- }
- // TODO: This should be an error.
- return filedesc.PlaceholderEnum(protoreflect.FullName(s[1:])), nil
- // return nil, errors.New("could not resolve enum: %v", name)
+// findTarget finds an enum or message descriptor if k is an enum, message,
+// group, or unknown. If unknown, and the name could be resolved, the kind
+// returned kind is set based on the type of the resolved descriptor.
+func (r *resolver) findTarget(k protoreflect.Kind, scope protoreflect.FullName, ref partialName, isWeak bool) (protoreflect.Kind, protoreflect.EnumDescriptor, protoreflect.MessageDescriptor, error) {
+ switch k {
+ case protoreflect.EnumKind:
+ ed, err := r.findEnumDescriptor(scope, ref, isWeak)
+ if err != nil {
+ return 0, nil, nil, err
}
- return ed, nil
- }
- return nil, errors.New("invalid descriptor type")
-}
-
-func findMessageDescriptor(s string, isWeak bool, r *resolver) (protoreflect.MessageDescriptor, error) {
- d, err := findDescriptor(s, isWeak, r)
- if err != nil {
- if err == protoregistry.NotFound {
- if isWeak {
- return filedesc.PlaceholderMessage(protoreflect.FullName(s[1:])), nil
- }
- // TODO: This should be an error.
- return filedesc.PlaceholderMessage(protoreflect.FullName(s[1:])), nil
- // return nil, errors.New("could not resolve enum: %v", name)
+ return k, ed, nil, nil
+ case protoreflect.MessageKind, protoreflect.GroupKind:
+ md, err := r.findMessageDescriptor(scope, ref, isWeak)
+ if err != nil {
+ return 0, nil, nil, err
}
- return nil, err
+ return k, nil, md, nil
+ case 0:
+ // Handle unspecified kinds (possible with parsers that operate
+ // on a per-file basis without knowledge of dependencies).
+ d, err := r.findDescriptor(scope, ref)
+ if err == protoregistry.NotFound && (r.allowUnresolvable || isWeak) {
+ return k, filedesc.PlaceholderEnum(ref.FullName()), filedesc.PlaceholderMessage(ref.FullName()), nil
+ } else if err == protoregistry.NotFound {
+ return 0, nil, nil, errors.New("%q not found", ref.FullName())
+ } else if err != nil {
+ return 0, nil, nil, err
+ }
+ switch d := d.(type) {
+ case protoreflect.EnumDescriptor:
+ return protoreflect.EnumKind, d, nil, nil
+ case protoreflect.MessageDescriptor:
+ return protoreflect.MessageKind, nil, d, nil
+ default:
+ return 0, nil, nil, errors.New("unknown kind")
+ }
+ default:
+ if ref != "" {
+ return 0, nil, nil, errors.New("target name cannot be specified for %v", k)
+ }
+ if !k.IsValid() {
+ return 0, nil, nil, errors.New("invalid kind: %d", k)
+ }
+ return k, nil, nil, nil
}
- if md, ok := d.(protoreflect.MessageDescriptor); ok {
- return md, nil
- }
- return nil, errors.New("invalid descriptor type")
}
-func findDescriptor(s string, isWeak bool, r *resolver) (protoreflect.Descriptor, error) {
- if !strings.HasPrefix(s, ".") {
- return nil, errors.New("identifier name must be fully qualified with a leading dot: %v", s)
+// findDescriptor finds the descriptor by name,
+// which may be a relative name within some scope.
+//
+// Suppose the scope was "fizz.buzz" and the reference was "Foo.Bar",
+// then the following full names are searched:
+// * fizz.buzz.Foo.Bar
+// * fizz.Foo.Bar
+// * Foo.Bar
+func (r *resolver) findDescriptor(scope protoreflect.FullName, ref partialName) (protoreflect.Descriptor, error) {
+ if !ref.IsValid() {
+ return nil, errors.New("invalid name reference: %q", ref)
}
- name := protoreflect.FullName(strings.TrimPrefix(s, "."))
- d, err := r.FindDescriptorByName(name)
- if err != nil {
+ if ref.IsFull() {
+ scope, ref = "", ref[1:]
+ }
+ var foundButNotImported protoreflect.Descriptor
+ for {
+ // Derive the full name to search.
+ s := protoreflect.FullName(ref)
+ if scope != "" {
+ s = scope + "." + s
+ }
+
+ // Check the current file for the descriptor.
+ if d, ok := r.local[s]; ok {
+ return d, nil
+ }
+
+ // Check the remote registry for the descriptor.
+ d, err := r.remote.FindDescriptorByName(s)
+ if err == nil {
+ // Only allow descriptors covered by one of the imports.
+ if r.imports[d.ParentFile().Path()] {
+ return d, nil
+ }
+ foundButNotImported = d
+ } else if err != protoregistry.NotFound {
+ return nil, err
+ }
+
+ // Continue on at a higher level of scoping.
+ if scope == "" {
+ if d := foundButNotImported; d != nil {
+ return nil, errors.New("resolved %q, but %q is not imported", d.FullName(), d.ParentFile().Path())
+ }
+ return nil, protoregistry.NotFound
+ }
+ scope = scope.Parent()
+ }
+}
+
+func (r *resolver) findEnumDescriptor(scope protoreflect.FullName, ref partialName, isWeak bool) (protoreflect.EnumDescriptor, error) {
+ d, err := r.findDescriptor(scope, ref)
+ if err == protoregistry.NotFound && (r.allowUnresolvable || isWeak) {
+ return filedesc.PlaceholderEnum(ref.FullName()), nil
+ } else if err == protoregistry.NotFound {
+ return nil, errors.New("%q not found", ref.FullName())
+ } else if err != nil {
return nil, err
}
- if err := r.imports.check(d); err != nil {
+ ed, ok := d.(protoreflect.EnumDescriptor)
+ if !ok {
+ return nil, errors.New("resolved %q, but it is not an enum", d.FullName())
+ }
+ return ed, nil
+}
+
+func (r *resolver) findMessageDescriptor(scope protoreflect.FullName, ref partialName, isWeak bool) (protoreflect.MessageDescriptor, error) {
+ d, err := r.findDescriptor(scope, ref)
+ if err == protoregistry.NotFound && (r.allowUnresolvable || isWeak) {
+ return filedesc.PlaceholderMessage(ref.FullName()), nil
+ } else if err == protoregistry.NotFound {
+ return nil, errors.New("%q not found", ref.FullName())
+ } else if err != nil {
return nil, err
}
- return d, nil
+ md, ok := d.(protoreflect.MessageDescriptor)
+ if !ok {
+ return nil, errors.New("resolved %q, but it is not an message", d.FullName())
+ }
+ return md, nil
+}
+
+// partialName is the partial name. A leading dot means that the name is full,
+// otherwise the name is relative to some current scope.
+// See google.protobuf.FieldDescriptorProto.type_name.
+type partialName string
+
+func (s partialName) IsFull() bool {
+ return len(s) > 0 && s[0] == '.'
+}
+
+func (s partialName) IsValid() bool {
+ if s.IsFull() {
+ return protoreflect.FullName(s[1:]).IsValid()
+ }
+ return protoreflect.FullName(s).IsValid()
+}
+
+const unknownPrefix = "*."
+
+// FullName converts the partial name to a full name on a best-effort basis.
+// If relative, it creates an invalid full name, using a "*." prefix
+// to indicate that the start of the full name is unknown.
+func (s partialName) FullName() protoreflect.FullName {
+ if s.IsFull() {
+ return protoreflect.FullName(s[1:])
+ }
+ return protoreflect.FullName(unknownPrefix + s)
+}
+
+func unmarshalDefault(s string, fd protoreflect.FieldDescriptor, allowUnresolvable bool) (protoreflect.Value, protoreflect.EnumValueDescriptor, error) {
+ var evs protoreflect.EnumValueDescriptors
+ if fd.Enum() != nil {
+ evs = fd.Enum().Values()
+ }
+ v, ev, err := defval.Unmarshal(s, fd.Kind(), evs, defval.Descriptor)
+ if err != nil && allowUnresolvable && evs != nil && protoreflect.Name(s).IsValid() {
+ v = protoreflect.ValueOf(protoreflect.EnumNumber(0))
+ if evs.Len() > 0 {
+ v = protoreflect.ValueOf(evs.Get(0).Number())
+ }
+ ev = filedesc.PlaceholderEnumValue(fd.Enum().FullName().Parent().Append(protoreflect.Name(s)))
+ } else if err != nil {
+ return v, ev, err
+ }
+ // TODO: Validate that the default is not specified under proto3.
+ // TODO: Validate that the default is not specified for composite types.
+ return v, ev, nil
}
diff --git a/reflect/protodesc/file_test.go b/reflect/protodesc/file_test.go
new file mode 100644
index 0000000..45910fc
--- /dev/null
+++ b/reflect/protodesc/file_test.go
@@ -0,0 +1,249 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package protodesc
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "google.golang.org/protobuf/encoding/prototext"
+ "google.golang.org/protobuf/proto"
+ "google.golang.org/protobuf/reflect/protoregistry"
+
+ "google.golang.org/protobuf/types/descriptorpb"
+)
+
+func mustParseFile(s string) *descriptorpb.FileDescriptorProto {
+ pb := new(descriptorpb.FileDescriptorProto)
+ if err := prototext.Unmarshal([]byte(s), pb); err != nil {
+ panic(err)
+ }
+ return pb
+}
+
+func cloneFile(in *descriptorpb.FileDescriptorProto) *descriptorpb.FileDescriptorProto {
+ out := new(descriptorpb.FileDescriptorProto)
+ proto.Merge(out, in)
+ return out
+}
+
+func TestNewFile(t *testing.T) {
+ tests := []struct {
+ label string
+ inDeps []*descriptorpb.FileDescriptorProto
+ inDesc *descriptorpb.FileDescriptorProto
+ inOpts []option
+ wantDesc *descriptorpb.FileDescriptorProto
+ wantErr string
+ }{{
+ label: "resolve relative reference",
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ message_type: [{
+ name: "A"
+ field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:"B.C"}]
+ nested_type: [{name: "B"}]
+ }, {
+ name: "B"
+ nested_type: [{name: "C"}]
+ }]
+ `),
+ wantDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ message_type: [{
+ name: "A"
+ field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:".fizz.buzz.B.C"}]
+ nested_type: [{name: "B"}]
+ }, {
+ name: "B"
+ nested_type: [{name: "C"}]
+ }]
+ `),
+ }, {
+ label: "resolve the wrong type",
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: ""
+ message_type: [{
+ name: "M"
+ field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:"E"}]
+ enum_type: [{name: "E" value: [{name:"V0" number:0}, {name:"V1" number:1}]}]
+ }]
+ `),
+ wantErr: `message field "M.F" cannot resolve type: resolved "M.E", but it is not an message`,
+ }, {
+ label: "auto-resolve unknown kind",
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: ""
+ message_type: [{
+ name: "M"
+ field: [{name:"F" number:1 label:LABEL_OPTIONAL type_name:"E"}]
+ enum_type: [{name: "E" value: [{name:"V0" number:0}, {name:"V1" number:1}]}]
+ }]
+ `),
+ wantDesc: mustParseFile(`
+ name: "test.proto"
+ package: ""
+ message_type: [{
+ name: "M"
+ field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_ENUM type_name:".M.E"}]
+ enum_type: [{name: "E" value: [{name:"V0" number:0}, {name:"V1" number:1}]}]
+ }]
+ `),
+ }, {
+ label: "unresolved import",
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ dependency: "remote.proto"
+ `),
+ wantErr: `could not resolve import "remote.proto": not found`,
+ }, {
+ label: "unresolved message field",
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ message_type: [{
+ name: "M"
+ field: [{name:"F1" number:1 label:LABEL_OPTIONAL type:TYPE_ENUM type_name:"some.other.enum" default_value:"UNKNOWN"}]
+ }]
+ `),
+ wantErr: `message field "fizz.buzz.M.F1" cannot resolve type: "*.some.other.enum" not found`,
+ }, {
+ label: "unresolved default enum value",
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ message_type: [{
+ name: "M"
+ field: [{name:"F1" number:1 label:LABEL_OPTIONAL type:TYPE_ENUM type_name:"E" default_value:"UNKNOWN"}]
+ enum_type: [{name:"E" value:[{name:"V0" number:0}]}]
+ }]
+ `),
+ wantErr: `message field "fizz.buzz.M.F1" has invalid default: could not parse value for enum: "UNKNOWN"`,
+ }, {
+ label: "allowed unresolved default enum value",
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ message_type: [{
+ name: "M"
+ field: [{name:"F1" number:1 label:LABEL_OPTIONAL type:TYPE_ENUM type_name:".fizz.buzz.M.E" default_value:"UNKNOWN"}]
+ enum_type: [{name:"E" value:[{name:"V0" number:0}]}]
+ }]
+ `),
+ inOpts: []option{allowUnresolvable()},
+ }, {
+ label: "unresolved extendee",
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ extension: [{name:"X" number:1 label:LABEL_OPTIONAL extendee:"some.extended.message" type:TYPE_MESSAGE type_name:"some.other.message"}]
+ `),
+ wantErr: `extension field "fizz.buzz.X" cannot resolve extendee: "*.some.extended.message" not found`,
+ }, {
+ label: "unresolved method input",
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ service: [{
+ name: "S"
+ method: [{name:"M" input_type:"foo.bar.input" output_type:".absolute.foo.bar.output"}]
+ }]
+ `),
+ wantErr: `service method "fizz.buzz.S.M" cannot resolve input: "*.foo.bar.input" not found`,
+ }, {
+ label: "allowed unresolved references",
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ dependency: "remote.proto"
+ message_type: [{
+ name: "M"
+ field: [{name:"F1" number:1 label:LABEL_OPTIONAL type_name:"some.other.enum" default_value:"UNKNOWN"}]
+ }]
+ extension: [{name:"X" number:1 label:LABEL_OPTIONAL extendee:"some.extended.message" type:TYPE_MESSAGE type_name:"some.other.message"}]
+ service: [{
+ name: "S"
+ method: [{name:"M" input_type:"foo.bar.input" output_type:".absolute.foo.bar.output"}]
+ }]
+ `),
+ inOpts: []option{allowUnresolvable()},
+ }, {
+ label: "resolved but not imported",
+ inDeps: []*descriptorpb.FileDescriptorProto{mustParseFile(`
+ name: "dep.proto"
+ package: "fizz"
+ message_type: [{name:"M" nested_type:[{name:"M"}]}]
+ `)},
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ message_type: [{
+ name: "M"
+ field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:"M.M"}]
+ }]
+ `),
+ wantErr: `message field "fizz.buzz.M.F" cannot resolve type: resolved "fizz.M.M", but "dep.proto" is not imported`,
+ }, {
+ label: "resolved from remote import",
+ inDeps: []*descriptorpb.FileDescriptorProto{mustParseFile(`
+ name: "dep.proto"
+ package: "fizz"
+ message_type: [{name:"M" nested_type:[{name:"M"}]}]
+ `)},
+ inDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ dependency: "dep.proto"
+ message_type: [{
+ name: "M"
+ field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:"M.M"}]
+ }]
+ `),
+ wantDesc: mustParseFile(`
+ name: "test.proto"
+ package: "fizz.buzz"
+ dependency: "dep.proto"
+ message_type: [{
+ name: "M"
+ field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:".fizz.M.M"}]
+ }]
+ `),
+ }}
+
+ for _, tt := range tests {
+ t.Run(tt.label, func(t *testing.T) {
+ r := new(protoregistry.Files)
+ for i, dep := range tt.inDeps {
+ f, err := newFile(dep, r)
+ if err != nil {
+ t.Fatalf("dependency %d: unexpected NewFile() error: %v", i, err)
+ }
+ if err := r.Register(f); err != nil {
+ t.Fatalf("dependency %d: unexpected Register() error: %v", i, err)
+ }
+ }
+ var gotDesc *descriptorpb.FileDescriptorProto
+ if tt.wantErr == "" && tt.wantDesc == nil {
+ tt.wantDesc = cloneFile(tt.inDesc)
+ }
+ gotFile, err := newFile(tt.inDesc, r, tt.inOpts...)
+ if gotFile != nil {
+ gotDesc = ToFileDescriptorProto(gotFile)
+ }
+ if !proto.Equal(gotDesc, tt.wantDesc) {
+ t.Errorf("NewFile() mismatch:\ngot %v\nwant %v", gotDesc, tt.wantDesc)
+ }
+ if ((err == nil) != (tt.wantErr == "")) || !strings.Contains(fmt.Sprint(err), tt.wantErr) {
+ t.Errorf("NewFile() error:\ngot: %v\nwant: %v", err, tt.wantErr)
+ }
+ })
+ }
+}
diff --git a/reflect/protodesc/proto.go b/reflect/protodesc/proto.go
index f3fa81e..575250e 100644
--- a/reflect/protodesc/proto.go
+++ b/reflect/protodesc/proto.go
@@ -7,6 +7,7 @@
import (
"fmt"
"reflect"
+ "strings"
"google.golang.org/protobuf/internal/encoding/defval"
"google.golang.org/protobuf/internal/scalar"
@@ -102,16 +103,18 @@
Name: scalar.String(string(field.Name())),
Number: scalar.Int32(int32(field.Number())),
Label: descriptorpb.FieldDescriptorProto_Label(field.Cardinality()).Enum(),
- Type: descriptorpb.FieldDescriptorProto_Type(field.Kind()).Enum(),
Options: clone(field.Options()).(*descriptorpb.FieldOptions),
}
if field.IsExtension() {
p.Extendee = fullNameOf(field.ContainingMessage())
}
- switch field.Kind() {
- case protoreflect.EnumKind:
+ if field.Kind().IsValid() {
+ p.Type = descriptorpb.FieldDescriptorProto_Type(field.Kind()).Enum()
+ }
+ if field.Enum() != nil {
p.TypeName = fullNameOf(field.Enum())
- case protoreflect.MessageKind, protoreflect.GroupKind:
+ }
+ if field.Message() != nil {
p.TypeName = fullNameOf(field.Message())
}
if field.HasJSONName() {
@@ -119,7 +122,9 @@
}
if field.HasDefault() {
def, err := defval.Marshal(field.Default(), field.DefaultEnumValue(), field.Kind(), defval.Descriptor)
- if err != nil {
+ if err != nil && field.DefaultEnumValue() != nil {
+ def = string(field.DefaultEnumValue().Name()) // occurs for unresolved enum values
+ } else if err != nil {
panic(fmt.Sprintf("%v: %v", field.FullName(), err))
}
p.DefaultValue = scalar.String(def)
@@ -207,6 +212,9 @@
if d == nil {
return nil
}
+ if strings.HasPrefix(string(d.FullName()), unknownPrefix) {
+ return scalar.String(string(d.FullName()[len(unknownPrefix):]))
+ }
return scalar.String("." + string(d.FullName()))
}
diff --git a/reflect/protodesc/protodesc_test.go b/reflect/protodesc/protodesc_test.go
index 24938a4..dabda29 100644
--- a/reflect/protodesc/protodesc_test.go
+++ b/reflect/protodesc/protodesc_test.go
@@ -8,21 +8,12 @@
"strings"
"testing"
- "google.golang.org/protobuf/encoding/prototext"
"google.golang.org/protobuf/internal/scalar"
"google.golang.org/protobuf/reflect/protoregistry"
"google.golang.org/protobuf/types/descriptorpb"
)
-func mustParseFile(s string) *descriptorpb.FileDescriptorProto {
- pb := new(descriptorpb.FileDescriptorProto)
- if err := prototext.Unmarshal([]byte(s), pb); err != nil {
- panic(err)
- }
- return pb
-}
-
// Tests validation logic for malformed descriptors.
func TestNewFile_ValidationErrors(t *testing.T) {
testCases := []struct {
@@ -144,7 +135,7 @@
}},
}},
},
- wantErr: "type_name",
+ wantErr: `message field "foo.BadMessage.bad_field" cannot resolve type: target name cannot be specified for int32`,
}, {
name: "type_name on string extension",
deps: []*descriptorpb.FileDescriptorProto{{
@@ -180,7 +171,7 @@
TypeName: scalar.String("AnEnum"),
}},
},
- wantErr: "type_name",
+ wantErr: `extension field "bar.my_ext" cannot resolve type: target name cannot be specified for string`,
}, {
name: "oneof_index on extension",
deps: []*descriptorpb.FileDescriptorProto{{
@@ -299,7 +290,7 @@
}},
}},
},
- wantErr: "foo.Foo without import of foo.proto",
+ wantErr: `message field "bar.Bar.foo" cannot resolve type: resolved "foo.Foo", but "foo.proto" is not imported`,
}, {
name: "enum dependency without import",
deps: []*descriptorpb.FileDescriptorProto{{
@@ -334,7 +325,7 @@
}},
}},
},
- wantErr: "foo.Foo without import of foo.proto",
+ wantErr: `message field "bar.Bar.foo" cannot resolve type: resolved "foo.Foo", but "foo.proto" is not imported`,
}, {
name: "message dependency on without import on file imported by a public import",
deps: []*descriptorpb.FileDescriptorProto{{
@@ -377,7 +368,7 @@
}},
}},
},
- wantErr: "foo.Foo without import of foo.proto",
+ wantErr: `message field "bar.Bar.foo" cannot resolve type: resolved "foo.Foo", but "foo.proto" is not imported`,
}}
for _, tc := range testCases {