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

WIP: Support for collecting leftover fields into a container #1177

Closed

Conversation

mitsuhiko
Copy link
Contributor

@mitsuhiko mitsuhiko commented Mar 13, 2018

This is a work in progress pull request to add support for #941.

Unlike what was proposed there I added a container attribute #[collect_unknown_fields_into="a_field"]. This I did for two reasons. One is that there is already deny_unknown_fields so this seems somewhat consistent. Secondly it makes it pretty obvious that it should only be added once. Internally in the ast processing step it adds a flag on the field.

I'm not sure if this approach is entirely correct and clearly there are more issues with this at the current stage. However I appreciate feedback on if what I'm doing here makes any sense at all.

Notes on things I know are not great:

  • The need_value marker I introduced for deserialize_generated_identifier seems questionable. There must be a better approach
  • A field that is internally marked as a collect field also returns true for the skip checks. This makes the code generation do the right thing as far as I can tell. Probably this should be done better though.
  • Only strings keys are supported at this point
  • Handling of duplicated keys is not implemented
  • Serializer is missing at this point.

Example:

#[derive(Deserialize, Debug)]
#[serde(collect_unknown_fields_into="data")]
pub struct Breadcrumb {
    #[serde(rename="type")]
    ty: String,
    timestamp: f64,
    data: HashMap<String, String>,
}

@mitsuhiko
Copy link
Contributor Author

mitsuhiko commented Mar 13, 2018

Upon implementing serialization I ran into a pretty significant issue. I planned on serializing the data through iter()ating over the values and invoking serialize_field on the items. However it turns out that SerializeStruct::serialize_field requires the key to be static. More importantly: that is public API so removing the 'static lifetime on it seems impossible.

I pushed up a version that mem::transmutes the static way for testing purposes. Generated code looks like this:

for (ref __key, ref __value) in self.other.iter() {
    match _serde::ser::SerializeStruct::serialize_field(
        &mut __serde_state,
        unsafe { ::std::mem::transmute(__key.as_str()) },
        __value,
    ) {
        ::result::Result::Ok(val) => val,
        ::result::Result::Err(err) => {
            return ::result::Result::Err(::convert::From::from(err))
        }
    };
}

@mitsuhiko
Copy link
Contributor Author

I think an argument could be made that structs with a unknown field collection attribute actually are serialized like hashmaps even for known fields. This obviously means this entire feature really only applies to self describing formats like json. This however would also have the added benefit that non string keys could also be supported.

@mitsuhiko mitsuhiko force-pushed the feature/collect-unknown-fields branch 2 times, most recently from 3bb9ea9 to dcd7589 Compare March 14, 2018 08:23
@mitsuhiko mitsuhiko force-pushed the feature/collect-unknown-fields branch from dcd7589 to 324028b Compare March 14, 2018 08:25
@mitsuhiko
Copy link
Contributor Author

The more I think about this, the more I get the feeling that for this feature it really only makes sense to force the serialization and deserialization to maps instead of structs. This also would make slightly more sense for non self describing formats and it gets around the limitations of the lifetime of the field key.

@mitsuhiko
Copy link
Contributor Author

Closing this PR for #1178. I think this one is stuck because i do not see a way to make it safe. The other approach based on map serialization makes more sense.

@mitsuhiko mitsuhiko closed this Mar 14, 2018
@mitsuhiko mitsuhiko mentioned this pull request Mar 16, 2018
8 tasks
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

1 participant