Derive serde Deserialize, Serialize for structs

[anxiousmodernman]

I'd like to take values returned from the API and serialize them to disk with bincode (or similar), which wants an implementation of serde::Serialize.

Since the values of structs are usually Options, Vec, and primitives, it seems like it should be possible to derive the implementation.

Is this feasible? I imagine it would extend compilation time.

[matthewkmayer]

Yes, this is certainly feasible! 😄 Rusoto used to implement Serialize and Deserialize for every struct, but compilation times got out of hand.

I think we could add a flag to our code to conditionally enable struct Serialization. That way if a project doesn't use it, it doesn't have to pay the compilation cost.

Can you give a specific example of a returned struct you would like to use? That will let us have a discussion point on solutions. 👍

[anxiousmodernman]

Sure thing! I made a fork on Gitlab for my filthy hacks: https://gitlab.com/keyvalue/rusoto/blob/hack_codegen/service_crategen/src/commands/generate/codegen/mod.rs#L399

If you search that file for "hack" you should see some other changes. In particular, I needed to avoid the renaming of struct fields.

The types I'm interested in are the "real" model objects, e.g. not the request types or response types, but the things themselves. Vpc, Instance, etc. However, I can imagine use cases for serializing request types, too.

Conditional compilation sounds like the way to go. Maybe features like serde_derive_inputs, serde_derive_entities, serde_derive_outputs. That is, if the AWS api breaks down cleanly along those lines 🤷‍♂️ ?

I have a private project I'm using this with. A snippet of my Cargo.toml here. I don't actually hack types in all these crates, but cargo got mad if when only used my fork in ec2.

[dependencies]
rusoto_core = {git = "https://gitlab.com/keyvalue/rusoto", branch = "hack_codegen"}
rusoto_cloudwatch = {git = "https://gitlab.com/keyvalue/rusoto", branch = "hack_codegen"}
rusoto_credential = {git = "https://gitlab.com/keyvalue/rusoto", branch = "hack_codegen"}
rusoto_ec2 = {git = "https://gitlab.com/keyvalue/rusoto", branch = "hack_codegen"}
rusoto_sts = {git = "https://gitlab.com/keyvalue/rusoto", branch = "hack_codegen"}
rusoto_elb = {git = "https://gitlab.com/keyvalue/rusoto", branch = "hack_codegen"}

To be clear, I started out by simply adding Serialize, Deserialize to generated code by hand, and merely adding the derives worked great. There were so many types to update, though (and I'll find more, I'm sure), that I decided to hack the generator.

[softprops]

Fwiw I added a Serialize impl to output structs a while back but with a config attr so they would only be derived under a test config. The usecase was to add some missing convenience to mock testing interfaces which previously forced you use strings. The conscious decision to only do this under test was to limit the impact this had on compile times which are already large for the amount of code generation rusoto generates. I would suggest feature adding a feature flag to impl Serialize in non test cfgs to not add compile time cost to those that wouldn't need to serialize responses to disk. It seems like a very special case need.

[masongup-mdsol]

I sometimes write small apps to perform bulk actions on various AWS services, and I think this would be extremely handy too, even as a feature option on the crates. A few recent cases include:

• I wrote a bulk CloudWatch data fetcher to retrieve selected datapoints over a specified timespan, and store them to CSV for later analysis. I have the datapoint selection hard-coded, and as the relevant structs (MetricDataQuery, MetricStat, Metric, etc) do not implement Deserialize, there is no easy way to make it configurable. Instead of writing the equivalent as JSON and parsing directly into the rusoto structs, I would have to parse into my own structs and then create the Rusoto structs manually.
  • I wrote a data fetcher to retrieve all of the Simple Workflow Foundation events for a particular timespan and domain, and wanted to store them to JSON for analysis. It would have been simple to do if WorkflowExecutionDetail etc implemented Serialize, but I had to do some messy workarounds, since they didn't.

I'll take a crack at doing this myself at some point if nobody else is looking at it.

[softprops]

Since size is a particular thing under consideration, what do you think about adding a compiler flag feature to enable that with it disabled by default to keep generated code as small as it can be?

[masongup-mdsol]

That would be fine with me - I don't see how you could need this, but not know it at compile time. And if you only need the extra Impls for one of the Rusoto crates, but are using 5, then you wouldn't have to get the extra size and compile time for the other 4.

[plazma-prizma]

Hi, I have a similar need also. I asked the following on stackoverflow but there was no help. https://stackoverflow.com/questions/56851581/how-can-i-compile-my-dependency-with-the-test-configuration-attribute-enabled?noredirect=1#comment100260038_56851581

[matthewkmayer]

I believe this is covered in https://github.com/rusoto/rusoto/pull/1560 . Please reopen if there's more to do. 👍

[masongup-mdsol]

For my two example cases, this nicely solves the issue for WorkflowExecutionDetail in SWF 🎉

However, it doesn't seem to apply to the CloudWatch crate - the code diff doesn't even show Serialize being implemented anywhere. And Deserialize is what I was really looking for there, which #1560 doesn't cover at all.

The CloudWatch code looks to be handled by a different branch of the generator, and there doesn't appear to be an equivalent test feature for Deserialize, so it might be better to create a different issue for these. What do you think @matthewkmayer?

[matthewkmayer]

Reopening this one seems to be the easiest way to track there's more to be done. 😄

[rinde]

Is there a specific reason that RusotoError hasn't gotten the Serialize/Deserialize implementation? I think error values are part of the public API, and should therefore be considered data structures. As per the Rust API Guidelines data structures should implement Serialize/Deserialize.

My concrete use case is that I'm writing a Lambda function and I want to be able to return the error object from rusoto_dynamodb as a JSON object.

[masongup-mdsol]

I started doing some work on this, and have made some good progress. I have a branch that has some straightforward changes along the lines of the #1560 to add what should be all of the missing structs to be serializable with the feature enabled, and the deserialize feature. All of the service crates build with the serialize feature enabled. For the deserialize feature, most of the crates build with it enabled, but about 20 of them fail, all with errors specifying that various child structs don't implement Deserialize.

It seems the existing methods for deciding which traits need the derivation may need to be tweaked. I'm looking into how to do this now. If you want, I can open a PR with what I have now as a place to discuss the changes.

[masongup-mdsol]

I managed to figure out a minor update that got all of the crates except for S3 working with Deserialize again. Just have to sort that out, and then will be ready to open a PR.

[masongup-mdsol]

Okay opened #1639 to address this. CI looks good so far.

[Raniz85]

This looks like it has been merged and released?

[masongup-mdsol]

Yes @Raniz85, confirm that this is released and working in version 0.43, which has been released to crates.io. IMO, this issue should be closed, and you may open another issue if it doesn't work for you in the released 0.43 version.

[azriel91]

Heya, this is implemented for the service crates, but is missing from rusoto_core. Would a PR be accepted to also add the feature gate for that crate?

Specifically I'm looking at serialization for rusoto_core::RusotoError and friends, though I'm guessing such a PR would add it for all data types.