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

Added #[serde(case_insensitive)] container attribute for case-insensitive identifier deserialization #1902

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

lasa01
Copy link

@lasa01 lasa01 commented Oct 6, 2020

Added an attritube that enables case-insensitive container field/variant identifier deserialization.

A similar attribute been requested in #586, which was closed because case-insensitive parsing was provided by the serde-aux crate.
The case insensitive implementation there however seems to first parse the data into a map, convert all keys into lowercase and then deserialize the map into the target type.
This is definitely slower than having the code generated by derive compare the field names using case-insensitive comparison.
The serde_aux implementation also depends on serde_json::Value and might not work for other data formats.

This PR uses eq_ignore_ascii_case() for case-insensitive comparison, which limits it to ASCII characters. This should be sufficient for most use cases, and the standard library currently doesn't have a case insensitive comparsion function that supports any characters.

Also added tests test_case_insensitive_struct, test_case_insensitive_enum and test_case_insensitive_bytes.

Uses eq_ignore_ascii_case() for case-insensitive comparison.
Added tests test_case_insensitive_struct, test_case_insensitive_enum, test_case_insensitive_bytes.
@lasa01
Copy link
Author

lasa01 commented Oct 7, 2020

Found a small bug, deriving Deserialize on an empty struct with case_insensitive attribute causes the following error:

expected expression, found keyword `else`
expected expression

main.rs(5, 10): Error originated from macro here
proc-macro derive produced unparseable tokens

Updated tests.
Refactored generated code to use match for case insensitive comparison too.
@lasa01
Copy link
Author

lasa01 commented Oct 23, 2020

This doesn't work on flattened structs, need to investigate it further.

@ctaggart
Copy link

Does this allow enum variants to be case insensitive? Here, I want to be able to deserialize NetworkSourceDeleted or NETWORKSOURCEDELETED values as well.

    #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
    pub enum State {
        #[serde(rename = "provisioning")]
        Provisioning,
        #[serde(rename = "deprovisioning")]
        Deprovisioning,
        #[serde(rename = "succeeded")]
        Succeeded,
        #[serde(rename = "failed")]
        Failed,
        #[serde(rename = "networkSourceDeleted")]
        NetworkSourceDeleted,
    }

@lasa01
Copy link
Author

lasa01 commented Jan 13, 2021 via email

Both the parent struct and the flattened struct must have the case insensitive attribute.
@lasa01
Copy link
Author

lasa01 commented Jan 22, 2021

It works now on flattened structs. However, you need to add the #[serde(case_insensitive)] attribute on both the parent struct and the flattened struct. If the attribute is only on the parent struct, the parent struct's fields are case insensitive and the flattened struct's fields case sensitive. If the attribute is not on the parent struct, everything is case sensitive regardless of the flattened struct's attributes.

Also, there is quite a lot of duplicate code between CaseInsensitiveFlatStructAccess and FlatStructAccess and between CaseInsensitiveFlatMapDeserializer and FlatMapDeserializer, not sure if this can be improved.

Looks like the build also now fails on older Rust versions without the eq_ignore_ascii_case function. Previously the function was used only in the code the derive generated, and the derive already required Rust 1.31 or newer, so it wasn't a problem. Now it's also used in the case insensitive flat map deserializer which is part of the main crate even without derives enabled. Not sure how this should be solved.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the build also now fails on older Rust versions without the eq_ignore_ascii_case function. Previously the function was used only in the code the derive generated, and the derive already required Rust 1.31 or newer, so it wasn't a problem. Now it's also used in the case insensitive flat map deserializer which is part of the main crate even without derives enabled.

I landed #1966 to skip compiling the derive helpers on older builds. They should only be needed on 1.31+.

@sagudev
Copy link

sagudev commented Apr 4, 2021

Any progress?

Copy link

@ctaggart ctaggart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request is looking pretty polished to me. I just tested the latest commit 11ee221 in ctaggart/azure-sdk-for-rust#2 and it fixes the deserialization problems with the Azure Storage service. I'd love to see this merged and released.

@avnerbarr
Copy link

is this being released?

@lasa01 lasa01 requested a review from dtolnay May 5, 2021 15:33
@cataggar
Copy link

@dtolnay, merging this would really help Azure SDK for Rust. What can I do to help this along?

@avitex
Copy link

avitex commented Dec 27, 2021

@dtolnay If docs are holding this back, I can make a PR for documenting this in https://github.com/serde-rs/serde-rs.github.io/blob/master/_src/container-attrs.md

@avitex
Copy link

avitex commented Jan 23, 2022

Looking for comments on #2161 which is an alternative solution that is more generic over the problem

corneliusroemer added a commit to nextstrain/nextclade_data that referenced this pull request May 20, 2022
This PR could help here, unfortunately, seems to be stalling serde-rs/serde#1902
@albx79
Copy link

albx79 commented Oct 4, 2022

I came here looking for case-insensitive enum deserialization. What's the state of this PR? I don't understand if it's been superseded (and should be closed), or if it's still to be merged (what's keeping it?).

Thanks

@erikjara
Copy link

Almost 2024 💀

@deminearchiver
Copy link

It's 2024, will this ever be merged?

@DuckyBlender
Copy link

please guys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.