serde icon indicating copy to clipboard operation
serde copied to clipboard

Serde behavior for some deserializers with Option and missing fields is counterintuitive

Open pcd1193182 opened this issue 2 years ago • 5 comments

Hey all, I ran across some behavior today I found puzzling. A missing field is silently deserialized as None in serde_json. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2cce1e00209ad22adfcd3488afe10e14 is a simple demo of the behavior, though it is probably already well known, given that issues like serde-rs/json#376 and #984 are based on it, and seem to silently assume it as a fact of life. My question is two fold:

  1. Why was this behavior selected? Why does Option get special treatment unlike the rest of the data model, where it is implicitly default when nothing else is, and there's no annotation to make it behave like everything else? This seems deeply counterintuitive with the design of the rest of the library. I'm sure there was a reason, but I haven't been able to find it documented anywhere, and that would be helpful to me.
  2. Could this behavior be well documented somewhere, instead of being left as a lurking landmine? This page seems like a good candidate; just add an Option field to Request to show that it automatically becomes None if the field is omitted.

pcd1193182 avatar May 05 '22 23:05 pcd1193182

Hi! I just encountered this issue as well.

For my case have configuration files based on structs that are generated from a corresponding enum with a field for each variant. These structs act in a similar role as a match in rust code where we provide a value that corresponds to each variant. In this context, it is useful for the deserialization to fail if any entries are missing so they can be updated when new enum variants are added (similar to an exhausitve match).

In one case, we happened to be using an Option for these values and it went undiscovered for a while that this was allowing a few entries to be omitted.

So documenting this behavior would be useful.

Additionally, would it be possible to add a container level option to the derive macro to not call into https://github.com/serde-rs/serde/blob/6c098e497e9098f8fc09ebed5317a97637bf23a3/serde/src/private/de.rs#L20 and instead generate an error?

Imberflur avatar Jun 30 '22 22:06 Imberflur

  1. The behavior was selected because in my experience, this is overwhelmingly the behavior that people assume and desire for fields whose type is Option.

  2. In general I am skeptical that more documentation would be a net benefit. Like in Imberflur's case: there is basically no chance that the suggested documentation change in the suggested location would have led to a different outcome.

dtolnay avatar Jul 09 '23 07:07 dtolnay

  1. Okay, but in that case, why not have a mechanism to have the default behavior for every other data type? As it is, there is no simple way to get that functionality with the default Option type, requiring instead either a custom Option type or a custom Deserializer.
  2. It seems to me that documenting edge cases and exceptions in the model is strictly beneficial; why would it not be a net positive? Thus far at least two people have commented that they ran into this problem and wish it had been documented, so it is likely there are more out there.

pcd1193182 avatar Jul 09 '23 08:07 pcd1193182

  1. As a workaround, #[serde(deserialize_with = "Deserialize::deserialize")] should work to disable this behavior, I think.

  2. The more documentation that exists, the less of it people read. So of all the edge cases that are present in the docs today, people would read about less of those as more edge cases behavior is documented, not more. This is counterproductive against your goal of users being more informed about edge cases. The reason for the counterproductive effect is that edge case documentation is by definition relevant to few people. If most of the documentation today is relevant to most people, most people will read most of it. If it's expanded to also cover everything that is relevant to 2 out of every 10,000 users, then few would be inclined to read any part of it.

dtolnay avatar Jul 09 '23 08:07 dtolnay