Cyclic dependencies in decorators

[paullen]

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.

[JacobOaks]

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

[paullen]

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]

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

[JacobOaks]

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.