uber-go / dig Goto Github PK

Need to look into and document how dig behaves when encountering private types from other packages.

I'd love to get in-lined coverage information on PRs. I don't have a particular preference for which service is going to provide it, but seems like other OSS projects are using codecov and it's working well.

While it's true reflection-based code can panic quite abit - recoving in dig means also recovering panics located in user-land code - which is not desirable.

Thoughts? (continued discussion from #51)

I thought about this a bit in the context (:drum:) of #132.

Want to open this up for a discussion as we're storming towards v1. Does it feel completely out of place to accept context?

Problem

While we did make room for ProvideOption and InvokeOption, neither are currently implemented and I don't see them fulfilling all the future use cases.

At the very least to implement each an additional datastructure for each is going to be required. Something like provideConfig, or provideContext or provider which encapsulates rules an options for a particular Provide call, like hooks, or rules for how to skip frames.

context.Context can actually fill some of those roles.

Implications

User-facing API

User-facing consumption of dig is going to suffer due to an additional parameter. I don't think this is a big deal, as we've spun dig to be a library to build frameworks, rather than for direct consumption.

Fx

Fx API won't be affected, we can keep them as is. That should allow us to smooth over the aforementioned issue.

Potential confusion

If we start forking between Options and context.Context it might be confusing what goes where.

???

Are there any more drawbacks?

Benefits

Timeouts & cancellation

Invocation on a type can actually take a long time, depending on the size of the graph. I think it would be wise to leave room for timeouts and context.Context things.

c := c.New()
// ...
err := c.Invoke(context.WithTimeout(10 * time.Second), func(A,B,C) { ... })

???

Let me know if I'm missing something

I have two packages named bar that both expose Type struct. Putting them both into the graph works fine, but results in confusing String() function:

$ go run main.go
2017/05/17 16:39:28 {nodes:
*bar.Type -> (object) obj: *bar.Type, deps: nil, cached: true, cachedValue: &{bar}
*bar.Type -> (object) obj: *bar.Type, deps: nil, cached: true, cachedValue: &{foo/bar}
}
$ cat **/*
package bar

// Type of bar
type Type struct {
        Path string
}
package bar

// Type of bar
type Type struct {
        Path string
}
package main

import (
        "log"

        "go.uber.org/dig"
        "go.uber.org/dig/examples/whoops/bar"
        foobar "go.uber.org/dig/examples/whoops/foo/bar"
)

func main() {
        c := dig.New()
        err := c.Provide(
                &bar.Type{Path: "bar"},
                &foobar.Type{Path: "foo/bar"},
        )
        if err != nil {
                log.Fatal("sadness", err)
        }
        log.Println(c)
}

We should consider how to better identify those types and where they came from. It's not clear to me which bar.Type is from which package.

39 commits on master since the last release (some pretty major). The API seems in a good place to at least take it for another spin.

Do not use my false pattern of forgetting the patch version :( Lets get all three digits in

Errors defined in container right now are not useful in mentioning which constructor failed the execution.

example, A useful error would be -
"constructor return type must be a pointer, constructor: func foo() *ReturnType"

Dig should always return errs so that FX can control what those errs mean.

Now that dig is completely hidden from the user's eye through fx, I think one of the suggestions was to drop all the locks, I'm guessing with the assumption that it makes the dig implementation code simpler.

I'm all for making dig internals as clear as possible, but please let me know your thoughts around this @breerly @anuptalwalkar

The current Provide API supports passing both instances and constructors. That is, if I want *Foo available in the graph, I can do either Provide(func() *Foo {return &Foo{}}) or I can do Provide(&Foo{}) directly.

This is sometimes really convenient, since it removes the need for some anonymous shim functions in modules. Unfortunately, it also introduces a number of inconsistencies. These inconsistencies all make sense if you understand Go's type system and reflection capabilities well, but I suspect that they'll be very confusing to users. I've highlighted some examples in integration tests: see this note about providing functions, this one about interfaces, and this one about errors.

Because functions carry more reflect-able type information with them, providing constructors doesn't have any of these issues.

I suspect that these inconsistencies (especially the one about providing interface instances) will require a lot of detailed documentation and will prompt a lot of support requests. At least for Fx's initial launch, let's consider only supporting constructors.

Currently, dig doesn't have an elegant way to put an ordered collection into the graph.

Consider middleware: typically, many libraries want to provide instances of a Middleware interface, but the order in which the middleware is composed together is critical. (This order-dependence distinguishes middleware from groups/tags, which are discussed in #127.)

Today, each middleware provider would provide a named instance of Middleware, and the end user needs to collect and order the named instances. This is cumbersome, and it pushes all the ordering complexity on end users.

Ideally, we'd find a simple, easily-debuggable way for middleware authors to express ordering constraints and have dig automatically order the collection.

Calling Invoke on provided and unprovided funcs can result in executing constructors more than once and overwriting the graph with the resulted return objects.
for provided constructors -

ctor1(a *A, b *B) (x *x)
ctor2() (a *A)

Invoking constructors in order will result in ctor1 resolving *A and *B.
Invoking ctor2 will result in resolving *A again.

The following e2e test doesn't work:

	t.Run("embedding out pointers works", func(t *testing.T) {
		c := New()
		type A struct{}
		type B struct {
			Out
			A
		}
		type C struct {
			*B // pointer to B which is a dig.In with A
		}
		require.NoError(t, c.Provide(func() C { return C{} }))
		require.NoError(t, c.Invoke(func(A) {}))
	})

It works if type C is written to embed type B directly, rather than the pointer to B:

type C struct {
  B // direct embed of type B
}

Question is... do we care? embedsType function seems to solely focus on structs:

if t.Kind() != reflect.Struct {
  continue
}

Should be easy to modify to look at Elems() of a pointer instead.

Update Invoke to allow:

c.Invoke(func(dep1 *Dep1, dep2 *Dep2) {
  // do something w/ dep1, dep2
})
``

We're seeing many cases where there is no error to return, so users are just returning `nil`.

I think we are pretty close to a 1.0 API surface. To get there, I'd like to propose:

• Container.Provide - provide types into the container
• Container.Resolve - locate types out of the container

This streamlined API has a tiny surface, uses dependency injection terms in naming, and should prove easy to use for init/framework developers.

Here's what we should discuss:

• No more Must* funcs #35
• Remove *All funcs #40
• Discuss combining Resolve and Invoke

from @breerly

The term "graph" collides with "application graph" and "object graph", which is a term that relates to the users set of shapes that must come together.

Using "graph" in DIG's terminology is problematic for the same reason that using the term "inject" was: these terms exist to describe user-land concepts.

Overloading these terms creates confusion. Consider the following phrase:

The proposal is that all deps in the application graph, including clients and handlers, get registered into dig's graph so that the full graph can be resolved.

Here I used the term "application graph" to mean "all the shapes in user-land that must be wired using DI". This is confusing because DIG has a term "graph", which overloads the graph with the things which must make their way into the graph.

My suggestion is to use the classical term for this: "Container".

All deps in the application graph, including clients and handlers, get registered into a DIG container so that the graph can be resolved.

This will prevent confusion and make it easier for users to understand and utilize the concepts in DIG:

// create a new dependency injection container
container := dig.NewContainer()

// register all shapes into the container
container.Register(...)

// build the object graph
container.Resolve()

When you ask the container to resolve dependencies, that is called the "service locator" pattern.

Combining Resolve, ResolveAll, and Invoke into a Locate func will streamline the experience:

// locate 1 or more types
var logger *zap.Logger
var dispatcher *yarpc.Dispatcher
container.Locate(&logger, &dispatcher)

// same as above, using a func
container.Locate(func(logger *zap.Logger, dispatcher *yarpc.Dispatcher) {
 
})

This has the benefit of:

• Being more specific, giving a nod to the pattern actually being used here
• Reduces API surface and simplifies using DIG

There is now an Advanced section but it's pending documentation

We need to cut an 0.1 so that projects using DIG have something to pin to.

For example, UberFX should pin to ~0.1 so that patches are let in, e.g. 0.1.1, but not new minor releases which might contain breaking changes, e.g. 0.2.

Currently, dig only works with either a pointer to an object, or an interface. Slices and maps can sort be used by using a pointer to slice/map, or wrapping them in another struct.

Ideally, dig would support slices and maps as first class citizens.

NOTE: there may be api implications for constructor parameters that are slices/maps. For example, if dig was handling the following constructor

func ([]typeA, typeB) typeC { }

What does it mean?

1. Does it mean that there is exactly one slice of type []typeA registered in the graph and we should return that?
2. Does it mean there may be multiple constructors that return func() typeA or func() []typeA and we should fetch them all and put them into an slice?

Not handling case2 should probably be a starting point: there is only one slice of a given type that can live in the graph, we can deal with merging/extending existing slices through dig options in the future.

The Must* methods provide a nice convenience for end users, but aren't necessary because:

DIG is not for end users. DIG should be used to build init/application frameworks.

Forcing init/app frameworks to explicitly manage errors means a reduced API surface, and overall simplified experience.

If A has an optional dependency on B, and B has a dependency on C,

A --opt--> B -------> C

And C is not present in the graph, currently, we will fail to resolve A.

Instead, we should invoke A and pretend like B isn't present in the graph
because all its dependencies aren't present.

Relevant failing test:

type C struct{}

type B struct{ C *C }

newB := func(c *C) *B { return &B{C: c} }

type A struct{ B *B }

type AParams struct {
  dig.In
  
  B *B `optional:"yes"`
}

newA := func(p AParams) *A { return &A{B: p.B} }

// We Provide A and B but not C.
c := dig.New()
c.Provide(newA, newB)

assert.NoError(t, c.Invoke(func(a *A) {
  assert.Nil(t, a.B)
}), "invoke failed")

container.nodes map is being used in exported functions that are explicitly locked. We need to improve container locking once #21 is landed. Invoke is taking too long, we don't want to lock the entire dig container.

We check that it is either a pointer or interface, but not an error, that could lead to some ambiguity. https://github.com/uber-go/dig/blob/master/container.go#L106

Lets say we have a type A:foo in the graph.

Currently, requesting for type A will result in an error, since unique identification for an object is type+key.

We can make this experience a lot better. The high-level behavior can be like this:

• Requesting for A will return A:foo, if there is only one in the graph
• Requesting for A when A:foo and A:bar are provided -> error
• Obviously requesting for A:foo will never return A:bar, even if that's the only object provided

This would get tricky if Invoke is called on type A in between Provide of A:foo and A:bar

Discussion on the subject: #30

Now that we have a concept of tagging special structs via embedding dig.Param, we can expand the concept and support named instances of the same type in the graph as well.

Goes something like this:

type Param struct {
	dig.In

	A1 *A `named:"firstA"`
	A2 *A `named:"secondA" optional:"true"`
}

type Output struct {
	dig.Out

	// Two instances of B will be contributed to the graph
	// with different names
	B1 *B `named:"mainB"`
	B2 *B `named:"secondaryB"`
}

func Constructor(p Param) *Output {
	// use p.A1 and p.A2
	// ...
	// return a custom type
}

Rough plan of attack:

• Rename dig.Param to dig.In
• Add a new type dig.Out
• Migrate container to use a composite key (reflect.Type + string name)
• Add support for named:"<name>" on dig.In and dig.Out types

Currently dig MustRegister takes funcs that return single object or (object, error) for registration in Provide. We need to add support so that users can write functions that need multiple object registration. For example, tracing initialization

There are package level functions that operate on the default graph (much like package log), but this is not desired as it makes testing and discovery of the dependencies difficult.

I've been waiting for a semver to drop the global graph, seems like a good target for v0.2

Today, constructors are registered (provided) in sequential order in which they are received and can cause inconsistent behavior. eg, registering following constructors will cause different behavior depending on the order in which they are registered -

ctor1(a *A, b *B) (x *x, y *Y)
ctor2() (x *x)

result of registration -

//ctor1 registered first
y -> ctor1 
x -> ctor2 (registering ctor2 will override x -> ctor1)
 OR 
//ctor2 registered first -
y -> ctor1 
x -> ctor1 (registering ctor1 will override x -> ctor2)

Same issue occurs during call to Invoke as today dig just overwrites the values.

Thoughts?

1. continue overwrite and explicitly state as in the document
2. Avoid overwriting constructors and invoke function. If object exist in dig cache, ignore. This will allow partial execution in any remaining objects that are not resolved.
3. Panic on providing constructor or invoking function if existing object type already has a funcNode or objNode associated with it.

Now that v0.1 was cut and v0.2 will contain some breaking changes, time to start tracking it in a changelog

To match the Github convention.

Now that we've renamed some of the main methods and removed the default graph, now seems like a good time to cut 0.2.

Let's say that I have a constructor func (A, B) (C, error) { // codez }.

In order to partake in dig, I'd have to first Provide said constructor it to the graph, then declare a new variable of type C, and ask the graph to resolve:

container := dig.New()
container.Provide(func (A, B) (C, error) { // codez })
var c C
conatiner.Resolve(&c)
// c is now usable

I propose to add the Call function to Container interface, to make this process a lot smoother. Call would execute the function, inject the dependencies (without permanently adding it to the graph) and return the interface{} result (ready for casting).

The new experience would be:

container := dig.New()
c := container.Call(func (A, B) (C, error) { // codez }).(C)
// c is bueno to use

Not only this improves the API, but also allows users to create multiple objects of the same type that should they choose to.

Background #34

In order to provide a tight API surface, let's remove the *All functions, leaving just:

• Provide
• Resolve
• Invoke

This will mean combining the ProvideAll and ResolveAll into Provide and Resolve, so that those functions deal with variadic arguments so they can operate on one or many deps.

Currently providing the same type twice overwrites the original type.

This is going to lead to tons of difficult to understand and debug issues, resulting in our customers swarming our help channels and generally being not happy.

Instead, let's clamp down on this early and err if the same type is provided once. That way we won't get in the habit of abusing this, instead we'll be forced to develop more explicit patterns.

Porting over the context to the appropriate repository per uber-go/fx#568 discussion.

Better introspection into dig activities, given it's highly magical properties, is very desirable.

One of the proposed solutions is to expose an event system which can notify listeners of the events that are happening, i.e. invoke that resolved types A,B,C just happened.

This is a fairly common pattern (observer, listener, etc) in other libraries and languages, so lots of source material to get inspired by.

v0.2 has been out for a month, lets cut v0.3 so that libs depending on dig have a chance to test out the new commits.

Assuming dig needs to support multiple instances of the same type in the graph, perhaps indentified by a string tag or something simiar.

This has a pretty large implication on what the Provide and Resolve functions are going to look like.

I think the Provide is simpler and something along the lines of func (graph) Provide (interface{}, ...Option) {} should be sufficient.

For example,

c := dig.New()
var main, backup *DBConnection
c.Provide(main, dig.Named("master"))
c.Provide(backup, dig.Named("follower"))

The flip side if much harder, especially from a constructor perspective. Lets look at the following constructor:

func BusinessLogic(l *zap.Logger, c *DBConnection) *Logic {
    // codez
}

How can a constructor like that specifically request a master database connection object?

It feels very unfair towards them:
https://github.com/uber-go/dig/blob/master/container.go#L113

https://github.com/uber-go/dig/blob/master/internal/graph/node.go#L126

https://github.com/uber-go/fx/blob/58da98fb4496d237d2557b14c32a1285fd54ac20/Makefile#L163

For safety, we should panic if we try to resolve more than... maybe 10k levels deep.

We should take care to update the version constant in the next release.

Before we get further into an RC series, New needs to take a variadic number of Options, even if we don't yet have any concrete Option implementations. Adding this after 1.0 is a breaking change.

Typically, variadic arguments are optional - the function needs to handle len(varargs)==0 already. Making dig automatically treat varargs as optional would make a wider variety of plain-Go functions work out of the box.

Currently DIG support providing types in 2 ways:

• Providing a constructor function to be executed in order to create type Foo
• Providing an already instantiated instance of type Foo

In addition to that, we should allow for the following common use-case:

Providing type Foo directly, so that DIG can instantiate it during resolve-time.

This would remove the need to define constructors like so:

func NewFoo(a,b Handler) *Foo {
  return &Foo(a: a, b: b)
}

Instead, one could pass Foo to DIG like so:

container.Provide(new(Foo))

Then, DIG will instantiate a struct of type Foo, treating the properties on Foo as it would the params in the constructor.

Dig behaves differently when registering an interface as an object vs a function.

Eg.
type Scope interface {}
type scopeStruct struct {}
var s Scope = scopeStruct{}

1. As object:
   dig.MustRegister(s)
   dig.MustResolve(&s) -> dependency of type Scope is not registered
2. As function:
   dig.MustRegister(func() (Scope, error) {return scope,nil})
   dig.MustResolve(&s) -> works fine

In case 1, dig registers it as a *scopeStruct struct instead of Scope interface.

Is this the behavior we want from dig? It would be great if both are consistent and dig.Resolve works in case 1 as well but seems harder to do.

I've been talking to @abhinav about this, In order to support use cases where a dig parameter depends on multiple inputs of the same type we need the ability to aggregate dig types together.

So if we have the following outputs:

  Output1 MyType `name:"output1"`
  Output2 MyType `name:"output2"`

I'd want to be able to grab all the "MyType"s in a single attribute.

Solutions

1: Add another tag to indicate that the type should be a fan-in collection:

type In struct {
  dig.In
  Outputs map[string]MyType `collection:"true"` // tag name TBD
}

The string key in the Outputs variable will be the name of the attributes that were injected into the graph.

2: Add a dig.Key (or similar) type for map keys. Any map of these types will be recognized as a fan-in collection.

type In struct {
  dig.In
  Outputs map[dig.Key]MyType
}

The dig.Key would be a dig struct where we could embed information on the type. Currently that would only be the "name", but we could add more information later.

Use cases

YARPC Middleware

Currently we don't have a good answer for middleware to ensure that we can take multiple middleware inputs and effectively construct a middleware graph without forcing the yarpcfx module to have a hard dependency on every middleware library (which gets out of control). With this system we'd be able to grab all middleware that was inserted into the graph. Since this is a map, the yarpcfx module would need to have an ordered list of middleware names it was expecting beforehand (all names should be optional) and it can construct the middleware chain itself. Of course this would only support names that yarpcfx explicitly expects, but that is necessary for the middleware ordering.

YARPC Specs:

The yarpc config options allow inserting custom Transport/PeerList/Updater specs into the yarpc config builder. Having support for this would allow us to detach the yarpc fx module from the implementations of each transport. For instance, a user could inject a "grpcfx" module into the graph which provided a "TransportSpec" for grpc that integrated seamlessly into the yarpc configuration.

Need to take a pass. Includes things like:

err := dig.Resolve(&o) // notice the pointer to a pointer as param type
if err == nil {
    // o is ready to use
}

err = g.Resolve(&f1)
require.NoError(t, err)