Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#[serde(flatten)] error behavior is different to normal one #1957

Closed
edwin0cheng opened this issue Jan 20, 2021 · 5 comments
Closed

#[serde(flatten)] error behavior is different to normal one #1957

edwin0cheng opened this issue Jan 20, 2021 · 5 comments

Comments

@edwin0cheng
Copy link

edwin0cheng commented Jan 20, 2021

Example:

use serde::Deserialize;
#[derive(Deserialize, Debug)]
#[serde(deny_unknown_fields)]
struct B {
    s: String,
}
#[derive(Deserialize, Debug)]
#[serde(deny_unknown_fields)]
struct A {
    #[serde(flatten)]
    b: B
}
fn main() {
    let json = r#"{"non_exists" : "", "s": ""}"#;
    if let Err(err) = serde_json::from_str::<B>(json) {
        println!("B=> {:?}", err);
    };
    if let Err(err) = serde_json::from_str::<A>(json) {
        println!("A=> {:?}", err);
    };
}

Output:

B=> Error("unknown field `non_exists`, expected `s`", line: 1, column: 14)
A=> Error("unknown field `non_exists`", line: 1, column: 29)

I would expect both errors should be equal, but it is different. Maybe I miss something important here ?

[Edit] A better example and output

@jonasbb
Copy link
Contributor

jonasbb commented Jan 20, 2021

Is there any reason why you assume that the error message should be identical in both cases? Both seem appropriate and serde can only report one error at a time.

Also be careful with combining deny_unknown_field with flatten. They don't mix well.

@edwin0cheng
Copy link
Author

@jonasbb I updated the example and output , it should be better to reflect the problem.

My question is , why 'A' is missing the "expected s" part ?

@jonasbb
Copy link
Contributor

jonasbb commented Jan 20, 2021

@edwin0cheng Maybe check how the macros expand using cargo expand (also available in the playground). I would guess the message get's emitted by A, which has no expected fields, thus instead of showing an empty list that part is removed.

@edwin0cheng
Copy link
Author

Okay, I think the problem here is the error handling code is different between two structs.

A is using the following path, which didn't handle that expected part.

serde/serde_derive/src/de.rs

Lines 2519 to 2530 in b0c99ed

let collected_deny_unknown_fields = if cattrs.has_flatten() && cattrs.deny_unknown_fields() {
Some(quote! {
if let _serde::__private::Some(_serde::__private::Some((__key, _))) =
__collect.into_iter().filter(_serde::__private::Option::is_some).next()
{
if let _serde::__private::Some(__key) = __key.as_str() {
return _serde::__private::Err(
_serde::de::Error::custom(format_args!("unknown field `{}`", &__key)));
} else {
return _serde::__private::Err(
_serde::de::Error::custom(format_args!("unexpected map key")));
}

Maybe we could collect the __values in there to report the error as how 'B' one do. ?

@dtolnay
Copy link
Member

dtolnay commented Jul 9, 2023

I think this is all right as currently implemented.

If you do manage to come up with a PR that makes identical error messages in this situation, I would be interested to take a look.

@dtolnay dtolnay closed this as completed Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants