Skip to content

Commit

Permalink
Treat deserialization errors from empty env vars as unset env var
Browse files Browse the repository at this point in the history
Fixes #35

I'm a bit worried that this might seem too much like magic, but I think
this is just the most useful behavior in all situations I can think of.
This covers not only Booleans, but also numbers and anything else for
which an empty string is not a valid value. It also leaves types with
empty strings as valid values untouched.

I've been thinking about how this behavior could confuse users. And I
could only think of two cases:

- A config value has a default value, and the user thinks that empty
  string is a valid value, when in fact it isn't. Then, setting `FOO=`
  will result in the default value, when the user expected an empty
  string. Here, a clear error "empty string invalid" would be more
  helpful.

- A config value with a type that can be deserialized from empty string,
  but the user thinks it cannot. Maybe the config value is a list of
  numbers, where  "" is just an empty list, but the user thought it was
  a single number. Then setting `FOO=` means setting the value to an
  empty list.

Not sure if any of these cases will happen often enough or is important
enough to not merge this?
  • Loading branch information
LukasKalbertodt committed Oct 11, 2024
1 parent 9de8d67 commit cd6b25f
Showing 1 changed file with 9 additions and 4 deletions.
13 changes: 9 additions & 4 deletions src/internal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,18 @@ pub fn from_env_with_parser<T, E: std::error::Error + Send + Sync + 'static>(
parse: fn(&str) -> Result<T, E>,
) -> Result<Option<T>, Error> {
let v = get_env_var!(key, field);
parse(&v)
.map(Some)
.map_err(|err| {
let is_empty = v.is_empty();
match parse(&v) {
Ok(v) => Ok(Some(v)),
Err(_) if is_empty => Ok(None),
Err(err) => Err(
ErrorInner::EnvParseError {
field: field.to_owned(),
key: key.to_owned(),
err: Box::new(err),
}.into()
})
),
}
}

pub fn from_env_with_deserializer<T>(
Expand All @@ -83,9 +86,11 @@ pub fn from_env_with_deserializer<T>(
deserialize: fn(crate::env::Deserializer) -> Result<T, crate::env::DeError>,
) -> Result<Option<T>, Error> {
let s = get_env_var!(key, field);
let is_empty = s.is_empty();

match deserialize(crate::env::Deserializer::new(s)) {
Ok(v) => Ok(Some(v)),
Err(_) if is_empty => Ok(None),
Err(e) => Err(ErrorInner::EnvDeserialization {
key: key.into(),
field: field.into(),
Expand Down

0 comments on commit cd6b25f

Please sign in to comment.