-
Notifications
You must be signed in to change notification settings - Fork 9
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
base: master
Are you sure you want to change the base?
Conversation
Thank you for your contribution! |
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 valuesTo 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 ozzo-validation approachtype 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 approachtype 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 messagesSince ozzo-validation does not have access to the internal name generation functions of go-env (or ozzo-validation approachtype 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 approachtype 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 ThoughtsCertainly, adding this specific validation to go-env is not the only solution. Other potential solutions:
However, these other solutions are more complex. What do you think is the best approach? |
Thank you for doing the detailed comparisons!
|
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.
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.
I don’t think the question should be whether go-env (or 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. |
(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 hypotheticalPort
field to80
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.