ZTC-1545: Log a warning when parsing settings if yaml contains unused keys #43
Conversation
| ignored_key_callback: Option<Box<dyn FnMut(IgnoredPath)>>, | ||
| } | ||
|
|
||
| impl SettingsDeserializationOptionsBuilder { |
There was a problem hiding this comment.
Created this in case we want to add any other settings in the future in a backwards-compatible manner.
If this is overkill, it could all be replaced by just making a method from_yaml_str_with_ignored_key_callback or the like. That would also let us take a concrete impl FnMut(IgnoredPath) instead of Boxing it.
| Box::new(|path| { | ||
| #[cfg(feature = "logging")] | ||
| if let IgnoredPath::Map { parent: _, key } = &path { | ||
| if !key.ends_with(YAML_KEY_ALLOW_UNUSED_SUFFIX) { |
There was a problem hiding this comment.
This suffix solution is rather clunky, but lets us avoid false positive in a very simple manner. The main alternative I'd prefer would be to automatically detect this scenario by inspecting the YAML, but I think that would be much more work and would likely have its own bugs. This should work well enough for now.
nmldiegues
requested review from
nox,
inikulin,
nmldiegues,
norwoodj and
Noah-Kennedy
| Box::new(|path| { | ||
| #[cfg(feature = "logging")] | ||
| if let IgnoredPath::Map { parent: _, key } = &path { | ||
| if !key.ends_with(YAML_KEY_ALLOW_UNUSED_SUFFIX) { |
| /// Any values left unspecified will use default values. | ||
| pub fn build(self) -> SettingsDeserializationOptions { | ||
| SettingsDeserializationOptions { | ||
| ignored_key_callback: self.ignored_key_callback.unwrap_or_else(|| { |
There was a problem hiding this comment.
This is changing the default behavior to log. Should the default be to do nothing, and let users opt-in?
There was a problem hiding this comment.
There are two contexts here - the context of just this builder and the whole context of the default behavior of the from_yaml_str method (which is eventually called by Cli::new). I'm not clear which you're referring to. Though maybe it doesn't matter, as I think the default should match in both cases.
I believe the chain through Cli::new is the main way this method is invoked - for example, that's how Oxy gets its settings initially. Both opt-in and opt-out are difficult in this context. I didn't see a clean way to add an option to that top-level method (and pass it down) without either a breaking API change or a non-breaking change by doing something similar to what I did with the builder + new method. A breaking API change for such a small bit of functionality didn't seem right to me, and I'm hesitant to add to the top-level API in Cli either.
As far as whether it should be the default - that's a philosophical choice about the design of foundations that I could go either way on. But I lean towards it being on by default since it could be helpful and makes foundations a bit more of a 'batteries included' kind of library. There is also currently the (clunky) way to opt-out using the YAML_KEY_ALLOW_UNUSED_SUFFIX in the yaml itself.
OmegaJak
force-pushed
the
jkruger/warn-unused-settings
branch
from
e7724bd
to
fe97f42
Compare
This is undesired behaviour. We don't want any implicit behavior or reporting unless settings for that are specified explicitly. This is one of the key principles of the library and we won't change it. If something in settings can cause an incident, then that should be a hard error (i.e. reported to sentry and terminate the process), rather than a "warning" in operational logs. |
|
Closing in favor of #49 following internal discussion |
…ed keys (#49) 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. If the opt-in foundations feature `settings-deny-unknown-fields` is enabled, all settings containers using the `#[settings]` macro will have the `#[serde(deny_unknown_fields)]` attribute set on them. --------- Co-authored-by: Jackson Kruger <[email protected]>
This uses serde_ignored while deserializing settings from YAML to log a warning if keys are unused. This could help prevent accidental misconfigurations of services. For example, if the setting is reverted in code without also changing the name in the YAML, a warning will be logged for that key.
There were three things complicating this implementation:
Discardlogger to one using the defaultLoggingSettings. This way, warnings can be logged to stdout at least, following the usual format, while deserializing settings.foundationsmay be serializing and re-deserializing their Settings with omitted fields to remove data from copies of the settings. In this case, we do not want warnings logged for those omitted fields. Thus, an additional method was added to deserialize from a yaml string with an option controlling what action is taken when an ignored field is encountered.