Use TryInto for more permissive deserialization for integers

* Attempt to convert between integer types using `TryInto`-based conversions rather than blanket failing for some source and destination types. * Use `into_uint` instead of `into_int` in `Value` Deserialize implementations for unsigned integer types. Previously, we were converting from signed types to unsigned types using `as`, which can lead to surprise integer values conversions (#93). Fixes #352 and #93 (cherry picked from commit 7db2e8b) Signed-off-by: Matthias Beyer <[email protected]>
rust-cli · Aug 2, 2022 · 0132737 · 0132737
1 parent c375cad
commit 0132737
Show file tree

Hide file tree

Showing 4 changed files with 158 additions and 49 deletions.
diff --git a/src/de.rs b/src/de.rs
@@ -62,25 +62,25 @@ impl<'de> de::Deserializer<'de> for Value {
     #[inline]
     fn deserialize_u8<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
         // FIXME: This should *fail* if the value does not fit in the requets integer type
-        visitor.visit_u8(self.into_int()? as u8)
+        visitor.visit_u8(self.into_uint()? as u8)
     }
 
     #[inline]
     fn deserialize_u16<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
         // FIXME: This should *fail* if the value does not fit in the requets integer type
-        visitor.visit_u16(self.into_int()? as u16)
+        visitor.visit_u16(self.into_uint()? as u16)
     }
 
     #[inline]
     fn deserialize_u32<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
         // FIXME: This should *fail* if the value does not fit in the requets integer type
-        visitor.visit_u32(self.into_int()? as u32)
+        visitor.visit_u32(self.into_uint()? as u32)
     }
 
     #[inline]
     fn deserialize_u64<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
         // FIXME: This should *fail* if the value does not fit in the requets integer type
-        visitor.visit_u64(self.into_int()? as u64)
+        visitor.visit_u64(self.into_uint()? as u64)
     }
 
     #[inline]

diff --git a/src/value.rs b/src/value.rs
@@ -1,3 +1,4 @@
+use std::convert::TryInto;
 use std::fmt;
 use std::fmt::Display;
 
@@ -264,21 +265,27 @@ impl Value {
     pub fn into_int(self) -> Result<i64> {
         match self.kind {
             ValueKind::I64(value) => Ok(value),
-            ValueKind::I128(value) => Err(ConfigError::invalid_type(
-                self.origin,
-                Unexpected::I128(value),
-                "an signed 64 bit or less integer",
-            )),
-            ValueKind::U64(value) => Err(ConfigError::invalid_type(
-                self.origin,
-                Unexpected::U64(value),
-                "an signed 64 bit or less integer",
-            )),
-            ValueKind::U128(value) => Err(ConfigError::invalid_type(
-                self.origin,
-                Unexpected::U128(value),
-                "an signed 64 bit or less integer",
-            )),
+            ValueKind::I128(value) => value.try_into().map_err(|_| {
+                ConfigError::invalid_type(
+                    self.origin,
+                    Unexpected::I128(value),
+                    "an signed 64 bit or less integer",
+                )
+            }),
+            ValueKind::U64(value) => value.try_into().map_err(|_| {
+                ConfigError::invalid_type(
+                    self.origin,
+                    Unexpected::U64(value),
+                    "an signed 64 bit or less integer",
+                )
+            }),
+            ValueKind::U128(value) => value.try_into().map_err(|_| {
+                ConfigError::invalid_type(
+                    self.origin,
+                    Unexpected::U128(value),
+                    "an signed 64 bit or less integer",
+                )
+            }),
 
             ValueKind::String(ref s) => {
                 match s.to_lowercase().as_ref() {
@@ -325,11 +332,13 @@ impl Value {
             ValueKind::I64(value) => Ok(value.into()),
             ValueKind::I128(value) => Ok(value),
             ValueKind::U64(value) => Ok(value.into()),
-            ValueKind::U128(value) => Err(ConfigError::invalid_type(
-                self.origin,
-                Unexpected::U128(value),
-                "an signed 128 bit integer",
-            )),
+            ValueKind::U128(value) => value.try_into().map_err(|_| {
+                ConfigError::invalid_type(
+                    self.origin,
+                    Unexpected::U128(value),
+                    "an signed 128 bit integer",
+                )
+            }),
 
             ValueKind::String(ref s) => {
                 match s.to_lowercase().as_ref() {
@@ -375,21 +384,27 @@ impl Value {
     pub fn into_uint(self) -> Result<u64> {
         match self.kind {
             ValueKind::U64(value) => Ok(value),
-            ValueKind::U128(value) => Err(ConfigError::invalid_type(
-                self.origin,
-                Unexpected::U128(value),
-                "an unsigned 64 bit or less integer",
-            )),
-            ValueKind::I64(value) => Err(ConfigError::invalid_type(
-                self.origin,
-                Unexpected::I64(value),
-                "an unsigned 64 bit or less integer",
-            )),
-            ValueKind::I128(value) => Err(ConfigError::invalid_type(
-                self.origin,
-                Unexpected::I128(value),
-                "an unsigned 64 bit or less integer",
-            )),
+            ValueKind::U128(value) => value.try_into().map_err(|_| {
+                ConfigError::invalid_type(
+                    self.origin,
+                    Unexpected::U128(value),
+                    "an unsigned 64 bit or less integer",
+                )
+            }),
+            ValueKind::I64(value) => value.try_into().map_err(|_| {
+                ConfigError::invalid_type(
+                    self.origin,
+                    Unexpected::I64(value),
+                    "an unsigned 64 bit or less integer",
+                )
+            }),
+            ValueKind::I128(value) => value.try_into().map_err(|_| {
+                ConfigError::invalid_type(
+                    self.origin,
+                    Unexpected::I128(value),
+                    "an unsigned 64 bit or less integer",
+                )
+            }),
 
             ValueKind::String(ref s) => {
                 match s.to_lowercase().as_ref() {
@@ -435,16 +450,20 @@ impl Value {
         match self.kind {
             ValueKind::U64(value) => Ok(value.into()),
             ValueKind::U128(value) => Ok(value),
-            ValueKind::I64(value) => Err(ConfigError::invalid_type(
-                self.origin,
-                Unexpected::I64(value),
-                "an unsigned 128 bit or less integer",
-            )),
-            ValueKind::I128(value) => Err(ConfigError::invalid_type(
-                self.origin,
-                Unexpected::I128(value),
-                "an unsigned 128 bit or less integer",
-            )),
+            ValueKind::I64(value) => value.try_into().map_err(|_| {
+                ConfigError::invalid_type(
+                    self.origin,
+                    Unexpected::I64(value),
+                    "an unsigned 128 bit or less integer",
+                )
+            }),
+            ValueKind::I128(value) => value.try_into().map_err(|_| {
+                ConfigError::invalid_type(
+                    self.origin,
+                    Unexpected::I128(value),
+                    "an unsigned 128 bit or less integer",
+                )
+            }),
 
             ValueKind::String(ref s) => {
                 match s.to_lowercase().as_ref() {

diff --git a/tests/env.rs b/tests/env.rs
@@ -151,6 +151,39 @@ fn test_parse_int() {
     env::remove_var("INT_VAL");
 }
 
+#[test]
+fn test_parse_uint() {
+    // using a struct in an enum here to make serde use `deserialize_any`
+    #[derive(Deserialize, Debug)]
+    #[serde(tag = "tag")]
+    enum TestUintEnum {
+        Uint(TestUint),
+    }
+
+    #[derive(Deserialize, Debug)]
+    struct TestUint {
+        int_val: u32,
+    }
+
+    temp_env::with_var("INT_VAL", Some("42"), || {
+        let environment = Environment::default().try_parsing(true);
+
+        let config = Config::builder()
+            .set_default("tag", "Uint")
+            .unwrap()
+            .add_source(environment)
+            .build()
+            .unwrap();
+
+        let config: TestUintEnum = config.try_deserialize().unwrap();
+
+        assert!(matches!(
+            config,
+            TestUintEnum::Uint(TestUint { int_val: 42 })
+        ));
+    })
+}
+
 #[test]
 fn test_parse_float() {
     // using a struct in an enum here to make serde use `deserialize_any`
@@ -572,3 +605,43 @@ fn test_parse_off_string() {
 
     env::remove_var("STRING_VAL_1");
 }
+
+#[test]
+fn test_parse_int_default() {
+    #[derive(Deserialize, Debug)]
+    struct TestInt {
+        int_val: i32,
+    }
+
+    let environment = Environment::default().try_parsing(true);
+
+    let config = Config::builder()
+        .set_default("int_val", 42_i32)
+        .unwrap()
+        .add_source(environment)
+        .build()
+        .unwrap();
+
+    let config: TestInt = config.try_deserialize().unwrap();
+    assert_eq!(config.int_val, 42);
+}
+
+#[test]
+fn test_parse_uint_default() {
+    #[derive(Deserialize, Debug)]
+    struct TestUint {
+        int_val: u32,
+    }
+
+    let environment = Environment::default().try_parsing(true);
+
+    let config = Config::builder()
+        .set_default("int_val", 42_u32)
+        .unwrap()
+        .add_source(environment)
+        .build()
+        .unwrap();
+
+    let config: TestUint = config.try_deserialize().unwrap();
+    assert_eq!(config.int_val, 42);
+}
diff --git a/tests/integer_range.rs b/tests/integer_range.rs
@@ -33,3 +33,20 @@ fn nonwrapping_u32() {
     let port: u32 = c.get("settings.port").unwrap();
     assert_eq!(port, 66000);
 }
+
+#[test]
+#[should_panic]
+fn invalid_signedness() {
+    let c = Config::builder()
+        .add_source(config::File::from_str(
+            r#"
+            [settings]
+            port = -1
+            "#,
+            config::FileFormat::Toml,
+        ))
+        .build()
+        .unwrap();
+
+    let _: u32 = c.get("settings.port").unwrap();
+}