Skip to content
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.

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

refactor: Relax runtime checks in MergedRegistry #43

Merged
merged 13 commits into from
Feb 16, 2023
Merged

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Feb 15, 2023

Previous PR #37 introduced 2 runtime checks:

  • require that import paths are correct
  • require that file descriptors match

This PR still performs those checks, but logs warnings instead of throwing error.


When calling proto.MergedRegistry, we would have the following warning:

Got 2 file descriptor import path errors and 14 file descriptor mismatches. Run `proto.DebugFileDescriptorsMismatch` to debug them.

This is probably hard to debug, so I added a new helper function DebugFileDescriptorsMismatch, which errors with:

-- snip --
Got 2 file descriptor import path errors:
	file name internal/errors.proto does not start with expected grpc/gateway/runtime/; please make sure your folder structure matches the proto files fully-qualified names
	file name grpc/binlog/v1/binarylog.proto does not start with expected grpc/binarylog/v1/; please make sure your folder structure matches the proto files fully-qualified names
Got 14 file descriptor mismatches. Make sure gogoproto and protoregistry use the same .proto files. '-' lines are from protoregistry, '+' lines from gogo's registry.

	Mismatch in cosmos/msg/v1/msg.proto:
  (*descriptorpb.FileDescriptorProto)(Inverse(protocmp.Transform, protocmp.Message{
  	... // 2 identical entries
  	"extension": []protocmp.Message{{"@type": s"google.protobuf.FieldDescriptorProto", "extendee": string(".google.protobuf.ServiceOptions"), "json_name": string("service"), "label": s"LABEL_OPTIONAL", ...}, {"@type": s"google.protobuf.FieldDescriptorProto", "extendee": string(".google.protobuf.MessageOptions"), "json_name": string("signer"), "label": s"LABEL_REPEATED", ...}},
  	"name":      string("cosmos/msg/v1/msg.proto"),
  	"options": protocmp.Message{
+ 		"63017":            protoreflect.RawFields{0xc8, 0xe2, 0x1e, 0x01},
+ 		"63018":            protoreflect.RawFields{0xd0, 0xe2, 0x1e, 0x01},
+ 		"63020":            protoreflect.RawFields{0xe0, 0xe2, 0x1e, 0x01},
+ 		"63026":            protoreflect.RawFields{0x90, 0xe3, 0x1e, 0x00},
+ 		"63034":            protoreflect.RawFields{0xd0, 0xe3, 0x1e, 0x00},
+ 		"63035":            protoreflect.RawFields{0xd8, 0xe3, 0x1e, 0x00},
  		"@type":            s"google.protobuf.FileOptions",
- 		"csharp_namespace": string("Cosmos.Msg.V1"),
  		"go_package": strings.Join({
- 			"cosmossdk.io/api/cosmos/msg/v1;msgv1",
+ 			"github.com/cosmos/cosmos-sdk/types/msgservice",
  		}, ""),
- 		"java_multiple_files":    bool(true),
- 		"java_outer_classname":   string("MsgProto"),
- 		"java_package":           string("com.cosmos.msg.v1"),
- 		"objc_class_prefix":      string("CMX"),
- 		"php_metadata_namespace": string(`Cosmos\Msg\V1\GPBMetadata`),
- 		"php_namespace":          string(`Cosmos\Msg\V1`),
- 		"ruby_package":           string("Cosmos::Msg::V1"),
  	},
  	"package": string("cosmos.msg.v1"),
  	"syntax":  string("proto3"),
  }))
-- snip --

@amaury1093 amaury1093 marked this pull request as ready for review February 15, 2023 13:25
@amaury1093 amaury1093 requested a review from a team as a code owner February 15, 2023 13:25
@amaury1093 amaury1093 marked this pull request as draft February 16, 2023 10:44
@amaury1093 amaury1093 marked this pull request as ready for review February 16, 2023 11:50
proto/merge.go Outdated Show resolved Hide resolved
proto/merge.go Outdated Show resolved Hide resolved
amaury1093 and others added 2 commits February 16, 2023 16:28
@amaury1093 amaury1093 merged commit 61eccf0 into main Feb 16, 2023
@amaury1093 amaury1093 deleted the am/relax-checks branch February 16, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants