baoyachi / duration-str

duration string parser write by Rust.Playground:
https://baoyachi.github.io/duration-str/
30 stars 9 forks source link

Optional fields fail to deserialize when missing #27

Closed mike-lloyd03 closed 6 months ago

mike-lloyd03 commented 6 months ago

When using deserialize_option_duration_time and deserializing a yaml file, if the field is missing from the yaml file, serde_yaml::from_reader will raise an error that the field is missing. This should not happen for fields of type Option<T>.

#[derive(Default, Deserialize)]
pub struct Config {
    pub default_shell: Option<ShellType>,
    #[serde(deserialize_with = "deserialize_option_duration_time")]
    pub session_lifetime: Option<time::Duration>,
}

impl Config {
    pub fn load(config_dir: &Path) -> Result<Self> {
        let config_file_path = config_dir.join("config.yaml");
        let config_file = match std::fs::File::open(config_file_path) {
            Ok(c) => c,
            Err(e) => {
                eprintln!("Failed to open config file: {e}");
                eprintln!("Using defaults");
                return Ok(Self::default());
            }
        };

        Ok(serde_yaml::from_reader(config_file)?)
    }
}

For these two tests, the first one passes and the second one fails:

    #[test]
    fn test_load_config() -> Result<()> {
        let test_dir = "testdata/test_config1";
        std::fs::create_dir_all(test_dir)?;

        let config_path = test_dir.to_string() + "/config.yaml";
        let mut file = File::create(config_path)?;
        file.write_all(b"default_shell: fish\nsession_lifetime: 6h")?;

        let config = Config::load(&Path::new(test_dir))?;

        assert_eq!(config.default_shell, Some(ShellType::Fish));
        assert_eq!(config.session_lifetime, Some(Duration::hours(6)));

        std::fs::remove_dir_all(test_dir)?;

        Ok(())
    }

    #[test]
    fn test_load_config_missing_field() -> Result<()> {
        let test_dir = "testdata/test_config2";
        std::fs::create_dir_all(test_dir)?;

        let config_path = test_dir.to_string() + "/config.yaml";
        let mut file = File::create(config_path)?;
        file.write_all(b"default_shell: fish")?;

        let config = Config::load(&Path::new(test_dir))?;

        assert_eq!(config.default_shell, Some(ShellType::Fish));
        assert_eq!(config.session_lifetime, None);

        std::fs::remove_dir_all(test_dir)?;

        Ok(())
    }
failures:

---- config::session_tests::test_load_config_missing_field stdout ----
Error: missing field `session_lifetime`

Furthermore, if I remove deserialize_option_duration_time and change session_lifetime to Option<String>, the test does not raise an error for a missing field.

baoyachi commented 6 months ago

@mike-lloyd03 you need add default keyword in serde marco.

See example: https://github.com/baoyachi/duration-str/blob/3c8d5eed4647ad991f847f5d1b4a597ddba346f6/src/lib.rs#L975-L1000.

#[derive(Default, Deserialize)]
pub struct Config {
    pub default_shell: Option<ShellType>,
+    #[serde(default,deserialize_with = "deserialize_option_duration_time")]
    pub session_lifetime: Option<time::Duration>,
}
mike-lloyd03 commented 6 months ago

Ahh that'll do it. Thanks for the quick response.