dtolnay / serde-yaml

Strongly typed YAML library for Rust
Apache License 2.0
964 stars 164 forks source link

Deserializing to `Value` silently ignores duplicate keys #299

Closed stevenengler closed 2 years ago

stevenengler commented 2 years ago

The YAML spec disallows duplicate keys:

The content of a mapping node is an unordered set of key: value node pairs, with the restriction that each of the keys is unique.

When deserializing to a custom serde-derive struct we can use #[serde(with = "serde_with::rust::maps_duplicate_key_is_error")] to prevent duplicate keys. But if we want to deserialize to a serde_yaml::Value, serde-yaml's mappings will ignore duplicate keys. Is there any way to achieve the same behaviour when deserializing to a serde_yaml::Value?

The reason for wanting to deserialize to a serde_yaml::Value is that I would like to take advantage of the new apply_merge() feature. So what I'm doing is:

let mut config: serde_yaml::Value = serde_yaml::from_reader(file).unwrap();
config.apply_merge().unwrap();
let config: MyConfig = serde_yaml::from_value(config).unwrap();
dtolnay commented 2 years ago

Fixed in 0.9.4. I didn't realize this was specified as disallowed by the YAML spec.

stevenengler commented 2 years ago

Wow that was quick, thanks!

NigelThorpe commented 2 years ago

Except that the majority of YAML parsers out there allow duplicate keys, and take a "last key wins" philosophy. See e.g. https://github.com/chyh1990/yaml-rust/issues/19

I've got some things which were considered valid before but now aren't. I'd argue this was a breaking change, despite the YAML spec.

conradludgate commented 2 years ago

Being devils advocate, semver is based around documented behaviour. Since it's documented in the YAML spec as invalid behaviour, and since serde-yaml doesn't document otherwise, this is a valid bug fix.

Obligatory XKCD: XKCD on breaking changes obviously this is hyperbole, but no matter where you draw the line someone will find an issue with the change.

That being said, it did cause our codebase to break 😅