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_as does not process enum variant attributes #499

Closed
ljoonal opened this issue Aug 8, 2022 · 3 comments · Fixed by #502
Closed

serde_as does not process enum variant attributes #499

ljoonal opened this issue Aug 8, 2022 · 3 comments · Fixed by #502

Comments

@ljoonal
Copy link

ljoonal commented Aug 8, 2022

In my attempt to update to serde_with@2.0.0 I noticed that nested JSON support is gone, with a changelog stating that:

json::nested can be replaced with #[serde_as(as = "json::JsonString")].

It doesn't seem quite as simple as that though, as I can't figure out a way to achieve what worked with json::nested using JsonString.

Minimal example which contains example JSON files and tests: https://github.com/ljoonal/serde_with_nested_json

running 4 tests
test enum_nested_json ... ok
test struct_json_string ... ok
test enum_json_string ... FAILED
test struct_nested_json ... ok

failures:

---- enum_json_string stdout ----
thread 'enum_json_string' panicked at 'called Result::unwrap() on an Err value: Error("invalid type: string "{\"id\":\"R-5c5d3057-bb97-4c31-a2e0-26ea055b2bcf\"}", expected struct MessagePayload", line: 6, column: 2)', src/lib.rs:32:70

My expectation is that JsonString should work just like json:nested did, but instead it fails for a reason that is beyond my debugging skills. But I've at least narrowed it down to working with the structs but breaking with the enums in the linked example repo.

As to why support for this would be nice, a real world usecase is de-serializing an external JSON API's responses that contain nested json of different types, but also sometimes just plain strings.

@jonasbb
Copy link
Owner

jonasbb commented Aug 8, 2022

I think I identified the issue. The problem is that the serde_as macro does not recognize the attribute in this position. Currently, I think it only supports attributes on fields, but not on enum variants.
Since the attribute is not recognized, it will not be translated into something serde understands.

    #[serde_with::serde_as]
    #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
    #[serde(tag = "messageType", content = "content")]
    /// The contents of a message combined with the `MessageType`
    pub enum MessageContentJsonStringEnum {
        /// A normal message
        Text(String),
        /// Fancy object message
        #[serde_as(as = "serde_with::json::JsonString")]
        Object(crate::MessagePayload),
    }

Instead, you can write this.

/// Fancy object message
Object(
	  #[serde_as(as = "serde_with::json::JsonString")]
	  crate::MessagePayload
),

It might be worthwhile to extend the macro to support variant attributes too.

@jonasbb jonasbb changed the title JsonString fails deserializing nested JSON inside of an adjacently tagged enum serde_as does not process enum variant attributes Aug 8, 2022
@ljoonal
Copy link
Author

ljoonal commented Aug 9, 2022

Instead, you can write this.
...

Ah that makes sense, than you, that solves the main issue I had 👍

It might be worthwhile to extend the macro to support variant attributes too.

If that basically achieves the same result, a simpler alternative might just be to add an example of the attribute being used "within" the enum definitions to the docs? :)

jonasbb added a commit that referenced this issue Aug 14, 2022
Change the serde_as macro to warn on serde_as attributes on enum variant
fields. They are unsupported (in contrast to serde_derive), so showing
an error is better and avoids confusion when migrating.
Also update the main examples to include an enum.
jonasbb added a commit that referenced this issue Aug 14, 2022
Change the serde_as macro to warn on serde_as attributes on enum variant
fields. They are unsupported (in contrast to serde_derive), so showing
an error is better and avoids confusion when migrating.
Also update the main examples to include an enum.
@jonasbb
Copy link
Owner

jonasbb commented Aug 14, 2022

I decided not to support serde_as on enum variants for now. But instead, an error will be issued when this happens. Your idea of showing an example how to use it with enums is good. So I changed one of the examples in the README to use an enum.
The updated example is in #502.

@jonasbb jonasbb closed this as completed Aug 14, 2022
bors bot added a commit that referenced this issue Aug 14, 2022
502: Better handle serde_as on enum variants #499 r=jonasbb a=jonasbb

Change the serde_as macro to warn on serde_as attributes on enum variant
fields. They are unsupported (in contrast to serde_derive), so showing
an error is better and avoids confusion when migrating.
Also update the main examples to include an enum.

Closes #499

Co-authored-by: Jonas Bushart <jonas@bushart.org>
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

Successfully merging a pull request may close this issue.

2 participants