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

feat(depinject): resolve interface types #12169

Merged
merged 49 commits into from
Jun 9, 2022
Merged

Conversation

kocubinski
Copy link
Member

@kocubinski kocubinski commented Jun 6, 2022

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 and BindInterfaceInModule to the depinject 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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

depinject/container.go Fixed Show fixed Hide fixed
depinject/container.go Fixed Show fixed Hide fixed
found = true
}
}
}
Copy link
Member

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

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)
Copy link
Member

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

depinject/container.go Fixed Show fixed Hide fixed
@kocubinski kocubinski marked this pull request as ready for review June 7, 2022 00:20
@kocubinski kocubinski requested a review from a team as a code owner June 7, 2022 00:20
depinject/container.go Fixed Show fixed Hide fixed
Copy link
Member

@aaronc aaronc left a 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

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}
Copy link
Member

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

@kocubinski kocubinski force-pushed the kocubinski/depinject-prefer branch from 6962758 to e8d085f Compare June 7, 2022 20:05
Comment on lines 51 to 58
// 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
Copy link
Member

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 😉

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)
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

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

Comment on lines 10 to 20
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 {
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah and doc strings

// 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 {
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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

@kocubinski kocubinski changed the title feat(depinject): auto resolve interface types feat(depinject): resolve interface types Jun 7, 2022
@kocubinski kocubinski requested a review from aaronc June 7, 2022 20:15
depinject/container.go Fixed Show fixed Hide fixed
depinject/container.go Fixed Show fixed Hide fixed
depinject/preference.go Fixed Show fixed Hide fixed
"github.com/cosmos/cosmos-sdk/depinject/internal/graphviz"
"github.com/pkg/errors"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism

```go
package main

import (
"fmt"

"cosmossdk.io/depinject"
"github.com/cosmos/cosmos-sdk/depinject"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this reverted?

Copy link
Member Author

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.

Copy link
Member

@aaronc aaronc left a 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

"github.com/cosmos/cosmos-sdk/depinject"
)

func TestPrefer(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func TestPrefer(t *testing.T) {
func TestBindInterface(t *testing.T) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Comment on lines 19 to 26
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Member

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

Comment on lines 54 to 57
// BindInterface(
// "github.com/cosmos/cosmos-sdk/depinject_test/depinject_test.Duck",
// "github.com/cosmos/cosmos-sdk/depinject_test/depinject_test.Canvasback")
//
Copy link
Member

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

Copy link
Contributor

@JeancarloBarrios JeancarloBarrios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

```

Now `depinject` has enough information to provide `Mallard` as an input to `APond`.

### Full example in real app

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!!

"github.com/cosmos/cosmos-sdk/depinject"
)

func TestPrefer(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@kocubinski
Copy link
Member Author

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

#12214

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide useful error when an interface can't be resolved but there are implementing types in depinject
4 participants