Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(config): unrecognized config properties don't cause config error #4547

Merged
merged 4 commits into from Nov 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 29 additions & 1 deletion src/config.rs
Expand Up @@ -48,7 +48,17 @@ impl<'a, T: Deserialize<'a> + Default> ModuleConfig<'a, ValueError> for T {
/// Create `ValueDeserializer` wrapper and use it to call `Deserialize::deserialize` on it.
fn from_config(config: &'a Value) -> Result<Self, ValueError> {
let deserializer = ValueDeserializer::new(config);
T::deserialize(deserializer)
T::deserialize(deserializer).or_else(|err| {
// If the error is an unrecognized key, print a warning and run
// deserialize ignoring that error. Otherwise, just return the error
if err.to_string().contains("Unknown key") {
log::warn!("{}", err);
let deserializer2 = ValueDeserializer::new(config).with_allow_unknown_keys();
T::deserialize(deserializer2)
Comment on lines +55 to +57
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you add or edit and existing test? Did it get lost during rebasing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just dug through my reflog to confirm, and it looks like I had only updated the test in serde_utils.rs. I thought the coverage reports showed this code branch was covered by that test change, but maybe I'm misremembering 🤔 I'll do some further testing/experimenting locally in the morning to see what's up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, would you mind adding a test that uses this path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Sorry for the crazy delay here- I caught the flu a couple weeks ago and it ended up wiping me out pretty bad. I'm slowly getting back in the saddle here though 😅 I just wrapped up work today and will get this last test added this evening

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to step away for a while while CI was running, but looks like that test has properly covered that block now 👍

} else {
Err(err)
}
})
}
}

Expand Down Expand Up @@ -582,6 +592,24 @@ mod tests {
assert_eq!(rust_config.switch_c, Switch::Off);
}

#[test]
fn test_load_unknown_key_config() {
#[derive(Clone, Default, Deserialize)]
#[serde(default)]
struct TestConfig<'a> {
pub foo: &'a str,
}

let config = toml::toml! {
foo = "test"
bar = "ignore me"
};
let rust_config = TestConfig::from_config(&config);

assert!(rust_config.is_ok());
assert_eq!(rust_config.unwrap().foo, "test");
}

#[test]
fn test_from_string() {
let config = Value::String(String::from("S"));
Expand Down
34 changes: 31 additions & 3 deletions src/serde_utils.rs
Expand Up @@ -13,6 +13,7 @@ pub struct ValueDeserializer<'de> {
value: &'de Value,
info: Option<StructInfo>,
current_key: Option<&'de str>,
error_on_ignored: bool,
}

/// When deserializing a struct, this struct stores information about the struct.
Expand All @@ -28,14 +29,28 @@ impl<'de> ValueDeserializer<'de> {
value,
info: None,
current_key: None,
error_on_ignored: true,
}
}

fn with_info(value: &'de Value, info: Option<StructInfo>, current_key: &'de str) -> Self {
fn with_info(
value: &'de Value,
info: Option<StructInfo>,
current_key: &'de str,
ignored: bool,
) -> Self {
ValueDeserializer {
value,
info,
current_key: Some(current_key),
error_on_ignored: ignored,
}
}

pub fn with_allow_unknown_keys(self) -> Self {
ValueDeserializer {
error_on_ignored: false,
..self
}
}
}
Expand Down Expand Up @@ -83,7 +98,12 @@ impl<'de> Deserializer<'de> for ValueDeserializer<'de> {
let map = MapDeserializer::new(t.iter().map(|(k, v)| {
(
k.as_str(),
ValueDeserializer::with_info(v, self.info, k.as_str()),
ValueDeserializer::with_info(
v,
self.info,
k.as_str(),
self.error_on_ignored,
),
)
}));
map.deserialize_map(visitor)
Expand Down Expand Up @@ -131,6 +151,10 @@ impl<'de> Deserializer<'de> for ValueDeserializer<'de> {
return visitor.visit_none();
}

if !self.error_on_ignored {
return visitor.visit_none();
}

let did_you_mean = match (self.current_key, self.info) {
(Some(key), Some(StructInfo { fields, .. })) => fields
.iter()
Expand Down Expand Up @@ -322,13 +346,17 @@ mod test {
let value = toml::toml! {
unknown_key = "foo"
};
let deserializer = ValueDeserializer::new(&value);

let deserializer = ValueDeserializer::new(&value);
let result = StarshipRootConfig::deserialize(deserializer).unwrap_err();
assert_eq!(
format!("{result}"),
"Error in 'StarshipRoot' at 'unknown_key': Unknown key"
);

let deserializer2 = ValueDeserializer::new(&value).with_allow_unknown_keys();
let result = StarshipRootConfig::deserialize(deserializer2);
assert!(result.is_ok());
}

#[test]
Expand Down