Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require fields to be present if not tagged with optional #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fumoboy007
Copy link

@fumoboy007 fumoboy007 commented Oct 29, 2020

(Requires major version bump due to default behavior change.)

Currently, fields are optional by default, which means go-env will not return an error if an environment variable is missing. There is also no mechanism to specify that a field should be required. This functionality is important due to the classic Go problem of not being able to distinguish between a default value and a missing value.

The question of whether the default behavior should be optional or required is tricky. For server applications, I think most use cases would want environment variables to be required. For command-line applications, I think most use cases would want environment variables to be optional.

I think requiring a tag for optional makes more sense because it pushes the programmer to consider what the default value of the field should be if the environment variable is not set. (For example, a programmer may set a hypothetical Port field to 80 if the environment variable is not set.)

Therefore, I have changed the default behavior to be required and I have added a new optional option.

@qiangxue
Copy link
Owner

Thank you for your contribution!
The functionality you propose to add is better to be done by a validation library, such as https://github.com/go-ozzo/ozzo-validation
which not only can be used to make sure certain fields are required, but also to validate if they are of certain formats.

@fumoboy007
Copy link
Author

Ah, another great library. Thanks!

Although that library works, I don’t think it works as well as if the validation happened within go-env.

Example 1: Distinguishing between missing and default values

To distinguish between missing and default values using ozzo-validation, fields must be pointers. However, since exposing unnecessary pointers in the exported struct is not ideal, we would need boilerplate code to convert between an unexported, raw struct and the exported, clean struct.

If the validation is inside go-env, fields do not have to be pointers since go-env can tell whether an environment variable is missing by the return value of os.LookupEnv. Furthermore, doing the validation inside go-env allows the default behavior to be required, which I think makes more sense (as mentioned in my original comment).

ozzo-validation approach

type Config struct {
  Port int
}

type config struct {
  Port *int
}

func (c config) Validate() error {
  return validation.ValidateStruct(&c,
    validation.Field(&c.Port, validation.NotNil),
  )
}

func LoadConfig() Config {
  var rawConfig config
  if err := env.Load(&rawConfig); err != nil {
    // …
  }

  return Config{
    Port: *rawConfig.Port,
  }
}

go-env approach

type Config struct {
  Port int
}

func LoadConfig() Config {
  var config Config
  if err := env.Load(&config); err != nil {
    // …
  }

  return config
}

Example 2: Field names in error messages

Since ozzo-validation does not have access to the internal name generation functions of go-env (or encoding/json, etc.), the name that it uses in its error messages does not match the external name, which is not ideal. However, if the validation is inside go-env, the name in the error message can exactly match the external name: UPPER_SNAKE_CASE with the specified prefix.

ozzo-validation approach

type config struct {
  // Environment variable name: "APP_HOST_NAME"
  // ozzo-validation name in error: "HostName"
  HostName string

  // Environment variable name: "APP_NameWith,Comma"
  // ozzo-validation name in error: "NameWith"
  NameWithComma string `env:"NameWith,Comma,secret"`
}

go-env approach

type Config struct {
  // Environment variable name: "APP_HOST_NAME"
  // go-env name in error: "APP_HOST_NAME"
  HostName string

  // Environment variable name: "APP_NameWith,Comma"
  // go-env name in error: "APP_NameWith,Comma"
  NameWithComma string `env:"NameWith,Comma,secret"`
}

Further Thoughts

Certainly, adding this specific validation to go-env is not the only solution. Other potential solutions:

  • Integrate go-env with ozzo-validation somehow so that callers can pass validation rules directly to go-env. By doing so, go-env would be able to check for missing values without requiring pointers and it would be able to control the name used in the error message.
  • Create an interface in ozzo-validation that contains a method to generate a name from a field. go-env would implement this interface. (ozzo-validation could even implement the interface for Go’s built-in encoding/json.) That would solve the problem of the name in the error message not matching the external name.

However, these other solutions are more complex. What do you think is the best approach?

@qiangxue
Copy link
Owner

Thank you for doing the detailed comparisons!
Some clarifications:

  • struct fields don't need to be pointers in order to use the validation package.
  • the validation package allows you to specify the name used in error messages. By default, it's declared using json tag. However, this does require you to explicitly declare it because the default name generation used by the validation package is different from that of go-env. Like you suggested, there are multiple ways to do this, none of which is trivial. But please continue reading below.
  • usually go-env is used to populate a configuration struct, and the struct may also be populated from other sources, such as a YAML file. And the struct needs to be validated at the end. So it's better to keep the validation logic separately than within go-env.

@fumoboy007
Copy link
Author

  • struct fields don't need to be pointers in order to use the validation package.

Understood. However, they need to be pointers if you want to distinguish between a missing value and an empty value. If it’s not a pointer, you wouldn’t know whether the field was using the default value due to not being present or if it was explicitly set to the default value in the serialized object.

  • the validation package allows you to specify the name used in error messages. By default, it's declared using json tag. However, this does require you to explicitly declare it because the default name generation used by the validation package is different from that of go-env. Like you suggested, there are multiple ways to do this, none of which is trivial.

Right. Effectively, we always need to override the name used in the error messages, which is not ideal since we are effectively duplicating the name generation logic.

  • usually go-env is used to populate a configuration struct, and the struct may also be populated from other sources, such as a YAML file. And the struct needs to be validated at the end. So it's better to keep the validation logic separately than within go-env.

I don’t think the question should be whether go-env (or encoding/json, yaml.v2, etc.) should contain validation logic at all; they already do perform some validation in order to deserialize strings into int, time.Time, etc. I agree that these deserialization libraries should not contain all types of validation. However, I think the question should be: Should this specific validation (required values) be a part of the deserialization library or the general validation library?

I think the deserialization library should offer this validation for the reasons I mentioned: required can be made the default behavior, less boilerplate code to transform between pointers and non-pointers, and better error messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants