RFC: Serialization and deserialization

[thomas-k-cameron]

Motivation and Context

RFC for Implementation of Serialization/Deserialization on some datatypes.

relevant issues.
Add serde::Serialize/serde::Deserialize to fluent_builders #645
awslabs/aws-sdk-rust#645

[request]: Serialize/Deserialize of models for Lambda events #269
awslabs/aws-sdk-rust#269

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[thomas-k-cameron]

There was a mistake in one of the sentences. so I updated.

[Velfi]

I want to give you a big congratulation for being the first external customer to submit an RFC!
I'll dig into this later today. Thank you for putting so much effort into this proposal.

[thomas-k-cameron]

I want to give you a big congratulation for being the first external customer to submit an RFC! I'll dig into this later today. Thank you for putting so much effort into this proposal.

I always wanted to make a contribution to a reputable open source project.

I'm so happy to be the first person to ever submit a RFC!

looking forward to your feedback!

[Velfi]

I left a bunch of comments. I'll check back next week. Let me know if any of my questions don't make sense.

[thomas-k-cameron]

@Velfi
Thank you for your feedback.

I think I was able to answer to your question; Please let me know if something is wrong with it.

[Velfi]

It would also be good if your RFC paid some thought to how it would interact with the smithy required and default traits.

I believe that our builders will never have non-Option fields so worrying about required probably isn't necessary. As for defaults, I'd imagine that we wouldn't want to serialize unset fields in general.

[thomas-k-cameron]

I really appreciate your feedback.

I think I'd love to spend more time on fixing the problem.
Should I close this PR and open it again once I'm ready or should I just keep it open?

[Velfi]

Should I close this PR and open it again once I'm ready or should I just keep it open?

RFCs are a request for comments. It's ok if it's not perfect right at the start. Just leave it open and keep improving it.

[thomas-k-cameron]

Hi,

I just pushed some changes but it's still work in progress and there are still some work to do before it's ready for reviews.

Just letting you know...

[thomas-k-cameron]

@Velfi @jdisanti

I just want to let you know that I just pushed some changes.

RFC update

I added a section to discuss about data types that we should skip when serializing/deserializing.

Changes to kotlin codes

Except for few crates that uses data types from aws_smithy_http crate, it's ready to get compiled!
I also added some methods to data types so that it accepts *Input and Builder types.

For example, aws_sdk_s3::client::GetObject now has these methods.
The purpose of these methods are to allow users to pass deserialized data to the client.

I believe that we can easily remove them if you decide to reject this RFC in future.

I didn't put a lot of thoughts into the name of the methods and it's docs.
I really appreciate if you could give me some advice/feedback.

  #[derive(std::clone::Clone, std::fmt::Debug)]
  pub struct GetObject {
      handle: std::sync::Arc<super::Handle>,
      inner: crate::input::get_object_input::Builder,
  }
  impl GetObject {
      /// Creates a new `GetObject`.
      pub(crate) fn new(handle: std::sync::Arc<super::Handle>) -> Self {
          Self {
              handle,
              inner: Default::default(),
          }
      }
 
    /// This method replaces the existing parameter set on this data with the 2nd argument.  
    /// Existing parameters set on this data will be lost.  
    #[cfg(any(
        feature = "unstable-serde-serialize",
        feature = "unstable-serde-deserialize"
    ))]
    pub fn replace_parameter(
        &mut self,
        new_parameter: crate::input::get_object_input::Builder,
    ) {
        let _ = std::mem::replace(&mut self.inner, new_parameter);
    }
  
    /// This method sends a request with given input.  
    /// Method ignores any data that can be found in the builder type held on this struct.
    #[cfg(any(
        feature = "unstable-serde-serialize",
        feature = "unstable-serde-deserialize"
    ))]
    pub async fn send_with_input(
        self,
        input: crate::input::get_object_input::OutputShape,
    ) -> std::result::Result<
        crate::output::GetObjectOutput,
        aws_smithy_http::result::SdkError<crate::error::GetObjectError>,
    > {
        let op = input
            .make_operation(&self.handle.conf)
            .await
            .map_err(|err| {
                aws_smithy_http::result::SdkError::ConstructionFailure(err.into())
            })?;
        self.handle.client.call(op).await
    }
  }

[Velfi]

I've been quite busy this week. I'm hoping to have time next week to review your latest changes. Thanks again for working so hard on this.

[thomas-k-cameron]

@jdisanti
I think finished fixing up the things you mentioned except for this one.
#1944 (comment)

I'd love to wait for him to clarify things.

[Velfi]

I'd love to wait for him to clarify things.

Hey @thomas-k-cameron, I wanted to let you know that our team won't be as available over the next week or so what with the holidays and all. Thanks again for all you hard work on this!

I'll be around next week to answer questions and check if anything else needs to happen before we merge this accepted. I think we're pretty much there.

Happy Holidays.

[thomas-k-cameron]

@Velfi
Thanks for letting me know.
Happy holidays to you too!

[thomas-k-cameron]

@rcoh

Happy new year!
Would you kindly follow up on this one?
#1944 (comment)

[rcoh]

done, let me know if that makes sense

[thomas-k-cameron]

@rcoh
Thanks for the replay.

I left the comment, I appreciate another follow up.

Sorry for bothering you again and again, but I want to make sure that we are on the same pages.

[thomas-k-cameron]

@Velfi

#1944 (comment)

The thing that was unresolved is now fixed.
Can you check out if there is anything left?

[Velfi]

The thing that was unresolved is now fixed.
Can you check out if there is anything left?

@thomas-k-cameron Sure thing. I'm good to merge this, but let me confirm with @rcoh before doing so.

Would you mind fixing the merge conflict?

[thomas-k-cameron]

@Velfi
file name is fixed and conflict is resolved.

[Velfi]

OK, I've set this RFC to merge. Congratulations @thomas-k-cameron! Once it's merged, feel free to start requesting feedback on the implementation PR. Breaking the implementation PR up into smaller pieces where possible will ensure reviews go quicker.

[thomas-k-cameron]

Gotcha!

Do you happen to have a api document or guideline for developing the code-gen?
I'm not familiar with Kotlin so I might be just missing something.

[Velfi]

You can get a high level overview of the SDK and codegen by watching this presentation I made. Beyond that, we don't really have resources that are available to external contributors.

I think that the best way forward would be for you drill down on the changes checklist items you wrote. Most of the items can be their own PR.

Add feature gate for Serialize and Deserialize

This is the best place to start. You can add this to codegen, even if it doesn't activate anything yet.

• Add a CargoDependency entry for serde
• Create a SerdeDecorator class that extends ClientCodegenDecorator. This class should be declared in its own file, probably in the smithy-rs/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/customizations folder. When overriding the extras method, you get access to the RustCrate being generated and you can add features to it by calling rustCrate.mergeFeature(Feature("<your feature name>", default = true, listOf(<every CargoDependency needed by this feature>))). The decorator can add all features that you want to define.
• Write a test that ensures that the generated Cargo.toml contains the features declarations that you expect. The test would probably look similar to smithy-rs/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/CargoTomlGeneratorTest.kt and would be located near it too.

Implement human-readable + non-human-readable serialization for DateTime and Blob in aws_smithy_types

These two issues can be handled at the same time. You'd just submit an update to the aws-smithy-types runtime crate for this one I think. Extracting the declaration of Blob into its own module / folder would be a good idea. I don't want to add more to the lib.rs file if possible.

Implement Serialize and Deserialize for relevant data types in aws_smithy_types

I think this is really the same as the above right?

Modify Kotlin's codegen so that generated Builder and non-Builder types implement Serialize and Deserialize

We'll cross this bridge when we get to it. Once you've gotten through the other tasks, start a GitHub discussion and tag me on it and we can plan things out.

Prepare examples

Once all of the above is done, you'll have to submit a PR to the examples repo. A different team runs that repo but I can advise you if you have any trouble getting your example merged.

Prepare reproducible compile time benchmark

I'm not sure where this would go. I think it'd be nice to have this before everything else so that we can get info on pre-serde-implementation compile times. I'm guessing we'd create a new crate in smithy-rs/tools and then create a GitHub action to run it.