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

Add ability to use #[serde(default)] to deserialize internally tagged enums when no tag is provided #2476

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

Conversation

harrisonturton
Copy link

@harrisonturton harrisonturton commented Jun 14, 2023

This PR addresses #2231 and #1799

Summary

This PR adds support for "default enum variants". This allows serde to fallback to a specific type when it encounters a tagged enum, but no tag. This allows the following enum:

#[derive(Deserialize)]
#[serde(tag = "type")]
pub enum Test {
  #[serde(default)]
  One {
    foo: String,
    bar: String,
  },
  Two {
    baz: String,
    qux: String,
  }
}

To be deserialized from the JSON string (using serde_json)

{ "foo": "...", "bar": "..." }

Note that it has no "tag" field despite the #[serde(tag = "type")] attribute.

Alternatives

This could also be structured as an attribute on the enum itself, like:

#[derive(Deserialize)]
#[serde(tagged)]
#[serde(default_tag = "One")]
pub enum Test {
  One { ... },
  Two { ... },
}

But I opted to use #[serde(default)] field attribute because it matches the native Rust #[default] field attribute. It also makes it trivial to support attributes that transform the tags, like rename and rename_all.

However, this could be confusing because it overloads the meaning of #[serde(default)] which is also used to create default values for fields. I think this is fine since it's only used for enum variants, but this is potentially a good reason to use the "enum attribute" default_tag = "..." instead.

@harrisonturton harrisonturton changed the title Add support for a default variant when an internally tagged enum is untagged Add ability to use #[serde(default)] to specify default enum variants when tag is not found Jun 14, 2023
@harrisonturton harrisonturton force-pushed the harryt/add-default-for-internally-tagged-enums branch from f1bb618 to 8516ca9 Compare June 14, 2023 08:42
@harrisonturton harrisonturton force-pushed the harryt/add-default-for-internally-tagged-enums branch from 8516ca9 to 5733520 Compare June 14, 2023 08:43
@harrisonturton harrisonturton force-pushed the harryt/add-default-for-internally-tagged-enums branch from 1b7ac01 to 2fe69d9 Compare June 15, 2023 02:41
@harrisonturton harrisonturton force-pushed the harryt/add-default-for-internally-tagged-enums branch from 7ba2005 to 7d60f0f Compare June 15, 2023 06:28
@harrisonturton harrisonturton changed the title Add ability to use #[serde(default)] to specify default enum variants when tag is not found Add ability to use #[serde(default)] to deserialize internally tagged enums when no tag is provided Jun 15, 2023
@@ -812,6 +812,117 @@ fn test_internally_tagged_enum() {
);
}

#[test]
fn test_internally_tagged_enum_with_default_variant() {
Copy link
Author

Choose a reason for hiding this comment

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

Is this the best way to test the macro?

I had a dig around the codebase, and I felt it made sense, but unsure.

@harrisonturton
Copy link
Author

harrisonturton commented Jun 16, 2023

Ah, I've just found that a request for this feature was rejected last year for being an uncommon use-case.

We ran into this recently (hence this patch), and others ran into it in #1799, #2231 and #1410. This feature is very helpful for evolving a message into a sum type in a backwards compatible way.

For example, allowing GetUserRequestV2 to replace GetUserRequestV1 without any compatibility issues:

#[derive(Deserialize)]
struct GetUserRequestV1 {
  user_id: String
}

#[derive(Deserialize)]
#[serde(tag = "type")]
enum GetUserRequestV2 {
  #[serde(default)]
  GetUserById { user_id: String },
  GetUserByEmail { email: String },
}

This is possible today, but afaict it requires workarounds that need quite a bit of boilerplate, rely on an undocumented feature, or use a custom derive implementation.

Is this worth reconsidering?

@harrisonturton
Copy link
Author

@dtolnay would love to help get this feature over the line. WDYT of these changes/approach?

@MaxwellBo
Copy link

Hi @dtolnay, I'd be really interested in utilizing the behavior implemented by this PR - it really simplifies a large scale API migration I need to perform (and it seems like functionality a lot of people have been clamoring for). Was just wondering if you'd seen this PR, I noticed it hadn't had any activity in over a month?

Comment on lines 860 to 865
let tag = match seq.next_element()? {
Some(tag) => tag,
None => {
return Err(de::Error::missing_field(self.tag_name));
}
None => match self.default_variant {
Some(variant) => variant,
None => return Err(de::Error::missing_field(self.tag_name)),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, it is better to use .or(self.default_variant)

Copy link
Author

Choose a reason for hiding this comment

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

You're right, that's much nicer :)

Comment on lines 895 to 901
None => match self.default_variant {
Some(default) => Ok(TaggedContent {
tag: default,
content: Content::Map(vec),
}),
None => Err(de::Error::missing_field(self.tag_name)),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. tag.or(self.default_variant)

Comment on lines 164 to 167
if has_default {
cx.error_spanned_by(&variant.ident, "only one variant can be marked as default");
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not break after the first error so all errors can be found at once

@avkonst
Copy link

avkonst commented Oct 4, 2023

Can this be merged, please?

@jamuraa
Copy link

jamuraa commented Jan 17, 2024

Found this merge request looking for it. Currently there are ways to jump through hoops (an untagged enum that matches first to a tagged enum and then parses to an untagged one) to achieve this behavior, but they are unclear compared to the method proposed here.

Is there an update on merging of this PR?

@MaxwellBo
Copy link

@harrisonturton what happens when an unrecognized tag is provided? Does it resolve to the default, or does it error? Could it error?

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.

None yet

5 participants