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

Populate a map by combining name and group attributes #1036

Open
jquirke opened this issue Feb 10, 2023 · 7 comments
Open

Populate a map by combining name and group attributes #1036

jquirke opened this issue Feb 10, 2023 · 7 comments

Comments

@jquirke
Copy link

jquirke commented Feb 10, 2023

Currently FX supports injecting multiple different implementations of an interface type using groups
https://pkg.go.dev/go.uber.org/fx#hdr-Value_Groups

This allows me to Provide multiple implementations of an interface Foo provided they are annotated with group, and collect them

type Params struct {
   fx.In

   Foos []Foo `group:"myfoos"`

where a particular Foo can be provided as

type Out struct {
   fx.Out
   
   Foo `group:"myfoos"`

However, the Foos I received back are not able to be identified by name.

Describe the solution you'd like

I would like to be able to populate a map of named foos, using the existing name annnotation:

type Params struct {
   fx.In

   Foos map[string]Foo `group:"myfoos"`

where a particular Foo can be provided as

type Out struct {
   fx.Out
   
   Foo1 Foo `name:"foo1" group:"myfoos"`
   Foo2 Foo `name:"foo2" group:"myfoos"`

This allows me a powerful capability: To name and group a type together, and obtain it as an unordered slice, a single instance by name, or as a map of names to instances.

Describe alternatives you've considered

A trivial solution is to extend Foo to include a Name() method, and then simply organise this into a map[string]Foo.

foos := make(map[string]Foo)
for _, foo := range params.Foos {
    foos[foo.Name()] = foo
}

But there are many cases where components need to be looked up by name according to dynamic configuration, that re-implementing this for each case seems inelegant and would be nice to delegate to the DI framework.

Is this a breaking change?

It is difficult to imagine a scenario where this would cause a breaking change here; but of course, there is also the curse of hindsight

  1. Currently attaching a group attribute to a dependency type of map fails and the application would not start, therefore this cannot be a breaking change in a practical sense.
bad argument 1: bad field "Foo" of handler.Params: value groups may be consumed as slices only: field "Foo" (map[string]string) is not a slice
  1. Annotating a provided dependency with both a name and a group similarly fails:
bad result 1: bad field "Foo" of handler.Result: cannot use named values with value groups: name:"foo1" provided with group:"myfoos"
@sywhang
Copy link
Contributor

sywhang commented Feb 10, 2023

Internal Ref: GO-1864

@jquirke jquirke changed the title Populate a map by combining named and grouped arguments Populate a map by combining name and group attributes Feb 11, 2023
@jquirke
Copy link
Author

jquirke commented Feb 22, 2023

Some thoughts on the behaviour:
Consider:

type Out struct {    
   fx.Out    
   Foo Foo    `name:"foo1" group:"foos"`
   Foo2 Foo   `group:"foos2"`
   Foo3 Foo   `name:"foo3"`
}

type Params struct {    
   fx.In 

   MapOfFoos       map[string]Foo `group:"foos"`    
   GroupOfFoos     []Foo `name:"foo1" group:"foos"`   
   GroupOfFoos2    []Foo `group:"foos2"`  
   SingleFoo       Foo `name":foo1"`
   SingleFoo3      Foo `name":foo3"`
}

Before this proposed change, this will fail at runtime for at least 2 reasons as listed in the summary above.

But if we implement the logic, what should the behaviour be?

Should a dig output with both a name and a group be treated as special and only go in maps to maximise backward compatibility?

Should we avoid allowing a name or group to be re-used individually?

Example Behaviour 1 output (name+group, name, group all allowed - keys are decomposable)


MapOfFoos = map[string]Foo{"foo1": Foo}

GroupOfFoos = []{Foo}

GroupOfFoos2 = []{Foo2}

SingleFoo = Foo1

SingleFoo = Foo3

Example Behaviour 2 output (failure at Invoke time: a dig key is now treated as name+group){}

MapOfFoos = map[string]Foo{"foo1": Foo}

GroupOfFoos = <cause failure>

GroupOfFoos2 = []{Foo2}

SingleFoo1 = <cause failure>

SingleFoo3 = Foo3

Example Behaviour 2b (failure: a dig key is now treated as name+group, but both constituent components cannot be unique elsewhere in the container. Fail at Provide time).

e.g.

type Out struct { 
fx.Out 
   Foo Foo `name:"foo" group:"foos"`
   Foo2 Foo `group:"foos"` // illegal, runtime failure at provide time - foos was declared as the group in a name+group key already
   Foo3 Foo `name:"foo"` // illegal, runtime failure at provide time: foo was declared in a name+group already
} 

Other considerations:

Should this new mode be specifically and explicitly enabled via a dig Container or Scope Option? If so, at which level? Should this be exposed to Fx? If so, how?

@jquirke
Copy link
Author

jquirke commented Feb 22, 2023

Given the request in #998 by @mbolt35, it seems Behaviour 1 might be the best case that meets both requirements.

jquirke added a commit to jquirke/dig that referenced this issue Mar 6, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 6, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 6, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 6, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 7, 2023
As part of uber-go#380 we allowed names
and groups tags/options to co-exist to ultimately support Fx feature
request uber-go/fx#998.

We now intend to support Map value groups as per
uber-go/fx#1036. We will do this in 2 steps.

1. This PR will begin tracking any names passed into value groups with
out changing any external facing functionality.

2. a subsequent PR will exploit this structure to support Map value
groups.
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As part of uber-go#380 we allowed names
and groups tags/options to co-exist to ultimately support Fx feature
request uber-go/fx#998.

We now intend to support Map value groups as per
uber-go/fx#1036. We will do this in 2 steps.

1. This PR will begin tracking any names passed into value groups with
out changing any external facing functionality.

2. a subsequent PR will exploit this structure to support Map value
groups.
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As part of uber-go#380 we allowed names
and groups tags/options to co-exist to ultimately support Fx feature
request uber-go/fx#998.

We now intend to support Map value groups as per
uber-go/fx#1036. We will do this in 2 steps.

1. This PR will begin tracking any names passed into value groups with
out changing any external facing functionality.

2. a subsequent PR will exploit this structure to support Map value
groups.
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As part of uber-go#380 we allowed names
and groups tags/options to co-exist to ultimately support Fx feature
request uber-go/fx#998.

We now intend to support Map value groups as per
uber-go/fx#1036. We will do this in 2 steps.

1. This PR will begin tracking any names passed into value groups with
out changing any external facing functionality.

2. a subsequent PR will exploit this structure to support Map value
groups.
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As part of uber-go#380 we allowed names
and groups tags/options to co-exist to ultimately support Fx feature
request uber-go/fx#998.

We now intend to support Map value groups as per
uber-go/fx#1036. We will do this in 2 steps.

1. This PR will begin tracking any names passed into value groups with
out changing any external facing functionality.

2. a subsequent PR will exploit this structure to support Map value
groups.
@AFMiziara
Copy link

What is the current status of this?

I also have the following scenario where this would be really helpful:

package main

import (
	"context"
	"fmt"

	"go.uber.org/fx"
)

type Database struct {
	Name string
}

type InputParams struct {
	fx.In

	Db1 *Database `name:"db1"`
	Db2 *Database `name:"db2"`
}

type InputHookParams struct {
	fx.In

	Connections []Database `group:"sql_connections"`
}

type Databases struct {
	fx.Out

	Db1 *Database `name:"db1" group:"sql_connections"`
	Db2 *Database `name:"db2" group:"sql_connections"`
}

func SetupDatabases() (Databases, error) {
	return Databases{
		Db1: &Database{
			Name: "user",
		},
		Db2: &Database{
			Name: "cart",
		},
	}, nil
}

func RunExample(p InputParams) {
	fmt.Println("1: " + p.Db1.Name)
	fmt.Println("2: " + p.Db2.Name)
}

func RegisterOnStopHook(lc fx.Lifecycle, conns InputHookParams) {
	lc.Append(fx.Hook{
		OnStop: func(ctx context.Context) error {
			for _, c := range conns.Connections {
				fmt.Println("closing connection with " + c.Name + " ...")
			}

			return nil
		},
	})
}

func main() {
	fx.New(
		fx.Provide(SetupDatabases),
		fx.Invoke(RegisterOnStopHook),
		fx.Invoke(RunExample),
	).Run()
}

I want to inject multiple instances of *Database and each one will have specific configurations. That's why it is important to name them. Also, I need all of them to register a OnStop hook (which could be easily achieved by using the group tag).

Is there another way to achieve that?

@Juandavi1
Copy link

I think this #1096 issue is related to this

@lootarola
Copy link

I had this same issue on January and by the time after reading the docs and other issues, I thought that my use case was a bad implementation of Fx, now that I'm more experienced with the Framework I guess it wasn't.

I was going to write an Issue regarding this problem when I found this that matches my exact use case, is there any update on this @jquirke? Is there a reason why the PR haven't been approved and merged yet? Did you get to a dead end? May I help with something?

Thank you @jquirke for all the work you've done, it really was the exact thing I was looking for!

@jquirke
Copy link
Author

jquirke commented Sep 20, 2023

I was going to write an Issue regarding this problem when I found this that matches my exact use case, is there any update on this @jquirke? Is there a reason why the PR haven't been approved and merged yet? Did you get to a dead end? May I help with something?

No dead end, it's just there are a ton of corner cases to cover and test and it requires coordination with the Fx Layer as well

For the problem I was trying to solve within Uber there were other workable options, and it's something I've just let slip down the priority list.

Feel free to take what I've done so far and run with it; I may try and finish this given the demand for the feature both in public and inside Uber but unfortunately I can't make any promises.

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

No branches or pull requests

5 participants