From 70a2b9dfbc85713675879d416c23709774cbfd7f Mon Sep 17 00:00:00 2001 From: Jeancarlo Date: Mon, 1 Aug 2022 08:22:41 -0600 Subject: [PATCH 1/9] fix: prevent panic on depinject when input or output struct has an unexported fild --- depinject/struct_args.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/depinject/struct_args.go b/depinject/struct_args.go index 5de9eea7280f..1c17261042a3 100644 --- a/depinject/struct_args.go +++ b/depinject/struct_args.go @@ -1,6 +1,7 @@ package depinject import ( + "fmt" "reflect" "github.com/pkg/errors" @@ -74,7 +75,10 @@ func expandStructArgsFn(provider ProviderDescriptor) func(inputs []reflect.Value inputs1 := make([]reflect.Value, len(inParams)) for i, in := range inParams { if in.Type.AssignableTo(isInType) { - v, n := buildIn(in.Type, inputs[j:]) + v, n, err := buildIn(in.Type, inputs[j:]) + if err != nil { + return []reflect.Value{}, err + } inputs1[i] = v j += n } else { @@ -158,7 +162,7 @@ func structArgsOutTypes(typ reflect.Type) []ProviderOutput { return res } -func buildIn(typ reflect.Type, values []reflect.Value) (reflect.Value, int) { +func buildIn(typ reflect.Type, values []reflect.Value) (reflect.Value, int, error) { numFields := typ.NumField() j := 0 res := reflect.New(typ) @@ -167,11 +171,14 @@ func buildIn(typ reflect.Type, values []reflect.Value) (reflect.Value, int) { if f.Type.AssignableTo(isInType) { continue } + if !res.Elem().Field(i).CanSet() { + return reflect.Value{}, 0, fmt.Errorf("using value obtained using unexported field") + } res.Elem().Field(i).Set(values[j]) j++ } - return res.Elem(), j + return res.Elem(), j, nil } func extractFromOut(typ reflect.Type, value reflect.Value) []reflect.Value { From c763202edd489ca95b41f0dfa37ce5cb3b53b1c0 Mon Sep 17 00:00:00 2001 From: Jeancarlo Date: Mon, 1 Aug 2022 08:40:10 -0600 Subject: [PATCH 2/9] more verbose error message --- depinject/struct_args.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/depinject/struct_args.go b/depinject/struct_args.go index 1c17261042a3..adb5a48b323a 100644 --- a/depinject/struct_args.go +++ b/depinject/struct_args.go @@ -172,7 +172,7 @@ func buildIn(typ reflect.Type, values []reflect.Value) (reflect.Value, int, erro continue } if !res.Elem().Field(i).CanSet() { - return reflect.Value{}, 0, fmt.Errorf("using value obtained using unexported field") + return reflect.Value{}, 0, fmt.Errorf("using value obtained using unexported field %s from %s on package %s", f.Name, res.Elem().String(), f.PkgPath) } res.Elem().Field(i).Set(values[j]) From af4651d76d3a1c1b3f61d01c5de0b78850c27de7 Mon Sep 17 00:00:00 2001 From: Jeancarlo Date: Mon, 1 Aug 2022 11:27:18 -0600 Subject: [PATCH 3/9] add test for fix --- depinject/container_test.go | 54 +++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/depinject/container_test.go b/depinject/container_test.go index 16fc2ad3ecf2..e6a294697f7d 100644 --- a/depinject/container_test.go +++ b/depinject/container_test.go @@ -30,6 +30,11 @@ type KeeperB struct { msgClientA MsgClientA } +type KeeperC struct { + key KVStoreKey + msgClientA MsgClientA +} + type Handler struct { Handle func() } @@ -58,6 +63,8 @@ func (ModuleA) Provide(key KVStoreKey, moduleKey depinject.OwnModuleKey) (Keeper type ModuleB struct{} +type ModuleC struct{} + type BDependencies struct { depinject.In @@ -82,6 +89,53 @@ func (ModuleB) Provide(dependencies BDependencies) (BProvides, Handler, error) { }, Handler{}, nil } +type UnexportedFieldCDependencies struct { + depinject.In + + key KVStoreKey + A MsgClientA +} + +type UnexportedFieldCProvides struct { + depinject.Out + + keeperC KeeperC + Commands []Command +} + +func (ModuleC) Provide(dependencies BDependencies) (UnexportedFieldCProvides, Handler, error) { + return UnexportedFieldCProvides{ + keeperC: KeeperC{ + key: dependencies.Key, + msgClientA: dependencies.A, + }, + Commands: []Command{{}, {}}, + }, Handler{}, nil +} + +func TestUnexportedField(t *testing.T) { + var ( + handlers map[string]Handler + commands []Command + a KeeperA + b KeeperB + + scenarioConfig = depinject.Configs( + depinject.ProvideInModule("a", wrapMethod0(ModuleA{})), + depinject.ProvideInModule("b", wrapMethod0(ModuleC{})), + ) + ) + + require.Error(t, + depinject.Inject( + scenarioConfig, + &handlers, + &commands, + &a, + &b, + )) +} + var scenarioConfig = depinject.Configs( depinject.Provide(ProvideMsgClientA), depinject.ProvideInModule("runtime", ProvideKVStoreKey), From aa20d1efd41848a1420e3a9536e67c8b886a4668 Mon Sep 17 00:00:00 2001 From: Jeancarlo Barrios Date: Tue, 2 Aug 2022 08:50:37 -0600 Subject: [PATCH 4/9] Update depinject/struct_args.go Co-authored-by: Aaron Craelius --- depinject/struct_args.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/depinject/struct_args.go b/depinject/struct_args.go index adb5a48b323a..405ea0fe559a 100644 --- a/depinject/struct_args.go +++ b/depinject/struct_args.go @@ -172,7 +172,7 @@ func buildIn(typ reflect.Type, values []reflect.Value) (reflect.Value, int, erro continue } if !res.Elem().Field(i).CanSet() { - return reflect.Value{}, 0, fmt.Errorf("using value obtained using unexported field %s from %s on package %s", f.Name, res.Elem().String(), f.PkgPath) + return reflect.Value{}, 0, fmt.Errorf("depinject.In struct %s on package %s can't have unexported field", res.Elem().String(), f.PkgPath, f.Name) } res.Elem().Field(i).Set(values[j]) From 9ed566e44488e44b236212b1f454d2227072a01b Mon Sep 17 00:00:00 2001 From: Jeancarlo Date: Tue, 2 Aug 2022 13:29:31 -0600 Subject: [PATCH 5/9] add more error handling for depinject.Out --- depinject/container.go | 7 ++++ depinject/container_test.go | 72 ++++++++++++++++++++++++++++++------- depinject/struct_args.go | 8 +++-- 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/depinject/container.go b/depinject/container.go index 90b24f1444a2..2b95140594e3 100644 --- a/depinject/container.go +++ b/depinject/container.go @@ -446,6 +446,13 @@ func (c *container) build(loc Location, outputs ...interface{}) error { for i, output := range outputs { val := reflect.ValueOf(output) + if !val.Elem().CanSet() { + return []reflect.Value{}, fmt.Errorf("depinject.In struct %s on package can't have unexported field", val.String()) + } + if !values[i].CanInterface() { + return []reflect.Value{}, fmt.Errorf("depinject.Out struct %s on package can't have unexported field", values[i].String()) + + } val.Elem().Set(values[i]) } diff --git a/depinject/container_test.go b/depinject/container_test.go index e6a294697f7d..0e43bbc55478 100644 --- a/depinject/container_test.go +++ b/depinject/container_test.go @@ -63,8 +63,6 @@ func (ModuleA) Provide(key KVStoreKey, moduleKey depinject.OwnModuleKey) (Keeper type ModuleB struct{} -type ModuleC struct{} - type BDependencies struct { depinject.In @@ -75,13 +73,13 @@ type BDependencies struct { type BProvides struct { depinject.Out - KeeperB KeeperB + keeperB KeeperB Commands []Command } func (ModuleB) Provide(dependencies BDependencies) (BProvides, Handler, error) { return BProvides{ - KeeperB: KeeperB{ + keeperB: KeeperB{ key: dependencies.Key, msgClientA: dependencies.A, }, @@ -89,6 +87,18 @@ func (ModuleB) Provide(dependencies BDependencies) (BProvides, Handler, error) { }, Handler{}, nil } +type ModuleUnexportedDependency struct{} + +func (ModuleUnexportedDependency) Provide(dependencies UnexportedFieldCDependencies) (CProvides, Handler, error) { + return CProvides{ + KeeperC: KeeperC{ + key: dependencies.key, + msgClientA: dependencies.A, + }, + Commands: []Command{{}, {}}, + }, Handler{}, nil +} + type UnexportedFieldCDependencies struct { depinject.In @@ -96,6 +106,22 @@ type UnexportedFieldCDependencies struct { A MsgClientA } +type CProvides struct { + depinject.Out + + KeeperC KeeperC + Commands []Command +} + +type ModuleUnexportedProvides struct{} + +type CDependencies struct { + depinject.In + + Key KVStoreKey + A MsgClientA +} + type UnexportedFieldCProvides struct { depinject.Out @@ -103,7 +129,7 @@ type UnexportedFieldCProvides struct { Commands []Command } -func (ModuleC) Provide(dependencies BDependencies) (UnexportedFieldCProvides, Handler, error) { +func (ModuleUnexportedProvides) Provide(dependencies CDependencies) (UnexportedFieldCProvides, Handler, error) { return UnexportedFieldCProvides{ keeperC: KeeperC{ key: dependencies.Key, @@ -118,22 +144,44 @@ func TestUnexportedField(t *testing.T) { handlers map[string]Handler commands []Command a KeeperA - b KeeperB + c KeeperC + + scenarioConfigProvides = depinject.Configs( + depinject.Provide(ProvideMsgClientA), + depinject.ProvideInModule("runtime", ProvideKVStoreKey), + depinject.ProvideInModule("a", wrapMethod0(ModuleA{})), + depinject.ProvideInModule("c", wrapMethod0(ModuleUnexportedProvides{})), + ) - scenarioConfig = depinject.Configs( + scenarioConfigDependency = depinject.Configs( + depinject.Provide(ProvideMsgClientA), + depinject.ProvideInModule("runtime", ProvideKVStoreKey), depinject.ProvideInModule("a", wrapMethod0(ModuleA{})), - depinject.ProvideInModule("b", wrapMethod0(ModuleC{})), + depinject.ProvideInModule("c", wrapMethod0(ModuleUnexportedDependency{})), ) ) - require.Error(t, + require.ErrorContains(t, depinject.Inject( - scenarioConfig, + scenarioConfigProvides, &handlers, &commands, &a, - &b, - )) + &c, + ), + "depinject.Out struct", + ) + + require.ErrorContains(t, + depinject.Inject( + scenarioConfigDependency, + &handlers, + &commands, + &a, + &c, + ), + "depinject.In struct", + ) } var scenarioConfig = depinject.Configs( diff --git a/depinject/struct_args.go b/depinject/struct_args.go index 405ea0fe559a..3a26c3045ab8 100644 --- a/depinject/struct_args.go +++ b/depinject/struct_args.go @@ -171,8 +171,12 @@ func buildIn(typ reflect.Type, values []reflect.Value) (reflect.Value, int, erro if f.Type.AssignableTo(isInType) { continue } - if !res.Elem().Field(i).CanSet() { - return reflect.Value{}, 0, fmt.Errorf("depinject.In struct %s on package %s can't have unexported field", res.Elem().String(), f.PkgPath, f.Name) + if !res.Elem().Field(i).CanSet() && values[j].CanInterface() { + return reflect.Value{}, 0, fmt.Errorf("depinject.In struct %s on package %s can't have unexported field", res.Elem().String(), f.PkgPath) + } + if !values[j].CanInterface() { + return reflect.Value{}, 0, fmt.Errorf("depinject.Out struct %s on package %s can't have unexported field", res.Elem().String(), f.PkgPath) + } res.Elem().Field(i).Set(values[j]) From 8a9c40738d595944d0e045c0204760a8e0ee327d Mon Sep 17 00:00:00 2001 From: Jeancarlo Date: Tue, 2 Aug 2022 14:07:24 -0600 Subject: [PATCH 6/9] remove unesesary check --- depinject/struct_args.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/depinject/struct_args.go b/depinject/struct_args.go index 3a26c3045ab8..07dda03c9340 100644 --- a/depinject/struct_args.go +++ b/depinject/struct_args.go @@ -171,7 +171,7 @@ func buildIn(typ reflect.Type, values []reflect.Value) (reflect.Value, int, erro if f.Type.AssignableTo(isInType) { continue } - if !res.Elem().Field(i).CanSet() && values[j].CanInterface() { + if !res.Elem().Field(i).CanSet() { return reflect.Value{}, 0, fmt.Errorf("depinject.In struct %s on package %s can't have unexported field", res.Elem().String(), f.PkgPath) } if !values[j].CanInterface() { From 71acf78d8ee445278cf33d8fdbcc82a89c987772 Mon Sep 17 00:00:00 2001 From: Jeancarlo Date: Tue, 2 Aug 2022 15:40:01 -0600 Subject: [PATCH 7/9] added more coverage --- depinject/container.go | 4 +-- depinject/container_test.go | 50 +++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/depinject/container.go b/depinject/container.go index 2b95140594e3..47fd95d32b18 100644 --- a/depinject/container.go +++ b/depinject/container.go @@ -446,9 +446,7 @@ func (c *container) build(loc Location, outputs ...interface{}) error { for i, output := range outputs { val := reflect.ValueOf(output) - if !val.Elem().CanSet() { - return []reflect.Value{}, fmt.Errorf("depinject.In struct %s on package can't have unexported field", val.String()) - } + if !values[i].CanInterface() { return []reflect.Value{}, fmt.Errorf("depinject.Out struct %s on package can't have unexported field", values[i].String()) diff --git a/depinject/container_test.go b/depinject/container_test.go index 0e43bbc55478..33dea74f2119 100644 --- a/depinject/container_test.go +++ b/depinject/container_test.go @@ -35,6 +35,10 @@ type KeeperC struct { msgClientA MsgClientA } +type KeeperD struct { + key KVStoreKey +} + type Handler struct { Handle func() } @@ -139,12 +143,38 @@ func (ModuleUnexportedProvides) Provide(dependencies CDependencies) (UnexportedF }, Handler{}, nil } +type ModuleD struct{} + +type DDependencies struct { + depinject.In + + Key KVStoreKey + KeeperC KeeperC +} + +type DProvides struct { + depinject.Out + + KeeperD KeeperD + Commands []Command +} + +func (ModuleD) Provide(dependencies DDependencies) (DProvides, Handler, error) { + return DProvides{ + KeeperD: KeeperD{ + key: dependencies.Key, + }, + Commands: []Command{{}, {}}, + }, Handler{}, nil +} + func TestUnexportedField(t *testing.T) { var ( handlers map[string]Handler commands []Command a KeeperA c KeeperC + d KeeperD scenarioConfigProvides = depinject.Configs( depinject.Provide(ProvideMsgClientA), @@ -159,6 +189,14 @@ func TestUnexportedField(t *testing.T) { depinject.ProvideInModule("a", wrapMethod0(ModuleA{})), depinject.ProvideInModule("c", wrapMethod0(ModuleUnexportedDependency{})), ) + + scenarioConfigProvidesDependency = depinject.Configs( + depinject.Provide(ProvideMsgClientA), + depinject.ProvideInModule("runtime", ProvideKVStoreKey), + depinject.ProvideInModule("a", wrapMethod0(ModuleA{})), + depinject.ProvideInModule("c", wrapMethod0(ModuleUnexportedProvides{})), + depinject.ProvideInModule("d", wrapMethod0(ModuleD{})), + ) ) require.ErrorContains(t, @@ -182,6 +220,18 @@ func TestUnexportedField(t *testing.T) { ), "depinject.In struct", ) + + require.ErrorContains(t, + depinject.Inject( + scenarioConfigProvidesDependency, + &handlers, + &commands, + &a, + &c, + &d, + ), + "depinject.Out struct", + ) } var scenarioConfig = depinject.Configs( From 0d006f29daf26d618f04f220b2a7a04cb78cc685 Mon Sep 17 00:00:00 2001 From: Jeancarlo Date: Tue, 2 Aug 2022 15:44:20 -0600 Subject: [PATCH 8/9] t --- depinject/container_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/depinject/container_test.go b/depinject/container_test.go index 33dea74f2119..b5b03421e554 100644 --- a/depinject/container_test.go +++ b/depinject/container_test.go @@ -77,7 +77,7 @@ type BDependencies struct { type BProvides struct { depinject.Out - keeperB KeeperB + KeeperB KeeperB Commands []Command } From 38696369d951fb4ac7ffd040065d1bbdf458daed Mon Sep 17 00:00:00 2001 From: Jeancarlo Date: Tue, 2 Aug 2022 15:45:52 -0600 Subject: [PATCH 9/9] t --- depinject/container_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/depinject/container_test.go b/depinject/container_test.go index b5b03421e554..87a37f65123b 100644 --- a/depinject/container_test.go +++ b/depinject/container_test.go @@ -83,7 +83,7 @@ type BProvides struct { func (ModuleB) Provide(dependencies BDependencies) (BProvides, Handler, error) { return BProvides{ - keeperB: KeeperB{ + KeeperB: KeeperB{ key: dependencies.Key, msgClientA: dependencies.A, },