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

@@ -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

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

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

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=