uber-go / dig

A reflection based dependency injection toolkit for Go.
https://go.uber.org/dig
MIT License
3.78k stars 206 forks source link

Add support dig out anonymous field's member. #386

Closed xqbumu closed 9 months ago

xqbumu commented 1 year ago

Is your feature request related to a problem? Please describe.

Describe the solution you'd like I am a very lazy person and I want to output a member field with an independent structure from protobuf.

Describe alternatives you've considered If modify the export method, this effect can be achieved.

Is this a breaking change? No, I just modified the names and structures of variables and method names for some structures, which are internal.

Additional context Thank you for this great repo.

tchung1118 commented 1 year ago

Thank you for the suggestion. Could you maybe provide us with a use case to help us better understand the value of this feature? For example, could you provide us with before/after code samples of the use case that would benefit from having this feature? For context, not handling anonymous fields on a dig.Out struct was a conscious design decision, and we just want to make sure that a feature that changes this behavior has a good "value proposition".

xqbumu commented 1 year ago

Thank you for the suggestion. Could you maybe provide us with a use case to help us better understand the value of this feature? For example, could you provide us with before/after code samples of the use case that would benefit from having this feature? For context, not handling anonymous fields on a dig.Out struct was a conscious design decision, and we just want to make sure that a feature that changes this behavior has a good "value proposition".

My current scenario is to develop code based on protobuf. First, I define global configuration messages that are used by different public or private modules, and these modules depend on the global configuration definition to generate corresponding instances.

// file: api/v1/common.pb
message Database {
  string dsn = 1;
}

message Redis {
  string addr = 1;
  int32 db = 2;
}
// file: modules/database
func New(conf *pbv1.Database) (*sql.DB, error) {
  // TODO
}
// file: modules/redis
func New(conf *pbv1.Redis) (*redis.Client, error) {
  // TODO
}

In the business scenario, I will set different configuration requirements for different apps, such as:

// file: api/app1/v1/config.pb
message Config {
  common.Database database = 1;
  common.Redis redis = 2;
}
// file: api/app2/v1/config.pb
message Config {
  common.Database database = 1;
}

When the app starts up, an independent file will be passed in.

// file: config.yaml
database:
  dsn: mysql://root:123456@127.0.0.1:3306/db_name
redis:
  addr: 127.0.0.1:6379
  db: 1

Parsing the configuration file into a app1.Config structure and providing it to the module, the code looks like this:

Before:

// file: cmd/app1/bootstrap.go
type bsOut struct {
  fx.Out

  Config app1.Config
  Database *commonv1.Database
  Redis *commonv1.Redis
}

func NewBs(f string) (out bsOut, err error) {
  data, _ := os.ReadAll(f)
  err = yaml.Unmarshal(data, &out.Config)
  out.Database = out.Config.Database
  out.Redis = out.Config.Redis

  return
}

After:

// file: cmd/app1/bootstrap.go
type bsOut struct {
  fx.Out

  Config app1.Config `extra-anonymous:"true"`
}

func NewBootstrap(f string) (out bsOut, err error) {
  data, _ := os.ReadAll(f)
  err = yaml.Unmarshal(data, &out.Config)

  return
}

In the later stage, you can directly inject commonv1.Database and commonv1.Redis into database.New and redis.New respectively.

To summarize, the scenario involves defining global configuration messages that are used by different modules, setting different configuration requirements for different apps, parsing configuration files into a app1.Config structure, and injecting dependencies into the appropriate New() functions.

This is just a simple example and there may be some errors, but it should be sufficient to describe the corresponding use cases.

Of course, we can also increase the number of layers in the 'extra' configuration in tags for more flexible control. This feature could potentially be abused and cause trouble for developers, but the responsibility for using this feature properly is entirely in the hands of the developer.

JacobOaks commented 1 year ago

Thanks for the example. Just to confirm, you want Dig to inject embedded fields of a dig.Out struct, and if a struct field is tagged with extra-anonymous:"true", you want dig to also inject every sub-field of the tagged field. Is that right? This sounds like two different features to me.

xqbumu commented 1 year ago

Thanks for the example. Just to confirm, you want Dig to inject embedded fields of a dig.Out struct, and if a struct field is tagged with extra-anonymous:"true", you want dig to also inject every sub-field of the tagged field. Is that right? This sounds like two different features to me.

dig was originally able to extra anonymous field, and this issue is only to extra sub-fields from anonymous fields by adding additional tags, and treat this behavior as a non default behavior.

Should I modify the tag name? Is that what it means?

thanks for your reply.

tchung1118 commented 1 year ago

Thanks for the example. So the feature you're describing in this ticket is actually just the ability to automatically inject members of a struct that's provided as a field of a result struct that's embedding dig.Out, correct? Sort of like automatically applying dig.Out recursively to some of its fields. Whether or not that field is anonymous seems irrelevant for this particular feature, which is what I think @JacobOaks meant with his comment.

For me it seems like it's easy for users to make mistakes that would cause their whole dig container to error out with this feature. For example, if two different structs had fields of same types, and their members were injected using this new feature, users might be confused why their dig container is complaining about duplicate types being provided.

xqbumu commented 1 year ago

Thanks for the example. So the feature you're describing in this ticket is actually just the ability to automatically inject members of a struct that's provided as a field of a result struct that's embedding dig.Out, correct? Sort of like automatically applying dig.Out recursively to some of its fields. Whether or not that field is anonymous seems irrelevant for this particular feature, which is what I think @JacobOaks meant with his comment.

For me it seems like it's easy for users to make mistakes that would cause their whole dig container to error out with this feature. For example, if two different structs had fields of same types, and their members were injected using this new feature, users might be confused why their dig container is complaining about duplicate types being provided.

Yes, it just add an ability to dig.Out recursively to some of its fields, and I think this is an intuitive feature because anonymous fields can also be directly accessed in golang. But in order not to break the original default behavior, I added a tag at the beginning.

I have already considered this situation, so I added a tag 'extra-anonymous' (Perhaps we need to reconsider how to name and pass value on this tag. I apologize for the misunderstanding caused.) toggle as a non-default behavior. Users can also set the injection target by using the group and name tags, as seen in the unit test content in the PR.