Allow integer renames for struct fields #2209
Conversation
Other fields are collected for structs that have flattened members. Save that condition and to a helpfully names variable rather than doing the compound test each time. No functional change.
There is only one case that other inputs are ignored so rename the variable to something that doesn't suggest the other values are always unimportant. No functional change.
Move index, constructor and fallthrough preparation to the scope they are used to make the function easier to read. No functional change.
To be consistent across all types, use the fallthough to collect non-string field names rather than having different logic between the types. When collect_other_fields is true, it's already assumed that the fallthrough is going to expect __value to be a variant of Content. That assumption is now applied all the types, not just the string-like types.
Sometimes a struct is encoded as a map. This is currently only when the struct contains a flattened field but will also be the case when non-string names are supported. Add a predicate to determine whether a struct should be encoded as a map so the logic can be easily updated. No functional change.
All names are still strings, but the enum can be extended to handle names of different types.
Track whether a container has items that are renames to integer values. If this is the case for a struct, it will need to be serialized as a map. There's not currently support for integer renames so this value is always false.
Checking for unknown fields, duplicate fields and missing fields that are have integer renames.
The existing #[serde(rename = ...)] syntax is extended to support interger names for struct fields. The type of the literal informs the serialization function that is used for the field name with the default being i32, to match the Rust's default. Deserialization is more flexible and accepts any integer type, matching against the field values as 64-bit integers. This allows better compatibility with deserializers that don't know the exact bit width of integer values as well as those that do.
Test integer renames of struct fields including the interactions with flattened fields and the denial of unknown fields.
| @@ -2246,70 +2275,138 @@ fn deserialize_identifier( | |||
| None | |||
| }; | |||
|
|
|||
| let visitor_helper_impl = if collect_other_fields || has_int_rename { | |||
There was a problem hiding this comment.
Maybe "int_field_resolver" or something to make it more clear what this is? "helper" is awfully vague
| @@ -100,6 +101,12 @@ impl<'a> Container<'a> { | |||
| has_flatten = true; | |||
| } | |||
| field.attrs.rename_by_rules(attrs.rename_all_rules()); | |||
| if field.attrs.name().serialize_name().is_int() | |||
| @@ -2,8 +2,12 @@ use serde_derive::Serialize; | |||
|
|
|||
| #[derive(Serialize)] | |||
| struct S { | |||
| #[serde(rename = 100)] | |||
| integer: (), | |||
| #[serde(rename = 100i128)] | |||
There was a problem hiding this comment.
I think it might be nice to squash the test patch into this one, since it's a bit odd to have negative tests but no positive tests.
| @@ -643,6 +643,352 @@ fn test_unknown_field_rename_struct() { | |||
| ); | |||
| } | |||
|
|
|||
| #[derive(Debug, PartialEq, Serialize, Deserialize)] | |||
| #[serde(rename = "IgnoredRename")] | |||
| struct IntRenameStruct { | |||
There was a problem hiding this comment.
I think it will work as is, but it might be nice to have a test for renames to hex/binary/octal just in case.
|
This would be very useful, is there anyway that I could help to move this along if @AndrewScull is too busy? |
|
This is very useful for my specific use case, where I have an enum to be serialized with adjacent tags. The tags however, are numbers. {
"op": 8,
"d": {
"v": 4,
"heartbeat_interval" :13750.0
}
}This is the code that doesn't work unfortunately: #[derive(Serialize, Deserialize, Debug)]
#[serde(tag = "op", content = "d")]
pub enum DiscordPayload {
#[serde(rename = "8")]
Hello(Hello),
}It expects an identifier rather than an integer This change does have a valid use case! |
|
@frykher see #2056 for the MR pertaining to enums |
serde_derive does not currently support using integer values as map keys. CTAP uses integer values for various CBOR map keys. In order to properly serialize and deserialize types for the CTAP protocol, a forked version of serde from a pull request that adds support for renaming field keys as integer types is used. Tracked in <serde-rs/serde#2209>
|
I'd love to see this completed. It would be very useful for CBOR specifications that use integers in maps. |
|
As an example of working around this limitation for CBOR encoding, I made this declarative macro to rename the fields to numbers for CTAP2 encoding. You can see it' use in passkey-types::ctap2::make_credential |
| @@ -262,7 +262,9 @@ macro_rules! declare_error_trait { | |||
| /// Raised when a `Deserialize` struct type received a field with an | |||
| /// unrecognized name. | |||
| #[cold] | |||
| fn unknown_field(field: &str, expected: &'static [&'static str]) -> Self { | |||
| fn unknown_field<T>(field: T, expected: &'static [&'static str]) -> Self | |||
There was a problem hiding this comment.
I don't think this is backwards compatible
Closes #1773 and related to PR #2056.
Extends the serde_rename and alias annotations on struct fields to support integer names. Any struct with an integer name is serialized as a map, similarly to structs with any flattened fields, avoiding the need for changes in existing serializer implementations.
Integer names are typed and serialized based on that type, but deserialization is only based on the integer value because deserializers often don't know the exact type of integers.
There are a few unrelated refactors at the start of the series as I was learning the project, then the bulk of the changes are to represent non-string renames (similar to #2056) and to handle the cases of deserialization.