Add support for IDL 2.0 nullability semantics #1725

sugmanue · 2022-09-09T17:18:53Z

Motivation and Context

Changed the code to properly handle the nullability semantics for both, 1.0 and 2.0 models.

Description

Removed the use of the deprecated method isNullable and instead used the isMemberNullable with CheckMode.CLIENT_ZERO_VALUE_V1_NO_INPUT which will make the semantics equivalent to looking for a Box trait which will work for both 1.0 and 2.0 models.
Changed a unit test which now we need to specify is a 1.0 model in order for the loader to properly work
Changed the EC2 customization to add ClientOptionalTrait instead of adding a Box trait using a transformer, this will make will have the same semantic implications that the previous code.
Finally added a bunch of references to mavenLocal which was needed to test the changes.

Testing

Generated all the models using aws-sdk-rust and validated that there were no changes in the generated code.

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

jdisanti · 2022-09-09T17:24:57Z

codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/SymbolVisitor.kt

@@ -211,7 +211,7 @@ open class SymbolVisitor(
    private fun handleOptionality(symbol: Symbol, member: MemberShape): Symbol =
        if (config.handleRequired && member.isRequired) {
            symbol
-        } else if (nullableIndex.isNullable(member)) {
+        } else if (nullableIndex.isMemberNullable(member, NullableIndex.CheckMode.CLIENT_ZERO_VALUE_V1_NO_INPUT)) {


The server code generator is also using this code, so this may need to pass in a different CheckMode for its use-case?

Yes, the server code generator will need to useNullableIndex.CheckMode.SERVER for this check.

Let's add the CheckMode as a member of SymbolVisitorConfig so that it can be set correctly for the server codegen.

I have pushed and update for this.

jdisanti · 2022-09-09T17:26:50Z

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

+fun createModelFromLines(vararg lines: String): Model {
+    var source = lines.joinToString(separator = "\n")
+    return Model.assembler()
+        .addUnparsedModel("test.smithy", source)
+        .assemble()
+        .unwrap()
+}
+


We already have a String.asSmithyModel() extension function: https://github.com/awslabs/smithy-rs/blob/d81bf3c47a4ce35a2b6c66611323057f7e30181e/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/testutil/TestHelpers.kt#L91

Sounds good, I will remove this and use asSmithyModel.

jdisanti · 2022-09-09T17:28:49Z

codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/SymbolBuilderTest.kt

+        val model = createModelFromLines(
+            "\$version: \"1.0\"",
+            "namespace foo.bar",
+            "structure MyStruct {",
+            "    quux: $primitiveType",
+            "}"
+        )


Example of using asSmithyModel:

import software.amazon.smithy.rust.codegen.client.testutil.asSmithyModel // ... val model = """ namespace foo.bar structure MyStruct { quux: $primitiveType } """.asSmithyModel()

jdisanti · 2022-09-09T17:29:23Z

...k-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/customize/ec2/BoxPrimitiveShapes.kt

+        val updates = arrayListOf<Shape>()
+        for (struct in model.structureShapes) {
+            for (member in struct.allMembers.values) {
+                updates.add(member.toBuilder().addTrait(ClientOptionalTrait()).build())


Can we rename this customization to reflect what it's doing now?

Yes, I will do so.

this PR actually changes the behavior I think? it used to box primitives only but now it boxes everything?

jdisanti · 2022-09-09T17:42:41Z

Forgot to mention, the CHANGELOG.next.toml should have a smithy-rs entry added for the Smithy version upgrade, and for the nullability change.

david-perez

I can't test this patch locally. I get the following when invoking Gradle. I've tried clearing caches to no avail. Do you know what's going on?

> Task :buildSrc:compileKotlin FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':buildSrc:compileKotlin'.
> Error while evaluating property 'filteredArgumentsMap' of task ':buildSrc:compileKotlin'
   > Could not resolve all files for configuration ':buildSrc:compileClasspath'.
      > Could not find software.amazon.smithy:smithy-codegen-core:1.25.0.
        Searched in the following locations:
          - file:/home/ANT.AMAZON.COM/davidpz/.m2/repository/software/amazon/smithy/smithy-codegen-core/1.25.0/smithy-codegen-core-1.25.0.pom
          - https://repo.maven.apache.org/maven2/software/amazon/smithy/smithy-codegen-core/1.25.0/smithy-codegen-core-1.25.0.pom
          - https://dl.google.com/dl/android/maven2/software/amazon/smithy/smithy-codegen-core/1.25.0/smithy-codegen-core-1.25.0.pom
        Required by:
            project :buildSrc
      > Could not find software.amazon.smithy:smithy-utils:1.25.0.
        Searched in the following locations:
          - file:/home/ANT.AMAZON.COM/davidpz/.m2/repository/software/amazon/smithy/smithy-utils/1.25.0/smithy-utils-1.25.0.pom
          - https://repo.maven.apache.org/maven2/software/amazon/smithy/smithy-utils/1.25.0/smithy-utils-1.25.0.pom
          - https://dl.google.com/dl/android/maven2/software/amazon/smithy/smithy-utils/1.25.0/smithy-utils-1.25.0.pom
        Required by:
            project :buildSrc
      > Could not find software.amazon.smithy:smithy-protocol-test-traits:1.25.0.
        Searched in the following locations:
          - file:/home/ANT.AMAZON.COM/davidpz/.m2/repository/software/amazon/smithy/smithy-protocol-test-traits/1.25.0/smithy-protocol-test-traits-1.25.0.pom
          - https://repo.maven.apache.org/maven2/software/amazon/smithy/smithy-protocol-test-traits/1.25.0/smithy-protocol-test-traits-1.25.0.pom
          - https://dl.google.com/dl/android/maven2/software/amazon/smithy/smithy-protocol-test-traits/1.25.0/smithy-protocol-test-traits-1.25.0.pom
        Required by:
            project :buildSrc
      > Could not find software.amazon.smithy:smithy-aws-traits:1.25.0.
        Searched in the following locations:
          - file:/home/ANT.AMAZON.COM/davidpz/.m2/repository/software/amazon/smithy/smithy-aws-traits/1.25.0/smithy-aws-traits-1.25.0.pom
          - https://repo.maven.apache.org/maven2/software/amazon/smithy/smithy-aws-traits/1.25.0/smithy-aws-traits-1.25.0.pom
          - https://dl.google.com/dl/android/maven2/software/amazon/smithy/smithy-aws-traits/1.25.0/smithy-aws-traits-1.25.0.pom
        Required by:
            project :buildSrc
      > Could not find software.amazon.smithy:smithy-aws-iam-traits:1.25.0.
        Searched in the following locations:
          - file:/home/ANT.AMAZON.COM/davidpz/.m2/repository/software/amazon/smithy/smithy-aws-iam-traits/1.25.0/smithy-aws-iam-traits-1.25.0.pom
          - https://repo.maven.apache.org/maven2/software/amazon/smithy/smithy-aws-iam-traits/1.25.0/smithy-aws-iam-traits-1.25.0.pom
          - https://dl.google.com/dl/android/maven2/software/amazon/smithy/smithy-aws-iam-traits/1.25.0/smithy-aws-iam-traits-1.25.0.pom
        Required by:
            project :buildSrc
      > Could not find software.amazon.smithy:smithy-aws-cloudformation-traits:1.25.0.
        Searched in the following locations:
          - file:/home/ANT.AMAZON.COM/davidpz/.m2/repository/software/amazon/smithy/smithy-aws-cloudformation-traits/1.25.0/smithy-aws-cloudformation-traits-1.25.0.pom
          - https://repo.maven.apache.org/maven2/software/amazon/smithy/smithy-aws-cloudformation-traits/1.25.0/smithy-aws-cloudformation-traits-1.25.0.pom
          - https://dl.google.com/dl/android/maven2/software/amazon/smithy/smithy-aws-cloudformation-traits/1.25.0/smithy-aws-cloudformation-traits-1.25.0.pom
        Required by:
            project :buildSrc

david-perez · 2022-09-13T11:01:13Z

codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/SymbolVisitor.kt

@@ -211,7 +212,7 @@ open class SymbolVisitor(
    private fun handleOptionality(symbol: Symbol, member: MemberShape): Symbol =


So now that we have NullableIndex.CheckMode.SERVER, can't we simply implement this as below?

private fun handleOptionality(symbol: Symbol, member: MemberShape) = symbol.letIf(nullableIndex.isMemberNullable(member, config.nullabilityCheckMode)) { symbol.makeOptional() }

jdisanti · 2022-09-13T16:32:00Z

I can't test this patch locally. I get the following when invoking Gradle. I've tried clearing caches to no avail. Do you know what's going on?

Smithy 1.25 isn't released yet. @sugmanue is testing with a local Maven repository.

…ates

github-actions · 2022-09-16T18:14:28Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

…ges (#2988) ## Motivation and Context #1725 exposed the need for easily configuring builder behavior between client & server ## Description - extract builderGenerator interface and attach it to `codegenContext` to avoid loads of threading it up and down ## Testing - codegen unit tests - [x] codegen diff audit ## Checklist  - [x] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates - [x] 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._

sugmanue requested review from a team as code owners September 9, 2022 17:18

jdisanti reviewed Sep 9, 2022

View reviewed changes

sugmanue force-pushed the model-evolution-updates branch 2 times, most recently from 531cc4d to b853aca Compare September 12, 2022 18:25

jdisanti approved these changes Sep 12, 2022

View reviewed changes

jdisanti added the needs-server-review label Sep 12, 2022

david-perez reviewed Sep 13, 2022

View reviewed changes

david-perez approved these changes Sep 13, 2022

View reviewed changes

david-perez removed the needs-server-review label Sep 13, 2022

sugmanue force-pushed the model-evolution-updates branch 2 times, most recently from f6fc216 to 4839471 Compare September 15, 2022 21:48

Correctly determine nullability of members in IDLv2 models

fd73873

sugmanue force-pushed the model-evolution-updates branch from 4839471 to fd73873 Compare September 15, 2022 22:09

Merge remote-tracking branch 'upstream/main' into model-evolution-upd…

e417661

…ates

sugmanue force-pushed the model-evolution-updates branch from a75c205 to e417661 Compare September 15, 2022 23:48

Merge branch 'main' into model-evolution-updates

c2bbf43

jdisanti merged commit ffafbec into smithy-lang:main Sep 17, 2022

rcoh mentioned this pull request Sep 15, 2023

Extract builderInstantiator interface to prepare for nullability changes #2988

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for IDL 2.0 nullability semantics #1725

Add support for IDL 2.0 nullability semantics #1725

sugmanue commented Sep 9, 2022

jdisanti Sep 9, 2022

sugmanue Sep 9, 2022

jdisanti Sep 12, 2022

sugmanue Sep 12, 2022

jdisanti Sep 9, 2022

sugmanue Sep 9, 2022

jdisanti Sep 9, 2022

jdisanti Sep 9, 2022

sugmanue Sep 9, 2022

rcoh Oct 27, 2022

jdisanti commented Sep 9, 2022

david-perez left a comment

david-perez Sep 13, 2022

jdisanti commented Sep 13, 2022

github-actions bot commented Sep 16, 2022

		@@ -211,7 +212,7 @@ open class SymbolVisitor(
		private fun handleOptionality(symbol: Symbol, member: MemberShape): Symbol =

Add support for IDL 2.0 nullability semantics #1725

Add support for IDL 2.0 nullability semantics #1725

Conversation

sugmanue commented Sep 9, 2022

Motivation and Context

Description

Testing

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

jdisanti commented Sep 9, 2022

david-perez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdisanti commented Sep 13, 2022

github-actions bot commented Sep 16, 2022