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

Cyclic dependencies in decorators #391

Open
paullen opened this issue Jun 10, 2023 · 4 comments
Open

Cyclic dependencies in decorators #391

paullen opened this issue Jun 10, 2023 · 4 comments
Assignees

Comments

@paullen
Copy link
Contributor

paullen commented Jun 10, 2023

Describe the bug
Providing a decorator with params other than the decorated type does not use the singleton value already in the container but creates another instance of it. The entire point of DI, imo, is the singleton pattern of type usage. Please let me know if this is a design choice and some context around why this way was chosen, if you can.

To Reproduce
Consider the following code:

package main

import (
	"fmt"
	"log"

	"go.uber.org/dig"
)

type Logger struct{}
type LogType string

const (
	LogType_ERROR   LogType = "ERROR"
	LogType_INFO    LogType = "INFO"
	LogType_WARNING LogType = "WARNING"
)

func (l Logger) Log(logType LogType, message string) {
	log.Println(logType, message)
}

type LoggerOutput struct {
	dig.Out

	Logger *Logger
}

func NewLogger() LoggerOutput {
	return LoggerOutput{Logger: &Logger{}}
}

func NewLoggerDecorator(client *HttpClient, logger *Logger) LoggerOutput {
	logger.Log(LogType_INFO, fmt.Sprintf("%p", client))
	return LoggerOutput{Logger: logger}
}

type HttpClient struct {
	logger *Logger
}

func (client *HttpClient) Get(url string) (string, error) {
	client.logger.Log(LogType_INFO, "getting "+url)

	return "You got " + url, nil
}

type HttpClientInput struct {
	dig.In
	Logger *Logger
}

func NewHttpClient(input HttpClientInput) *HttpClient {
	return &HttpClient{logger: input.Logger}
}

func BuildContainer() (*dig.Container, error) {
	container := dig.New()

	if err := container.Provide(NewLogger); err != nil {
		return nil, err
	}
	if err := container.Provide(NewHttpClient); err != nil {
		return nil, err
	}
	if err := container.Decorate(NewLoggerDecorator); err != nil {
		return nil, err
	}

	return container, nil
}

func ExecuteFunction(client *HttpClient) {
	client.logger.Log(LogType_INFO, fmt.Sprintf("%p", client))

	res, _ := client.Get("foo.bar")

	fmt.Println(res)
}

func main() {
	container, err := BuildContainer()
	if err != nil {
		log.Println(err)
		return
	}
	if err := container.Invoke(ExecuteFunction); err != nil {
		log.Println(err)
	}

}

Expected behavior
The expected behaviour should be an error while calling the Decorate function letting the user know about the cyclic dependency.

Additional context
I have an implementation of decorators with support for cyclic checks and multiple decorators per type in a single container in my fork. Although it is a little hacky since it was done when I was just starting out, we could formalise the design and I could raise a PR with these features. Let me know if this is something y'all would like. There is a need for multiple decorators per type with the cyclic dependency check in my organisation and I hope to see these features in the original repo.

@r-hang r-hang assigned r-hang and JacobOaks and unassigned r-hang Jun 13, 2023
@JacobOaks
Copy link
Contributor

Thanks for reporting this, this looks like a bug. I'll investigate. Internal ref: GO-2096.

@paullen
Copy link
Contributor Author

paullen commented Jun 14, 2023

I'm curious about the solution for this. I propose this one:

  1. Add a function in the containerStore interface called getAllDecoratorDependencies, that'll get a union of all dependencies(excluding the type itself) for a type's decorators.
  2. While checking for cycles include the getAllDecoratorDependencies in the list of dependencies for any type.
  3. Also include the dependencies from getAllDecoratorDependencies in findMissingDependencies in the param list for all types.

Please let me know your thoughts on this. I could raise a PR with this solution.

@paullen
Copy link
Contributor Author

paullen commented Mar 18, 2024

Hi @JacobOaks. Just wanted to check up on where we are with this fix. I'd be happy to help with it.

@JacobOaks
Copy link
Contributor

JacobOaks commented Apr 2, 2024

Hey @paullen - sorry for the delayed response. This is a legitimate issue we should fix, but we are still thinking about the right way to go about it, since it has potential to break existing apps.

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

No branches or pull requests

3 participants