ZTC-1545: Error during settings deserialization if YAML contains unused keys #49
Conversation
nmldiegues
requested review from
nox,
ipostelnik,
inikulin,
nmldiegues,
norwoodj and
fisherdarling
foundations-macros/src/settings.rs
Outdated
| assert_eq!(actual, expected); | ||
| } | ||
|
|
||
| #[cfg(feature = "settings-deny-unknown-fields")] | ||
| fn add_deny_unknown_fields_attr(mut code: String) -> String { |
There was a problem hiding this comment.
I would prefer it to be done via the macro rather than code string substitution
There was a problem hiding this comment.
Not necessary now that there's no feature flag being used 👍
foundations/Cargo.toml
Outdated
| @@ -55,6 +55,9 @@ settings = [ | |||
| "dep:indexmap", | |||
| ] | |||
|
|
|||
| # Adds #[serde(deny_unknown_fields)] to all settings containers using #[settings] | |||
| settings-deny-unknown-fields = ["foundations-macros?/settings-deny-unknown-fields"] | |||
There was a problem hiding this comment.
Let's make it a macro parameter instead, e.g. #[settings(deny_unknown_fields = false)]. With true being default in 4.0.0
There was a problem hiding this comment.
Done. My initial opposition to this approach was thinking that users of intermediate dependencies like Oxy wouldn't have a way to opt out for shared settings structs. But, in that case, that intermediate dependency can add its own feature flag and use it for the shared settings structs like so:
#[cfg_attr(
feature = "settings-deny-unknown-fields",
settings(deny_unknown_fields = false)
)]
#[cfg_attr(not(feature = "settings-deny-unknown-fields"), settings)]
foundations/src/settings/mod.rs
Outdated
| /// | ||
| /// By default, this macro will automatically annotate types with `#[serde(deny_unknown_fields)]`. | ||
| /// This means any keys in the config file that don't map directly to fields in the type will | ||
| /// lead to a panic on deserialization. |
There was a problem hiding this comment.
Not panic, but error. We should probably also explain motivation here: that it's usually desirable for services to use up to date configuration scheme and it's better for services to fail early on a bootstrap stage rather than run misconfigured.
There was a problem hiding this comment.
Good points, thank you.
…rnal structs (#85) Following up on #49 It's been realized that there is no existing way to opt out of `deny_unknown_fields` for the settings structs defined within foundations itself. This adds a way to do that - by settings `default-features = false` in `Cargo.toml` and not passing the new `settings_deny_unknown_fields_by_default` feature. This keeps it enabled by default, but gives users of foundations a way to disable this feature. When the feature is not passed, since it only controls the default structs that don't specify, it is still possible to opt specific settings structs into this check.
…rnal structs (cloudflare#85) Following up on cloudflare#49 It's been realized that there is no existing way to opt out of `deny_unknown_fields` for the settings structs defined within foundations itself. This adds a way to do that - by settings `default-features = false` in `Cargo.toml` and not passing the new `settings_deny_unknown_fields_by_default` feature. This keeps it enabled by default, but gives users of foundations a way to disable this feature. When the feature is not passed, since it only controls the default structs that don't specify, it is still possible to opt specific settings structs into this check.
The goal is to help users of foundations catch possible configuration mistakes which, depending on the context, could have serious consequences. For example, if a config field is renamed, but the name is not changed in the deserialized YAML file, foundations will use the defaults for the renamed config value, which may differ substantially from the previous value. With this change, if the feature is enabled, foundations will return an error on deserializing the YAML file since it contains an unused key.
This takes a different approach from #43. This PR results in an error instead of a logged warning, making it much harder to ignore.
This adds a new option to the
#[settings]macro,default_deny_unknown_fields, which defaults to true. When true, foundations will error on unknown fields. If set to false, it won't.