From f239510ba13e37db6a2702173b4d020ff4ca69e0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 10 Jan 2025 09:10:32 -0600 Subject: [PATCH 1/5] test(error): Organize tests --- tests/testsuite/errors.rs | 152 +++++++++++++++----------------------- 1 file changed, 60 insertions(+), 92 deletions(-) diff --git a/tests/testsuite/errors.rs b/tests/testsuite/errors.rs index 74edd41a..6cb2e01c 100644 --- a/tests/testsuite/errors.rs +++ b/tests/testsuite/errors.rs @@ -5,7 +5,7 @@ use config::{Config, ConfigError, File, FileFormat, Map, Value}; #[test] #[cfg(feature = "json")] -fn test_error_path_index_bounds() { +fn test_path_index_bounds() { let c = Config::builder() .add_source(File::from_str( r#" @@ -28,7 +28,7 @@ fn test_error_path_index_bounds() { #[test] #[cfg(feature = "json")] -fn test_error_path_index_negative_bounds() { +fn test_path_index_negative_bounds() { let c = Config::builder() .add_source(File::from_str( r#" @@ -51,7 +51,7 @@ fn test_error_path_index_negative_bounds() { #[test] #[cfg(feature = "json")] -fn test_error_parse() { +fn test_parse() { let res = Config::builder() .add_source(File::from_str( r#" @@ -72,50 +72,28 @@ fn test_error_parse() { #[test] #[cfg(feature = "json")] -fn test_error_type() { - let c = Config::builder() - .add_source(File::from_str( - r#" -{ - "boolean_s_parse": "fals" -} -"#, - FileFormat::Json, - )) +fn test_root_not_table() { + let e = Config::builder() + .add_source(File::from_str(r#"false"#, FileFormat::Json)) .build() - .unwrap(); - - let res = c.get::("boolean_s_parse"); - - assert!(res.is_err()); - assert_data_eq!( - res.unwrap_err().to_string(), - str![[r#"invalid type: string "fals", expected a boolean for key `boolean_s_parse`"#]] - ); + .unwrap_err(); + match e { + ConfigError::FileParse { cause, .. } => assert_eq!( + "invalid type: boolean `false`, expected a map", + format!("{cause}") + ), + _ => panic!("Wrong error: {:?}", e), + } } #[test] #[cfg(feature = "json")] -fn test_error_deser_whole() { - #[derive(Deserialize, Debug)] - struct Place { - #[allow(dead_code)] - name: usize, // is actually s string - } - - #[derive(Deserialize, Debug)] - struct Output { - #[allow(dead_code)] - place: Place, - } - +fn test_get_invalid_type() { let c = Config::builder() .add_source(File::from_str( r#" { - "place": { - "name": "Torre di Pisa" - } + "boolean_s_parse": "fals" } "#, FileFormat::Json, @@ -123,16 +101,18 @@ fn test_error_deser_whole() { .build() .unwrap(); - let res = c.try_deserialize::(); + let res = c.get::("boolean_s_parse"); + + assert!(res.is_err()); assert_data_eq!( res.unwrap_err().to_string(), - str![[r#"invalid type: string "Torre di Pisa", expected an integer for key `place.name`"#]] + str![[r#"invalid type: string "fals", expected a boolean for key `boolean_s_parse`"#]] ); } #[test] #[cfg(feature = "json")] -fn test_error_type_detached() { +fn test_get_bool_invalid_type() { let c = Config::builder() .add_source(File::from_str( r#" @@ -145,24 +125,23 @@ fn test_error_type_detached() { .build() .unwrap(); - let value = c.get::("boolean_s_parse").unwrap(); - let res = value.try_deserialize::(); + let res = c.get_bool("boolean_s_parse"); assert!(res.is_err()); assert_data_eq!( res.unwrap_err().to_string(), - str![[r#"invalid type: string "fals", expected a boolean"#]] + str![[r#"invalid type: string "fals", expected a boolean for key `boolean_s_parse`"#]] ); } #[test] #[cfg(feature = "json")] -fn test_error_type_get_bool() { +fn test_get_table_invalid_type() { let c = Config::builder() .add_source(File::from_str( r#" { - "boolean_s_parse": "fals" + "debug": true } "#, FileFormat::Json, @@ -170,18 +149,18 @@ fn test_error_type_get_bool() { .build() .unwrap(); - let res = c.get_bool("boolean_s_parse"); + let res = c.get_table("debug"); assert!(res.is_err()); assert_data_eq!( res.unwrap_err().to_string(), - str![[r#"invalid type: string "fals", expected a boolean for key `boolean_s_parse`"#]] + str!["invalid type: boolean `true`, expected a map for key `debug`"] ); } #[test] #[cfg(feature = "json")] -fn test_error_type_get_table() { +fn test_get_array_invalid_type() { let c = Config::builder() .add_source(File::from_str( r#" @@ -194,23 +173,23 @@ fn test_error_type_get_table() { .build() .unwrap(); - let res = c.get_table("debug"); + let res = c.get_array("debug"); assert!(res.is_err()); assert_data_eq!( res.unwrap_err().to_string(), - str!["invalid type: boolean `true`, expected a map for key `debug`"] + str!["invalid type: boolean `true`, expected an array for key `debug`"] ); } #[test] #[cfg(feature = "json")] -fn test_error_type_get_array() { +fn test_value_deserialize_invalid_type() { let c = Config::builder() .add_source(File::from_str( r#" { - "debug": true + "boolean_s_parse": "fals" } "#, FileFormat::Json, @@ -218,17 +197,18 @@ fn test_error_type_get_array() { .build() .unwrap(); - let res = c.get_array("debug"); + let value = c.get::("boolean_s_parse").unwrap(); + let res = value.try_deserialize::(); assert!(res.is_err()); assert_data_eq!( res.unwrap_err().to_string(), - str!["invalid type: boolean `true`, expected an array for key `debug`"] + str![[r#"invalid type: string "fals", expected a boolean"#]] ); } #[test] -fn test_error_enum_de() { +fn test_value_deserialize_enum() { #[derive(Debug, Deserialize, PartialEq, Eq)] enum Diode { Off, @@ -262,57 +242,45 @@ fn test_error_enum_de() { #[test] #[cfg(feature = "json")] -fn error_with_path() { - #[derive(Debug, Deserialize)] - struct Inner { +fn test_deserialize_invalid_type() { + #[derive(Deserialize, Debug)] + struct Place { #[allow(dead_code)] - test: i32, + name: usize, // is actually s string } - #[derive(Debug, Deserialize)] - struct Outer { + #[derive(Deserialize, Debug)] + struct Output { #[allow(dead_code)] - inner: Inner, + place: Place, } - const CFG: &str = r#" + + let c = Config::builder() + .add_source(File::from_str( + r#" { - "inner": { - "test": "ABC" + "place": { + "name": "Torre di Pisa" } } -"#; - - let e = Config::builder() - .add_source(File::from_str(CFG, FileFormat::Json)) +"#, + FileFormat::Json, + )) .build() - .unwrap() - .try_deserialize::() - .unwrap_err(); + .unwrap(); + let res = c.try_deserialize::(); + let e = res.unwrap_err(); + assert_data_eq!( + e.to_string(), + str![[r#"invalid type: string "Torre di Pisa", expected an integer for key `place.name`"#]] + ); if let ConfigError::Type { key: Some(path), .. } = e { - assert_eq!(path, "inner.test"); + assert_eq!(path, "place.name"); } else { panic!("Wrong error {:?}", e); } } - -#[test] -#[cfg(feature = "json")] -fn test_error_root_not_table() { - match Config::builder() - .add_source(File::from_str(r#"false"#, FileFormat::Json)) - .build() - { - Ok(_) => panic!("Should not merge if root is not a table"), - Err(e) => match e { - ConfigError::FileParse { cause, .. } => assert_eq!( - "invalid type: boolean `false`, expected a map", - format!("{cause}") - ), - _ => panic!("Wrong error: {:?}", e), - }, - } -} From 58522620d8766a0005a5e7e0c887c9770edeb4ed Mon Sep 17 00:00:00 2001 From: Tommaso Allevi Date: Thu, 9 Jan 2025 12:43:10 +0100 Subject: [PATCH 2/5] test: add test to show the error is without path info --- tests/testsuite/errors.rs | 60 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/testsuite/errors.rs b/tests/testsuite/errors.rs index 6cb2e01c..8647044d 100644 --- a/tests/testsuite/errors.rs +++ b/tests/testsuite/errors.rs @@ -110,6 +110,33 @@ fn test_get_invalid_type() { ); } +#[test] +#[cfg(feature = "json")] +fn test_get_missing_field() { + #[derive(Debug, Deserialize)] + struct InnerSettings { + #[allow(dead_code)] + value: u32, + #[allow(dead_code)] + value2: u32, + } + + let c = Config::builder() + .add_source(File::from_str( + r#" +{ + "inner": { "value": 42 } +} + "#, + FileFormat::Json, + )) + .build() + .unwrap(); + + let res = c.get::("inner"); + assert_data_eq!(res.unwrap_err().to_string(), str!["missing field `value2`"]); +} + #[test] #[cfg(feature = "json")] fn test_get_bool_invalid_type() { @@ -284,3 +311,36 @@ fn test_deserialize_invalid_type() { panic!("Wrong error {:?}", e); } } + +#[test] +#[cfg(feature = "json")] +fn test_deserialize_missing_field() { + #[derive(Debug, Deserialize)] + struct Settings { + #[allow(dead_code)] + inner: InnerSettings, + } + + #[derive(Debug, Deserialize)] + struct InnerSettings { + #[allow(dead_code)] + value: u32, + #[allow(dead_code)] + value2: u32, + } + + let c = Config::builder() + .add_source(File::from_str( + r#" +{ + "inner": { "value": 42 } +} + "#, + FileFormat::Json, + )) + .build() + .unwrap(); + + let res = c.try_deserialize::(); + assert_data_eq!(res.unwrap_err().to_string(), str!["missing field `value2`"]); +} From fd5514454f5ca20554df35e2a373a3bbfe1ffe7c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 10 Jan 2025 09:27:31 -0600 Subject: [PATCH 3/5] fix: Add key tracking for all error types --- src/error.rs | 39 +++++++++++++++++++++++++++++++++++++-- tests/testsuite/errors.rs | 10 ++++++++-- tests/testsuite/log.rs | 2 +- 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/error.rs b/src/error.rs index 2d738b94..831e52b9 100644 --- a/src/error.rs +++ b/src/error.rs @@ -78,6 +78,16 @@ pub enum ConfigError { key: Option, }, + /// Custom message + At { + /// Error being extended with a path + error: Box, + + /// The key in the configuration hash of this value (if available where the + /// error is generated). + key: Option, + }, + /// Custom message Message(String), @@ -130,7 +140,15 @@ impl ConfigError { key: Some(key.into()), }, - _ => self, + Self::At { error, .. } => Self::At { + error, + key: Some(key.into()), + }, + + other => Self::At { + error: Box::new(other), + key: Some(key.into()), + }, } } @@ -157,8 +175,15 @@ impl ConfigError { expected, key: Some(concat(key)), }, + Self::At { error, key } => Self::At { + error, + key: Some(concat(key)), + }, Self::NotFound(key) => Self::NotFound(concat(Some(key))), - _ => self, + other => Self::At { + error: Box::new(other), + key: Some(concat(None)), + }, } } @@ -217,6 +242,16 @@ impl fmt::Display for ConfigError { Ok(()) } + ConfigError::At { ref error, ref key } => { + write!(f, "{error}")?; + + if let Some(ref key) = *key { + write!(f, " for key `{key}`")?; + } + + Ok(()) + } + ConfigError::FileParse { ref cause, ref uri } => { write!(f, "{cause}")?; diff --git a/tests/testsuite/errors.rs b/tests/testsuite/errors.rs index 8647044d..3a41d572 100644 --- a/tests/testsuite/errors.rs +++ b/tests/testsuite/errors.rs @@ -134,7 +134,10 @@ fn test_get_missing_field() { .unwrap(); let res = c.get::("inner"); - assert_data_eq!(res.unwrap_err().to_string(), str!["missing field `value2`"]); + assert_data_eq!( + res.unwrap_err().to_string(), + str!["missing field `value2` for key `inner`"] + ); } #[test] @@ -342,5 +345,8 @@ fn test_deserialize_missing_field() { .unwrap(); let res = c.try_deserialize::(); - assert_data_eq!(res.unwrap_err().to_string(), str!["missing field `value2`"]); + assert_data_eq!( + res.unwrap_err().to_string(), + str!["missing field `value2` for key `inner`"] + ); } diff --git a/tests/testsuite/log.rs b/tests/testsuite/log.rs index 29bf7eca..d8cda030 100644 --- a/tests/testsuite/log.rs +++ b/tests/testsuite/log.rs @@ -53,6 +53,6 @@ fn test_load_level_lowercase() { assert!(s.is_err()); assert_data_eq!( s.unwrap_err().to_string(), - str!["enum Level does not have variant constructor error"] + str!["enum Level does not have variant constructor error for key `log`"] ); } From da066d2c16c3f51e77f12153ee227c1270415393 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 10 Jan 2025 09:36:33 -0600 Subject: [PATCH 4/5] test(error): Show lack of origin --- tests/testsuite/deserialize-invalid-type.json | 5 + .../testsuite/deserialize-missing-field.json | 3 + tests/testsuite/errors.rs | 121 ++++++++++++++++++ tests/testsuite/get-invalid-type.json | 3 + tests/testsuite/get-missing-field.json | 3 + 5 files changed, 135 insertions(+) create mode 100644 tests/testsuite/deserialize-invalid-type.json create mode 100644 tests/testsuite/deserialize-missing-field.json create mode 100644 tests/testsuite/get-invalid-type.json create mode 100644 tests/testsuite/get-missing-field.json diff --git a/tests/testsuite/deserialize-invalid-type.json b/tests/testsuite/deserialize-invalid-type.json new file mode 100644 index 00000000..6435ab7b --- /dev/null +++ b/tests/testsuite/deserialize-invalid-type.json @@ -0,0 +1,5 @@ +{ + "place": { + "name": "Torre di Pisa" + } +} diff --git a/tests/testsuite/deserialize-missing-field.json b/tests/testsuite/deserialize-missing-field.json new file mode 100644 index 00000000..31bc3579 --- /dev/null +++ b/tests/testsuite/deserialize-missing-field.json @@ -0,0 +1,3 @@ +{ + "inner": { "value": 42 } +} diff --git a/tests/testsuite/errors.rs b/tests/testsuite/errors.rs index 3a41d572..e53b8fa6 100644 --- a/tests/testsuite/errors.rs +++ b/tests/testsuite/errors.rs @@ -110,6 +110,28 @@ fn test_get_invalid_type() { ); } +#[test] +#[cfg(feature = "json")] +fn test_get_invalid_type_file() { + let c = Config::builder() + .add_source(File::new( + "tests/testsuite/get-invalid-type.json", + FileFormat::Json, + )) + .build() + .unwrap(); + + let res = c.get::("boolean_s_parse"); + + assert!(res.is_err()); + assert_data_eq!( + res.unwrap_err().to_string(), + str![[ + r#"invalid type: string "fals", expected a boolean for key `boolean_s_parse` in tests/testsuite/get-invalid-type.json"# + ]] + ); +} + #[test] #[cfg(feature = "json")] fn test_get_missing_field() { @@ -140,6 +162,32 @@ fn test_get_missing_field() { ); } +#[test] +#[cfg(feature = "json")] +fn test_get_missing_field_file() { + #[derive(Debug, Deserialize)] + struct InnerSettings { + #[allow(dead_code)] + value: u32, + #[allow(dead_code)] + value2: u32, + } + + let c = Config::builder() + .add_source(File::new( + "tests/testsuite/get-missing-field.json", + FileFormat::Json, + )) + .build() + .unwrap(); + + let res = c.get::("inner"); + assert_data_eq!( + res.unwrap_err().to_string(), + str!["missing field `value2` for key `inner`"] + ); +} + #[test] #[cfg(feature = "json")] fn test_get_bool_invalid_type() { @@ -315,6 +363,47 @@ fn test_deserialize_invalid_type() { } } +#[test] +#[cfg(feature = "json")] +fn test_deserialize_invalid_type_file() { + #[derive(Deserialize, Debug)] + struct Place { + #[allow(dead_code)] + name: usize, // is actually s string + } + + #[derive(Deserialize, Debug)] + struct Output { + #[allow(dead_code)] + place: Place, + } + + let c = Config::builder() + .add_source(File::new( + "tests/testsuite/deserialize-invalid-type.json", + FileFormat::Json, + )) + .build() + .unwrap(); + + let res = c.try_deserialize::(); + let e = res.unwrap_err(); + assert_data_eq!( + e.to_string(), + str![[ + r#"invalid type: string "Torre di Pisa", expected an integer for key `place.name` in tests/testsuite/deserialize-invalid-type.json"# + ]] + ); + if let ConfigError::Type { + key: Some(path), .. + } = e + { + assert_eq!(path, "place.name"); + } else { + panic!("Wrong error {:?}", e); + } +} + #[test] #[cfg(feature = "json")] fn test_deserialize_missing_field() { @@ -350,3 +439,35 @@ fn test_deserialize_missing_field() { str!["missing field `value2` for key `inner`"] ); } + +#[test] +#[cfg(feature = "json")] +fn test_deserialize_missing_field_file() { + #[derive(Debug, Deserialize)] + struct Settings { + #[allow(dead_code)] + inner: InnerSettings, + } + + #[derive(Debug, Deserialize)] + struct InnerSettings { + #[allow(dead_code)] + value: u32, + #[allow(dead_code)] + value2: u32, + } + + let c = Config::builder() + .add_source(File::new( + "tests/testsuite/deserialize-missing-field.json", + FileFormat::Json, + )) + .build() + .unwrap(); + + let res = c.try_deserialize::(); + assert_data_eq!( + res.unwrap_err().to_string(), + str!["missing field `value2` for key `inner`"] + ); +} diff --git a/tests/testsuite/get-invalid-type.json b/tests/testsuite/get-invalid-type.json new file mode 100644 index 00000000..e1799c27 --- /dev/null +++ b/tests/testsuite/get-invalid-type.json @@ -0,0 +1,3 @@ +{ + "boolean_s_parse": "fals" +} diff --git a/tests/testsuite/get-missing-field.json b/tests/testsuite/get-missing-field.json new file mode 100644 index 00000000..31bc3579 --- /dev/null +++ b/tests/testsuite/get-missing-field.json @@ -0,0 +1,3 @@ +{ + "inner": { "value": 42 } +} From 74966992844877ba3ae0640139071089ca576b46 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 10 Jan 2025 09:27:31 -0600 Subject: [PATCH 5/5] fix: Future-proof for eventual `origin` tracking It looks like we lose track of `origin` in a lot of cases when merging sources which makes actual `origin` tracking meaningless until that is fixed which looked non-trivial. Adding this field at least gives us more of an option to do it later. --- src/error.rs | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/error.rs b/src/error.rs index 831e52b9..c8c1435d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -83,6 +83,11 @@ pub enum ConfigError { /// Error being extended with a path error: Box, + /// The URI that references the source that the value came from. + /// Example: `/path/to/config.json` or `Environment` or `etcd://localhost` + // TODO: Why is this called Origin but FileParse has a uri field? + origin: Option, + /// The key in the configuration hash of this value (if available where the /// error is generated). key: Option, @@ -140,13 +145,15 @@ impl ConfigError { key: Some(key.into()), }, - Self::At { error, .. } => Self::At { + Self::At { origin, error, .. } => Self::At { error, + origin, key: Some(key.into()), }, other => Self::At { error: Box::new(other), + origin: None, key: Some(key.into()), }, } @@ -175,13 +182,15 @@ impl ConfigError { expected, key: Some(concat(key)), }, - Self::At { error, key } => Self::At { + Self::At { error, origin, key } => Self::At { error, + origin, key: Some(concat(key)), }, Self::NotFound(key) => Self::NotFound(concat(Some(key))), other => Self::At { error: Box::new(other), + origin: None, key: Some(concat(None)), }, } @@ -242,13 +251,21 @@ impl fmt::Display for ConfigError { Ok(()) } - ConfigError::At { ref error, ref key } => { + ConfigError::At { + ref error, + ref origin, + ref key, + } => { write!(f, "{error}")?; if let Some(ref key) = *key { write!(f, " for key `{key}`")?; } + if let Some(ref origin) = *origin { + write!(f, " in {origin}")?; + } + Ok(()) }