Skip to content

Commit

Permalink
Update desc/sourceinfo to not wrap descriptors (#622)
Browse files Browse the repository at this point in the history
The `desc/sourceinfo` package previously wrapped descriptors, by
returning an implementation of the various `protoreflect.Descriptor`
interfaces that _embedded_ the original descriptor values. However,
these various interfaces are all defined as "[do not
implement](https://github.com/protocolbuffers/protobuf-go/blob/c33baa8f3a0d35fd5a39e43c22a50a050f707d34/reflect/protoreflect/type.go#L111)".
So, to avoid future issues where the protobuf runtime may type-assert
implementations to their own internal types (which would panic/fail with
the concrete wrapper types in this package), we no longer wrap this way.

This also happens to resolve #618 (since there's no more wrapping, an
incorrect typed-nil wrapper can't escape).
  • Loading branch information
jhump authored Sep 4, 2024
1 parent 645b9dd commit 06e5c08
Show file tree
Hide file tree
Showing 22 changed files with 879 additions and 854 deletions.
18 changes: 16 additions & 2 deletions desc/descriptor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,22 @@ func TestFileDescriptorObjectGraph(t *testing.T) {
},
},
}},
"enums": {(*FileDescriptor).GetEnumTypes, nil},
"services": {(*FileDescriptor).GetServices, nil},
"enums": {(*FileDescriptor).GetEnumTypes, nil},
"services": {(*FileDescriptor).GetServices, []descCase{
{
name: "testprotos.SomeService",
references: map[string]childCases{
"methods": {(*ServiceDescriptor).GetMethods, []descCase{
{
name: "testprotos.SomeService.SomeRPC",
},
{
name: "testprotos.SomeService.SomeOtherRPC",
},
}},
},
},
}},
"extensions": {(*FileDescriptor).GetExtensions, []descCase{
{
name: "testprotos.xtm",
Expand Down
36 changes: 0 additions & 36 deletions desc/imports.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ var (
// package or when the alternate path is only used from one file (so you don't
// want the alternate path used when loading every other file), use an
// ImportResolver instead.
//
// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
// correct and that descriptors can be linked during package initialization. So
// registering alternate paths is no longer useful or necessary.
func RegisterImportPath(registerPath, importPath string) {
if len(importPath) == 0 {
panic("import path cannot be empty")
Expand All @@ -60,10 +56,6 @@ func RegisterImportPath(registerPath, importPath string) {
// ResolveImport resolves the given import path. If it has been registered as an
// alternate via RegisterImportPath, the registered path is returned. Otherwise,
// the given import path is returned unchanged.
//
// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
// correct and that descriptors can be linked during package initialization. So
// registering alternate paths is no longer useful or necessary.
func ResolveImport(importPath string) string {
importPath = clean(importPath)
globalImportPathMu.RLock()
Expand Down Expand Up @@ -244,76 +236,48 @@ func (r *ImportResolver) registerImportPathFrom(registerPath, importPath, source
// LoadFileDescriptor is the same as the package function of the same name, but
// any alternate paths configured in this resolver are used when linking the
// given descriptor proto.
//
// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
// correct and that descriptors can be linked during package initialization. So
// registering alternate paths is no longer useful or necessary.
func (r *ImportResolver) LoadFileDescriptor(filePath string) (*FileDescriptor, error) {
return LoadFileDescriptor(filePath)
}

// LoadMessageDescriptor is the same as the package function of the same name,
// but any alternate paths configured in this resolver are used when linking
// files for the returned descriptor.
//
// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
// correct and that descriptors can be linked during package initialization. So
// registering alternate paths is no longer useful or necessary.
func (r *ImportResolver) LoadMessageDescriptor(msgName string) (*MessageDescriptor, error) {
return LoadMessageDescriptor(msgName)
}

// LoadMessageDescriptorForMessage is the same as the package function of the
// same name, but any alternate paths configured in this resolver are used when
// linking files for the returned descriptor.
//
// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
// correct and that descriptors can be linked during package initialization. So
// registering alternate paths is no longer useful or necessary.
func (r *ImportResolver) LoadMessageDescriptorForMessage(msg proto.Message) (*MessageDescriptor, error) {
return LoadMessageDescriptorForMessage(msg)
}

// LoadMessageDescriptorForType is the same as the package function of the same
// name, but any alternate paths configured in this resolver are used when
// linking files for the returned descriptor.
//
// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
// correct and that descriptors can be linked during package initialization. So
// registering alternate paths is no longer useful or necessary.
func (r *ImportResolver) LoadMessageDescriptorForType(msgType reflect.Type) (*MessageDescriptor, error) {
return LoadMessageDescriptorForType(msgType)
}

// LoadEnumDescriptorForEnum is the same as the package function of the same
// name, but any alternate paths configured in this resolver are used when
// linking files for the returned descriptor.
//
// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
// correct and that descriptors can be linked during package initialization. So
// registering alternate paths is no longer useful or necessary.
func (r *ImportResolver) LoadEnumDescriptorForEnum(enum protoEnum) (*EnumDescriptor, error) {
return LoadEnumDescriptorForEnum(enum)
}

// LoadEnumDescriptorForType is the same as the package function of the same
// name, but any alternate paths configured in this resolver are used when
// linking files for the returned descriptor.
//
// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
// correct and that descriptors can be linked during package initialization. So
// registering alternate paths is no longer useful or necessary.
func (r *ImportResolver) LoadEnumDescriptorForType(enumType reflect.Type) (*EnumDescriptor, error) {
return LoadEnumDescriptorForType(enumType)
}

// LoadFieldDescriptorForExtension is the same as the package function of the
// same name, but any alternate paths configured in this resolver are used when
// linking files for the returned descriptor.
//
// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
// correct and that descriptors can be linked during package initialization. So
// registering alternate paths is no longer useful or necessary.
func (r *ImportResolver) LoadFieldDescriptorForExtension(ext *proto.ExtensionDesc) (*FieldDescriptor, error) {
return LoadFieldDescriptorForExtension(ext)
}
Expand Down
7 changes: 4 additions & 3 deletions desc/load.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package desc

import (
"errors"
"fmt"
"reflect"
"sync"
Expand Down Expand Up @@ -33,7 +34,7 @@ var (
// re-processed if the same file is fetched again later.
func LoadFileDescriptor(file string) (*FileDescriptor, error) {
d, err := sourceinfo.GlobalFiles.FindFileByPath(file)
if err == protoregistry.NotFound {
if errors.Is(err, protoregistry.NotFound) {
// for backwards compatibility, see if this matches a known old
// alias for the file (older versions of libraries that registered
// the files using incorrect/non-canonical paths)
Expand All @@ -42,7 +43,7 @@ func LoadFileDescriptor(file string) (*FileDescriptor, error) {
}
}
if err != nil {
if err != protoregistry.NotFound {
if !errors.Is(err, protoregistry.NotFound) {
return nil, internal.ErrNoSuchFile(file)
}
return nil, err
Expand All @@ -64,7 +65,7 @@ func LoadFileDescriptor(file string) (*FileDescriptor, error) {
func LoadMessageDescriptor(message string) (*MessageDescriptor, error) {
mt, err := sourceinfo.GlobalTypes.FindMessageByName(protoreflect.FullName(message))
if err != nil {
if err == protoregistry.NotFound {
if errors.Is(err, protoregistry.NotFound) {
return nil, nil
}
return nil, err
Expand Down
7 changes: 7 additions & 0 deletions desc/protoprint/testfiles/desc_test1-compact.proto
Original file line number Diff line number Diff line change
Expand Up @@ -107,3 +107,10 @@ extend AnotherTestMessage {
// Comment for xui
optional uint64 xui = 103;
}
// Comment for SomeService
service SomeService {
// Comment for SomeRPC
rpc SomeRPC ( TestMessage ) returns ( TestMessage );
// Comment for SomeOtherRPC
rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
}
9 changes: 9 additions & 0 deletions desc/protoprint/testfiles/desc_test1-custom-sort.proto
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ extend AnotherTestMessage {
optional string xs = 101;
}

// Comment for SomeService
service SomeService {
// Comment for SomeRPC
rpc SomeRPC ( TestMessage ) returns ( TestMessage );

// Comment for SomeOtherRPC
rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
}

// Comment for TestMessage
message TestMessage {
// Comment for NestedEnum
Expand Down
9 changes: 9 additions & 0 deletions desc/protoprint/testfiles/desc_test1-default.proto
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,12 @@ extend AnotherTestMessage {
// Comment for xui
optional uint64 xui = 103;
}

// Comment for SomeService
service SomeService {
// Comment for SomeRPC
rpc SomeRPC ( TestMessage ) returns ( TestMessage );

// Comment for SomeOtherRPC
rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
}
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,12 @@ extend AnotherTestMessage {
/* Comment for xui */
optional uint64 xui = 103;
}

/* Comment for SomeService */
service SomeService {
/* Comment for SomeRPC */
rpc SomeRPC ( TestMessage ) returns ( TestMessage );

/* Comment for SomeOtherRPC */
rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
}
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,12 @@ extend AnotherTestMessage {
// Comment for xui
optional uint64 xui = 103;
}

// Comment for SomeService
service SomeService {
// Comment for SomeRPC
rpc SomeRPC ( TestMessage ) returns ( TestMessage );

// Comment for SomeOtherRPC
rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
}
9 changes: 9 additions & 0 deletions desc/protoprint/testfiles/desc_test1-only-doc-comments.proto
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,12 @@ extend AnotherTestMessage {
// Comment for xui
optional uint64 xui = 103;
}

// Comment for SomeService
service SomeService {
// Comment for SomeRPC
rpc SomeRPC ( TestMessage ) returns ( TestMessage );

// Comment for SomeOtherRPC
rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
}
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,15 @@ message TestMessage {
}
}

/* Comment for SomeService */
service SomeService {
/* Comment for SomeOtherRPC */
rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );

/* Comment for SomeRPC */
rpc SomeRPC ( TestMessage ) returns ( TestMessage );
}

/* Comment for AnotherTestMessage extensions (2) */
extend AnotherTestMessage {
/* Comment for xtm */
Expand Down
9 changes: 9 additions & 0 deletions desc/protoprint/testfiles/desc_test1-sorted.proto
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,15 @@ message TestMessage {
}
}

// Comment for SomeService
service SomeService {
// Comment for SomeOtherRPC
rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );

// Comment for SomeRPC
rpc SomeRPC ( TestMessage ) returns ( TestMessage );
}

// Comment for AnotherTestMessage extensions (2)
extend AnotherTestMessage {
// Comment for xtm
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,12 @@ extend AnotherTestMessage {
// Comment for xui
optional uint64 xui = 103;
}

// Comment for SomeService
service SomeService {
// Comment for SomeRPC
rpc SomeRPC ( TestMessage ) returns ( TestMessage );

// Comment for SomeOtherRPC
rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
}
17 changes: 17 additions & 0 deletions desc/sourceinfo/export_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package sourceinfo

import "google.golang.org/protobuf/reflect/protoreflect"

// exported for tests

func CanUpgrade(d protoreflect.Descriptor) bool {
return canUpgrade(d)
}

func UpdateDescriptor[D protoreflect.Descriptor](d D) (D, error) {
return updateDescriptor(d)
}

func UpdateField(fld protoreflect.FieldDescriptor) (protoreflect.FieldDescriptor, error) {
return updateField(fld)
}
Loading

0 comments on commit 06e5c08

Please sign in to comment.