Skip to content

Commit

Permalink
fix: correct handling of embedded structs in config (#29)
Browse files Browse the repository at this point in the history 
* add tests for FieldDescriber

Signed-off-by: Will Murphy <[email protected]>

* Stop panicking on config with embedded pointer

Signed-off-by: Will Murphy <[email protected]>

* chore: update tests with more specific naming

Signed-off-by: Keith Zantow <[email protected]>

* chore: update tests with more specific naming

Signed-off-by: Keith Zantow <[email protected]>

---------

Signed-off-by: Will Murphy <[email protected]>
Signed-off-by: Keith Zantow <[email protected]>
Co-authored-by: Keith Zantow <[email protected]>
  • Loading branch information
@willmurphyscode @kzantow
willmurphyscode and kzantow authored Nov 6, 2023
1 parent a8fc5a2 commit d96c8f3
Show file tree
Hide file tree
Showing 7 changed files with 256 additions and 86 deletions.
4 changes: 2 additions & 2 deletions field_describer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func (d *directDescriber) GetDescription(v reflect.Value, _ reflect.StructField)

func addFieldDescriptions(d FieldDescriptionSet, v reflect.Value) {
t := v.Type()
for isPtr(t) {
for isPtr(t) && v.CanInterface() {
o := v.Interface()
if p, ok := o.(FieldDescriber); ok && !isPromotedMethod(o, "DescribeFields") {
p.DescribeFields(d)
Expand All @@ -84,7 +84,7 @@ func addFieldDescriptions(d FieldDescriptionSet, v reflect.Value) {

for i := 0; i < v.NumField(); i++ {
f := t.Field(i)
if skipField(f) {
if !includeField(f) {
continue
}
v := v.Field(i)
Expand Down
57 changes: 57 additions & 0 deletions field_describer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,63 @@ func Test_fieldDescriber(t *testing.T) {
require.Contains(t, values, "fd test 3 value description")
}

func Test_FieldDescriberDoesNotPanicOnEmbeddedUnexportedStruct(t *testing.T) {
type moduleConfig struct {
ModuleBool bool `yaml:"module-bool" mapstructure:"module-bool"`
}

type specialModuleConfig struct {
moduleConfig `yaml:",inline" mapstructure:",squash"`
SpecialModuleBool bool `yaml:"special-module-bool" mapstructure:"special-module-bool"`
}

type TopLevelConfig struct {
Module1 moduleConfig `yaml:"module-1" mapstructure:"module-1"`
Module2 specialModuleConfig `yaml:"module-2" mapstructure:"module-2"`
}

cfgPtr := &TopLevelConfig{}
_ = NewFieldDescriber(cfgPtr)
}

func Test_FieldDescriberDoesNotPanicOnEmbeddedExportedStructPointer(t *testing.T) {
type ModuleConfig struct {
ModuleBool bool `yaml:"module-bool" mapstructure:"module-bool"`
}

type specialModuleConfig struct {
*ModuleConfig `yaml:",inline" mapstructure:",squash"`
SpecialModuleBool bool `yaml:"special-module-bool" mapstructure:"special-module-bool"`
}

type TopLevelConfig struct {
Module1 ModuleConfig `yaml:"module-1" mapstructure:"module-1"`
Module2 specialModuleConfig `yaml:"module-2" mapstructure:"module-2"`
}

cfgPtr := &TopLevelConfig{}
_ = NewFieldDescriber(cfgPtr)
}

func Test_FieldDescriberDoesNotPanicOnEmbeddedUnexportedStructPointer(t *testing.T) {
type moduleConfig struct {
ModuleBool bool `yaml:"module-bool" mapstructure:"module-bool"`
}

type specialModuleConfig struct {
*moduleConfig `yaml:",inline" mapstructure:",squash"`
SpecialModuleBool bool `yaml:"special-module-bool" mapstructure:"special-module-bool"`
}

type TopLevelConfig struct {
Module1 moduleConfig `yaml:"module-1" mapstructure:"module-1"`
Module2 specialModuleConfig `yaml:"module-2" mapstructure:"module-2"`
}

cfgPtr := &TopLevelConfig{}
_ = NewFieldDescriber(cfgPtr)
}

type fdTest1 struct {
called int
String string
Expand Down
6 changes: 4 additions & 2 deletions flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func addFlags(log logger.Logger, flags FlagSet, o any) {
if isStruct(t) {
for i := 0; i < t.NumField(); i++ {
f := t.Field(i)
if skipField(f) {
if !includeField(f) {
continue
}
v := v.Field(i)
Expand All @@ -45,7 +45,9 @@ func addFlags(log logger.Logger, flags FlagSet, o any) {
kind := v.Type().Elem().Kind()
if v.IsNil() && kind == reflect.Struct {
newV := reflect.New(v.Type().Elem())
v.Set(newV)
if v.CanSet() {
v.Set(newV)
}
}
} else {
v = v.Addr()
Expand Down
15 changes: 6 additions & 9 deletions load.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func configureViper(cfg Config, vpr *viper.Viper, v reflect.Value, flags flagRef
// for each field in the configuration struct, see if the field implements the defaultValueLoader interface and invoke it if it does
for i := 0; i < t.NumField(); i++ {
f := t.Field(i)
if skipField(f) {
if !includeField(f) {
continue
}

Expand Down Expand Up @@ -238,7 +238,7 @@ func postLoadStruct(v reflect.Value) error {

for i := 0; i < v.NumField(); i++ {
f := t.Field(i)
if skipField(f) {
if !includeField(f) {
continue
}

Expand Down Expand Up @@ -397,11 +397,8 @@ func isNotFoundErr(err error) bool {
return err != nil && errors.As(err, &notFound)
}

// skipField determines whether to skip a field
// when using reflection to process the application's config
// structure. Skip fields that are unexported, unless
// they are anonymous, since anonymous fields are embedded fields,
// which must be processed in case they contain exported members.
func skipField(f reflect.StructField) bool {
return !f.IsExported() && !f.Anonymous
// includeField determines whether to include or skip a field when processing the application's nested configuration load.
// fields that are processed include: public/exported fields, embedded structs (not pointer private/unexported embedding)
func includeField(f reflect.StructField) bool {
return (f.Anonymous && !isPtr(f.Type)) || f.IsExported()
}
144 changes: 73 additions & 71 deletions load_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -584,97 +584,99 @@ func Test_PostLoad(t *testing.T) {
require.Equal(t, "ptr-v", r.Ptr.Sv2)
}

func Test_EmbeddedStructInConfigMember(t *testing.T) {
type moduleConfig struct {
ModuleBool bool `yaml:"module-bool" mapstructure:"module-bool"`
}

type specialModuleConfig struct {
moduleConfig `yaml:",inline" mapstructure:",squash"`
SpecialModuleBool bool `yaml:"special-module-bool" mapstructure:"special-module-bool"`
}
type Public struct {
Value bool `json:"value" yaml:"value" mapstructure:"value"`
}

type TopLevelConfig struct {
Module1 moduleConfig `yaml:"module-1" mapstructure:"module-1"`
Module2 specialModuleConfig `yaml:"module-2" mapstructure:"module-2"`
}
type private struct {
Value bool `json:"value" yaml:"value" mapstructure:"value"`
}

cfgPtr := &TopLevelConfig{}
func Test_EmbeddedPublicStruct(t *testing.T) {
val := &struct {
Public `yaml:",inline" mapstructure:",squash"`
PublicField struct {
Public `yaml:",inline" mapstructure:",squash"`
} `yaml:"field" mapstructure:"field"`
}{}

cfg := NewConfig("my-app")
t.Setenv("MY_APP_MODULE_1_MODULE_BOOL", "true")
t.Setenv("MY_APP_MODULE_2_MODULE_BOOL", "true")
t.Setenv("MY_APP_MODULE_2_SPECIAL_MODULE_BOOL", "true")
cfg := NewConfig("app")
t.Setenv("APP_VALUE", "true")
t.Setenv("APP_FIELD_VALUE", "true")

cmd := &cobra.Command{}
err := Load(cfg, cmd, cfgPtr)
err := Load(cfg, cmd, val)

require.NoError(t, err)
require.True(t, cfgPtr.Module1.ModuleBool)
require.True(t, cfgPtr.Module2.ModuleBool)
require.True(t, cfgPtr.Module2.SpecialModuleBool)
require.NotNil(t, val.Public)
require.True(t, val.Public.Value)
require.NotNil(t, val.PublicField.Public)
require.True(t, val.PublicField.Public.Value)
}

func Test_EmbeddedStructPointerInConfigMember(t *testing.T) {
type ModuleConfig struct {
ModuleBool bool `yaml:"module-bool" mapstructure:"module-bool"`
}

// Note that, unlike Test_EmbeddedStructInConfigMember above,
// in this test, ModuleConfig is exported. This is a language
// limitation. See https://go-review.googlesource.com/c/go/+/53643
// and https://github.com/golang/go/issues/21357
type specialModuleConfig struct {
*ModuleConfig `yaml:",inline" mapstructure:",squash"`
SpecialModuleBool bool `yaml:"special-module-bool" mapstructure:"special-module-bool"`
}

type TopLevelConfig struct {
Module1 ModuleConfig `yaml:"module-1" mapstructure:"module-1"`
Module2 specialModuleConfig `yaml:"module-2" mapstructure:"module-2"`
}

cfgPtr := &TopLevelConfig{}
func Test_EmbeddedPublicStructPointer(t *testing.T) {
val := &struct {
*Public `yaml:",inline" mapstructure:",squash"`
PublicField struct {
*Public `yaml:",inline" mapstructure:",squash"`
} `yaml:"field" mapstructure:"field"`
}{}

cfg := NewConfig("my-app")
t.Setenv("MY_APP_MODULE_1_MODULE_BOOL", "true")
t.Setenv("MY_APP_MODULE_2_MODULE_BOOL", "true")
t.Setenv("MY_APP_MODULE_2_SPECIAL_MODULE_BOOL", "true")
cfg := NewConfig("app")
t.Setenv("APP_VALUE", "true")
t.Setenv("APP_FIELD_VALUE", "true")

cmd := &cobra.Command{}
err := Load(cfg, cmd, cfgPtr)
err := Load(cfg, cmd, val)

require.NoError(t, err)
require.True(t, cfgPtr.Module1.ModuleBool)
require.True(t, cfgPtr.Module2.ModuleBool)
require.True(t, cfgPtr.Module2.SpecialModuleBool)
require.NotNil(t, val.Public)
require.True(t, val.Public.Value)
require.NotNil(t, val.PublicField.Public)
require.True(t, val.PublicField.Value)
}

func Test_EmbeddedPrivateStructPointerInConfigMemberDoesNotPanic(t *testing.T) {
// Embedded an unexported struct via a pointer cannot be set by reflection,
// see comment in test case above.
// Assert that in this case fangs does not panic, and bubbles up the error from mapstructure.
type moduleConfig struct {
ModuleBool bool `yaml:"module-bool" mapstructure:"module-bool"`
}
func Test_EmbeddedPrivateStruct(t *testing.T) {
val := &struct {
private `yaml:",inline" mapstructure:",squash"`
PublicField struct {
private `yaml:",inline" mapstructure:",squash"`
} `yaml:"field" mapstructure:"field"`
}{}

type specialModuleConfig struct {
*moduleConfig `yaml:",inline" mapstructure:",squash"`
SpecialModuleBool bool `yaml:"special-module-bool" mapstructure:"special-module-bool"`
}
cfg := NewConfig("app")
t.Setenv("APP_VALUE", "true")
t.Setenv("APP_FIELD_VALUE", "true")

type TopLevelConfig struct {
Module1 moduleConfig `yaml:"module-1" mapstructure:"module-1"`
Module2 specialModuleConfig `yaml:"module-2" mapstructure:"module-2"`
}
cmd := &cobra.Command{}
err := Load(cfg, cmd, val)

cfgPtr := &TopLevelConfig{}
require.NoError(t, err)
require.NotNil(t, val.private)
require.True(t, val.private.Value)
require.NotNil(t, val.PublicField.private)
require.True(t, val.PublicField.private.Value)
}

cfg := NewConfig("my-app")
t.Setenv("MY_APP_MODULE_1_MODULE_BOOL", "true")
t.Setenv("MY_APP_MODULE_2_MODULE_BOOL", "true")
t.Setenv("MY_APP_MODULE_2_SPECIAL_MODULE_BOOL", "true")
func Test_EmbeddedPrivateStructPointer(t *testing.T) {
// Note that, unlike Test_EmbeddedPublicStructPointer above,
// in this test, *private is not exported and cannot be set or addressed.
// This is a language limitation. See https://go-review.googlesource.com/c/go/+/53643
// and https://github.com/golang/go/issues/21357
val := &struct {
*private `yaml:",inline" mapstructure:",squash"`
PublicField struct {
*private `yaml:",inline" mapstructure:",squash"`
} `yaml:"field" mapstructure:"field"`
}{}

cfg := NewConfig("app")
t.Setenv("APP_VALUE", "true")
t.Setenv("APP_FIELD_VALUE", "true")

cmd := &cobra.Command{}
err := Load(cfg, cmd, cfgPtr)
err := Load(cfg, cmd, val)

// https://github.com/mitchellh/mapstructure/blob/bf980b35cac4dfd34e05254ee5aba086504c3f96/mapstructure.go#L1338
assert.ErrorContains(t, err, "unsupported type for squash")
}
Expand Down
4 changes: 2 additions & 2 deletions summarize.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func summarize(cfg Config, descriptions DescriptionProvider, s *section, value r

for i := 0; i < t.NumField(); i++ {
f := t.Field(i)
if skipField(f) {
if !includeField(f) {
continue
}

Expand Down Expand Up @@ -134,7 +134,7 @@ func printVal(cfg Config, value reflect.Value, indent string) string {
case isStruct(t):
for i := 0; i < t.NumField(); i++ {
f := t.Field(i)
if skipField(f) {
if !includeField(f) {
continue
}

Expand Down
Loading

0 comments on commit d96c8f3

Please sign in to comment.