Proposal: Default values already set in the passed struct pointer should be respected

[Bios-Marcel]

Hey

Currently, doing something like this:

type config struct {
    Field string `env:"field,required,notEmpty"`
}

func New() *config {
    return &config{
        Field: FunctionThatComputesFieldDefault(),
    }
}

func (c *config) Load() error {
    // Error indicating that "Field" isn't set
    return Parse(c)
}

Will result in error, as config.Field could not be found while parsing the environment, even though it had been set way before that.

Therefore, I propose respecting the pre-existing struct values when checking for required and notEmpty.

This should not be a breaking change, as it basically introduces a new feature that wasn't possible before. The upside of this, as shown in the example, is that you can compute a default, instead of just passing literals, but even just passing literals makes the whole thing much more readable than having everything in some magic string.

We would be willing to be the ones to implement this, but we wanted to check whether you'd want to accept this change at all. Alternatively, we could also make this an option, such as RespectPreExistingStructValues or something like that.

The change would be something along the lines of this:

diff --git a/env.go b/env.go
index be8ee53..04a7a7e 100644
--- a/env.go
+++ b/env.go
@@ -198,6 +198,10 @@ func doParse(ref reflect.Value, opts Options) error {
 				agrErr.Errors = append(agrErr.Errors, err)
 			}
 		}
+
+		if err := validateField(refField, refTypeField); err != nil {
+			agrErr.Errors = append(agrErr.Errors, err)
+		}
 	}
 
 	if len(agrErr.Errors) == 0 {
@@ -207,6 +211,11 @@ func doParse(ref reflect.Value, opts Options) error {
 	return agrErr
 }
 
+func validateField(refField reflect.Value, refTypeField reflect.StructField) error {
+	// Check for emptiness and requiredness
+	return nil
+}
+
 func doParseField(refField reflect.Value, refTypeField reflect.StructField, opts Options) error {
 	if !refField.CanSet() {
 		return nil
@@ -282,7 +291,7 @@ func get(field reflect.StructField, opts Options) (val string, err error) {
 	prefix := opts.Prefix
 	key := prefix + ownKey
 	defaultValue, defExists := field.Tag.Lookup("envDefault")
-	val, exists, isDefault = getOr(key, defaultValue, defExists, opts.Environment)
+	val, exists, isDefault = getOr( /* Pass exsting field reference to get `isDefault`*/ key, defaultValue, defExists, opts.Environment)
 
 	if expand {
 		val = os.ExpandEnv(val)
@@ -292,14 +301,6 @@ func get(field reflect.StructField, opts Options) (val string, err error) {
 		defer os.Unsetenv(key)
 	}
 
-	if required && !exists && len(ownKey) > 0 {
-		return "", newEnvVarIsNotSet(key)
-	}
-
-	if notEmpty && val == "" {
-		return "", newEmptyEnvVarError(key)
-	}
-
 	if loadFile && val != "" {
 		filename := val
 		val, err = getFromFile(filename)

[caarlos0]

sounds good, feel free to PR it 🙏

[Bios-Marcel]

With or without the flag?

We'll be taking some time for this though, don't expect it in the next days :)

[caarlos0]

I think we can make it the default, and do a v10 :P

[Nikurasuu]

We just realised, that for certain types, we can't know whether the value has been explicitly specified. For example, for int 0 we couldn't tell if it was set explicitly or not. This means, that both required and notEmpty can't really be checked in these cases.

This would mean, that we could only avoid throwin an error, if the value was non empty. The solution of the library user would be, to "simply" not make such fields required or nonEmpty if those fields allow zero values, which are also the default.

For pointer types however, we could be a bit more concrete. If it is a pointer to a zero value, we can say it fullfils required, but not notEmpty. The same goes for arrays, slices and maps. Should we still proceed with this? It is a little inconsistent, but the type system doesn't hold more information, necessary for a more consistent solution.

So what we'd do instead, is to keep the current checks as they are, but extend them with edge cases where the errors are omitted, in case we can be sure a value has been set / set to someting non empty.

We would also clarify this behaviour in the readme and maybe provide an example or two. Either way, this still wouldn't change behaviour for people that still do things the v9 way.