From b2107fbd8d01726dec0f018b42b78e5e88d0f8ca Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Sat, 29 Jun 2019 01:31:37 -0700 Subject: [PATCH] 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 --- internal/encoding/defval/default.go | 4 +- reflect/protodesc/desc.go | 57 ++++- reflect/protodesc/desc_resolve.go | 313 +++++++++++++++++----------- reflect/protodesc/file_test.go | 249 ++++++++++++++++++++++ reflect/protodesc/proto.go | 18 +- reflect/protodesc/protodesc_test.go | 19 +- 6 files changed, 506 insertions(+), 154 deletions(-) create mode 100644 reflect/protodesc/file_test.go diff --git a/internal/encoding/defval/default.go b/internal/encoding/defval/default.go index f03acdf4..7cde9450 100644 --- a/internal/encoding/defval/default.go +++ b/internal/encoding/defval/default.go @@ -112,7 +112,7 @@ func Unmarshal(s string, k pref.Kind, evs pref.EnumValueDescriptors, f Format) ( 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 @@ func Marshal(v pref.Value, ev pref.EnumValueDescriptor, k pref.Kind, f Format) ( 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 4c39996d..012748bc 100644 --- a/reflect/protodesc/desc.go +++ b/reflect/protodesc/desc.go @@ -9,6 +9,7 @@ package protodesc 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 @@ type Resolver interface { 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 @@ type Resolver interface { // 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 @@ func NewFile(fd *descriptorpb.FileDescriptorProto, r Resolver) (protoreflect.Fil 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 @@ func NewFile(fd *descriptorpb.FileDescriptorProto, r Resolver) (protoreflect.Fil } // 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 @@ func (is importSet) importPublic(imps protoreflect.FileImports) { } } } - -// 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 43721543..25e2c3bf 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 @@ import ( "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 @@ func (r *resolver) resolveMessageDependencies(ms []filedesc.Message, mds []*desc } 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 @@ func (r *resolver) resolveMessageDependencies(ms []filedesc.Message, mds []*desc 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 @@ func (r *resolver) resolveServiceDependencies(ss []filedesc.Service, sds []*desc 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 { - return nil, err + if ref.IsFull() { + scope, ref = "", ref[1:] } - if err := r.imports.check(d); err != nil { - return nil, err + 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() } - return d, nil +} + +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 + } + 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 + } + 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 00000000..45910fcf --- /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 f3fa81e0..575250eb 100644 --- a/reflect/protodesc/proto.go +++ b/reflect/protodesc/proto.go @@ -7,6 +7,7 @@ package protodesc import ( "fmt" "reflect" + "strings" "google.golang.org/protobuf/internal/encoding/defval" "google.golang.org/protobuf/internal/scalar" @@ -102,16 +103,18 @@ func ToFieldDescriptorProto(field protoreflect.FieldDescriptor) *descriptorpb.Fi 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 @@ func ToFieldDescriptorProto(field protoreflect.FieldDescriptor) *descriptorpb.Fi } 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 @@ func fullNameOf(d protoreflect.Descriptor) *string { 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 24938a44..dabda29b 100644 --- a/reflect/protodesc/protodesc_test.go +++ b/reflect/protodesc/protodesc_test.go @@ -8,21 +8,12 @@ import ( "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 @@ func TestNewFile_ValidationErrors(t *testing.T) { }}, }}, }, - 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 @@ func TestNewFile_ValidationErrors(t *testing.T) { 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 @@ func TestNewFile_ValidationErrors(t *testing.T) { }}, }}, }, - 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 @@ func TestNewFile_ValidationErrors(t *testing.T) { }}, }}, }, - 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 @@ func TestNewFile_ValidationErrors(t *testing.T) { }}, }}, }, - 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 {