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.

Sign up for GitHub

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

Jump to bottom

Produce `&[T]` instead of `Option<&[T]>` for list fields #2995

Merged

rcoh merged 6 commits into main from auto-unwrap-or-default

Sep 28, 2023

Collaborator

rcoh commented Sep 21, 2023 •

edited

Loading

Motivation and Context

When the returned field is a list, there is almost never a reason to know if the returned value was null or [] — this addresses the 99% case.

Before:

fn blah(&self) -> Option<&[T]> { self.blah.as_deref() }

After:

fn blah(&self) -> &[T] { self.blah.as_deref().unwrap_or_default() }

note: no impact on servers by default, see codegen diff.

Description

Update accessors for lists.

Testing

codegen diff audit (no server changes)
unit 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.

rcoh requested review from a team as code owners

September 21, 2023 16:59

github-actions bot commented Sep 21, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

rcoh force-pushed the auto-unwrap-or-default branch from 3af0d0c to 3a4e5f5 Compare

September 21, 2023 18:25

github-actions bot commented Sep 21, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

rcoh added 2 commits

September 21, 2023 15:43


          Produce &[T] instead of Option<&[T]> for list fields

e7424f0

This improves API ergonomics when the output is a list


          Fix various examples

103f35e

github-actions bot commented Sep 21, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

rcoh force-pushed the auto-unwrap-or-default branch from 91ae6e7 to 46631d7 Compare

September 21, 2023 20:21

github-actions bot commented Sep 21, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.


          Make list flattening configurable

ada8fcb

rcoh force-pushed the auto-unwrap-or-default branch from 46631d7 to ada8fcb Compare

September 22, 2023 15:47

rcoh added the breaking-change label

github-actions bot commented Sep 22, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.


          Fix defaults for servers

83ea856

github-actions bot commented Sep 22, 2023

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)
Server Test Typescript (ignoring whitespace)

A new doc preview is ready to view.


          Remove default for parameter and ensure its specified everywhere

7a939a9

rcoh requested review from a team as code owners

September 22, 2023 18:31

rcoh requested a review from unexge

September 22, 2023 18:31

github-actions bot commented Sep 22, 2023

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.

rcoh added needs-sdk-review needs-server-review labels

Velfi approved these changes

View reviewed changes

Velfi removed the needs-sdk-review label


          Merge branch 'main' into auto-unwrap-or-default

ed5a19d

github-actions bot commented Sep 26, 2023

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.

DavidSouther mentioned this pull request

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

Closed

drganjoo reviewed

View reviewed changes

.../kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/error/ErrorGenerator.kt

               import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderCustomization
               import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderGenerator
               import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderSection
+              import software.amazon.smithy.rust.codegen.core.smithy.generators.StructSettings

Contributor

drganjoo Sep 27, 2023

nit: which namespace do the rest of the settings lie in? For example, we don't have a software.amazon.smithy.rust.codegen.core.smithy.generators.settings namespace?

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

...main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/StructureGenerator.kt Show resolved Hide resolved

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/TestHelpers.kt

@@ @@ -194,7 +195,7 @@ fun StructureShape.renderWithModelBuilder( @@
               ) {
                   val struct = this
                   rustCrate.withModule(symbolProvider.moduleForShape(struct)) {
-                      StructureGenerator(model, symbolProvider, this, struct, emptyList()).render()
+                      StructureGenerator(model, symbolProvider, this, struct, emptyList(), StructSettings(true)).render()

Contributor

drganjoo Sep 27, 2023

Do we have a test someplace for StructSettings(false)?

Collaborator Author

rcoh Sep 28, 2023

actually no! let me add one, good catch. (Although all existing server tests are testing this)

...st/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGeneratorTest.kt Show resolved Hide resolved

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

                       fun fromNode(node: Optional<ObjectNode>): CoreCodegenConfig =
                           if (node.isPresent) {
                               CoreCodegenConfig(
                                   formatTimeoutSeconds = node.get().getNumberMemberOrDefault("formatTimeoutSeconds", defaultFormatTimeoutSeconds).toInt(),
                                   debugMode = node.get().getBooleanMemberOrDefault("debugMode", defaultDebugMode),
+                                  flattenCollectionAccessors = node.get().getBooleanMemberOrDefault("flattenCollectionAccessors", defaultFlattenMode),

Contributor

drganjoo Sep 28, 2023

Will this change allow someone on the server side to override how we generate accessor methods for collection types by setting "flattenCollectionAccessor" in smithy-build.json?

If the user cannot set this in the settings, then disregard the following:

Currently, on the server side that behavior is controlled by whether the field is marked as @required in the model.

This approach is fine for optional server inputs, where not sending a value for a collection (resulting in None) and sending an empty collection are treated the same by the business logic. However, there are scenarios where the server's output could make a meaningful distinction between an empty list and a None list. Additionally, using $memberName.is_none() on the client side to check whether the server sent an output will be unreliable if the server sends an empty list instead of None.

Collaborator Author

rcoh Sep 28, 2023

this is only the accessor method—it has absolutely no impact on serialization, it's just for convenience when reading the field.

Additionally, using $memberName.is_none() on the client side to check whether the server sent an output will be unreliable if the server sends an empty list instead of None.

The only reason to do that is explicitly if you are trying to tell the difference.

Contributor

drganjoo Sep 28, 2023

Oh yes! only the accessor gets changed. The underlying serialization format and the field remains the same.

...er/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCodegenVisitor.kt

@@ @@ -294,6 +294,7 @@ open class ServerCodegenVisitor( @@
                               this,
                               shape,
                               codegenDecorator.structureCustomizations(codegenContext, emptyList()),
+                              structSettings = codegenContext.structSettings(),

Contributor

drganjoo Sep 28, 2023

If there is no chance for the user to set this flag on the server side then this is okay. Otherwise, I think let's set this to: StructSettings(flattenVecAccessors = false) for now.

Contributor

drganjoo Sep 28, 2023

Disregard. Only the accessor is getting changed so let's keep it the way you have it right now.

...amazon/smithy/rust/codegen/server/python/smithy/generators/PythonServerStructureGenerator.kt

@@ @@ -40,7 +40,7 @@ class PythonServerStructureGenerator( @@
                   private val codegenContext: ServerCodegenContext,
                   private val writer: RustWriter,
                   private val shape: StructureShape,
-              ) : StructureGenerator(model, codegenContext.symbolProvider, writer, shape, emptyList()) {
+              ) : StructureGenerator(model, codegenContext.symbolProvider, writer, shape, emptyList(), codegenContext.structSettings()) {

Contributor

drganjoo Sep 28, 2023

If there is no chance for the user to set this flag on the server side then this is okay. Otherwise, I think let's set this to: StructSettings(flattenVecAccessors = false) for now.

Collaborator Author

rcoh Sep 28, 2023

a customer can set this on the server side with the smithy-build.json setting

Contributor

drganjoo Sep 28, 2023

Keep it like it is. I wrongly thought that the underlying field is also going to get changed. Only the accessor gets changed, so this is good.

drganjoo approved these changes

View reviewed changes

rcoh added this pull request to the merge queue

Merged via the queue into main with commit aaf71c8

40 of 41 checks passed

rcoh deleted the auto-unwrap-or-default branch

September 28, 2023 16:23

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


          Handle nullability change for smithy-lang/smithy-rs#2995

6d2153a

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


          Handle updates to Rust nullability

9822a08

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

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


          Handle updates to Rust nullability

4e0e6fe

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

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


          Handle updates to Rust nullability

d48328c

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

meyertst-aws pushed a commit to DavidSouther/aws-doc-sdk-examples that referenced this pull request


          Handle updates to Rust nullability

83c85fe

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

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


          Handle updates to Rust nullability

46cf641

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

Velfi removed the needs-server-review label

NickDarvey mentioned this pull request

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

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change