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(deny_unknown_fields)] does not work as expected on unit variants of tagged enum #2294

Open
Skgland opened this issue Oct 8, 2022 · 3 comments

Comments

@Skgland
Copy link

Skgland commented Oct 8, 2022

I would expect deny_unknown_fields to work with unit variants of tagged enums such that in the following example, both test pass.

Instead the test using the NewEnumUsingUnit enum fails as the token field is ignored unexpectedly.

Edit: Updated Example after #2294 (comment)

#[derive(serde::Serialize, serde::Deserialize, Debug)]
#[serde(tag = "type", deny_unknown_fields)]
enum OldEnum {
    A { token: String },
    B,
}

#[derive(serde::Serialize, serde::Deserialize, Debug)]
#[serde(tag = "type", deny_unknown_fields)]
enum NewEnumUsingUnit {
    A,
    B,
}

#[derive(serde::Serialize, serde::Deserialize, Debug)]
#[serde(tag = "type", deny_unknown_fields)]
enum NewEnumUsingEmptyStruct {
    A {},
    B,
}

#[test]
fn test_enum_migration_with_unit_variant() {
    let old = OldEnum::A {
        token: String::from("testToken"),
    };
    let serialized = toml::ser::to_string_pretty(&old).unwrap();
    println!("{}", serialized);
    toml::de::from_str::<'_, NewEnumUsingUnit>(&serialized)
        .expect_err("old has a field (token) new does not know and should deny");
}

#[test]
fn test_enum_migration_with_struct_variant() {
    let old = OldEnum::A {
        token: String::from("testToken"),
    };
    let serialized = toml::ser::to_string_pretty(&old).unwrap();
    println!("{}", serialized);
    toml::de::from_str::<'_, NewEnumUsingEmptyStruct>(&serialized)
        .expect_err("old has a field (token) new does not know and should deny");
}

play-ground

@kangalio
Copy link

Smaller repro:

#[derive(serde::Deserialize)]
#[serde(tag = "type", deny_unknown_fields)]
enum Enum {
    A,
    B,
}

fn main() {
    serde_json::from_str::<Enum>(r#"{"type": "A", "token": "testToken"}"#).unwrap();
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6ca35eee70212b03ba56a728757e1202

@uint
Copy link

uint commented Dec 23, 2022

I'm building a JSON Typedef derive lib and stumbled into this. Any updates? I should treat this as a bug and not expected behavior, right?

@anp
Copy link

anp commented Jan 27, 2023

I hit this as well when changing a variant from no payload to an empty struct payload (for reasons), here's an interesting repro:

#[derive(Debug, serde::Deserialize)]
#[serde(tag = "type", deny_unknown_fields)]
enum EnumBareVariant {
    A,
}

#[derive(Debug, serde::Deserialize)]
#[serde(tag = "type", deny_unknown_fields)]
enum EnumEmptyStructVariant {
    A {},
}


fn main() {
    let with_unknown_field = r#"{"type": "A", "token": "testToken"}"#;
    serde_json::from_str::<EnumEmptyStructVariant>(with_unknown_field).expect_err("this passes");
    serde_json::from_str::<EnumBareVariant>(with_unknown_field).expect_err("this fails");
}

It seems that this gap is specific to having a bare enum variant.

@Skgland Skgland changed the title '#![serde(deny_unknown_fields)]' does not work as expected with tagged enum #![serde(deny_unknown_fields)] does not work as expected with tagged enum Jan 28, 2023
@Skgland Skgland changed the title #![serde(deny_unknown_fields)] does not work as expected with tagged enum #![serde(deny_unknown_fields)] does not work as expected on unit variants of tagged enum Jan 28, 2023
gnoliyil pushed a commit to gnoliyil/fuchsia that referenced this issue Jan 27, 2024
serde doesn't correctly enforce deny_unknown_fields when
deserializing a bare enum variant:
serde-rs/serde#2294.

Icfa57c7b2d9a9c02f7586f7344d550076bb534bd makes these struct
variants which causes the driver-manager config attribute we don't
yet support to fail to build. It might take time for that CL to land,
so we workaround the bug with this change in the interim.

Bug: 99074
Change-Id: Ia7487aa5b68e6a40540b3000f17a7c3a1710c3c3
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/794100
Fuchsia-Auto-Submit: Adam Perry <adamperry@google.com>
Reviewed-by: Sarah Chan <spqchan@google.com>
Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com>
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

4 participants