From 035fd45318192fb2afb665553624d3ed0eeb0345 Mon Sep 17 00:00:00 2001
From: Lukas Kalbertodt <lukas.kalbertodt@gmail.com>
Date: Fri, 11 Oct 2024 11:21:11 +0200
Subject: [PATCH 1/2] Treat deserialization errors from empty env vars as unset
 env var

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?
---
 src/internal.rs | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/internal.rs b/src/internal.rs
index 3ad175f..dd7efc3 100644
--- a/src/internal.rs
+++ b/src/internal.rs
@@ -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>(
@@ -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(),

From 06db476d613299fed3f33dadb919e11031fff42f Mon Sep 17 00:00:00 2001
From: Lukas Kalbertodt <lukas.kalbertodt@gmail.com>
Date: Fri, 18 Oct 2024 14:18:41 +0200
Subject: [PATCH 2/2] Add docs and tests about "empty var" behavior

---
 src/lib.rs   |  3 +++
 tests/env.rs | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/src/lib.rs b/src/lib.rs
index bff0f51..abec6f3 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -337,6 +337,9 @@ pub use crate::{
 /// Assigns an environment variable to this field. In [`Partial::from_env`], the
 /// variable is checked and deserialized into the field if present.
 ///
+/// If the env var is set to an empty string and if the field fails to
+/// parse/deserialize from it, it is treated as unset.
+///
 /// ### `parse_env`
 ///
 /// ```ignore
diff --git a/tests/env.rs b/tests/env.rs
index 91a4ca9..9a5bced 100644
--- a/tests/env.rs
+++ b/tests/env.rs
@@ -1,5 +1,6 @@
 use serde::Deserialize;
-use confique::{Config};
+use confique::{Config, Partial};
+use pretty_assertions::assert_eq;
 
 #[derive(Debug, Deserialize)]
 enum Foo { A, B, C }
@@ -17,3 +18,47 @@ fn enum_env() {
     let conf = Conf::builder().env().load();
     assert!(matches!(conf, Ok(Conf { foo: Foo::B })));
 }
+
+fn my_parser(s: &str) -> Result<u32, impl std::error::Error> {
+    s.trim().parse()
+}
+
+#[test]
+fn empty_error_is_unset() {
+    #[derive(Config)]
+    #[config(partial_attr(derive(PartialEq, Debug)))]
+    #[allow(dead_code)]
+    struct Conf {
+        #[config(env = "EMPTY_ERROR_IS_UNSET_FOO")]
+        foo: u32,
+
+        #[config(env = "EMPTY_ERROR_IS_UNSET_BAR", parse_env = my_parser)]
+        bar: u32,
+
+        #[config(env = "EMPTY_ERROR_IS_UNSET_BAZ")]
+        baz: String,
+    }
+
+    type Partial = <Conf as Config>::Partial;
+
+    std::env::set_var("EMPTY_ERROR_IS_UNSET_FOO", "");
+    assert_eq!(Partial::from_env().unwrap(), Partial {
+        foo: None,
+        bar: None,
+        baz: None,
+    });
+
+    std::env::set_var("EMPTY_ERROR_IS_UNSET_BAR", "");
+    assert_eq!(Partial::from_env().unwrap(), Partial {
+        foo: None,
+        bar: None,
+        baz: None,
+    });
+
+    std::env::set_var("EMPTY_ERROR_IS_UNSET_BAZ", "");
+    assert_eq!(Partial::from_env().unwrap(), Partial {
+        foo: None,
+        bar: None,
+        baz: Some("".into()),
+    });
+}