Add initial implementation of a Serde Decorator #3753

rcoh · 2024-07-11T13:52:55Z

Motivation and Context

Customers want to be able to use serde with Smithy models. For details, see the RFC

Description

Implementation of serde::Serialize for smithy-rs code generators. This takes the approach of a trait, SerializeConfigured which returns a type that implements Serialize. This allows customers to control serde behavior for their use case.

/// Trait that allows configuring serialization
/// **This trait should not be implemented directly!** Instead, `impl Serialize for ConfigurableSerdeRef<T>`**
pub trait SerializeConfigured {
    /// Return a `Serialize` implementation for this object that owns the object. This is what you want
    /// If you need to pass something that `impl`s serialize elsewhere.
    fn serialize_owned(self, settings: SerializationSettings) -> impl Serialize;

    /// Return a `Serialize` implementation for this object that borrows from the given object
    fn serialize_ref<'a>(&'a self, settings: &'a SerializationSettings) -> impl Serialize + 'a;
}

This can be used as follows:

serde_json::to_string(&my_struct.serialize_ref(&SerializationSettings::redact_sensitive_fields());

Currently, this codegen plugin is not used by anything. It can be enabled by bringing it in scope during code generation & adding the @serde trait to the model for the fields where serialization is desired. This will allow the SDK (if they choose to) roll it out only for specific types or services that have customer demand.

There are a number of follow on items required:

Ensure the generated impls work for Server codegen
Generate Deserialize implementations
Test the implementation all Smithy protocol test models (ensure it compiles)

Testing

Unit test testing serialization with a kitchen-sink model

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

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

github-actions · 2024-07-11T14:10:53Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-07-12T18:49:37Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-07-12T19:20:09Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-07-15T15:29:43Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-07-15T15:46:54Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Velfi · 2024-07-15T16:13:31Z

codegen-serde/build.gradle.kts

+}
+
+java {
+    sourceCompatibility = JavaVersion.VERSION_11


Aren't we on v17 now?

oh dono, I copied this from codegen core, can change it to whatever.

...erde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/KotlinClientSerdeDecorator.kt

Velfi · 2024-07-15T16:21:58Z

...erde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/KotlinClientSerdeDecorator.kt

+        return RuntimeType.forInlineFun(codegenContext.symbolProvider.toSymbol(shape).rustType().toString(), Module) {
+            implSerializeConfigured(codegenContext.symbolProvider.toSymbol(shape)) {
+                val baseValue =
+                    writable { rust("self.value") }.letIf(shape.isStringShape) { it.plus(writable(".as_str()")) }


This is super cool, I didn't realize we could conditionally extend writables like this!

...erde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/KotlinClientSerdeDecorator.kt

codegen-serde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/Traits.kt

github-actions · 2024-07-16T15:34:26Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-07-17T13:13:10Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

...en-serde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/SerializeImplGenerator.kt

Velfi

Good idea separating the implementation of serialization and deserialization.

github-actions · 2024-07-17T15:30:10Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

ysaito1001

Brilliant piece of work!

design/src/rfcs/rfc0045_configurable_serde.md

github-actions · 2024-07-18T14:55:19Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

david-perez

I like the solution explained in the RFC.

Main issue with current implementation is that it won't handle constrained shapes in the server, for which we generate Rust newtypes.

Something that would make us gain confidence in the implementation is if we integration tested against the constraints.smithy model (or any large model like the restJson1 one), post transforming it to add @serde to the service shape.

Have a look at this test:

smithy-rs/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/serialize/CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest.kt

Line 116 in c63e792

var model = Model.assembler().discoverModels().assemble().result.get()

design/src/rfcs/rfc0045_configurable_serde.md

david-perez · 2024-07-18T14:16:23Z

design/src/rfcs/rfc0045_configurable_serde.md

+This RFC defines how smithy-rs will enable customers to use the [serde](serde.rs) library with generated clients & servers. This is a common request
+for myriad reasons, but as we have written about [before](https://github.com/awslabs/aws-sdk-rust/issues/269#issuecomment-1227518721) this is a challenging design area. This RFC proposes a new approach: **Rather than implement `Serialize` directly, add a method to types that returns a type that implements `Serialize`.** This solves a number of issues:
+
+  1. It is minimally impactful: It doesn't lock us into one `Serialize` implementation. It contains only one public trait, `SerializeConfigured`. This trait will initially be defined on a per-crate basis to avoid the orphan-trait rule. It doesn't also doesn't have any impact on shared runtime crates (since no types actually need to implement serialize).


Suggested change

1. It is minimally impactful: It doesn't lock us into one `Serialize` implementation. It contains only one public trait, `SerializeConfigured`. This trait will initially be defined on a per-crate basis to avoid the orphan-trait rule. It doesn't also doesn't have any impact on shared runtime crates (since no types actually need to implement serialize).

1. It is minimally impactful: It doesn't lock us into one `Serialize` implementation. It contains only one public trait, `SerializeConfigured`. This trait will initially be defined on a per-crate basis to avoid the orphan-trait rule. It also doesn't have any impact on shared runtime crates (since no types actually need to implement serialize).

design/src/rfcs/rfc0045_configurable_serde.md

david-perez · 2024-07-18T15:13:20Z

...en-serde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/SerializeImplGenerator.kt

+        return writable {
+            serializerFn(target) {
+                rust("&$memberRef")
+            }.plus { rust(".serialize_ref(&self.settings)") }(this)


Hmm Clippy should catch this, there's no need for & here. Which makes me think that we're not running cargo clippy --all-features in CI.

Indeed we aren't. I'm guessing toggling --all-features will cause other things to fail, so we should probably cut a separate GitHub issue.

smithy-rs/buildSrc/src/main/kotlin/CodegenTestCommon.kt

Line 370 in c63e792

commandLine("cargo", "clippy")

fixed clippy lints in the generated code

The unnecessary iters are still there. Can we run integration tests with --all-features in CI, or at least have a tracking issue for it?

codegen-serde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/SerdeDecorator.kt

...en-serde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/SerializeImplGenerator.kt

david-perez · 2024-07-18T15:33:21Z

...en-serde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/SerializeImplGenerator.kt

+                         #{Document}::Object(v) => {
+                             use #{serde}::ser::SerializeMap;
+                             let mut map = serializer.serialize_map(Some(v.len()))?;
+                             for (k, v) in v.iter() {


Are these iter() needed?

no, removed.

They're still there.

codegen-serde/src/test/kotlin/software/amazon/smithy/rust/codegen/serde/SerdeDecoratorTest.kt

landonxjames · 2024-07-19T02:30:11Z

codegen-serde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/SerdeDecorator.kt

+import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext
+import software.amazon.smithy.rust.codegen.server.smithy.customize.ServerCodegenDecorator
+
+val SerdeFeature = Feature("serde", false, listOf("dep:serde"))


Is the feature name overridable? If we were to add this to the AWS SDK I would imagine we would want to introduce it under something like experimental-serde to let it bake for awhile before we finalize it.

not currently, but this would be an easy follow-on

codegen-serde/src/test/kotlin/software/amazon/smithy/rust/codegen/serde/SerdeDecoratorTest.kt

github-actions · 2024-07-23T20:19:02Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-07-26T21:07:19Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-07-29T19:00:18Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-08-05T19:10:40Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Co-authored-by: david-perez <[email protected]>

github-actions · 2024-08-06T18:03:01Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

david-perez · 2024-08-09T18:48:45Z

...gen-serde/src/test/kotlin/software/amazon/smithy/rust/codegen/serde/SerdeProtocolTestTest.kt

+                cargoCommand = "cargo test --all-features",
+                additionalSettings = constrainedShapesSettings,
+            ),
+        ) { clientCodegenContext, rustCrate ->


Suggested change

) { clientCodegenContext, rustCrate ->

) { serverCodegenContext, rustCrate ->

david-perez · 2024-08-09T18:54:18Z

...en-serde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/SerializeImplGenerator.kt

+                    """
+                    use #{serde}::ser::SerializeMap;
+                    let mut map = serializer.serialize_map(Some(#{value}.len()))?;
+                    for (k, v) in #{value}.iter() {


unnecessary iters still here

these are load bearing

rcoh force-pushed the configurable-serde branch from c4f5e56 to a5a247d Compare July 12, 2024 18:32

rcoh force-pushed the configurable-serde branch from a5a247d to 10fc203 Compare July 12, 2024 19:03

rcoh marked this pull request as ready for review July 15, 2024 15:36

rcoh requested review from a team as code owners July 15, 2024 15:36

Velfi reviewed Jul 15, 2024

View reviewed changes

rcoh force-pushed the configurable-serde branch 2 times, most recently from 971662e to a88f021 Compare July 16, 2024 15:07

rcoh force-pushed the configurable-serde branch from 6caee67 to 986201a Compare July 17, 2024 12:55

Velfi reviewed Jul 17, 2024

View reviewed changes

...en-serde/src/main/kotlin/software/amazon/smithy/rust/codegen/serde/SerializeImplGenerator.kt Outdated Show resolved Hide resolved

Velfi approved these changes Jul 17, 2024

View reviewed changes

ysaito1001 added the needs-server-review label Jul 17, 2024

ysaito1001 approved these changes Jul 17, 2024

View reviewed changes

design/src/rfcs/rfc0045_configurable_serde.md Outdated Show resolved Hide resolved

david-perez requested changes Jul 18, 2024

View reviewed changes

landonxjames mentioned this pull request Jul 19, 2024

Implementation of Struct Conversion to DynamoDB Attribute Item HashMap awslabs/aws-sdk-rust#1172

Open

2 tasks

landonxjames reviewed Jul 19, 2024

View reviewed changes

rcoh force-pushed the configurable-serde branch from 4539f31 to 0e2f994 Compare July 23, 2024 20:01

rcoh and others added 21 commits August 6, 2024 13:43

Add initial implementation of a serde decorator

20e0a77

Support Maps, Enums, and DateTimes

218b117

ConfigurableSerde and ConfigurableSerdeRef are actually pub crate

8f229aa

cut down on redundant impls

7199d49

Add support for document shapes, refactor into class

04dc416

Fix typos

adcf8ae

Add support for sparse datastructures & blobs

f8187db

Cleanup the trait implementation

d425744

wip

53aa47b

Add RFC

7d24394

Fix mdbook build, remove dead code

fc2ad4a

constrained shapes

4fb0159

Support constrained shapes

0fb68f9

Apply suggestions from code review

c75b8e7

Co-authored-by: david-perez <[email protected]>

Support Constraint Traits & Sparse collections of collections

e2bf1f8

Add a unit test of CBOR encoding

218bb1d

Ensure each shape gets serialized in its own module

18bc9e9

Add a test of recursive shapes

7ef31bf

Simplify bytes method

a3da7a0

Fix clippy issues

d3ad538

Test on the constraints model with and without publicConstrained types

5aa1794

rcoh force-pushed the configurable-serde branch from a0ab7a4 to 5aa1794 Compare August 6, 2024 17:44

david-perez approved these changes Aug 9, 2024

View reviewed changes

rcoh added this pull request to the merge queue Aug 12, 2024

Merged via the queue into main with commit a107c01 Aug 12, 2024
44 checks passed

rcoh deleted the configurable-serde branch August 12, 2024 17:14

david-perez removed the needs-server-review label Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial implementation of a Serde Decorator #3753

Add initial implementation of a Serde Decorator #3753

rcoh commented Jul 11, 2024 •

edited

Loading

github-actions bot commented Jul 11, 2024

github-actions bot commented Jul 12, 2024

github-actions bot commented Jul 12, 2024

github-actions bot commented Jul 15, 2024

github-actions bot commented Jul 15, 2024

Velfi Jul 15, 2024

rcoh Jul 16, 2024

Velfi Jul 15, 2024

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 17, 2024

Velfi left a comment

github-actions bot commented Jul 17, 2024

ysaito1001 left a comment

github-actions bot commented Jul 18, 2024

david-perez left a comment

david-perez Jul 18, 2024

david-perez Jul 18, 2024

david-perez Jul 18, 2024

rcoh Jul 29, 2024

david-perez Aug 2, 2024

david-perez Jul 18, 2024

rcoh Jul 26, 2024

david-perez Aug 2, 2024

landonxjames Jul 19, 2024

rcoh Jul 29, 2024

github-actions bot commented Jul 23, 2024

github-actions bot commented Jul 26, 2024

github-actions bot commented Jul 29, 2024

github-actions bot commented Aug 5, 2024

github-actions bot commented Aug 6, 2024

david-perez Aug 9, 2024

david-perez Aug 9, 2024

rcoh Aug 9, 2024

	1. It is minimally impactful: It doesn't lock us into one `Serialize` implementation. It contains only one public trait, `SerializeConfigured`. This trait will initially be defined on a per-crate basis to avoid the orphan-trait rule. It doesn't also doesn't have any impact on shared runtime crates (since no types actually need to implement serialize).
	1. It is minimally impactful: It doesn't lock us into one `Serialize` implementation. It contains only one public trait, `SerializeConfigured`. This trait will initially be defined on a per-crate basis to avoid the orphan-trait rule. It also doesn't have any impact on shared runtime crates (since no types actually need to implement serialize).

	) { clientCodegenContext, rustCrate ->
	) { serverCodegenContext, rustCrate ->

Add initial implementation of a Serde Decorator #3753

Add initial implementation of a Serde Decorator #3753

Conversation

rcoh commented Jul 11, 2024 • edited Loading

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Jul 11, 2024

github-actions bot commented Jul 12, 2024

github-actions bot commented Jul 12, 2024

github-actions bot commented Jul 15, 2024

github-actions bot commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 17, 2024

Velfi left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 17, 2024

ysaito1001 left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 18, 2024

david-perez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 23, 2024

github-actions bot commented Jul 26, 2024

github-actions bot commented Jul 29, 2024

github-actions bot commented Aug 5, 2024

github-actions bot commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcoh commented Jul 11, 2024 •

edited

Loading