From f195eadcc73e825c634e2d4607572569e2b1ac25 Mon Sep 17 00:00:00 2001 From: Josephine Lee Date: Mon, 7 Feb 2022 15:40:17 -0800 Subject: [PATCH 01/15] fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. --- annotated.go | 10 ++++++++-- annotated_test.go | 48 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/annotated.go b/annotated.go index a3f1aa52d..4fa880735 100644 --- a/annotated.go +++ b/annotated.go @@ -360,7 +360,7 @@ func (ann *annotated) parameters() ( // No parameter annotations. Return the original types // and an identity function. - if len(ann.ParamTags) == 0 { + if len(ann.ParamTags) == 0 && !ft.IsVariadic() { return types, func(args []reflect.Value) []reflect.Value { return args } @@ -381,9 +381,15 @@ func (ann *annotated) parameters() ( Type: t, } + var paramTag string if i < len(ann.ParamTags) { - field.Tag = reflect.StructTag(ann.ParamTags[i]) + paramTag = ann.ParamTags[i] } + if i == ft.NumIn()-1 && ft.IsVariadic() && !strings.Contains(paramTag, "optional:") && + !strings.Contains(paramTag, "group:") { + paramTag = paramTag + " optional:\"true\"" + } + field.Tag = reflect.StructTag(paramTag) inFields = append(inFields, field) } diff --git a/annotated_test.go b/annotated_test.go index 583c890bb..a17eee6e5 100644 --- a/annotated_test.go +++ b/annotated_test.go @@ -522,6 +522,10 @@ func TestAnnotate(t *testing.T) { newSliceA := func(sa ...*a) *sliceA { return &sliceA{sa} } + newSliceAWithB := func(b *b, sa ...*a) *sliceA { + total := append(sa, b.a) + return &sliceA{total} + } t.Run("Provide with optional", func(t *testing.T) { t.Parallel() @@ -599,6 +603,50 @@ func TestAnnotate(t *testing.T) { assert.Len(t, got.sa, 2) }) + t.Run("Provide variadic function named with no given params", func(t *testing.T) { + t.Parallel() + + var got *sliceA + app := fxtest.New(t, + fx.Provide( + fx.Annotate(newSliceA, fx.ParamTags(`name:"a"`)), + ), + fx.Populate(&got), + ) + defer app.RequireStart().RequireStop() + require.NoError(t, app.Err()) + + assert.Len(t, got.sa, 0) + }) + + t.Run("Invoke variadic function with multiple params", func(t *testing.T) { + t.Parallel() + + app := fxtest.New(t, + fx.Supply( + fx.Annotate(newB(newA())), + ), + fx.Invoke(fx.Annotate(newSliceAWithB)), + ) + + defer app.RequireStart().RequireStop() + require.NoError(t, app.Err()) + }) + + t.Run("Invoke variadic with a missing dependency", func(t *testing.T) { + t.Parallel() + + app := NewForTest(t, + fx.Invoke( + fx.Annotate(newSliceA, fx.ParamTags(`optional:"false"`)), + ), + ) + err := app.Err() + require.Error(t, err) + assert.Contains(t, err.Error(), `missing dependencies`) + assert.Contains(t, err.Error(), `missing type: []*fx_test.a`) + }) + t.Run("Invoke with variadic function", func(t *testing.T) { t.Parallel() From ab3ccde615b4907165ec1dc4a2d7bfe5f5b453e3 Mon Sep 17 00:00:00 2001 From: josephinedotlee <84732210+josephinedotlee@users.noreply.github.com> Date: Tue, 8 Feb 2022 10:35:16 -0800 Subject: [PATCH 02/15] Update annotated_test.go Co-authored-by: Sung Yoon Whang --- annotated_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/annotated_test.go b/annotated_test.go index a17eee6e5..029246051 100644 --- a/annotated_test.go +++ b/annotated_test.go @@ -633,7 +633,7 @@ func TestAnnotate(t *testing.T) { require.NoError(t, app.Err()) }) - t.Run("Invoke variadic with a missing dependency", func(t *testing.T) { + t.Run("Invoke non-optional variadic function with a missing dependency", func(t *testing.T) { t.Parallel() app := NewForTest(t, From 5a8abec3057968a5b7f51f51dc094f4c153b199a Mon Sep 17 00:00:00 2001 From: Josephine Lee Date: Mon, 7 Feb 2022 15:40:17 -0800 Subject: [PATCH 03/15] fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. --- annotated.go | 10 ++++++++-- annotated_test.go | 48 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/annotated.go b/annotated.go index a3f1aa52d..4fa880735 100644 --- a/annotated.go +++ b/annotated.go @@ -360,7 +360,7 @@ func (ann *annotated) parameters() ( // No parameter annotations. Return the original types // and an identity function. - if len(ann.ParamTags) == 0 { + if len(ann.ParamTags) == 0 && !ft.IsVariadic() { return types, func(args []reflect.Value) []reflect.Value { return args } @@ -381,9 +381,15 @@ func (ann *annotated) parameters() ( Type: t, } + var paramTag string if i < len(ann.ParamTags) { - field.Tag = reflect.StructTag(ann.ParamTags[i]) + paramTag = ann.ParamTags[i] } + if i == ft.NumIn()-1 && ft.IsVariadic() && !strings.Contains(paramTag, "optional:") && + !strings.Contains(paramTag, "group:") { + paramTag = paramTag + " optional:\"true\"" + } + field.Tag = reflect.StructTag(paramTag) inFields = append(inFields, field) } diff --git a/annotated_test.go b/annotated_test.go index 583c890bb..8334dc148 100644 --- a/annotated_test.go +++ b/annotated_test.go @@ -522,6 +522,10 @@ func TestAnnotate(t *testing.T) { newSliceA := func(sa ...*a) *sliceA { return &sliceA{sa} } + newSliceAWithB := func(b *b, sa ...*a) *sliceA { + total := append(sa, b.a) + return &sliceA{total} + } t.Run("Provide with optional", func(t *testing.T) { t.Parallel() @@ -599,6 +603,50 @@ func TestAnnotate(t *testing.T) { assert.Len(t, got.sa, 2) }) + t.Run("Provide variadic function named with no given params", func(t *testing.T) { + t.Parallel() + + var got *sliceA + app := fxtest.New(t, + fx.Provide( + fx.Annotate(newSliceA, fx.ParamTags(`name:"a"`)), + ), + fx.Populate(&got), + ) + defer app.RequireStart().RequireStop() + require.NoError(t, app.Err()) + + assert.Len(t, got.sa, 0) + }) + + t.Run("Invoke variadic function with multiple params", func(t *testing.T) { + t.Parallel() + + app := fxtest.New(t, + fx.Supply( + fx.Annotate(newB(newA()), fx.ResultTags(`name:"b"`)), + ), + fx.Invoke(fx.Annotate(newSliceAWithB, fx.ParamTags(`name:"b"`))), + ) + + defer app.RequireStart().RequireStop() + require.NoError(t, app.Err()) + }) + + t.Run("Invoke variadic with a missing dependency", func(t *testing.T) { + t.Parallel() + + app := NewForTest(t, + fx.Invoke( + fx.Annotate(newSliceA, fx.ParamTags(`optional:"false"`)), + ), + ) + err := app.Err() + require.Error(t, err) + assert.Contains(t, err.Error(), `missing dependencies`) + assert.Contains(t, err.Error(), `missing type: []*fx_test.a`) + }) + t.Run("Invoke with variadic function", func(t *testing.T) { t.Parallel() From 24c501b0f6ef35ebb99d6e3fab6fbbcb7a9682b5 Mon Sep 17 00:00:00 2001 From: josephinedotlee <84732210+josephinedotlee@users.noreply.github.com> Date: Tue, 8 Feb 2022 10:35:16 -0800 Subject: [PATCH 04/15] Update annotated_test.go Co-authored-by: Sung Yoon Whang --- annotated_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/annotated_test.go b/annotated_test.go index 8334dc148..f51945202 100644 --- a/annotated_test.go +++ b/annotated_test.go @@ -633,7 +633,7 @@ func TestAnnotate(t *testing.T) { require.NoError(t, app.Err()) }) - t.Run("Invoke variadic with a missing dependency", func(t *testing.T) { + t.Run("Invoke non-optional variadic function with a missing dependency", func(t *testing.T) { t.Parallel() app := NewForTest(t, From 34519fa602c768383dce77f9ab0087f1abb3b512 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Thu, 20 Jan 2022 12:13:29 -0800 Subject: [PATCH 05/15] Temporarily pin Dig dependency to master (#827) This temporarily pins the Dig dependency in Fx to the master branch which has dig.Scope in preparation for adding fx.Module which is the corresponding user-facing API in Fx. In addition, this fixes a few tests to expect the new error message format that was changed with the graph refactoring PR in uber-go/dig#301. --- app_test.go | 4 ++-- go.mod | 2 ++ go.sum | 5 ++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app_test.go b/app_test.go index 70f5a3730..97615c0f5 100644 --- a/app_test.go +++ b/app_test.go @@ -130,8 +130,8 @@ func TestNewApp(t *testing.T) { errMsg := err.Error() assert.Contains(t, errMsg, "cycle detected in dependency graph") - assert.Contains(t, errMsg, "depends on fx_test.A") - assert.Contains(t, errMsg, "depends on fx_test.B") + assert.Contains(t, errMsg, "depends on func(fx_test.A) fx_test.B") + assert.Contains(t, errMsg, "depends on func(fx_test.B) fx_test.A") }) t.Run("ProvidesDotGraph", func(t *testing.T) { diff --git a/go.mod b/go.mod index 8e48cab53..eac0e7f1f 100644 --- a/go.mod +++ b/go.mod @@ -11,3 +11,5 @@ require ( go.uber.org/zap v1.16.0 golang.org/x/sys v0.0.0-20210903071746-97244b99971b ) + +replace go.uber.org/dig => github.com/uber-go/dig v1.13.1-0.20220106194054-29dd17211ed4 diff --git a/go.sum b/go.sum index b320780c8..613bd4598 100644 --- a/go.sum +++ b/go.sum @@ -22,11 +22,11 @@ github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UV github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/uber-go/dig v1.13.1-0.20220106194054-29dd17211ed4 h1:lTskNvD4R8nSk2vH8AwwU7N1ZpFE7AzBv2CGTfCAxlw= +github.com/uber-go/dig v1.13.1-0.20220106194054-29dd17211ed4/go.mod h1:jHAn/z1Ld1luVVyGKOAIFYz/uBFqKjjEEdIqVAqfQ2o= github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k= go.uber.org/atomic v1.6.0 h1:Ezj3JGmsOnG1MoRWQkPBsKLe9DwWD9QeXzTRzzldNVk= go.uber.org/atomic v1.6.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ= -go.uber.org/dig v1.12.0 h1:l1GQeZpEbss0/M4l/ZotuBndCrkMdjnygzgcuOjAdaY= -go.uber.org/dig v1.12.0/go.mod h1:X34SnWGr8Fyla9zQNO2GSO2D+TIuqB14OS8JhYocIyw= go.uber.org/goleak v1.1.11 h1:wy28qYRKZgnJTxGxvye5/wgWr1EKjmUDGYox5mGlRlI= go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ= go.uber.org/multierr v1.5.0 h1:KCa4XfM8CWFCpxXRGok+Q0SS/0XBhMDbHHGABQLvD2A= @@ -64,7 +64,6 @@ golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3 golang.org/x/tools v0.0.0-20190621195816-6e04913cbbac/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20191029041327-9cc4af7d6b2c/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20191029190741-b9c20aec41a5/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= -golang.org/x/tools v0.0.0-20191030062658-86caa796c7ab/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.1.5 h1:ouewzE6p+/VEB31YYnTbEJdi8pFqKp4P4n85vwo3DHA= golang.org/x/tools v0.1.5/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= From d173aa3cb9809b950fd78162f10044907b0034d7 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 8 Feb 2022 09:51:33 -0800 Subject: [PATCH 06/15] Add fx.Module (#830) This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta Co-authored-by: Abhinav Gupta Co-authored-by: Abhinav Gupta --- app.go | 176 +++++++++++++--------- app_internal_test.go | 8 +- app_test.go | 27 +++- module.go | 103 +++++++++++++ module_test.go | 340 +++++++++++++++++++++++++++++++++++++++++++ supply.go | 4 +- supply_test.go | 23 ++- 7 files changed, 603 insertions(+), 78 deletions(-) create mode 100644 module.go create mode 100644 module_test.go diff --git a/app.go b/app.go index da3ca2307..a56ae3c4b 100644 --- a/app.go +++ b/app.go @@ -53,7 +53,7 @@ const DefaultTimeout = 15 * time.Second type Option interface { fmt.Stringer - apply(*App) + apply(*module) } // Provide registers any number of constructor functions, teaching the @@ -98,9 +98,9 @@ type provideOption struct { Stack fxreflect.Stack } -func (o provideOption) apply(app *App) { +func (o provideOption) apply(mod *module) { for _, target := range o.Targets { - app.provides = append(app.provides, provide{ + mod.provides = append(mod.provides, provide{ Target: target, Stack: o.Stack, }) @@ -145,9 +145,9 @@ type invokeOption struct { Stack fxreflect.Stack } -func (o invokeOption) apply(app *App) { +func (o invokeOption) apply(mod *module) { for _, target := range o.Targets { - app.invokes = append(app.invokes, invoke{ + mod.invokes = append(mod.invokes, invoke{ Target: target, Stack: o.Stack, }) @@ -174,8 +174,8 @@ func Error(errs ...error) Option { type errorOption []error -func (errs errorOption) apply(app *App) { - app.err = multierr.Append(app.err, multierr.Combine(errs...)) +func (errs errorOption) apply(mod *module) { + mod.app.err = multierr.Append(mod.app.err, multierr.Combine(errs...)) } func (errs errorOption) String() string { @@ -219,9 +219,9 @@ func Options(opts ...Option) Option { type optionGroup []Option -func (og optionGroup) apply(app *App) { +func (og optionGroup) apply(mod *module) { for _, opt := range og { - opt.apply(app) + opt.apply(mod) } } @@ -240,8 +240,13 @@ func StartTimeout(v time.Duration) Option { type startTimeoutOption time.Duration -func (t startTimeoutOption) apply(app *App) { - app.startTimeout = time.Duration(t) +func (t startTimeoutOption) apply(m *module) { + if m.parent != nil { + m.app.err = fmt.Errorf("fx.StartTimeout Option should be passed to top-level App, " + + "not to fx.Module") + } else { + m.app.startTimeout = time.Duration(t) + } } func (t startTimeoutOption) String() string { @@ -255,8 +260,13 @@ func StopTimeout(v time.Duration) Option { type stopTimeoutOption time.Duration -func (t stopTimeoutOption) apply(app *App) { - app.stopTimeout = time.Duration(t) +func (t stopTimeoutOption) apply(m *module) { + if m.parent != nil { + m.app.err = fmt.Errorf("fx.StopTimeout Option should be passed to top-level App, " + + "not to fx.Module") + } else { + m.app.stopTimeout = time.Duration(t) + } } func (t stopTimeoutOption) String() string { @@ -288,10 +298,16 @@ type withLoggerOption struct { Stack fxreflect.Stack } -func (l withLoggerOption) apply(app *App) { - app.logConstructor = &provide{ - Target: l.constructor, - Stack: l.Stack, +func (l withLoggerOption) apply(m *module) { + if m.parent != nil { + // loggers shouldn't differ based on Module. + m.app.err = fmt.Errorf("fx.WithLogger Option should be passed to top-level App, " + + "not to fx.Module") + } else { + m.app.logConstructor = &provide{ + Target: l.constructor, + Stack: l.Stack, + } } } @@ -316,9 +332,14 @@ func Logger(p Printer) Option { type loggerOption struct{ p Printer } -func (l loggerOption) apply(app *App) { - np := writerFromPrinter(l.p) - app.log = fxlog.DefaultLogger(np) // assuming np is thread-safe. +func (l loggerOption) apply(m *module) { + if m.parent != nil { + m.app.err = fmt.Errorf("fx.StartTimeout Option should be passed to top-level App, " + + "not to fx.Module") + } else { + np := writerFromPrinter(l.p) + m.app.log = fxlog.DefaultLogger(np) // assuming np is thread-safe. + } } func (l loggerOption) String() string { @@ -366,11 +387,12 @@ var NopLogger = WithLogger(func() fxevent.Logger { return fxevent.NopLogger }) type App struct { err error clock fxclock.Clock - container *dig.Container lifecycle *lifecycleWrapper - // Constructors and its dependencies. - provides []provide - invokes []invoke + + container *dig.Container + root *module + modules []*module + // Used to setup logging within fx. log fxevent.Logger logConstructor *provide // set only if fx.WithLogger was used @@ -424,8 +446,8 @@ func ErrorHook(funcs ...ErrorHandler) Option { type errorHookOption []ErrorHandler -func (eho errorHookOption) apply(app *App) { - app.errorHooks = append(app.errorHooks, eho...) +func (eho errorHookOption) apply(m *module) { + m.app.errorHooks = append(m.app.errorHooks, eho...) } func (eho errorHookOption) String() string { @@ -455,8 +477,13 @@ type validateOption struct { validate bool } -func (o validateOption) apply(app *App) { - app.validate = o.validate +func (o validateOption) apply(m *module) { + if m.parent != nil { + m.app.err = fmt.Errorf("fx.validate Option should be passed to top-level App, " + + "not to fx.Module") + } else { + m.app.validate = o.validate + } } func (o validateOption) String() string { @@ -521,9 +548,11 @@ func New(opts ...Option) *App { startTimeout: DefaultTimeout, stopTimeout: DefaultTimeout, } + app.root = &module{app: app} + app.modules = append(app.modules, app.root) for _, opt := range opts { - opt.apply(app) + opt.apply(app.root) } // There are a few levels of wrapping on the lifecycle here. To quickly @@ -562,17 +591,21 @@ func New(opts ...Option) *App { dig.DryRun(app.validate), ) - for _, p := range app.provides { - app.provide(p) + for _, m := range app.modules { + m.build(app, app.container) + } + + for _, m := range app.modules { + m.provideAll() } frames := fxreflect.CallerStack(0, 0) // include New in the stack for default Provides - app.provide(provide{ + app.root.provide(provide{ Target: func() Lifecycle { return app.lifecycle }, Stack: frames, }) - app.provide(provide{Target: app.shutdowner, Stack: frames}) - app.provide(provide{Target: app.dotGraph, Stack: frames}) + app.root.provide(provide{Target: app.shutdowner, Stack: frames}) + app.root.provide(provide{Target: app.dotGraph, Stack: frames}) // If you are thinking about returning here after provides: do not (just yet)! // If a custom logger was being used, we're still buffering messages. @@ -597,7 +630,7 @@ func New(opts ...Option) *App { return app } - if err := app.executeInvokes(); err != nil { + if err := app.root.executeInvokes(); err != nil { app.err = err if dig.CanVisualizeError(err) { @@ -827,14 +860,14 @@ func (app *App) dotGraph() (DotGraph, error) { return DotGraph(b.String()), err } -func (app *App) provide(p provide) { - if app.err != nil { +func (m *module) provide(p provide) { + if m.app.err != nil { return } constructor := p.Target if _, ok := constructor.(Option); ok { - app.err = fmt.Errorf("fx.Option should be passed to fx.New directly, "+ + m.app.err = fmt.Errorf("fx.Option should be passed to fx.New directly, "+ "not to fx.Provide: fx.Provide received %v from:\n%+v", constructor, p.Stack) return @@ -843,6 +876,7 @@ func (app *App) provide(p provide) { var info dig.ProvideInfo opts := []dig.ProvideOption{ dig.FillProvideInfo(&info), + dig.Export(true), } defer func() { var ev fxevent.Event @@ -851,7 +885,7 @@ func (app *App) provide(p provide) { case p.IsSupply: ev = &fxevent.Supplied{ TypeName: p.SupplyType.String(), - Err: app.err, + Err: m.app.err, } default: @@ -861,39 +895,40 @@ func (app *App) provide(p provide) { } ev = &fxevent.Provided{ - ConstructorName: fxreflect.FuncName(constructor), + ConstructorName: fxreflect.FuncName(p.Target), OutputTypeNames: outputNames, - Err: app.err, + Err: m.app.err, } } - - app.log.LogEvent(ev) + m.app.log.LogEvent(ev) }() + c := m.scope switch constructor := constructor.(type) { case annotationError: // fx.Annotate failed. Turn it into an Fx error. - app.err = fmt.Errorf( + m.app.err = fmt.Errorf( "encountered error while applying annotation using fx.Annotate to %s: %+v", fxreflect.FuncName(constructor.target), constructor.err) return case annotated: - c, err := constructor.Build() + ctor, err := constructor.Build() if err != nil { - app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", constructor, p.Stack, err) + m.app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", constructor, p.Stack, err) return } - if err := app.container.Provide(c, opts...); err != nil { - app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", constructor, p.Stack, err) + if err := c.Provide(ctor, opts...); err != nil { + m.app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", constructor, p.Stack, err) + return } case Annotated: ann := constructor switch { case len(ann.Group) > 0 && len(ann.Name) > 0: - app.err = fmt.Errorf( + m.app.err = fmt.Errorf( "fx.Annotated may specify only one of Name or Group: received %v from:\n%+v", ann, p.Stack) return @@ -903,8 +938,9 @@ func (app *App) provide(p provide) { opts = append(opts, dig.Group(ann.Group)) } - if err := app.container.Provide(ann.Target, opts...); err != nil { - app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", ann, p.Stack, err) + if err := c.Provide(ann.Target, opts...); err != nil { + m.app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", ann, p.Stack, err) + return } default: @@ -915,7 +951,7 @@ func (app *App) provide(p provide) { t := ft.Out(i) if t == reflect.TypeOf(Annotated{}) { - app.err = fmt.Errorf( + m.app.err = fmt.Errorf( "fx.Annotated should be passed to fx.Provide directly, "+ "it should not be returned by the constructor: "+ "fx.Provide received %v from:\n%+v", @@ -925,40 +961,42 @@ func (app *App) provide(p provide) { } } - if err := app.container.Provide(constructor, opts...); err != nil { - app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", fxreflect.FuncName(constructor), p.Stack, err) + if err := c.Provide(constructor, opts...); err != nil { + m.app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", fxreflect.FuncName(constructor), p.Stack, err) + return } } - } -// Execute invokes in order supplied to New, returning the first error -// encountered. -func (app *App) executeInvokes() error { - // TODO: consider taking a context to limit the time spent running invocations. - - for _, i := range app.invokes { - if err := app.executeInvoke(i); err != nil { +func (m *module) executeInvokes() error { + for _, invoke := range m.invokes { + if err := m.executeInvoke(invoke); err != nil { return err } } + for _, m := range m.modules { + if err := m.executeInvokes(); err != nil { + return err + } + } return nil } -func (app *App) executeInvoke(i invoke) (err error) { +func (m *module) executeInvoke(i invoke) (err error) { fn := i.Target - fnName := fxreflect.FuncName(fn) + fnName := fxreflect.FuncName(i.Target) - app.log.LogEvent(&fxevent.Invoking{FunctionName: fnName}) + m.app.log.LogEvent(&fxevent.Invoking{FunctionName: fnName}) defer func() { - app.log.LogEvent(&fxevent.Invoked{ + m.app.log.LogEvent(&fxevent.Invoked{ FunctionName: fnName, Err: err, Trace: fmt.Sprintf("%+v", i.Stack), // format stack trace as multi-line }) }() + c := m.scope switch fn := fn.(type) { case Option: return fmt.Errorf("fx.Option should be passed to fx.New directly, "+ @@ -966,14 +1004,14 @@ func (app *App) executeInvoke(i invoke) (err error) { fn, i.Stack) case annotated: - c, err := fn.Build() + af, err := fn.Build() if err != nil { return err } - return app.container.Invoke(c) + return c.Invoke(af) default: - return app.container.Invoke(fn) + return c.Invoke(fn) } } diff --git a/app_internal_test.go b/app_internal_test.go index 197e4f73c..82de85e2c 100644 --- a/app_internal_test.go +++ b/app_internal_test.go @@ -85,8 +85,8 @@ func (o withExitOption) String() string { return fmt.Sprintf("WithExit(%v)", fxreflect.FuncName(o)) } -func (o withExitOption) apply(app *App) { - app.osExit = o +func (o withExitOption) apply(m *module) { + m.app.osExit = o } // WithClock specifies how Fx accesses time operations. @@ -98,8 +98,8 @@ func WithClock(clock fxclock.Clock) Option { type withClockOption struct{ clock fxclock.Clock } -func (o withClockOption) apply(app *App) { - app.clock = o.clock +func (o withClockOption) apply(m *module) { + m.app.clock = o.clock } func (o withClockOption) String() string { diff --git a/app_test.go b/app_test.go index 97615c0f5..128c53542 100644 --- a/app_test.go +++ b/app_test.go @@ -130,8 +130,8 @@ func TestNewApp(t *testing.T) { errMsg := err.Error() assert.Contains(t, errMsg, "cycle detected in dependency graph") - assert.Contains(t, errMsg, "depends on func(fx_test.A) fx_test.B") assert.Contains(t, errMsg, "depends on func(fx_test.B) fx_test.A") + assert.Contains(t, errMsg, "depends on func(fx_test.A) fx_test.B") }) t.Run("ProvidesDotGraph", func(t *testing.T) { @@ -1580,6 +1580,31 @@ func TestErrorHook(t *testing.T) { assert.Contains(t, graphStr, `"fx_test.B" [color=red];`) assert.Contains(t, graphStr, `"fx_test.A" [color=orange];`) }) + + t.Run("GraphWithErrorInModule", func(t *testing.T) { + t.Parallel() + + type A struct{} + type B struct{} + + var errStr, graphStr string + h := errHandlerFunc(func(err error) { + errStr = err.Error() + graphStr, _ = VisualizeError(err) + }) + NewForTest(t, + Module("module", + Provide(func() (B, error) { return B{}, fmt.Errorf("great sadness") }), + Provide(func(B) A { return A{} }), + Invoke(func(A) {}), + ErrorHook(&h), + ), + ) + assert.Contains(t, errStr, "great sadness") + assert.Contains(t, graphStr, `"fx_test.B" [color=red];`) + assert.Contains(t, graphStr, `"fx_test.A" [color=orange];`) + }) + } func TestOptionString(t *testing.T) { diff --git a/module.go b/module.go new file mode 100644 index 000000000..d73f483ed --- /dev/null +++ b/module.go @@ -0,0 +1,103 @@ +// Copyright (c) 2022 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package fx + +import ( + "fmt" + + "go.uber.org/dig" +) + +// Module is a named group of zero or more fx.Options. +func Module(name string, opts ...Option) Option { + mo := moduleOption{ + name: name, + options: opts, + } + return mo +} + +type moduleOption struct { + name string + options []Option +} + +func (o moduleOption) String() string { + return fmt.Sprintf("fx.Module(%q, %v)", o.name, o.options) +} + +func (o moduleOption) apply(mod *module) { + // This get called on any submodules' that are declared + // as part of another module. + + // 1. Create a new module with the parent being the specified + // module. + // 2. Apply child Options on the new module. + // 3. Append it to the parent module. + newModule := &module{ + name: o.name, + parent: mod, + app: mod.app, + } + for _, opt := range o.options { + opt.apply(newModule) + } + mod.modules = append(mod.modules, newModule) +} + +type module struct { + parent *module + name string + scope *dig.Scope + provides []provide + invokes []invoke + modules []*module + app *App +} + +// builds the Scopes using the App's Container. Note that this happens +// after applyModules' are called because the App's Container needs to +// be built for any Scopes to be initialized, and applys' should be called +// before the Container can get initialized. +func (m *module) build(app *App, root *dig.Container) { + if m.parent == nil { + m.scope = root.Scope(m.name) + // TODO: Once fx.Decorate is in-place, + // use the root container instead of subscope. + } else { + parentScope := m.parent.scope + m.scope = parentScope.Scope(m.name) + } + + for _, mod := range m.modules { + mod.build(app, root) + } +} + +func (m *module) provideAll() { + for _, p := range m.provides { + m.provide(p) + } + + for _, m := range m.modules { + m.provideAll() + } +} diff --git a/module_test.go b/module_test.go new file mode 100644 index 000000000..66606dac5 --- /dev/null +++ b/module_test.go @@ -0,0 +1,340 @@ +// Copyright (c) 2022 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package fx_test + +import ( + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/fx" + "go.uber.org/fx/fxevent" + "go.uber.org/fx/fxtest" + "go.uber.org/fx/internal/fxlog" +) + +func TestModuleSuccess(t *testing.T) { + t.Parallel() + + type Logger struct { + Name string + } + + t.Run("provide a dependency from a submodule", func(t *testing.T) { + t.Parallel() + + redis := fx.Module("redis", + fx.Provide(func() *Logger { + return &Logger{Name: "redis"} + }), + ) + + app := fxtest.New(t, + redis, + fx.Invoke(func(l *Logger) { + assert.Equal(t, "redis", l.Name) + }), + ) + + defer app.RequireStart().RequireStop() + }) + + t.Run("provide a dependency from nested modules", func(t *testing.T) { + t.Parallel() + app := fxtest.New(t, + fx.Module("child", + fx.Module("grandchild", + fx.Provide(func() *Logger { + return &Logger{Name: "redis"} + }), + ), + ), + fx.Invoke(func(l *Logger) { + assert.Equal(t, "redis", l.Name) + }), + ) + defer app.RequireStart().RequireStop() + }) + + t.Run("invoke from nested module", func(t *testing.T) { + t.Parallel() + invokeRan := false + app := fxtest.New(t, + fx.Provide(func() *Logger { + return &Logger{ + Name: "redis", + } + }), + fx.Module("child", + fx.Module("grandchild", + fx.Invoke(func(l *Logger) { + assert.Equal(t, "redis", l.Name) + invokeRan = true + }), + ), + ), + ) + require.True(t, invokeRan) + require.NoError(t, app.Err()) + defer app.RequireStart().RequireStop() + }) + + t.Run("invoke in module with dep from top module", func(t *testing.T) { + t.Parallel() + child := fx.Module("child", + fx.Invoke(func(l *Logger) { + assert.Equal(t, "my logger", l.Name) + }), + ) + app := fxtest.New(t, + child, + fx.Provide(func() *Logger { + return &Logger{Name: "my logger"} + }), + ) + defer app.RequireStart().RequireStop() + }) + + t.Run("provide in module with annotate", func(t *testing.T) { + t.Parallel() + child := fx.Module("child", + fx.Provide(fx.Annotate(func() *Logger { + return &Logger{Name: "good logger"} + }, fx.ResultTags(`name:"goodLogger"`))), + ) + app := fxtest.New(t, + child, + fx.Invoke(fx.Annotate(func(l *Logger) { + assert.Equal(t, "good logger", l.Name) + }, fx.ParamTags(`name:"goodLogger"`))), + ) + defer app.RequireStart().RequireStop() + }) + + t.Run("invoke in module with annotate", func(t *testing.T) { + t.Parallel() + ranInvoke := false + child := fx.Module("child", + // use something provided by the root module. + fx.Invoke(fx.Annotate(func(l *Logger) { + assert.Equal(t, "good logger", l.Name) + ranInvoke = true + })), + ) + app := fxtest.New(t, + child, + fx.Provide(fx.Annotate(func() *Logger { + return &Logger{Name: "good logger"} + })), + ) + defer app.RequireStart().RequireStop() + assert.True(t, ranInvoke) + }) + + t.Run("use Options in Module", func(t *testing.T) { + t.Parallel() + + opts := fx.Options( + fx.Provide(fx.Annotate(func() string { + return "dog" + }, fx.ResultTags(`group:"pets"`))), + fx.Provide(fx.Annotate(func() string { + return "cat" + }, fx.ResultTags(`group:"pets"`))), + ) + + petModule := fx.Module("pets", opts) + + app := fxtest.New(t, + petModule, + fx.Invoke(fx.Annotate(func(pets []string) { + assert.ElementsMatch(t, []string{"dog", "cat"}, pets) + }, fx.ParamTags(`group:"pets"`))), + ) + + defer app.RequireStart().RequireStop() + }) +} + +func TestModuleFailures(t *testing.T) { + t.Parallel() + + t.Run("invoke from submodule failed", func(t *testing.T) { + t.Parallel() + + type A struct{} + type B struct{} + + sub := fx.Module("sub", + fx.Provide(func() *A { return &A{} }), + fx.Invoke(func(*A, *B) { // missing dependency. + require.Fail(t, "this should not be called") + }), + ) + + app := NewForTest(t, + sub, + fx.Invoke(func(a *A) { + assert.NotNil(t, a) + }), + ) + + err := app.Err() + require.Error(t, err) + assert.Contains(t, err.Error(), "missing type: *fx_test.B") + }) + + t.Run("provide the same dependency from multiple modules", func(t *testing.T) { + t.Parallel() + + type A struct{} + + app := NewForTest(t, + fx.Module("mod1", fx.Provide(func() A { return A{} })), + fx.Module("mod2", fx.Provide(func() A { return A{} })), + fx.Invoke(func(a A) {}), + ) + + err := app.Err() + require.Error(t, err) + assert.Contains(t, err.Error(), "already provided by ") + }) + + t.Run("providing Modules should fail", func(t *testing.T) { + t.Parallel() + app := NewForTest(t, + fx.Module("module", + fx.Provide( + fx.Module("module"), + ), + ), + ) + err := app.Err() + require.Error(t, err) + assert.Contains(t, err.Error(), "fx.Option should be passed to fx.New directly, not to fx.Provide") + }) + + t.Run("invoking Modules should fail", func(t *testing.T) { + t.Parallel() + app := NewForTest(t, + fx.Module("module", + fx.Invoke( + fx.Invoke("module"), + ), + ), + ) + err := app.Err() + require.Error(t, err) + assert.Contains(t, err.Error(), "fx.Option should be passed to fx.New directly, not to fx.Invoke") + }) + + t.Run("annotate failure in Module", func(t *testing.T) { + t.Parallel() + + type A struct{} + newA := func() A { + return A{} + } + + app := NewForTest(t, + fx.Module("module", + fx.Provide(fx.Annotate(newA, + fx.ParamTags(`"name:"A1"`), + fx.ParamTags(`"name:"A2"`), + )), + ), + ) + err := app.Err() + require.Error(t, err) + + assert.Contains(t, err.Error(), "encountered error while applying annotation") + assert.Contains(t, err.Error(), "cannot apply more than one line of ParamTags") + }) + + t.Run("provider in Module fails", func(t *testing.T) { + t.Parallel() + + type A struct{} + type B struct{} + + newA := func() (A, error) { + return A{}, nil + } + newB := func() (B, error) { + return B{}, errors.New("minor sadness") + } + + app := NewForTest(t, + fx.Module("module", + fx.Provide( + newA, + newB, + ), + ), + fx.Invoke(func(A, B) { + assert.Fail(t, "this should never run") + }), + ) + + err := app.Err() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to build fx_test.B") + assert.Contains(t, err.Error(), "minor sadness") + }) + + t.Run("invalid Options in Module", func(t *testing.T) { + t.Parallel() + + tests := []struct { + desc string + opt fx.Option + }{ + { + desc: "StartTimeout Option", + opt: fx.StartTimeout(time.Second), + }, + { + desc: "StopTimeout Option", + opt: fx.StopTimeout(time.Second), + }, + { + desc: "WithLogger Option", + opt: fx.WithLogger(func() fxevent.Logger { return new(fxlog.Spy) }), + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.desc, func(t *testing.T) { + t.Parallel() + + app := NewForTest(t, + fx.Module("module", + tt.opt, + ), + ) + require.Error(t, app.Err()) + }) + } + }) +} diff --git a/supply.go b/supply.go index 232a49357..5813833cf 100644 --- a/supply.go +++ b/supply.go @@ -86,9 +86,9 @@ type supplyOption struct { Stack fxreflect.Stack } -func (o supplyOption) apply(app *App) { +func (o supplyOption) apply(m *module) { for i, target := range o.Targets { - app.provides = append(app.provides, provide{ + m.provides = append(m.provides, provide{ Target: target, Stack: o.Stack, IsSupply: true, diff --git a/supply_test.go b/supply_test.go index d3dd622c6..3c0bf7cca 100644 --- a/supply_test.go +++ b/supply_test.go @@ -64,6 +64,25 @@ func TestSupply(t *testing.T) { require.Same(t, bIn, bOut) }) + t.Run("SupplyInModule", func(t *testing.T) { + t.Parallel() + + aIn, bIn := &A{}, &B{} + var aOut *A + var bOut *B + + app := fxtest.New( + t, + fx.Module("module", + fx.Supply(aIn, bIn), + ), + fx.Populate(&aOut, &bOut), + ) + defer app.RequireStart().RequireStop() + require.Same(t, aIn, aOut) + require.Same(t, bIn, bOut) + }) + t.Run("AnnotateIsSupplied", func(t *testing.T) { t.Parallel() @@ -104,8 +123,7 @@ func TestSupply(t *testing.T) { require.NotPanicsf( t, func() { fx.Supply(A{}, (*B)(nil)) }, - "a wrapped nil should not panic", - ) + "a wrapped nil should not panic") require.PanicsWithValuef( t, @@ -134,4 +152,5 @@ func TestSupply(t *testing.T) { require.NoError(t, supplied[0].(*fxevent.Supplied).Err) require.Error(t, supplied[1].(*fxevent.Supplied).Err) }) + } From d9ebe58b3cb279a2f972d82ccf15bede8c19afad Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 8 Feb 2022 10:36:25 -0800 Subject: [PATCH 07/15] fx.Module: Reorganize code (#832) In #830, I reverted some of the code moves to make reviewing the change easier. This change adds back those moves on top of that PR. This reverts commit e4d006b9cb4eb6eede8e1ca672cb4d74f679ddf2. This reverts commit 8aa68c04a85ac15123bdbaffa70ce7dac478660e. Co-authored-by: Abhinav Gupta --- app.go | 261 ----------------------------------------------------- invoke.go | 94 +++++++++++++++++++ module.go | 73 +++++++++++++++ provide.go | 155 +++++++++++++++++++++++++++++++ 4 files changed, 322 insertions(+), 261 deletions(-) create mode 100644 invoke.go create mode 100644 provide.go diff --git a/app.go b/app.go index a56ae3c4b..0942d9870 100644 --- a/app.go +++ b/app.go @@ -56,112 +56,6 @@ type Option interface { apply(*module) } -// Provide registers any number of constructor functions, teaching the -// application how to instantiate various types. The supplied constructor -// function(s) may depend on other types available in the application, must -// return one or more objects, and may return an error. For example: -// -// // Constructs type *C, depends on *A and *B. -// func(*A, *B) *C -// -// // Constructs type *C, depends on *A and *B, and indicates failure by -// // returning an error. -// func(*A, *B) (*C, error) -// -// // Constructs types *B and *C, depends on *A, and can fail. -// func(*A) (*B, *C, error) -// -// The order in which constructors are provided doesn't matter, and passing -// multiple Provide options appends to the application's collection of -// constructors. Constructors are called only if one or more of their returned -// types are needed, and their results are cached for reuse (so instances of a -// type are effectively singletons within an application). Taken together, -// these properties make it perfectly reasonable to Provide a large number of -// constructors even if only a fraction of them are used. -// -// See the documentation of the In and Out types for advanced features, -// including optional parameters and named instances. -// -// Constructor functions should perform as little external interaction as -// possible, and should avoid spawning goroutines. Things like server listen -// loops, background timer loops, and background processing goroutines should -// instead be managed using Lifecycle callbacks. -func Provide(constructors ...interface{}) Option { - return provideOption{ - Targets: constructors, - Stack: fxreflect.CallerStack(1, 0), - } -} - -type provideOption struct { - Targets []interface{} - Stack fxreflect.Stack -} - -func (o provideOption) apply(mod *module) { - for _, target := range o.Targets { - mod.provides = append(mod.provides, provide{ - Target: target, - Stack: o.Stack, - }) - } -} - -func (o provideOption) String() string { - items := make([]string, len(o.Targets)) - for i, c := range o.Targets { - items[i] = fxreflect.FuncName(c) - } - return fmt.Sprintf("fx.Provide(%s)", strings.Join(items, ", ")) -} - -// Invoke registers functions that are executed eagerly on application start. -// Arguments for these invocations are built using the constructors registered -// by Provide. Passing multiple Invoke options appends the new invocations to -// the application's existing list. -// -// Unlike constructors, invocations are always executed, and they're always -// run in order. Invocations may have any number of returned values. If the -// final returned object is an error, it's assumed to be a success indicator. -// All other returned values are discarded. -// -// Typically, invoked functions take a handful of high-level objects (whose -// constructors depend on lower-level objects) and introduce them to each -// other. This kick-starts the application by forcing it to instantiate a -// variety of types. -// -// To see an invocation in use, read through the package-level example. For -// advanced features, including optional parameters and named instances, see -// the documentation of the In and Out types. -func Invoke(funcs ...interface{}) Option { - return invokeOption{ - Targets: funcs, - Stack: fxreflect.CallerStack(1, 0), - } -} - -type invokeOption struct { - Targets []interface{} - Stack fxreflect.Stack -} - -func (o invokeOption) apply(mod *module) { - for _, target := range o.Targets { - mod.invokes = append(mod.invokes, invoke{ - Target: target, - Stack: o.Stack, - }) - } -} - -func (o invokeOption) String() string { - items := make([]string, len(o.Targets)) - for i, f := range o.Targets { - items[i] = fxreflect.FuncName(f) - } - return fmt.Sprintf("fx.Invoke(%s)", strings.Join(items, ", ")) -} - // Error registers any number of errors with the application to short-circuit // startup. If more than one error is given, the errors are combined into a // single error. @@ -860,161 +754,6 @@ func (app *App) dotGraph() (DotGraph, error) { return DotGraph(b.String()), err } -func (m *module) provide(p provide) { - if m.app.err != nil { - return - } - - constructor := p.Target - if _, ok := constructor.(Option); ok { - m.app.err = fmt.Errorf("fx.Option should be passed to fx.New directly, "+ - "not to fx.Provide: fx.Provide received %v from:\n%+v", - constructor, p.Stack) - return - } - - var info dig.ProvideInfo - opts := []dig.ProvideOption{ - dig.FillProvideInfo(&info), - dig.Export(true), - } - defer func() { - var ev fxevent.Event - - switch { - case p.IsSupply: - ev = &fxevent.Supplied{ - TypeName: p.SupplyType.String(), - Err: m.app.err, - } - - default: - outputNames := make([]string, len(info.Outputs)) - for i, o := range info.Outputs { - outputNames[i] = o.String() - } - - ev = &fxevent.Provided{ - ConstructorName: fxreflect.FuncName(p.Target), - OutputTypeNames: outputNames, - Err: m.app.err, - } - } - m.app.log.LogEvent(ev) - }() - - c := m.scope - switch constructor := constructor.(type) { - case annotationError: - // fx.Annotate failed. Turn it into an Fx error. - m.app.err = fmt.Errorf( - "encountered error while applying annotation using fx.Annotate to %s: %+v", - fxreflect.FuncName(constructor.target), constructor.err) - return - - case annotated: - ctor, err := constructor.Build() - if err != nil { - m.app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", constructor, p.Stack, err) - return - } - - if err := c.Provide(ctor, opts...); err != nil { - m.app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", constructor, p.Stack, err) - return - } - - case Annotated: - ann := constructor - switch { - case len(ann.Group) > 0 && len(ann.Name) > 0: - m.app.err = fmt.Errorf( - "fx.Annotated may specify only one of Name or Group: received %v from:\n%+v", - ann, p.Stack) - return - case len(ann.Name) > 0: - opts = append(opts, dig.Name(ann.Name)) - case len(ann.Group) > 0: - opts = append(opts, dig.Group(ann.Group)) - } - - if err := c.Provide(ann.Target, opts...); err != nil { - m.app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", ann, p.Stack, err) - return - } - - default: - if reflect.TypeOf(constructor).Kind() == reflect.Func { - ft := reflect.ValueOf(constructor).Type() - - for i := 0; i < ft.NumOut(); i++ { - t := ft.Out(i) - - if t == reflect.TypeOf(Annotated{}) { - m.app.err = fmt.Errorf( - "fx.Annotated should be passed to fx.Provide directly, "+ - "it should not be returned by the constructor: "+ - "fx.Provide received %v from:\n%+v", - fxreflect.FuncName(constructor), p.Stack) - return - } - } - } - - if err := c.Provide(constructor, opts...); err != nil { - m.app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", fxreflect.FuncName(constructor), p.Stack, err) - return - } - } -} - -func (m *module) executeInvokes() error { - for _, invoke := range m.invokes { - if err := m.executeInvoke(invoke); err != nil { - return err - } - } - - for _, m := range m.modules { - if err := m.executeInvokes(); err != nil { - return err - } - } - return nil -} - -func (m *module) executeInvoke(i invoke) (err error) { - fn := i.Target - fnName := fxreflect.FuncName(i.Target) - - m.app.log.LogEvent(&fxevent.Invoking{FunctionName: fnName}) - defer func() { - m.app.log.LogEvent(&fxevent.Invoked{ - FunctionName: fnName, - Err: err, - Trace: fmt.Sprintf("%+v", i.Stack), // format stack trace as multi-line - }) - }() - - c := m.scope - switch fn := fn.(type) { - case Option: - return fmt.Errorf("fx.Option should be passed to fx.New directly, "+ - "not to fx.Invoke: fx.Invoke received %v from:\n%+v", - fn, i.Stack) - - case annotated: - af, err := fn.Build() - if err != nil { - return err - } - - return c.Invoke(af) - default: - return c.Invoke(fn) - } -} - type withTimeoutParams struct { log fxevent.Logger hook string diff --git a/invoke.go b/invoke.go new file mode 100644 index 000000000..c7c089499 --- /dev/null +++ b/invoke.go @@ -0,0 +1,94 @@ +// Copyright (c) 2019-2021 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package fx + +import ( + "fmt" + "strings" + + "go.uber.org/fx/internal/fxreflect" +) + +// Invoke registers functions that are executed eagerly on application start. +// Arguments for these invocations are built using the constructors registered +// by Provide. Passing multiple Invoke options appends the new invocations to +// the application's existing list. +// +// Unlike constructors, invocations are always executed, and they're always +// run in order. Invocations may have any number of returned values. If the +// final returned object is an error, it's assumed to be a success indicator. +// All other returned values are discarded. +// +// Typically, invoked functions take a handful of high-level objects (whose +// constructors depend on lower-level objects) and introduce them to each +// other. This kick-starts the application by forcing it to instantiate a +// variety of types. +// +// To see an invocation in use, read through the package-level example. For +// advanced features, including optional parameters and named instances, see +// the documentation of the In and Out types. +func Invoke(funcs ...interface{}) Option { + return invokeOption{ + Targets: funcs, + Stack: fxreflect.CallerStack(1, 0), + } +} + +type invokeOption struct { + Targets []interface{} + Stack fxreflect.Stack +} + +func (o invokeOption) apply(mod *module) { + for _, target := range o.Targets { + mod.invokes = append(mod.invokes, invoke{ + Target: target, + Stack: o.Stack, + }) + } +} + +func (o invokeOption) String() string { + items := make([]string, len(o.Targets)) + for i, f := range o.Targets { + items[i] = fxreflect.FuncName(f) + } + return fmt.Sprintf("fx.Invoke(%s)", strings.Join(items, ", ")) +} +func runInvoke(c container, i invoke) error { + fn := i.Target + switch fn := fn.(type) { + case Option: + return fmt.Errorf("fx.Option should be passed to fx.New directly, "+ + "not to fx.Invoke: fx.Invoke received %v from:\n%+v", + fn, i.Stack) + + case annotated: + af, err := fn.Build() + if err != nil { + return err + } + + return c.Invoke(af) + default: + return c.Invoke(fn) + } +} diff --git a/module.go b/module.go index d73f483ed..47d99ecea 100644 --- a/module.go +++ b/module.go @@ -24,8 +24,20 @@ import ( "fmt" "go.uber.org/dig" + "go.uber.org/fx/fxevent" + "go.uber.org/fx/internal/fxreflect" ) +// A container represents a set of constructors to provide +// dependencies, and a set of functions to invoke once all the +// dependencies have been initialized. +// +// This definition corresponds to the dig.Container and dig.Scope. +type container interface { + Invoke(interface{}, ...dig.InvokeOption) error + Provide(interface{}, ...dig.ProvideOption) error +} + // Module is a named group of zero or more fx.Options. func Module(name string, opts ...Option) Option { mo := moduleOption{ @@ -101,3 +113,64 @@ func (m *module) provideAll() { m.provideAll() } } + +func (m *module) provide(p provide) { + if m.app.err != nil { + return + } + + var info dig.ProvideInfo + if err := runProvide(m.scope, p, dig.FillProvideInfo(&info), dig.Export(true)); err != nil { + m.app.err = err + } + var ev fxevent.Event + switch { + case p.IsSupply: + ev = &fxevent.Supplied{ + TypeName: p.SupplyType.String(), + Err: m.app.err, + } + + default: + outputNames := make([]string, len(info.Outputs)) + for i, o := range info.Outputs { + outputNames[i] = o.String() + } + + ev = &fxevent.Provided{ + ConstructorName: fxreflect.FuncName(p.Target), + OutputTypeNames: outputNames, + Err: m.app.err, + } + } + m.app.log.LogEvent(ev) +} + +func (m *module) executeInvokes() error { + for _, invoke := range m.invokes { + if err := m.executeInvoke(invoke); err != nil { + return err + } + } + + for _, m := range m.modules { + if err := m.executeInvokes(); err != nil { + return err + } + } + return nil +} + +func (m *module) executeInvoke(i invoke) (err error) { + fnName := fxreflect.FuncName(i.Target) + m.app.log.LogEvent(&fxevent.Invoking{ + FunctionName: fnName, + }) + err = runInvoke(m.scope, i) + m.app.log.LogEvent(&fxevent.Invoked{ + FunctionName: fnName, + Err: err, + Trace: fmt.Sprintf("%+v", i.Stack), // format stack trace as multi-line + }) + return err +} diff --git a/provide.go b/provide.go new file mode 100644 index 000000000..52b01e317 --- /dev/null +++ b/provide.go @@ -0,0 +1,155 @@ +// Copyright (c) 2022 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package fx + +import ( + "fmt" + "reflect" + "strings" + + "go.uber.org/dig" + "go.uber.org/fx/internal/fxreflect" +) + +// Provide registers any number of constructor functions, teaching the +// application how to instantiate various types. The supplied constructor +// function(s) may depend on other types available in the application, must +// return one or more objects, and may return an error. For example: +// +// // Constructs type *C, depends on *A and *B. +// func(*A, *B) *C +// +// // Constructs type *C, depends on *A and *B, and indicates failure by +// // returning an error. +// func(*A, *B) (*C, error) +// +// // Constructs types *B and *C, depends on *A, and can fail. +// func(*A) (*B, *C, error) +// +// The order in which constructors are provided doesn't matter, and passing +// multiple Provide options appends to the application's collection of +// constructors. Constructors are called only if one or more of their returned +// types are needed, and their results are cached for reuse (so instances of a +// type are effectively singletons within an application). Taken together, +// these properties make it perfectly reasonable to Provide a large number of +// constructors even if only a fraction of them are used. +// +// See the documentation of the In and Out types for advanced features, +// including optional parameters and named instances. +// +// Constructor functions should perform as little external interaction as +// possible, and should avoid spawning goroutines. Things like server listen +// loops, background timer loops, and background processing goroutines should +// instead be managed using Lifecycle callbacks. +func Provide(constructors ...interface{}) Option { + return provideOption{ + Targets: constructors, + Stack: fxreflect.CallerStack(1, 0), + } +} + +type provideOption struct { + Targets []interface{} + Stack fxreflect.Stack +} + +func (o provideOption) apply(mod *module) { + for _, target := range o.Targets { + mod.provides = append(mod.provides, provide{ + Target: target, + Stack: o.Stack, + }) + } +} + +func (o provideOption) String() string { + items := make([]string, len(o.Targets)) + for i, c := range o.Targets { + items[i] = fxreflect.FuncName(c) + } + return fmt.Sprintf("fx.Provide(%s)", strings.Join(items, ", ")) +} + +func runProvide(c container, p provide, opts ...dig.ProvideOption) error { + constructor := p.Target + if _, ok := constructor.(Option); ok { + return fmt.Errorf("fx.Option should be passed to fx.New directly, "+ + "not to fx.Provide: fx.Provide received %v from:\n%+v", + constructor, p.Stack) + } + + switch constructor := constructor.(type) { + case annotationError: + // fx.Annotate failed. Turn it into an Fx error. + return fmt.Errorf( + "encountered error while applying annotation using fx.Annotate to %s: %+v", + fxreflect.FuncName(constructor.target), constructor.err) + + case annotated: + ctor, err := constructor.Build() + if err != nil { + return fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", constructor, p.Stack, err) + } + + if err := c.Provide(ctor, opts...); err != nil { + return fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", constructor, p.Stack, err) + } + + case Annotated: + ann := constructor + switch { + case len(ann.Group) > 0 && len(ann.Name) > 0: + return fmt.Errorf( + "fx.Annotated may specify only one of Name or Group: received %v from:\n%+v", + ann, p.Stack) + case len(ann.Name) > 0: + opts = append(opts, dig.Name(ann.Name)) + case len(ann.Group) > 0: + opts = append(opts, dig.Group(ann.Group)) + } + + if err := c.Provide(ann.Target, opts...); err != nil { + return fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", ann, p.Stack, err) + } + + default: + if reflect.TypeOf(constructor).Kind() == reflect.Func { + ft := reflect.ValueOf(constructor).Type() + + for i := 0; i < ft.NumOut(); i++ { + t := ft.Out(i) + + if t == reflect.TypeOf(Annotated{}) { + return fmt.Errorf( + "fx.Annotated should be passed to fx.Provide directly, "+ + "it should not be returned by the constructor: "+ + "fx.Provide received %v from:\n%+v", + fxreflect.FuncName(constructor), p.Stack) + } + } + } + + if err := c.Provide(constructor, opts...); err != nil { + return fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", fxreflect.FuncName(constructor), p.Stack, err) + } + } + return nil +} From 3735ca96f4d38deefd08000b6925842a6ec81594 Mon Sep 17 00:00:00 2001 From: Josephine Lee Date: Tue, 8 Feb 2022 15:06:40 -0800 Subject: [PATCH 08/15] Finish incomplete merge --- annotated_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/annotated_test.go b/annotated_test.go index 36b20cfdc..f51945202 100644 --- a/annotated_test.go +++ b/annotated_test.go @@ -624,15 +624,9 @@ func TestAnnotate(t *testing.T) { app := fxtest.New(t, fx.Supply( -<<<<<<< HEAD fx.Annotate(newB(newA()), fx.ResultTags(`name:"b"`)), ), fx.Invoke(fx.Annotate(newSliceAWithB, fx.ParamTags(`name:"b"`))), -======= - fx.Annotate(newB(newA())), - ), - fx.Invoke(fx.Annotate(newSliceAWithB)), ->>>>>>> 7ea7b903df9a5c0747e341c3f5a18d7a12c36912 ) defer app.RequireStart().RequireStop() From 5595e7ecd84ff1f11a60b38cadb10356cde751b4 Mon Sep 17 00:00:00 2001 From: Josephine Lee Date: Thu, 10 Feb 2022 11:02:48 -0800 Subject: [PATCH 09/15] Only make variadic params optional if no other tags are specified --- annotated.go | 13 ++++++------- annotated_test.go | 10 +++++----- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/annotated.go b/annotated.go index 4fa880735..712c7486c 100644 --- a/annotated.go +++ b/annotated.go @@ -300,6 +300,9 @@ func (ann *annotated) Build() (interface{}, error) { return nil, err } + fmt.Println(ft.IsVariadic()) + fmt.Println(paramTypes) + newFnType := reflect.FuncOf(paramTypes, resultTypes, false) origFn := reflect.ValueOf(ann.Target) newFn := reflect.MakeFunc(newFnType, func(args []reflect.Value) []reflect.Value { @@ -381,15 +384,11 @@ func (ann *annotated) parameters() ( Type: t, } - var paramTag string if i < len(ann.ParamTags) { - paramTag = ann.ParamTags[i] - } - if i == ft.NumIn()-1 && ft.IsVariadic() && !strings.Contains(paramTag, "optional:") && - !strings.Contains(paramTag, "group:") { - paramTag = paramTag + " optional:\"true\"" + field.Tag = reflect.StructTag(ann.ParamTags[i]) + } else if i == ft.NumIn()-1 && ft.IsVariadic() { + field.Tag = reflect.StructTag("optional:\"true\"") } - field.Tag = reflect.StructTag(paramTag) inFields = append(inFields, field) } diff --git a/annotated_test.go b/annotated_test.go index f51945202..1b5df1e04 100644 --- a/annotated_test.go +++ b/annotated_test.go @@ -607,16 +607,16 @@ func TestAnnotate(t *testing.T) { t.Parallel() var got *sliceA - app := fxtest.New(t, + app := NewForTest(t, fx.Provide( fx.Annotate(newSliceA, fx.ParamTags(`name:"a"`)), ), fx.Populate(&got), ) - defer app.RequireStart().RequireStop() - require.NoError(t, app.Err()) - - assert.Len(t, got.sa, 0) + err := app.Err() + require.Error(t, err) + assert.Contains(t, err.Error(), `missing dependencies`) + assert.Contains(t, err.Error(), `missing type: []*fx_test.a[name="a"]`) }) t.Run("Invoke variadic function with multiple params", func(t *testing.T) { From 16b497e741ade0f55cc87c6e61335a576a183ce7 Mon Sep 17 00:00:00 2001 From: Josephine Lee Date: Thu, 10 Feb 2022 11:05:22 -0800 Subject: [PATCH 10/15] remove print statements --- annotated.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/annotated.go b/annotated.go index 712c7486c..0dc88380d 100644 --- a/annotated.go +++ b/annotated.go @@ -300,9 +300,6 @@ func (ann *annotated) Build() (interface{}, error) { return nil, err } - fmt.Println(ft.IsVariadic()) - fmt.Println(paramTypes) - newFnType := reflect.FuncOf(paramTypes, resultTypes, false) origFn := reflect.ValueOf(ann.Target) newFn := reflect.MakeFunc(newFnType, func(args []reflect.Value) []reflect.Value { From 1fbdc13c5954487f488e9c196e4edf010fc82fb8 Mon Sep 17 00:00:00 2001 From: josephinedotlee <84732210+josephinedotlee@users.noreply.github.com> Date: Thu, 10 Feb 2022 11:44:28 -0800 Subject: [PATCH 11/15] Update annotated.go Co-authored-by: Sung Yoon Whang --- annotated.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/annotated.go b/annotated.go index 0dc88380d..008f604d7 100644 --- a/annotated.go +++ b/annotated.go @@ -384,7 +384,7 @@ func (ann *annotated) parameters() ( if i < len(ann.ParamTags) { field.Tag = reflect.StructTag(ann.ParamTags[i]) } else if i == ft.NumIn()-1 && ft.IsVariadic() { - field.Tag = reflect.StructTag("optional:\"true\"") + field.Tag = reflect.StructTag(`optional:"true"`) } inFields = append(inFields, field) From 96c8e4c3414983323664a4151271aa53235c9a0e Mon Sep 17 00:00:00 2001 From: Josephine Lee Date: Thu, 10 Feb 2022 11:58:17 -0800 Subject: [PATCH 12/15] add extra test --- annotated_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/annotated_test.go b/annotated_test.go index 1b5df1e04..4673f6a3a 100644 --- a/annotated_test.go +++ b/annotated_test.go @@ -603,6 +603,21 @@ func TestAnnotate(t *testing.T) { assert.Len(t, got.sa, 2) }) + t.Run("Provide variadic function with no optional params", func(t *testing.T) { + t.Parallel() + + app := fxtest.New(t, + fx.Provide( + fx.Annotate(newSliceA, + fx.ResultTags(`name:"as"`), + ), + fx.Annotate(func(sa sliceA) []*a { return sa.sa }, fx.ParamTags(`name:"as"`)), + ), + ) + defer app.RequireStart().RequireStop() + require.NoError(t, app.Err()) + }) + t.Run("Provide variadic function named with no given params", func(t *testing.T) { t.Parallel() From a987c12d1d7990637ce34fc67006893e1fb2260d Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Thu, 10 Feb 2022 12:14:54 -0800 Subject: [PATCH 13/15] Fix test "Provide variadic function with no optional params" --- annotated_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/annotated_test.go b/annotated_test.go index 4673f6a3a..66738bcb8 100644 --- a/annotated_test.go +++ b/annotated_test.go @@ -611,7 +611,7 @@ func TestAnnotate(t *testing.T) { fx.Annotate(newSliceA, fx.ResultTags(`name:"as"`), ), - fx.Annotate(func(sa sliceA) []*a { return sa.sa }, fx.ParamTags(`name:"as"`)), + fx.Annotate(func(sa *sliceA) []*a { return sa.sa }, fx.ParamTags(`name:"as"`)), ), ) defer app.RequireStart().RequireStop() From 2926652bf0df2274e9a3f05cee354466bf3dc8d2 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Thu, 10 Feb 2022 12:15:10 -0800 Subject: [PATCH 14/15] Explain why we're adding the optional tag --- annotated.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/annotated.go b/annotated.go index 008f604d7..06a15fff2 100644 --- a/annotated.go +++ b/annotated.go @@ -384,6 +384,9 @@ func (ann *annotated) parameters() ( if i < len(ann.ParamTags) { field.Tag = reflect.StructTag(ann.ParamTags[i]) } else if i == ft.NumIn()-1 && ft.IsVariadic() { + // If a variadic argument is unannotated, mark it optional, + // so that just wrapping a function in fx.Annotate does not + // suddenly introduce a required []arg dependency. field.Tag = reflect.StructTag(`optional:"true"`) } From f8878d1c461887d5223146d9158679735979e3ac Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Thu, 10 Feb 2022 12:22:59 -0800 Subject: [PATCH 15/15] Fix test case, verified failing on master --- annotated_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/annotated_test.go b/annotated_test.go index 66738bcb8..19bed2caf 100644 --- a/annotated_test.go +++ b/annotated_test.go @@ -606,16 +606,23 @@ func TestAnnotate(t *testing.T) { t.Run("Provide variadic function with no optional params", func(t *testing.T) { t.Parallel() + var got struct { + fx.In + + Result *sliceA `name:"as"` + } app := fxtest.New(t, + fx.Supply([]*a{{}, {}, {}}), fx.Provide( fx.Annotate(newSliceA, fx.ResultTags(`name:"as"`), ), - fx.Annotate(func(sa *sliceA) []*a { return sa.sa }, fx.ParamTags(`name:"as"`)), ), + fx.Populate(&got), ) defer app.RequireStart().RequireStop() require.NoError(t, app.Err()) + assert.Len(t, got.Result.sa, 3) }) t.Run("Provide variadic function named with no given params", func(t *testing.T) {