Fix tests for handling env key overlaps / errors

Author: bearcage

src/cargo/util/config/de.rs

@@ -298,20 +298,11 @@ impl<'config> ConfigMapAccess<'config> {
         // If the caller is interested in a field which we can provide from
         // the environment, get it from there.
         for field in given_fields {
-            let is_prefix = given_fields
-                .iter()
-                .any(|f| f != field && f.starts_with(field));
             let mut field_key = de.key.clone();
             field_key.push(field);
             for env_key in de.config.env.keys() {
-                if is_prefix {
-                    if env_key == field_key.as_env_key() {
-                        fields.insert(KeyKind::Normal(field.to_string()));
-                    }
-                } else {
-                    if env_key.starts_with(field_key.as_env_key()) {
-                        fields.insert(KeyKind::Normal(field.to_string()));
-                    }
+                if env_key.starts_with(field_key.as_env_key()) {
+                    fields.insert(KeyKind::Normal(field.to_string()));
                 }
             }
         }

tests/testsuite/config.rs

@@ -1299,8 +1299,13 @@ fn overlapping_env_config() {
 }
 
 #[cargo_test]
-fn overlapping_env_with_defaults() {
+fn overlapping_env_with_defaults_errors_out() {
     // Issue where one key is a prefix of another.
+    // This is a limitation of mapping environment variables on to a hierarchy.
+    // Check that we error out when we hit ambiguity in this way, rather than
+    // the more-surprising defaulting through.
+    // If, in the future, we can handle this more correctly, feel free to delete
+    // this test.
     #[derive(Deserialize, Default)]
     #[serde(default, rename_all = "kebab-case")]
     struct Ambig {
@@ -1310,10 +1315,8 @@ fn overlapping_env_with_defaults() {
     let config = ConfigBuilder::new()
         .env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true")
         .build();
-
-    let s: Ambig = config.get("ambig").unwrap();
-    assert_eq!(s.debug_assertions, true);
-    assert_eq!(s.debug, u32::default());
+    let err = config.get::<Ambig>("ambig").err().unwrap();
+    assert!(format!("{}", err).contains("missing config key `ambig.debug`"));
 
     let config = ConfigBuilder::new().env("CARGO_AMBIG_DEBUG", "5").build();
     let s: Ambig = config.get("ambig").unwrap();
@@ -1340,7 +1343,12 @@ fn struct_with_overlapping_inner_struct_and_defaults() {
     }
 
     // Containing struct with a prefix of inner
-    // Check that the nested struct can have fields defined by env
+    //
+    // This is a limitation of mapping environment variables on to a hierarchy.
+    // Check that we error out when we hit ambiguity in this way, rather than
+    // the more-surprising defaulting through.
+    // If, in the future, we can handle this more correctly, feel free to delete
+    // this case.
     #[derive(Deserialize, Default)]
     #[serde(default)]
     struct PrefixContainer {
@@ -1350,27 +1358,38 @@ fn struct_with_overlapping_inner_struct_and_defaults() {
     let config = ConfigBuilder::new()
         .env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12")
         .build();
+    let err = config
+        .get::<PrefixContainer>("prefixcontainer")
+        .err()
+        .unwrap();
+    assert!(format!("{}", err).contains("missing config key `prefixcontainer.inn`"));
+    let config = ConfigBuilder::new()
+        .env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12")
+        .env("CARGO_PREFIXCONTAINER_INN", "true")
+        .build();
     let f: PrefixContainer = config.get("prefixcontainer").unwrap();
-    assert_eq!(f.inn, bool::default());
     assert_eq!(f.inner.value, 12);
+    assert_eq!(f.inn, true);
 
     // Containing struct where the inner value's field is a prefix of another
-    // Check that the nested struct can have fields defined by env
+    //
+    // This is a limitation of mapping environment variables on to a hierarchy.
+    // Check that we error out when we hit ambiguity in this way, rather than
+    // the more-surprising defaulting through.
+    // If, in the future, we can handle this more correctly, feel free to delete
+    // this case.
     #[derive(Deserialize, Default)]
     #[serde(default)]
     struct InversePrefixContainer {
         inner_field: bool,
         inner: Inner,
     }
     let config = ConfigBuilder::new()
-        .env("CARGO_INVERSEREFIXCONTAINER_INNER_VALUE", "12")
+        .env("CARGO_INVERSEPREFIXCONTAINER_INNER_VALUE", "12")
         .build();
     let f: InversePrefixContainer = config.get("inverseprefixcontainer").unwrap();
     assert_eq!(f.inner_field, bool::default());
-    // NB. This is a limitation of our env variable parsing. We can't currently
-    //     handle situations where just a value of the inner struct is set, but
-    //     it's also named as a prefix of another field on the outer struct.
-    // assert_eq!(f.inner.value, 12);
+    assert_eq!(f.inner.value, 12);
 }
 
 #[cargo_test]