From 1a03b30a6252c74d47291a03cafd215951279f76 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Wed, 13 Jul 2022 22:13:22 -0400 Subject: [PATCH] Only overwrite values when environment variables are set (#62) The previous implementation of `overwrite` would always overwrite values in the given struct, even if values existed. While this is the definition of `overwrite`, it unintentionally extended to `default` values as well. So even if a value was explicitly set on a struct, it would be overwritten with the "default" value set in envconfig. This was an unexpected behavior, since defaults should take the lowest precedence. The new implementation has the following behavior with `overwrite`: - If the struct field has the zero value and a default is set: - If no environment variable is specified, the struct field will be populated with the default value. - If an environment variable is specified, the struct field will be populate with the environment variable value. - If the struct field has a non-zero value and a default is set: - If no environment variable is specified, the struct field's existing value will be used (the default is ignored). - If an environment variable is specified, the struct field's existing value will be overwritten with the environment variable value. As part of this change, decoder interfaces are only processed when an environment (or a default) is present. --- envconfig.go | 64 ++++++++++++++-------- envconfig_test.go | 134 ++++++++++++++++++++++++++++++++++++++++++++-- go.mod | 2 +- go.sum | 6 +-- 4 files changed, 175 insertions(+), 31 deletions(-) diff --git a/envconfig.go b/envconfig.go index 3719648..145d285 100644 --- a/envconfig.go +++ b/envconfig.go @@ -330,18 +330,20 @@ func processWith(ctx context.Context, i interface{}, l Lookuper, parentNoInit bo // Lookup the value, ignoring an error if the key isn't defined. This is // required for nested structs that don't declare their own `env` keys, // but have internal fields with an `env` defined. - val, err := lookup(key, opts, l) + val, found, usedDefault, err := lookup(key, opts, l) if err != nil && !errors.Is(err, ErrMissingKey) { return fmt.Errorf("%s: %w", tf.Name, err) } - if ok, err := processAsDecoder(val, ef); ok { - if err != nil { - return err - } + if found || usedDefault { + if ok, err := processAsDecoder(val, ef); ok { + if err != nil { + return err + } - setNilStruct(ef) - continue + setNilStruct(ef) + continue + } } plu := l @@ -368,23 +370,34 @@ func processWith(ctx context.Context, i interface{}, l Lookuper, parentNoInit bo continue } - // The field already has a non-zero value and overwrite is false, do not overwrite. + // The field already has a non-zero value and overwrite is false, do not + // overwrite. if !ef.IsZero() && !opts.Overwrite { continue } - val, err := lookup(key, opts, l) + val, found, usedDefault, err := lookup(key, opts, l) if err != nil { return fmt.Errorf("%s: %w", tf.Name, err) } + // If the field already has a non-zero value and there was no value directly + // specified, do not overwrite the existing field. We only want to overwrite + // when the envvar was provided directly. + if !ef.IsZero() && !found { + continue + } + // Apply any mutators. Mutators are applied after the lookup, but before any - // type conversions. They always resolve to a string (or error) - for _, fn := range fns { - if fn != nil { - val, err = fn(ctx, key, val) - if err != nil { - return fmt.Errorf("%s: %w", tf.Name, err) + // type conversions. They always resolve to a string (or error), so we don't + // call mutators when the environment variable was not set. + if found || usedDefault { + for _, fn := range fns { + if fn != nil { + val, err = fn(ctx, key, val) + if err != nil { + return fmt.Errorf("%s: %w", tf.Name, err) + } } } } @@ -450,29 +463,32 @@ LOOP: return key, &opts, nil } -// lookup looks up the given key using the provided Lookuper and options. -func lookup(key string, opts *options, l Lookuper) (string, error) { +// lookup looks up the given key using the provided Lookuper and options. The +// first boolean parameter indicates whether the value was found in the +// lookuper. The second boolean parameter indicates whether the default value +// was used. +func lookup(key string, opts *options, l Lookuper) (string, bool, bool, error) { if key == "" { // The struct has something like `env:",required"`, which is likely a // mistake. We could try to infer the envvar from the field name, but that // feels too magical. - return "", ErrMissingKey + return "", false, false, ErrMissingKey } if opts.Required && opts.Default != "" { // Having a default value on a required value doesn't make sense. - return "", ErrRequiredAndDefault + return "", false, false, ErrRequiredAndDefault } // Lookup value. - val, ok := l.Lookup(key) - if !ok { + val, found := l.Lookup(key) + if !found { if opts.Required { if pl, ok := l.(*prefixLookuper); ok { key = pl.prefix + key } - return "", fmt.Errorf("%w: %s", ErrMissingRequired, key) + return "", false, false, fmt.Errorf("%w: %s", ErrMissingRequired, key) } if opts.Default != "" { @@ -485,10 +501,12 @@ func lookup(key string, opts *options, l Lookuper) (string, error) { } return "" }) + + return val, false, true, nil } } - return val, nil + return val, found, false, nil } // processAsDecoder processes the given value as a decoder or custom diff --git a/envconfig_test.go b/envconfig_test.go index 64fe875..d7e9cc6 100644 --- a/envconfig_test.go +++ b/envconfig_test.go @@ -876,6 +876,106 @@ func TestProcessWith(t *testing.T) { "FIELD": "foo", }), }, + { + name: "overwrite/does_not_overwrite_no_value", + input: &struct { + Field string `env:"FIELD, overwrite"` + }{ + Field: "inside", + }, + exp: &struct { + Field string `env:"FIELD, overwrite"` + }{ + Field: "inside", + }, + lookuper: MapLookuper(nil), + }, + { + name: "overwrite/env_overwrites_existing", + input: &struct { + Field string `env:"FIELD, overwrite"` + }{ + Field: "inside", + }, + exp: &struct { + Field string `env:"FIELD, overwrite"` + }{ + Field: "env", + }, + lookuper: MapLookuper(map[string]string{ + "FIELD": "env", + }), + }, + { + name: "overwrite/env_overwrites_empty", + input: &struct { + Field string `env:"FIELD, overwrite"` + }{}, + exp: &struct { + Field string `env:"FIELD, overwrite"` + }{ + Field: "env", + }, + lookuper: MapLookuper(map[string]string{ + "FIELD": "env", + }), + }, + { + name: "overwrite/default_does_not_overwrite_no_value", + input: &struct { + Field string `env:"FIELD, overwrite, default=default"` + }{ + Field: "inside", + }, + exp: &struct { + Field string `env:"FIELD, overwrite, default=default"` + }{ + Field: "inside", + }, + lookuper: MapLookuper(nil), + }, + { + name: "overwrite/default_env_overwrites_existing", + input: &struct { + Field string `env:"FIELD, overwrite, default=default"` + }{ + Field: "inside", + }, + exp: &struct { + Field string `env:"FIELD, overwrite, default=default"` + }{ + Field: "env", + }, + lookuper: MapLookuper(map[string]string{ + "FIELD": "env", + }), + }, + { + name: "overwrite/default_env_overwrites_empty", + input: &struct { + Field string `env:"FIELD, overwrite, default=default"` + }{}, + exp: &struct { + Field string `env:"FIELD, overwrite, default=default"` + }{ + Field: "env", + }, + lookuper: MapLookuper(map[string]string{ + "FIELD": "env", + }), + }, + { + name: "overwrite/default_uses_default_when_unspecified", + input: &struct { + Field string `env:"FIELD, overwrite, default=default"` + }{}, + exp: &struct { + Field string `env:"FIELD, overwrite, default=default"` + }{ + Field: "default", + }, + lookuper: MapLookuper(nil), + }, // Required { @@ -1276,16 +1376,44 @@ func TestProcessWith(t *testing.T) { input: &struct { field *CustomDecoderType `env:"FIELD"` }{}, - lookuper: MapLookuper(map[string]string{}), - err: ErrPrivateField, + lookuper: MapLookuper(map[string]string{ + "FIELD": "foo", + }), + err: ErrPrivateField, }, { name: "custom_decoder/error", input: &struct { Field CustomTypeError `env:"FIELD"` }{}, + lookuper: MapLookuper(map[string]string{ + "FIELD": "foo", + }), + errMsg: "broken", + }, + { + name: "custom_decoder/called_for_empty_string", + input: &struct { + Field CustomTypeError `env:"FIELD"` + }{}, + lookuper: MapLookuper(map[string]string{ + "FIELD": "", + }), + errMsg: "broken", + }, + { + name: "custom_decoder/not_called_on_unset_envvar", + input: &struct { + Field CustomTypeError `env:"FIELD"` + }{}, + exp: &struct { + Field CustomTypeError `env:"FIELD"` + }{ + Field: CustomTypeError{}, + }, lookuper: MapLookuper(map[string]string{}), - errMsg: "broken", + // Note: We explicitly want no error here. The custom marshaller should + // not have been called, since the environment variables was not defined. }, // Expand diff --git a/go.mod b/go.mod index b2ff131..d2c2d9c 100644 --- a/go.mod +++ b/go.mod @@ -2,4 +2,4 @@ module github.com/sethvargo/go-envconfig go 1.17 -require github.com/google/go-cmp v0.4.1 +require github.com/google/go-cmp v0.5.8 diff --git a/go.sum b/go.sum index 210ab77..e9b099c 100644 --- a/go.sum +++ b/go.sum @@ -1,4 +1,2 @@ -github.com/google/go-cmp v0.4.1 h1:/exdXoGamhu5ONeUJH0deniYLWYvQwW66yvlfiiKTu0= -github.com/google/go-cmp v0.4.1/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= -golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4= -golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +github.com/google/go-cmp v0.5.8 h1:e6P7q2lk1O+qJJb4BtCQXlK8vWEO8V1ZeuEdJNOqZyg= +github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=