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

Problem deserializing maps when deserialization target expect owned String keys #511

Closed
grindvoll opened this issue Sep 29, 2023 · 2 comments

Comments

@grindvoll
Copy link
Contributor

I just encountered this somewhat strange behavior. Trying to deserialize a very simple RON string into a serde_json::Value fails with an ExpectedIdentifier error:

    let json: serde_json::Value = ron::from_str("(f1: 0, f2: 1)").unwrap();

I know that RON is not meant to be a self describing format and supports more types than JSON, but I expected this simple structure to be deserialized correctly; the identifiers should support deserializing to string keys.

The trigger of the error is that the serde_json::Value type expect owned String keys. After some investigating, it seems that the problem is related to the id::Deserializer variant, which supports deserialize_str(..), but NOT deserialize_string(..), which I find somewhat inconsistent. These methods should give the same result; the reason for having two methods is to give hints about expected string ownership (for performance reasons).

The following patch in de/id.rs file makes the code example work as expected:

// de::id.rs, line 147:

    fn deserialize_string<V>(self, visitor: V) -> Result<V::Value>
    where
        V: Visitor<'b>,
    {
        // Err(Error::ExpectedIdentifier)
        self.deserialize_identifier(visitor)  // Same as deserialize_str behavior
    }
@grindvoll grindvoll changed the title Problem deserializing Structs when deserialization target expect owned String keys Problem deserializing maps when deserialization target expect owned String keys Sep 29, 2023
@juntyr
Copy link
Member

juntyr commented Sep 30, 2023

Thank you @grindvoll for reporting and investigating this issue! This sounds like an easy-enough change to me. Would you like to file a PR yourself that also adds a test with your usecase? I'm unfortunately quite swamped with uni at the moment.

@grindvoll
Copy link
Contributor Author

Ok, I will try to file a PR with unit test as soon as possible.

grindvoll added a commit to grindvoll/ron that referenced this issue Oct 2, 2023
grindvoll added a commit to grindvoll/ron that referenced this issue Oct 2, 2023
@juntyr juntyr closed this as completed in 2f3e5a8 Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants