Skip to content

Commit

Permalink
Only overwrite values when environment variables are set (#62)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@sethvargo
sethvargo committed Jul 14, 2022
1 parent 39c9bc7 commit 1a03b30
Show file tree
Hide file tree
Showing 4 changed files with 175 additions and 31 deletions.
64 changes: 41 additions & 23 deletions envconfig.go
View file
Expand Up @@ -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
Expand All @@ -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)
}
}
}
}
Expand Down Expand Up @@ -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 != "" {
Expand All @@ -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
Expand Down
134 changes: 131 additions & 3 deletions envconfig_test.go
View file
Expand Up @@ -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
{
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Expand Up @@ -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
6 changes: 2 additions & 4 deletions go.sum
View file
@@ -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=

0 comments on commit 1a03b30

Please sign in to comment.