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

Feat/allow generic v to be arbitrary struct #1079

Conversation

Azgrom
Copy link

@Azgrom Azgrom commented Oct 24, 2023

Hey everyone!

Currently I am finding myself having to deserialize a JSON that might have N number of arbitrarily named properties. It is +/- as follows:

{
    "headers": {
        "FirstArbitraryObject": {
            "User-Agent": "Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/118.0",
            "Id": 64
        },
        "SecondAnyGivenObject": {
            "User-Agent": "NULL",
            "Id": 0
        }
    }
}

Where in this case I wrote only two objects for brevity's sake. In this scenario I have not a list of objects, but a outer object with unknown size.

And each object follows a specific contract (data structure), though some properties might be optional. Considering this, I wanted to define my data structs as follows:

#[derive(Serialize, Deserialize, Debug, Default, PartialEq)]
struct ArbitraryContent {
    #[serde(rename = "User-Agent")]
    pub user_agent: String,
    #[serde(rename = "Id")]
    pub id: u64,
}

#[derive(Serialize, Deserialize)]
struct ArbitraryBody {
    pub headers: Map<String, ArbitraryContent>,
}
But this way, in the current `master` branch, pointing to commit `39f5ad1`, I get the errors inside this spoiler dropdown:
error[E0277]: the trait bound `serde_json::Map<std::string::String, ArbitraryContent>: Serialize` is not satisfied
    --> wks/rest_api/tests/http_requests_tests.rs:92:10
     |
92   | #[derive(Serialize, Deserialize)]
     |          ^^^^^^^^^ the trait `Serialize` is not implemented for `serde_json::Map<std::string::String, ArbitraryContent>`
93   | struct ArbitraryBody {
94   |     pub headers: Map<String, ArbitraryContent>,
     |     --- required by a bound introduced by this call
     |
     = help: the trait `Serialize` is implemented for `serde_json::Map<std::string::String, Value>`
note: required by a bound in `_::_serde::ser::SerializeStruct::serialize_field`
    --> /home/rafaell/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.189/src/ser/mod.rs:1865:12
     |
1859 |     fn serialize_field<T: ?Sized>(
     |        --------------- required by a bound in this associated function
...
1865 |         T: Serialize;
     |            ^^^^^^^^^ required by this bound in `SerializeStruct::serialize_field`
error[E0277]: the trait bound `serde_json::Map<std::string::String, ArbitraryContent>: Deserialize<'_>` is not satisfied
    --> wks/rest_api/tests/http_requests_tests.rs:94:18
     |
94   |     pub headers: Map<String, ArbitraryContent>,
     |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `serde_json::Map<std::string::String, ArbitraryContent>`
     |
     = help: the trait `Deserialize<'de>` is implemented for `serde_json::Map<std::string::String, Value>`
note: required by a bound in `next_element`
    --> /home/rafaell/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.189/src/de/mod.rs:1724:12
     |
1722 |     fn next_element<T>(&mut self) -> Result<Option<T>, Self::Error>
     |        ------------ required by a bound in this associated function
1723 |     where
1724 |         T: Deserialize<'de>,
     |            ^^^^^^^^^^^^^^^^ required by this bound in `SeqAccess::next_element`
error[E0277]: the trait bound `serde_json::Map<std::string::String, ArbitraryContent>: Deserialize<'_>` is not satisfied
    --> wks/rest_api/tests/http_requests_tests.rs:94:18
     |
94   |     pub headers: Map<String, ArbitraryContent>,
     |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `serde_json::Map<std::string::String, ArbitraryContent>`
     |
     = help: the trait `Deserialize<'de>` is implemented for `serde_json::Map<std::string::String, Value>`
note: required by a bound in `next_value`
    --> /home/rafaell/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.189/src/de/mod.rs:1863:12
     |
1861 |     fn next_value<V>(&mut self) -> Result<V, Self::Error>
     |        ---------- required by a bound in this associated function
1862 |     where
1863 |         V: Deserialize<'de>,
     |            ^^^^^^^^^^^^^^^^ required by this bound in `MapAccess::next_value`
error[E0277]: the trait bound `serde_json::Map<std::string::String, ArbitraryContent>: Deserialize<'_>` is not satisfied
  --> wks/rest_api/tests/http_requests_tests.rs:94:5
   |
94 |     pub headers: Map<String, ArbitraryContent>,
   |     ^^^ the trait `Deserialize<'_>` is not implemented for `serde_json::Map<std::string::String, ArbitraryContent>`
   |
   = help: the trait `Deserialize<'de>` is implemented for `serde_json::Map<std::string::String, Value>`
note: required by a bound in `_::_serde::__private::de::missing_field`
  --> /home/rafaell/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.189/src/private/de.rs:25:8
   |
23 | pub fn missing_field<'de, V, E>(field: &'static str) -> Result<V, E>
   |        ------------- required by a bound in this function
24 | where
25 |     V: Deserialize<'de>,
   |        ^^^^^^^^^^^^^^^^ required by this bound in `missing_field`

By looking at the Map<K, V> definition I see that every implementation of this generic struct considers the specific case where Map<String, Value>. So it does not care if a given user struct implements the Deserialize trait.

On the other hand, I believe every serde user should not implement a custom map to make it work with a custom object/struct that implements the Deserialize trait. For two main reasons:

  • It does not make justice to the good implementations of Map<K, V>;
  • It is relatively simple to make V generic, instead of hard-coding it to always be Value;
  • It is responsibility of Map<K, V> to accept any given V that implements Serialize and Deserialize.

Considering this I am bringing this PR where I generalized all the Map<K, V> implementations. Here are the unit test that I ran with this implementation:

#[test]
fn test() {
    let body = r#"
    {
        "headers": {
            "FirstArbitraryObject": {
                "User-Agent": "Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/118.0",
                "Id": 64
            },
            "SecondAnyGivenObject": {
                "User-Agent": "NULL",
                "Id": 0
            }
        }
    }
    "#;

    let arbitrary_body = serde_json::from_str::<ArbitraryBody>(body).unwrap();

    assert_eq!(
        arbitrary_body.headers["FirstArbitraryObject"].type_id(),
        ArbitraryContent::default().type_id()
    );
    assert_eq!(
        arbitrary_body.headers["FirstArbitraryObject"],
        ArbitraryContent {
            user_agent:
                "Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/118.0"
                    .parse()
                    .unwrap(),
            id: 64
        }
    );
    assert_eq!(
        arbitrary_body.headers["SecondAnyGivenObject"],
        ArbitraryContent {
            user_agent: "NULL".to_string(),
            id: 0
        }
    );
}

For facilitate the reviewers life, here are the dependency pointing to the implementation, or current master on my fork:

[dependencies]
serde_json = { git = "https://github.com/Azgrom/serde_json.git", branch = "master" }#branch = "feat/allow-generic-V-to-be-arbitrary-struct" }

All tests passed

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.

Thanks for the PR!

For your use case, you should be able to use any other map type, like HashMap<String, ArbitraryContent>. I think I would prefer to keep serde_json::Map just for String->Value.

@dtolnay dtolnay closed this Oct 25, 2023
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.

2 participants