From eb3730ac9e0fa5a53b85b376c4dbd9f2f1022c7b Mon Sep 17 00:00:00 2001 From: Jacob Oaks Date: Thu, 30 May 2024 10:09:21 -0400 Subject: [PATCH] Support fx.Private w/ fx.Supply (#1207) `fx.Supply` is essentially an API that allows for conveniently `fx.Provide`ing an exact value, rather than a function that will return that value. For example, `fx.Provide(func() int { return 5 })` is equivalent to `fx.Supply(5)`. `fx.Private` allows for usage of a provided constructor's results to be restricted to the current module and its child modules. ```go fx.Module( "parent", fx.Invoke(func(int) { /* this will error out! */ }), fx.Module( "child", fx.Provide(func() int { return 5 }, fx.Private), ), ), ``` This PR allows for using `fx.Private` with `fx.Supply` as well, so that folks can enjoy the convenience of `fx.Supply` when they also wish to restrict the usage of the supplied value. ```go fx.Module( "parent" fx.Invoke(func(int) { /* this will error out! */ }), fx.Module( "child", fx.Supply(5, fx.Private), ), ), ``` Ref #1206 Since the behavior between Supply + Private and Provide + Private should be identical, I opted to generalize the existing `fx.Private` tests to run for both Provide and Supply. This keeps the tests a little more DRY but does complicate them/hurt readability. I feel like this is OK since there are a lot of tests, but I also am the one who wrote the tests, so I am biased regarding its readability. Thus, I am happy to break out Supply + Private into its own tests if folks feel strongly that these tests are hard to read. --- app_test.go | 307 ++++++++++++++++++++++++++++++++++------------------ provide.go | 2 +- supply.go | 31 ++++-- 3 files changed, 222 insertions(+), 118 deletions(-) diff --git a/app_test.go b/app_test.go index 727828be8..0a46d915c 100644 --- a/app_test.go +++ b/app_test.go @@ -391,134 +391,227 @@ func TestNewApp(t *testing.T) { }) } -func TestPrivateProvide(t *testing.T) { +// TestPrivate tests Private when used with both fx.Provide and fx.Supply. +func TestPrivate(t *testing.T) { t.Parallel() - t.Run("CanUsePrivateAndNonPrivateFromOuterModule", func(t *testing.T) { - t.Parallel() + testCases := []struct { + desc string - app := fxtest.New(t, - Module("SubModule", Invoke(func(a int, b string) {})), - Provide(func() int { return 0 }, Private), - Provide(func() string { return "" }), - ) - app.RequireStart().RequireStop() - }) + // provide is either a Supply or Provide wrapper around the given int + // that allows us to generalize these test cases for both APIs. + provide func(int, bool) Option + }{ + { + desc: "Provide", + provide: func(i int, private bool) Option { + opts := []any{func() int { return i }} + if private { + opts = append(opts, Private) + } + return Provide(opts...) + }, + }, + { + desc: "Supply", + provide: func(i int, private bool) Option { + opts := []any{i} + if private { + opts = append(opts, Private) + } + return Supply(opts...) + }, + }, + } - t.Run("CantUsePrivateFromSubModule", func(t *testing.T) { - t.Parallel() + for _, tt := range testCases { + t.Run(tt.desc, func(t *testing.T) { + t.Parallel() - app := NewForTest(t, - Module("SubModule", Provide(func() int { return 0 }, Private)), - Invoke(func(a int) {}), - ) - err := app.Err() - require.Error(t, err) - assert.Contains(t, err.Error(), "missing dependencies for function") - assert.Contains(t, err.Error(), "missing type: int") - }) + t.Run("CanUsePrivateFromParentModule", func(t *testing.T) { + t.Parallel() + + var invoked bool + app := fxtest.New(t, + Module("SubModule", Invoke(func(a int, b string) { + assert.Equal(t, 0, a) + invoked = true + })), + Provide(func() string { return "" }), + tt.provide(0, true /* private */), + ) + app.RequireStart().RequireStop() + assert.True(t, invoked) + }) - t.Run("DifferentModulesCanProvideSamePrivateType", func(t *testing.T) { - t.Parallel() + t.Run("CannotUsePrivateFromSubModule", func(t *testing.T) { + t.Parallel() - app := fxtest.New(t, - Module("SubModuleA", - Provide(func() int { return 1 }, Private), - Invoke(func(s int) { - assert.Equal(t, 1, s) - }), - ), - Module("SubModuleB", - Provide(func() int { return 2 }, Private), - Invoke(func(s int) { - assert.Equal(t, 2, s) - }), - ), - Provide(func() int { return 3 }), - Invoke(func(s int) { - assert.Equal(t, 3, s) - }), - ) - app.RequireStart().RequireStop() - }) + app := NewForTest(t, + Module("SubModule", tt.provide(0, true /* private */)), + Invoke(func(a int) {}), + ) + err := app.Err() + require.Error(t, err) + assert.Contains(t, err.Error(), "missing dependencies for function") + assert.Contains(t, err.Error(), "missing type: int") + }) + + t.Run("MultipleModulesSameType", func(t *testing.T) { + t.Parallel() + + var invoked int + app := fxtest.New(t, + Module("SubModuleA", + tt.provide(1, true /* private */), + Invoke(func(s int) { + assert.Equal(t, 1, s) + invoked++ + }), + ), + Module("SubModuleB", + tt.provide(2, true /* private */), + Invoke(func(s int) { + assert.Equal(t, 2, s) + invoked++ + }), + ), + tt.provide(3, false /* private */), + Invoke(func(s int) { + assert.Equal(t, 3, s) + invoked++ + }), + ) + app.RequireStart().RequireStop() + assert.Equal(t, 3, invoked) + }) + }) + } } func TestPrivateProvideWithDecorators(t *testing.T) { t.Parallel() - t.Run("DecoratedPublicOrPrivateTypeInSubModule", func(t *testing.T) { - t.Parallel() + testCases := []struct { + desc string - runApp := func(private bool) { - provideOpts := []interface{}{func() int { return 0 }} - if private { - provideOpts = append(provideOpts, Private) - } - app := NewForTest(t, - Module("SubModule", - Provide(provideOpts...), - Decorate(func(a int) int { return a + 2 }), - Invoke(func(a int) { assert.Equal(t, 2, a) }), - ), - Invoke(func(a int) { assert.Equal(t, 0, a) }), - ) - err := app.Err() - if private { - require.Error(t, err) - assert.Contains(t, err.Error(), "missing dependencies for function") - assert.Contains(t, err.Error(), "missing type: int") - } else { - require.NoError(t, err) - } - } + // provide is either a Supply or Provide wrapper around the given int + // that allows us to generalize these test cases for both APIs. + provide func(int) Option + private bool + }{ + { + desc: "Private/Provide", + provide: func(i int) Option { + return Provide( + func() int { return i }, + Private, + ) + }, + private: true, + }, + { + desc: "Private/Supply", + provide: func(i int) Option { + return Supply(i, Private) + }, + private: true, + }, + { + desc: "Public/Provide", + provide: func(i int) Option { + return Provide(func() int { return i }) + }, + private: false, + }, + { + desc: "Public/Supply", + provide: func(i int) Option { + return Supply(i) + }, + private: false, + }, + } - t.Run("Public", func(t *testing.T) { runApp(false) }) - t.Run("Private", func(t *testing.T) { runApp(true) }) - }) + for _, tt := range testCases { + t.Run(tt.desc, func(t *testing.T) { + t.Parallel() - t.Run("DecoratedPublicOrPrivateTypeInOuterModule", func(t *testing.T) { - t.Parallel() + t.Run("DecoratedTypeInSubModule", func(t *testing.T) { + t.Parallel() - runApp := func(private bool) { - provideOpts := []interface{}{func() int { return 0 }} - if private { - provideOpts = append(provideOpts, Private) - } - app := fxtest.New(t, - Provide(provideOpts...), - Decorate(func(a int) int { return a - 5 }), - Invoke(func(a int) { - assert.Equal(t, -5, a) - }), - Module("Child", - Decorate(func(a int) int { return a + 10 }), + var invoked bool + app := NewForTest(t, + Module("SubModule", + tt.provide(0), + Decorate(func(a int) int { return a + 2 }), + Invoke(func(a int) { assert.Equal(t, 2, a) }), + ), Invoke(func(a int) { - assert.Equal(t, 5, a) + // Decoration is always "private", + // so raw provided value is expected here + // when the submodule provides it as public. + assert.Equal(t, 0, a) + invoked = true }), - ), - ) - app.RequireStart().RequireStop() - } + ) + err := app.Err() + if tt.private { + require.Error(t, err) + assert.Contains(t, err.Error(), "missing dependencies for function") + assert.Contains(t, err.Error(), "missing type: int") + } else { + require.NoError(t, err) + assert.True(t, invoked) + } + }) - t.Run("Public", func(t *testing.T) { runApp(false) }) - t.Run("Private", func(t *testing.T) { runApp(true) }) - }) + t.Run("DecoratedTypeInParentModule", func(t *testing.T) { + t.Parallel() - t.Run("CannotDecoratePrivateChildType", func(t *testing.T) { - t.Parallel() + var invoked int + app := fxtest.New(t, + tt.provide(0), + Decorate(func(a int) int { return a - 5 }), + Invoke(func(a int) { + assert.Equal(t, -5, a) + invoked++ + }), + Module("Child", + Decorate(func(a int) int { return a + 10 }), + Invoke(func(a int) { + assert.Equal(t, 5, a) + invoked++ + }), + ), + ) + app.RequireStart().RequireStop() + assert.Equal(t, 2, invoked) + }) - app := NewForTest(t, - Module("Child", - Provide(func() int { return 0 }, Private), - ), - Decorate(func(a int) int { return a + 5 }), - Invoke(func(a int) {}), - ) - err := app.Err() - require.Error(t, err) - assert.Contains(t, err.Error(), "missing dependencies for function") - assert.Contains(t, err.Error(), "missing type: int") - }) + t.Run("ParentDecorateChildType", func(t *testing.T) { + t.Parallel() + + var invoked bool + app := NewForTest(t, + Module("Child", tt.provide(0)), + Decorate(func(a int) int { return a + 5 }), + Invoke(func(a int) { + invoked = true + }), + ) + err := app.Err() + if tt.private { + require.Error(t, err) + assert.Contains(t, err.Error(), "missing dependencies for function") + assert.Contains(t, err.Error(), "missing type: int") + } else { + require.NoError(t, err) + assert.True(t, invoked) + } + }) + }) + } } func TestWithLoggerErrorUseDefault(t *testing.T) { diff --git a/provide.go b/provide.go index 999262c36..72db3f83b 100644 --- a/provide.go +++ b/provide.go @@ -96,7 +96,7 @@ func (o provideOption) apply(mod *module) { type privateOption struct{} -// Private is an option that can be passed as an argument to [Provide] to +// Private is an option that can be passed as an argument to [Provide] or [Supply] to // restrict access to the constructors being provided. Specifically, // corresponding constructors can only be used within the current module // or modules the current module contains. Other modules that contain this diff --git a/supply.go b/supply.go index f3997289e..23d1a6ef0 100644 --- a/supply.go +++ b/supply.go @@ -56,6 +56,8 @@ import ( // // Supply panics if a value (or annotation target) is an untyped nil or an error. // +// [Private] can be used to restrict access to supplied values. +// // # Supply Caveats // // As mentioned above, Supply uses the most specific type of the provided @@ -78,29 +80,36 @@ import ( // fx.Annotate(handler, fx.As(new(http.Handler))), // ) func Supply(values ...interface{}) Option { - constructors := make([]interface{}, len(values)) // one function per value - types := make([]reflect.Type, len(values)) - for i, value := range values { + constructors := make([]interface{}, 0, len(values)) + types := make([]reflect.Type, 0, len(values)) + var private bool + for _, value := range values { + var ( + typ reflect.Type + ctor any + ) switch value := value.(type) { + case privateOption: + private = true + continue case annotated: - var typ reflect.Type value.Target, typ = newSupplyConstructor(value.Target) - constructors[i] = value - types[i] = typ + ctor = value case Annotated: - var typ reflect.Type value.Target, typ = newSupplyConstructor(value.Target) - constructors[i] = value - types[i] = typ + ctor = value default: - constructors[i], types[i] = newSupplyConstructor(value) + ctor, typ = newSupplyConstructor(value) } + constructors = append(constructors, ctor) + types = append(types, typ) } return supplyOption{ Targets: constructors, Types: types, Stack: fxreflect.CallerStack(1, 0), + Private: private, } } @@ -108,6 +117,7 @@ type supplyOption struct { Targets []interface{} Types []reflect.Type // type of value produced by constructor[i] Stack fxreflect.Stack + Private bool } func (o supplyOption) apply(m *module) { @@ -117,6 +127,7 @@ func (o supplyOption) apply(m *module) { Stack: o.Stack, IsSupply: true, SupplyType: o.Types[i], + Private: o.Private, }) } }