json icon indicating copy to clipboard operation
json copied to clipboard

A flattened Option masks parse errors inside the Option

Open spease opened this issue 5 years ago • 5 comments

I’ve noticed that wrapping a field type in Option, particularly when the field is a struct that contains other fields, masks genuine parse errors. For instance, the entire struct gets marked as None rather than an error being reported.

Is there a way to mark a field as optional and ignore it only as long as the field is nonexistent or the value is null, but error out if a value is specified and it isn’t valid?

spease avatar Mar 29 '20 21:03 spease

Can you provide some runnable code that reproduces this?

dtolnay avatar Mar 29 '20 21:03 dtolnay

Yes, I believe this is the same issue I'm currently having in some more complex code:

use serde::Deserialize;

#[derive(Debug, Deserialize)]
struct A {
    c: u64,
}

#[derive(Debug, Deserialize)]
struct B {
    #[serde(flatten)]
    a: Option<A>,
}

#[tokio::main]
pub async fn main() {
    let s = serde_json::from_str::<B>("{\"c\": \"Hello\"}").unwrap();
    println!("{:?}", s.a);
    let s = serde_json::from_str::<B>("{\"c\": \"64\"}").unwrap();
    println!("{:?}", s.a);
    let s = serde_json::from_str::<B>("{\"c\": 64}").unwrap();
    println!("{:?}", s.a);
}

Expected output:

None
None
Some(A { c: 64 })

Playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=dbe5c4724844ad4a652ec334b7b4b3c7

What I'd prefer to have happen in this sort of situation is to have it raise an error that 'c' is the wrong type. Since the tag names match, this seems more likely to be an inadvertent error in the type specified (or, in more complex code, parsing the field value). That being said, I'm not sure how commonly people expect / rely on the opposite behavior so I acknowledge backwards compatibility is a concern.

In my case, I have an optional sub-struct sub-struct with several fields on it, some of which are optional, and some of the fields also include custom deserialize implementations. If the sub-struct key is present, then all the required fields should be present, though the optional fields can be absent. If any field name matches one in the sub-struct, but the field value is a different type or returns an error from a custom deserialize, then it's an error. Unfortunately, what's happening is that it's simply setting the entire sub-struct to None. This has caused a lot of confusion, because I'm transcoding between serialization formats and values have been disappearing without any errors being generated. The scale of records being processed (tens of thousands or more) makes it difficult to isolate which field is triggering the parse error.

I'm not sure how to work around this apart from implementing a custom deserialize for the sub-struct field (a). Maybe I just need to basically call A::deserialize? I haven't yet started digging in to see what's happening under the hood for serde.

spease avatar Mar 31 '20 00:03 spease

I've just hit this. If a maintainer agrees that parse errors should not be masked by Option, I'd be happy to try fixing it sometime.

davidhewitt avatar Jul 14 '21 11:07 davidhewitt

For me it makes sense to raise an error. The serialized B should have no c property, or the c property should be a number. If it has a c that is not a u64 number, it should raise an error.

It should also raise an error if c is null IMO, because c actually comes from A, and it's not optional there (but could be undefined, because a is optional in B). So {} and { c: 10 } are valid JSONs, but not { c: null } neither { c: '10' }. To accept { c: null } the struct A should be:

struct A {
    c: Option<u64>,
}

lucasbasquerotto avatar Feb 18 '22 18:02 lucasbasquerotto