-
Notifications
You must be signed in to change notification settings - Fork 4.1k
feat(depinject/appconfig): support gogo proto module configs #20540
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
Changes from 12 commits
b4e4d53
e784156
d7d96dc
bd58e40
8f668c2
365842e
4371065
cbb40bd
2c30eb4
4404794
6cfcb8b
088a792
7380623
563dfed
360b40f
c4a56c6
84efdb5
6fc9b2c
e543c0a
e057097
217ecc1
9211087
ab8811c
886a1d9
1067839
f370a1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,25 +2,29 @@ package appconfig | |
|
|
||
| import ( | ||
| "fmt" | ||
| "reflect" | ||
| "strings" | ||
|
|
||
| "github.com/cosmos/cosmos-proto/anyutil" | ||
| gogoproto "github.com/cosmos/gogoproto/proto" | ||
| "google.golang.org/protobuf/encoding/protojson" | ||
| "google.golang.org/protobuf/proto" | ||
| protov2 "google.golang.org/protobuf/proto" | ||
| "google.golang.org/protobuf/reflect/protoreflect" | ||
| "google.golang.org/protobuf/reflect/protoregistry" | ||
| "google.golang.org/protobuf/types/known/anypb" | ||
| "sigs.k8s.io/yaml" | ||
|
|
||
| appv1alpha1 "cosmossdk.io/api/cosmos/app/v1alpha1" | ||
|
|
||
| "cosmossdk.io/depinject" | ||
| internal "cosmossdk.io/depinject/internal/appconfig" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usage of internal package not allowed. The use of Toolsgolangci-lint
|
||
| ) | ||
|
|
||
| // LoadJSON loads an app config in JSON format. | ||
| func LoadJSON(bz []byte) depinject.Config { | ||
| config := &appv1alpha1.Config{} | ||
| err := protojson.Unmarshal(bz, config) | ||
| err := protojson.UnmarshalOptions{ | ||
| Resolver: dynamicTypeResolver{resolver: gogoproto.HybridResolver}, | ||
| }.Unmarshal(bz, config) | ||
| if err != nil { | ||
| return depinject.Error(err) | ||
| } | ||
|
|
@@ -55,6 +59,11 @@ func Compose(appConfig *appv1alpha1.Config) depinject.Config { | |
| depinject.Supply(appConfig), | ||
| } | ||
|
|
||
| modules, err := internal.ModulesByModuleTypeName() | ||
| if err != nil { | ||
| return depinject.Error(err) | ||
| } | ||
|
|
||
| for _, module := range appConfig.Modules { | ||
| if module.Name == "" { | ||
| return depinject.Error(fmt.Errorf("module is missing name")) | ||
|
|
@@ -64,32 +73,45 @@ func Compose(appConfig *appv1alpha1.Config) depinject.Config { | |
| return depinject.Error(fmt.Errorf("module %q is missing a config object", module.Name)) | ||
| } | ||
|
|
||
| msgType, err := protoregistry.GlobalTypes.FindMessageByURL(module.Config.TypeUrl) | ||
| if err != nil { | ||
| return depinject.Error(err) | ||
| msgName := module.Config.TypeUrl | ||
| // strip type URL prefix | ||
| if slashIdx := strings.LastIndex(msgName, "/"); slashIdx >= 0 { | ||
| msgName = msgName[slashIdx+1:] | ||
| } | ||
|
|
||
| modules, err := internal.ModulesByProtoMessageName() | ||
| if err != nil { | ||
| return depinject.Error(err) | ||
| if msgName == "" { | ||
| return depinject.Error(fmt.Errorf("module %q is missing a type URL", module.Name)) | ||
| } | ||
|
|
||
| init, ok := modules[msgType.Descriptor().FullName()] | ||
| init, ok := modules[msgName] | ||
| if !ok { | ||
| modDesc := proto.GetExtension(msgType.Descriptor().Options(), appv1alpha1.E_Module).(*appv1alpha1.ModuleDescriptor) | ||
| if modDesc == nil { | ||
| return depinject.Error(fmt.Errorf("no module registered for type URL %s and that protobuf type does not have the option %s\n\n%s", | ||
| module.Config.TypeUrl, appv1alpha1.E_Module.TypeDescriptor().FullName(), dumpRegisteredModules(modules))) | ||
| if msgDesc, err := gogoproto.HybridResolver.FindDescriptorByName(protoreflect.FullName(msgName)); err == nil { | ||
| modDesc := protov2.GetExtension(msgDesc.Options(), appv1alpha1.E_Module).(*appv1alpha1.ModuleDescriptor) | ||
| if modDesc == nil { | ||
| return depinject.Error(fmt.Errorf("no module registered for type URL %s and that protobuf type does not have the option %s\n\n%s", | ||
| module.Config.TypeUrl, appv1alpha1.E_Module.TypeDescriptor().FullName(), dumpRegisteredModules(modules))) | ||
| } | ||
|
|
||
| return depinject.Error(fmt.Errorf("no module registered for type URL %s, did you forget to import %s: find more information on how to make a module ready for app wiring: https://docs.cosmos.network/main/building-modules/depinject\n\n%s", | ||
| module.Config.TypeUrl, modDesc.GoImport, dumpRegisteredModules(modules))) | ||
| } | ||
|
|
||
| return depinject.Error(fmt.Errorf("no module registered for type URL %s, did you forget to import %s: find more information on how to make a module ready for app wiring: https://docs.cosmos.network/main/building-modules/depinject\n\n%s", | ||
| module.Config.TypeUrl, modDesc.GoImport, dumpRegisteredModules(modules))) | ||
| } | ||
|
|
||
| config := init.ConfigProtoMessage.ProtoReflect().Type().New().Interface() | ||
| err = anypb.UnmarshalTo(module.Config, config, proto.UnmarshalOptions{}) | ||
| if err != nil { | ||
| return depinject.Error(err) | ||
| var config any | ||
| if configInit, ok := init.ConfigProtoMessage.(protov2.Message); ok { | ||
| configProto := configInit.ProtoReflect().Type().New().Interface() | ||
| err = anypb.UnmarshalTo(module.Config, configProto, protov2.UnmarshalOptions{}) | ||
| if err != nil { | ||
| return depinject.Error(err) | ||
| } | ||
| config = configProto | ||
| } else { | ||
| configProto := reflect.New(init.ConfigGoType.Elem()).Interface().(gogoproto.Message) | ||
| err = gogoproto.Unmarshal(module.Config.Value, configProto) | ||
| if err != nil { | ||
| return depinject.Error(err) | ||
| } | ||
| config = configProto | ||
| } | ||
|
|
||
| opts = append(opts, depinject.Supply(config)) | ||
|
|
@@ -114,10 +136,10 @@ func Compose(appConfig *appv1alpha1.Config) depinject.Config { | |
| return depinject.Configs(opts...) | ||
| } | ||
|
|
||
| func dumpRegisteredModules(modules map[protoreflect.FullName]*internal.ModuleInitializer) string { | ||
| func dumpRegisteredModules(modules map[string]*internal.ModuleInitializer) string { | ||
| var mods []string | ||
| for name := range modules { | ||
| mods = append(mods, " "+string(name)) | ||
| mods = append(mods, " "+name) | ||
| } | ||
| return fmt.Sprintf("registered modules are:\n%s", strings.Join(mods, "\n")) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import ( | |
| "cosmossdk.io/depinject/appconfig" | ||
| internal "cosmossdk.io/depinject/internal/appconfig" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usage of internal package not allowed. The use of Toolsgolangci-lint
|
||
| "cosmossdk.io/depinject/internal/appconfig/testpb" | ||
| testpbgogo "cosmossdk.io/depinject/internal/appconfiggogo/testpb" | ||
| ) | ||
|
|
||
| func expectContainerErrorContains(t *testing.T, option depinject.Config, contains string) { | ||
|
|
@@ -68,7 +69,10 @@ modules: | |
| "@type": testpb.TestModuleA | ||
| - name: b | ||
| config: | ||
| "@type": testpb.TestModuleB | ||
| "@type": /testpb.TestModuleB | ||
| - name: c | ||
| config: | ||
| "@type": /testpb.TestModuleGogo | ||
| `)) | ||
| assert.NilError(t, depinject.Inject(opt, &app)) | ||
| buf := &bytes.Buffer{} | ||
|
|
@@ -137,6 +141,10 @@ func init() { | |
| appconfig.RegisterModule(&testpb.TestModuleB{}, | ||
| appconfig.Provide(ProvideModuleB), | ||
| ) | ||
|
|
||
| appconfig.RegisterModule(&testpbgogo.TestModuleGogo{}, | ||
| appconfig.Provide(ProvideModuleC), | ||
| ) | ||
| } | ||
|
|
||
| func ProvideRuntimeState() *RuntimeState { | ||
|
|
@@ -220,3 +228,18 @@ type KeeperB interface { | |
| } | ||
|
|
||
| func (k keeperB) isKeeperB() {} | ||
|
|
||
| func ProvideModuleC(key StoreKey, b KeeperB) KeeperC { | ||
| return keeperC{key: key} | ||
| } | ||
|
|
||
| type keeperC struct { | ||
| key StoreKey | ||
| b KeeperB | ||
| } | ||
|
|
||
| type KeeperC interface { | ||
| isKeeperC() | ||
| } | ||
|
|
||
| func (k keeperC) isKeeperC() {} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| package appconfig | ||
|
|
||
| import ( | ||
| "strings" | ||
|
|
||
| "google.golang.org/protobuf/reflect/protodesc" | ||
| "google.golang.org/protobuf/reflect/protoreflect" | ||
| "google.golang.org/protobuf/reflect/protoregistry" | ||
| "google.golang.org/protobuf/types/dynamicpb" | ||
| ) | ||
|
|
||
| // dynamic resolver allows marshaling gogo proto messages from the gogoproto.HybridResolver as long as those | ||
| // files have been imported before calling LoadJSON. There is similar code in autocli, this should probably | ||
| // eventually be moved into a library. | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to refactor this code and the almost identical implementation in autocli to cosmos-proto possibly?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also move it to gogoproto because it uses the hybrid resolver and then call it here: https://github.com/cosmos/gogoproto/blob/6eec9731781bf8ec4feb0e4098ffb19ee74afb50/jsonpb/jsonpb.go#L792. So that gogoproto jsonpb.Unmarshal can be used to unmarshal v2 messages from json that contain some gogo proto messages in Any's. |
||
| type dynamicTypeResolver struct { | ||
| resolver protodesc.Resolver | ||
| } | ||
|
|
||
| func (r dynamicTypeResolver) FindExtensionByName(field protoreflect.FullName) (protoreflect.ExtensionType, error) { | ||
| ext, err := protoregistry.GlobalTypes.FindExtensionByName(field) | ||
| if err == nil { | ||
| return ext, nil | ||
| } | ||
|
|
||
| desc, err := r.resolver.FindDescriptorByName(field) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return dynamicpb.NewExtensionType(desc.(protoreflect.ExtensionTypeDescriptor)), nil | ||
| } | ||
|
|
||
| func (r dynamicTypeResolver) FindExtensionByNumber(message protoreflect.FullName, field protoreflect.FieldNumber) (protoreflect.ExtensionType, error) { | ||
| ext, err := protoregistry.GlobalTypes.FindExtensionByNumber(message, field) | ||
| if err == nil { | ||
| return ext, nil | ||
| } | ||
|
|
||
| desc, err := r.resolver.FindDescriptorByName(message) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| messageDesc := desc.(protoreflect.MessageDescriptor) | ||
| exts := messageDesc.Extensions() | ||
| n := exts.Len() | ||
| for i := 0; i < n; i++ { | ||
| ext := exts.Get(i) | ||
| if ext.Number() == field { | ||
| return dynamicpb.NewExtensionType(ext), nil | ||
| } | ||
| } | ||
|
|
||
| return nil, protoregistry.NotFound | ||
| } | ||
|
|
||
| func (r dynamicTypeResolver) FindMessageByName(message protoreflect.FullName) (protoreflect.MessageType, error) { | ||
| typ, err := protoregistry.GlobalTypes.FindMessageByName(message) | ||
| if err == nil { | ||
| return typ, nil | ||
| } | ||
|
|
||
| desc, err := r.resolver.FindDescriptorByName(message) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return dynamicpb.NewMessageType(desc.(protoreflect.MessageDescriptor)), nil | ||
| } | ||
|
|
||
| func (r dynamicTypeResolver) FindMessageByURL(url string) (protoreflect.MessageType, error) { | ||
| if i := strings.LastIndexByte(url, '/'); i >= 0 { | ||
| url = url[i+1:] | ||
| } | ||
|
|
||
| return r.FindMessageByName(protoreflect.FullName(url)) | ||
| } | ||
|
|
||
| var _ protoregistry.MessageTypeResolver = dynamicTypeResolver{} | ||
| var _ protoregistry.ExtensionTypeResolver = dynamicTypeResolver{} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| package appconfig | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "reflect" | ||
|
|
||
| "google.golang.org/protobuf/proto" | ||
| "github.com/cosmos/gogoproto/proto" | ||
|
|
||
| internal "cosmossdk.io/depinject/internal/appconfig" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usage of internal package not allowed. The use of Toolsgolangci-lint
|
||
| ) | ||
|
|
@@ -19,10 +20,15 @@ var Register = RegisterModule | |
| // Protobuf message types used for module configuration should define the | ||
| // cosmos.app.v1alpha.module option and must explicitly specify go_package | ||
| // to make debugging easier for users. | ||
| func RegisterModule(msg proto.Message, options ...Option) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why using any of we do a type check/cast just below. Cannot we just use the gogoproto.Message interface?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic I had was that we may want to support plan old go structs (not protobuf) eventually so one breaking change is better than two. When we discussed in architecture @tac0turtle said gogo is enough, although personally if people are annoyed with too much proto we should lower the barrier
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We okay to leave as is and merge this?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense 👍 |
||
| ty := reflect.TypeOf(msg) | ||
| func RegisterModule(config any, options ...Option) { | ||
| protoConfig, ok := config.(proto.Message) | ||
| if !ok { | ||
| panic(fmt.Errorf("expected config to be a proto.Message, got %T", config)) | ||
| } | ||
|
|
||
| ty := reflect.TypeOf(config) | ||
| init := &internal.ModuleInitializer{ | ||
| ConfigProtoMessage: msg, | ||
| ConfigProtoMessage: protoConfig, | ||
| ConfigGoType: ty, | ||
| } | ||
| internal.ModuleRegistry[ty] = init | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ go 1.20 | |
| require ( | ||
| cosmossdk.io/api v0.7.5 | ||
| github.com/cosmos/cosmos-proto v1.0.0-beta.5 | ||
| github.com/cosmos/gogoproto v1.4.11 | ||
|
||
| github.com/stretchr/testify v1.9.0 | ||
| golang.org/x/exp v0.0.0-20230811145659-89c5cff77bcb | ||
| google.golang.org/protobuf v1.34.1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,10 +4,10 @@ import ( | |
| "fmt" | ||
| "reflect" | ||
|
|
||
| "google.golang.org/protobuf/proto" | ||
| "google.golang.org/protobuf/reflect/protoreflect" | ||
|
|
||
| appv1alpha1 "cosmossdk.io/api/cosmos/app/v1alpha1" | ||
| gogoproto "github.com/cosmos/gogoproto/proto" | ||
| protov2 "google.golang.org/protobuf/proto" | ||
| "google.golang.org/protobuf/reflect/protoreflect" | ||
| ) | ||
|
|
||
| // ModuleRegistry is the registry of module initializers indexed by their golang | ||
|
|
@@ -17,38 +17,45 @@ var ModuleRegistry = map[reflect.Type]*ModuleInitializer{} | |
| // ModuleInitializer describes how to initialize a module. | ||
| type ModuleInitializer struct { | ||
| ConfigGoType reflect.Type | ||
| ConfigProtoMessage proto.Message | ||
| ConfigProtoMessage gogoproto.Message | ||
| Error error | ||
| Providers []interface{} | ||
| Invokers []interface{} | ||
| } | ||
|
|
||
| // ModulesByProtoMessageName should be used to retrieve modules by their protobuf name. | ||
| // ModulesByModuleTypeName should be used to retrieve modules by their module type name. | ||
| // This is done lazily after module registration to deal with non-deterministic issues | ||
| // that can occur with respect to protobuf descriptor initialization. | ||
| func ModulesByProtoMessageName() (map[protoreflect.FullName]*ModuleInitializer, error) { | ||
| res := map[protoreflect.FullName]*ModuleInitializer{} | ||
| func ModulesByModuleTypeName() (map[string]*ModuleInitializer, error) { | ||
| res := map[string]*ModuleInitializer{} | ||
|
|
||
| for _, initializer := range ModuleRegistry { | ||
| descriptor := initializer.ConfigProtoMessage.ProtoReflect().Descriptor() | ||
| fullName := descriptor.FullName() | ||
| if _, ok := res[fullName]; ok { | ||
| return nil, fmt.Errorf("duplicate module registration for %s", fullName) | ||
| var fullName string | ||
| if msgv2, ok := initializer.ConfigProtoMessage.(protov2.Message); ok { | ||
| fullName = string(msgv2.ProtoReflect().Descriptor().FullName()) | ||
| } else { | ||
| fullName = gogoproto.MessageName(initializer.ConfigProtoMessage) | ||
| } | ||
|
|
||
| modDesc := proto.GetExtension(descriptor.Options(), appv1alpha1.E_Module).(*appv1alpha1.ModuleDescriptor) | ||
| if modDesc == nil { | ||
| return nil, fmt.Errorf( | ||
| "protobuf type %s registered as a module should have the option %s", | ||
| fullName, | ||
| appv1alpha1.E_Module.TypeDescriptor().FullName()) | ||
| if desc, err := gogoproto.HybridResolver.FindDescriptorByName(protoreflect.FullName(fullName)); err == nil { | ||
| modDesc := protov2.GetExtension(desc.Options(), appv1alpha1.E_Module).(*appv1alpha1.ModuleDescriptor) | ||
| if modDesc == nil { | ||
| return nil, fmt.Errorf( | ||
| "protobuf type %s registered as a module should have the option %s", | ||
| fullName, | ||
| appv1alpha1.E_Module.TypeDescriptor().FullName()) | ||
| } | ||
|
|
||
| if modDesc.GoImport == "" { | ||
| return nil, fmt.Errorf( | ||
| "protobuf type %s registered as a module should have ModuleDescriptor.go_import specified", | ||
| fullName, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| if modDesc.GoImport == "" { | ||
| return nil, fmt.Errorf( | ||
| "protobuf type %s registered as a module should have ModuleDescriptor.go_import specified", | ||
| fullName, | ||
| ) | ||
| if _, ok := res[fullName]; ok { | ||
| return nil, fmt.Errorf("duplicate module registration for %s", fullName) | ||
|
Comment on lines
+53
to
+54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle potential duplicate module registration more gracefully. Consider logging a warning or providing a mechanism to resolve conflicts instead of returning an error immediately. |
||
| } | ||
|
|
||
| res[fullName] = initializer | ||
|
|
||
Check notice
Code scanning / CodeQL
Sensitive package import