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

Support for Flattening #1179

Merged
merged 51 commits into from Mar 22, 2018
Merged

Support for Flattening #1179

merged 51 commits into from Mar 22, 2018

Conversation

mitsuhiko
Copy link
Contributor

@mitsuhiko mitsuhiko commented Mar 16, 2018

Third time is the charm. This replaces #1177 and #1178 and adds support for #[serde(flatten)]. This pull request can provide the features for #119 as well as #941.

What it does

This pull request adds support for flattening:

  • It introduces a #[serde(flatten)] field attribute which tells serde to flatten the container.
  • It introduces an internal flag on a struct container that indicates that flattening is wanted on the container (at least one field has the flatten attribute).

Currently I only added support for maps and structs but I think this can be extended further to more types (including newtypes). Additionally right now the key must be a string. This requirement does not make a lot of sense and most likely any type of key can be supported with a bit of extra work.

Because of current limitations on the serializer interface structs that use flattening will be serialized as maps and not as structs.

Example usage

use std::collections::HashMap;

#[derive(Serialize, Deserialize, Debug)]
pub struct Context {
    info: String,
    #[serde(flatten)]
    data: Data,
    #[serde(flatten)]
    extra: HashMap<String, String>,
}

#[derive(Serialize, Deserialize, Debug)]
pub struct Data {
    foo: String,
    bar: String,
}

JSON:

{
  "info": "foo bar baz",
  "foo": "foo value",
  "bar": "bar value",
  "extra_key": "extra_value"
}

Rust debug output of struct:

Context {
    info: "foo bar baz",
    data: Data {
        foo: "foo value",
        bar: "bar value"
    },
    extra: {
        "extra_key": "extra_value"
    }
}

What's done

  • #[serde(flatten)] on map types
  • #[serde(flatten)] on structs
  • #[serde(flatten)] on enum structs (without special tagging)
  • tests
  • better error messages if flatten is used on incompatible types
  • #[serde(flatten)] on newtype
  • support for in-place deserialization feature = "deserialize_in_place"
  • non string key support

Thanks so much @dtolnay and @oli-obk for the help in getting this to the current state.

Notes for the reviewer

  • non string key support generates out a bunch of visit methods (visit_i32 etc.) for the field. This seems wasteful. Is there a better way to do this? In case not, maybe it's okay to throw away the handling for anything other than string and bytes?
  • Error messages are largely formatted with format_args! at the moment. Some of the current helper methods do not work with unknown field info.
  • bytes/string borrowing for keys involved in flattening is currently not implemented
  • in-place deserialization for flattening is not implemented
  • some helpers for Content are now public internally so I can reuse them.

@mitsuhiko
Copy link
Contributor Author

There are two issues with the current implementation I'm not sure how to resolve. One is that you cannot #[serde(flatten)] something which in itself is also #[serde(flatten)]. The second one is that you cannot #[serde(flatten)] an enum that has #[serde(tag = "some_field")].

The reason for both of them is the same: the inner type buffers up the values for the outer type as well. The solution I can see is that both generated deserializers would have to share the same buffer which would require a method on the Deserializer struct.

I'm not sure yet if that works but the idea is that instead of the TaggedContentVisitor and the FlatMapDeserializer both buffering there would have to be an abstraction in place. When a flatten level is reached or the TaggedContentVisitor would have to start buffering into a Content we would instead generate a wrapper type X that is either X::Value(Content) or X::MapEntries(Vec<Option<(Content, Content)>>) or something like that.

Then the generated enum deserializer would have to serializer over some sort of TaggedSubsetContentDeserializer which takes that wrapper type and the list of known fields and then only pull the known fields form it and leave the rest in the wrapper. For the non map case a X::Value(Content) is required.

Alternatively maybe #[serde(tag = "some_field")] should just be outright rejected in case of flatten for now?

@mitsuhiko mitsuhiko changed the title WIP: Support for Flattening Support for Flattening Mar 16, 2018
@mitsuhiko mitsuhiko mentioned this pull request Mar 16, 2018
@@ -2197,14 +2199,18 @@ impl<'a, 'de, E> MapAccess<'de> for FlatMapAccess<'a, 'de, E>
T: DeserializeSeed<'de>,
{
while let Some(item) = self.iter.next() {
Copy link
Member

Choose a reason for hiding this comment

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

I still find this logic hard to follow, can you at least add some comments so future contributors know what's going on

@mitsuhiko
Copy link
Contributor Author

I think I acted on all comments.

mitsuhiko added a commit to mitsuhiko/serde-rs.github.io that referenced this pull request Mar 21, 2018
This adds docs for the `#[serde(flatten)]` featur from this pull
request: serde-rs/serde#1179
mitsuhiko added a commit to mitsuhiko/serde-rs.github.io that referenced this pull request Mar 21, 2018
This adds docs for the `#[serde(flatten)]` feature from this pull
request: serde-rs/serde#1179
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

This is great work. Thank you! I am ready to accept this but will file some follow-up issues.

@dtolnay dtolnay dismissed oli-obk’s stale review March 22, 2018 21:13

has been addressed

@dtolnay dtolnay merged commit 5520202 into serde-rs:master Mar 22, 2018
@mitsuhiko mitsuhiko deleted the feature/flatten branch March 22, 2018 21:14
@Ralith
Copy link

Ralith commented Mar 23, 2018

Was this tested on tuple structs? Flattening newtypes specifically is the overwhelmingly significant usecase for me here.

@dtolnay
Copy link
Member

dtolnay commented Mar 23, 2018

@Ralith please file an issue for flattening newtypes.

@theduke
Copy link

theduke commented Apr 29, 2018

@mitsuhiko really wanted this as well, thanks for pushing it through.

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

7 participants