add support for nullable struct members when generating AWS SDKs #2916

Velfi · 2023-08-10T19:37:31Z

Motivation and Context

Description

This PR adds support for nullability i.e. much less unwraps will be required when using the AWS SDK. For generic clients, this new behavior can be enabled in codegen by setting nullabilityCheckMode: "Client" in their codegen config:

      "plugins": {
        "rust-client-codegen": {
          "codegen": {
            "includeFluentClient": false,
            "nullabilityCheckMode": "CLIENT_CAREFUL"
          },
     }

Testing

Ran existing tests

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 · 2023-08-10T20:45:45Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

github-actions · 2023-08-14T17:23:38Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

github-actions · 2023-08-14T18:32:36Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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 · 2023-08-14T20:49:44Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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 · 2023-08-14T21:51:59Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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.

rcoh · 2023-08-15T15:16:15Z

I think we should also note that certain fields are required in the generated docs

rcoh

I'm so excited about this!

rcoh · 2023-08-15T15:01:51Z

...ware/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ProtocolParserGenerator.kt

+                                val errorMessageMember = errorShape.errorMessageMember()
+                                // If the message member is optional and wasn't set, we set a generic error message.
+                                if (errorMessageMember != null) {
+                                    if (errorMessageMember.isOptional) {
+                                        rust(
+                                            """
+                                            if tmp.message.is_none() {
+                                                tmp.message = _error_message;
+                                            }
+                                            """,
+                                        )
+                                    }


is this dropping it if it's not optional?

AFAICT, this code is meant to set an error message. When the message is optional and wasn't provided, we set a generic message. This code isn't relevant when the message is required, as we can count on one being included in the response.

rcoh · 2023-08-15T15:03:45Z

...otlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt

                    } else {
-                        rust("Ok(Some(builder.build()))")
+                        if (hasFallibleBuilder(shape, symbolProvider)) {


I think this can be if (hasFaillibleBuilder || !returnSymbolToParse.isUnconstrained)?

Alternatively, can we push returnSymbolToParse as an argument to hasFallibleBuilder? That might help us make sure we caught all of these locations.

I don't see how the logic can be simplified that way. I also don't see how passing returnSymbolToParse as an argument to hasFallibleBuilder simplifies things. returnSymbolToParse is about whether we should parse a value for a shape into its associated unconstrained type. In the client this is always false because it has no notion of constraint traits.

rcoh · 2023-08-15T15:05:23Z

CHANGELOG.next.toml

+message = "Struct members modeled as required are no longer wrapped in `Option`s."
+references = ["smithy-rs#2916", "aws-sdk-rust#536"]


upgrade guidance would be helpful (it's simple but just listing it out is probably handy.)

An FAQ for why some members are still not optional would also be helpful

rcoh · 2023-08-15T15:06:49Z

aws/rust-runtime/aws-inlineable/src/route53_resource_id_preprocessor.rs

            return;
        }
    }
 }

 pub(crate) struct Route53ResourceIdInterceptor<G, T>
 where
-    G: for<'a> Fn(&'a mut T) -> &'a mut Option<String>,
+    G: for<'a> Fn(&'a mut T) -> Option<&'a mut String>,


side note, might be nice to replace these existential types with a trait for clarity

...codegen/src/main/kotlin/software/amazon/smithy/rustsdk/customize/route53/Route53Decorator.kt

rcoh · 2023-08-15T15:09:52Z

aws/sdk/integration-tests/dynamodb/tests/movies.rs

        .attribute_definitions(
            AttributeDefinition::builder()
                .attribute_name("title")
                .attribute_type(ScalarAttributeType::S)
-                .build(),
+                .build()
+                .unwrap(),


side note: I wonder if we want this API to accept impl TryInto<AttributeDefinitions>

put another way, what if users could pass a builder directly?

...nt/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientCodegenVisitor.kt

...gen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/CoreRustSettings.kt

...are/amazon/smithy/rust/codegen/core/smithy/protocols/parse/XmlBindingTraitParserGenerator.kt

rust-runtime/aws-smithy-http/src/operation/error.rs

github-actions · 2023-08-15T16:28:45Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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.

update code in response to PR comments

github-actions · 2023-08-15T16:57:02Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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.

update serde tests to cover more cases

…lability-now!

david-perez

I think the PR description needs to be updated. The option is nullabilityCheckMode.

github-actions · 2023-09-21T14:52:47Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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 · 2023-09-21T15:52:17Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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.

jdisanti · 2023-09-21T16:58:11Z

aws/sdk/integration-tests/dynamodb/tests/movies.rs

+                .build()
+                .unwrap(),


Should we change build to try_build and add a build that panics?

Switch to sdk branch = next Handle nullability change for smithy-lang/smithy-rs#2916 Handle nullability change for smithy-lang/smithy-rs#2995

add support for nullable struct members when generating AWS SDKs

9c17050

Velfi requested review from a team as code owners August 10, 2023 19:37

Velfi added 3 commits August 10, 2023 14:46

add CHANGELOG.next.toml entries

b1c7ec5

fix SDK adhoc tests config generation

0f27839

fix XML parseStructure

81f428b

Merge branch 'main' into zhessler-nullability-now!

633c257

Velfi added 2 commits August 14, 2023 12:45

fix server codegen issue

266f54f

remove TODO

86dc322

Velfi added 4 commits August 14, 2023 13:58

update route53 resource ID trimmer

a5565a3

add back the prefix loop that I accidentally removed

2d20462

more fixing for route53

acff7bd

fix query serializer generator ref

976f3a2

Velfi added the breaking-change This will require a breaking change label Aug 14, 2023

fix the QuerySerializerGenerator correctly

397c4ab

Velfi added the needs-sdk-review label Aug 14, 2023

rcoh requested changes Aug 15, 2023

View reviewed changes

fix idempotency token generation

80bb768

Velfi removed the needs-sdk-review label Aug 15, 2023

no really, fix idempotency token generation

a44ae99

update code in response to PR comments

Velfi added 2 commits August 16, 2023 14:00

fix serde bugs

916265a

update serde tests to cover more cases

fix more tests

761c1e4

rcoh added 2 commits September 20, 2023 14:06

Merge branch 'main' of github.com:awslabs/smithy-rs into zhessler-nul…

eee07db

…lability-now!

Add nullability test and fix some bugs

58cf184

david-perez approved these changes Sep 21, 2023

View reviewed changes

david-perez removed the needs-server-review label Sep 21, 2023

rcoh added 3 commits September 21, 2023 08:51

A few more fixes

7a4fd41

wip

e75ad16

Fix request id accessor when it is marked as required

99de898

rcoh force-pushed the zhessler-nullability-now! branch from a20dc37 to 99de898 Compare September 21, 2023 14:10

Refactor && remove println

969e32f

Fix unit test & add an ignore for useless question mark

7de1892

jdisanti reviewed Sep 21, 2023

View reviewed changes

jdisanti approved these changes Sep 21, 2023

View reviewed changes

rcoh added this pull request to the merge queue Sep 21, 2023

Merged via the queue into main with commit 1331dc5 Sep 21, 2023
40 of 41 checks passed

rcoh deleted the zhessler-nullability-now! branch September 21, 2023 17:42

DavidSouther mentioned this pull request Sep 26, 2023

Rust: Handle nullability SDK updates awsdocs/aws-doc-sdk-examples#5424

Closed

DavidSouther added a commit to DavidSouther/aws-doc-sdk-examples that referenced this pull request Sep 27, 2023

Handle updates to Rust nullability from smithy-lang/smithy-rs#2916

e1dc8c0

DavidSouther added a commit to DavidSouther/aws-doc-sdk-examples that referenced this pull request Sep 28, 2023

Handle updates to Rust nullability from smithy-lang/smithy-rs#2916

ff0628f

DavidSouther added a commit to DavidSouther/aws-doc-sdk-examples that referenced this pull request Sep 28, 2023

Handle updates to Rust nullability from smithy-lang/smithy-rs#2916

beb9c3d

Velfi removed needs-sdk-review needs-client-review Generic client labels Oct 19, 2023

NickDarvey mentioned this pull request Jun 25, 2024

chore: upgrade aws-config for aarch64-pc-windows support lancedb/lance#2510

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for nullable struct members when generating AWS SDKs #2916

add support for nullable struct members when generating AWS SDKs #2916

Velfi commented Aug 10, 2023 •

edited by rcoh

Loading

github-actions bot commented Aug 10, 2023

github-actions bot commented Aug 14, 2023

github-actions bot commented Aug 14, 2023

github-actions bot commented Aug 14, 2023

github-actions bot commented Aug 14, 2023

rcoh commented Aug 15, 2023

rcoh left a comment

rcoh Aug 15, 2023

Velfi Aug 15, 2023

rcoh Aug 15, 2023

david-perez Aug 22, 2023

rcoh Aug 15, 2023

Velfi Aug 17, 2023

rcoh Aug 15, 2023

rcoh Aug 15, 2023

Velfi Aug 15, 2023

github-actions bot commented Aug 15, 2023

github-actions bot commented Aug 15, 2023

david-perez left a comment

github-actions bot commented Sep 21, 2023

github-actions bot commented Sep 21, 2023

jdisanti Sep 21, 2023

		message = "Struct members modeled as required are no longer wrapped in `Option`s."
		references = ["smithy-rs#2916", "aws-sdk-rust#536"]

add support for nullable struct members when generating AWS SDKs #2916

add support for nullable struct members when generating AWS SDKs #2916

Conversation

Velfi commented Aug 10, 2023 • edited by rcoh Loading

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Aug 10, 2023

github-actions bot commented Aug 14, 2023

github-actions bot commented Aug 14, 2023

github-actions bot commented Aug 14, 2023

github-actions bot commented Aug 14, 2023

rcoh commented Aug 15, 2023

rcoh 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

github-actions bot commented Aug 15, 2023

github-actions bot commented Aug 15, 2023

david-perez left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 21, 2023

github-actions bot commented Sep 21, 2023

Choose a reason for hiding this comment

Velfi commented Aug 10, 2023 •

edited by rcoh

Loading