-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat(depinject): resolve interface types #12169
Conversation
found = true | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should cache the binding in resolvers
depinject/container.go
Outdated
c.logf("Found candidate resolver, implicitly binding interface %v to implementing type %v.", typ, k) | ||
if found { | ||
// TODO provide example YAML binding when the implementation is done | ||
msg := fmt.Sprintf("<stub> Multiple implementations found for interface %v. Specify an explicit type binding in app.yaml", typ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depinject doesn't know about yaml. We should return a typed error here and let appconfig translate it to something relating to the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need some conflict resolution. I suggest two config options Prefer and PreferInModule. They both would take string type names
depinject/container.go
Outdated
if k.Kind() != reflect.Interface && k.Implements(typ) { | ||
c.logf("Found candidate resolver, implicitly binding interface %v to implementing type %v.", typ, k) | ||
if found { | ||
return nil, &ErrMultipleImplicitInterfaceBindings{Interface: typ} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should return a list of all candidate matches here
6962758
to
e8d085f
Compare
depinject/config.go
Outdated
// Prefer defines a container configuration | ||
func Prefer(inTypeName string, outTypeName string) Config { | ||
return containerConfig(func(ctr *container) error { | ||
return prefer(ctr, inTypeName, outTypeName, "") | ||
}) | ||
} | ||
|
||
// PreferInModule defines a container configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need more detailed docs that than 😉
depinject/container.go
Outdated
res = c.resolvers[matches[0]] | ||
c.logf("Implicitly registering resolver %v for interface type %v", matches[0], typ) | ||
} else if len(matches) > 1 { | ||
p, found := findPreference(c.preferences, typ, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure we should check preferences first and not do a search at all if they're registered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the intent as written here. A hit on c.resolvers[typ]
should skip the whole block, have I mucked it up or misunderstood?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when a user registers a preference, there should be a map based on the in type name. when there's a request to resolve that in type, the preference should be used instead of any other search. it should supersede even a direct match
depinject/errors.go
Outdated
type ErrMultipleImplicitInterfaceBindings struct { | ||
error | ||
Interface reflect.Type | ||
Matches []reflect.Type | ||
} | ||
|
||
func (err ErrMultipleImplicitInterfaceBindings) Error() string { | ||
return fmt.Sprintf("Multiple implementations found for interface %v: %v", err.Interface, err.Matches) | ||
} | ||
|
||
type ErrExplicitBindingNotFound struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not following the difference in these errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrMultipleImplicitInterfaceBindings
multiple bindings of the same interface found without an explicit one.
ErrExplicitBindingNotFound
the type mentioned in the explicit binding was not found. Could probably use a better name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah and doc strings
depinject/preference.go
Outdated
// Preference defines a type binding preference to bind Interface to type Implementation when being provided as a | ||
// dependency to the module with ModuleName. If ModuleName is empty then the type binding is applied globally, | ||
// not module-scoped. | ||
type Preference struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this exported because of the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I had hoped to construct a detailed error message from this struct in appConfig I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work OK un-exported though
* tests(depinject): add preference feature * add features * updates * updates
depinject/README.md
Outdated
|
||
```go | ||
package main | ||
|
||
import ( | ||
"fmt" | ||
|
||
"cosmossdk.io/depinject" | ||
"github.com/cosmos/cosmos-sdk/depinject" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this reverted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intellij's smart parse of golang code blocks in markdown fails on cosmossdk.io
, I thought it was a mistake. If not I can revert my revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation and tests LGTM.
But could we move the README docs to a separate PR? I'd like to just go through and clean-up the wording a bit more
depinject/binding_test.go
Outdated
"github.com/cosmos/cosmos-sdk/depinject" | ||
) | ||
|
||
func TestPrefer(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func TestPrefer(t *testing.T) { | |
func TestBindInterface(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
depinject/features/bindings.feature
Outdated
Rule: preferences must point to a real type | ||
Example: a preferred type is not provided | ||
Given "Mallard" is provided | ||
And there is a global preference for a "Marbled" "Duck" | ||
When we try to resolve a "Duck" in global scope | ||
Then there is a "No type for explicit binding" error | ||
|
||
Rule: preferences supersede implicit type resolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule: preferences must point to a real type | |
Example: a preferred type is not provided | |
Given "Mallard" is provided | |
And there is a global preference for a "Marbled" "Duck" | |
When we try to resolve a "Duck" in global scope | |
Then there is a "No type for explicit binding" error | |
Rule: preferences supersede implicit type resolution | |
Rule: binding must point to a real type | |
Example: bound type is not provided | |
Given "Mallard" is provided | |
And there is a global preference for a "Marbled" "Duck" | |
When we try to resolve a "Duck" in global scope | |
Then there is a "No type for explicit binding" error | |
Rule: bindings supersede implicit type resolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the references to preferences need to be renamed here
depinject/config.go
Outdated
// BindInterface( | ||
// "github.com/cosmos/cosmos-sdk/depinject_test/depinject_test.Duck", | ||
// "github.com/cosmos/cosmos-sdk/depinject_test/depinject_test.Canvasback") | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't find this style of start sentence -> code sample -> finish sentence
very readable. It would be better IMHO to write a whole sentence, then a code sample, then more sentences (if needed). It's okay for now and I can follow but it might be hard for readers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
depinject/README.md
Outdated
``` | ||
|
||
Now `depinject` has enough information to provide `Mallard` as an input to `APond`. | ||
|
||
### Full example in real app | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!!
depinject/binding_test.go
Outdated
"github.com/cosmos/cosmos-sdk/depinject" | ||
) | ||
|
||
func TestPrefer(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
|
Description
Closes: #11908
When an interface type is specified as a provider input, instead of descriptive error logging described in #11908 we just auto-magically bind to a resolver providing a concrete interface implementation if there is only one match in the container. If there are multiple, warn and error.
Reverts the original
keys
feature implementation in #11910 in favor of this one.Adds
BindInterface
andBindInterfaceInModule
to thedepinject
API described here, but leaves out AppConfig changes for a follow up PR.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change