feat(codegen): improve nullability of generated types #968

aajtodd · 2023-10-02T17:44:45Z

Issue #

n/a

Description of changes

Previously we set nullability information on symbols coming out of KotlinSymbolProvider based on the underlying type
and whether a default value was provided. This meant things like List, String, Enum, etc were always considered nullable. This was based on IDL v1 semantics w.r.t `Primitive*" shapes being the only shapes that were non-nullable (since they had an implicit default).

This PR removes all notion of how we considered nullability previously to instead rely entirely on the NullableIndex
as the source of truth. The new mental model is that converting any old shape (in particular explicit member.target)
is not going to give you nullability information. Only member shapes will. This is because nullability is local to how
a target shape is used in the context of some container (e.g. structure) shape.

For operation inputs customers will now see a runtime exception for any @required member without a @default set.
This shouldn't affect anyone with a working application assuming services actually validated these fields as required.
For operation outputs any missing field will be corrected using client error correction introduced in #958 such that all non-nullable members of the output struct end up with a value instead of throwing an exception.

feat! (BREAKING): Add plugin setting to control nullability check mode and change the default to be CLIENT_CAREFUL.
We now generate structures with non-null members anywhere NullableIndex indicates a members is non-nullable. Previously this would have been only Primitive* shapes. We also update when we respect @default trait and only set a default in the builder (on the symbol) if NullableIndex indicates that it's non-nullable T
feat: Add plugin setting to control when default values are serialized. Previously we only serialized numbers and booleans when they were set to something other than the default or marked @required. The default settings is the same but can be updated to "always" which would serialize defaults even if they aren't different.
- I've added this as a setting because the current smithy spec recommends to always serialize defaults. I had implemented this change but it causes some issues with S3 not accepting default values. Smithy team maintains this may be fixed with model updates but who knows when they will come. Instead I've opted to keep the current default value serialization semantics but allowed for easy toggling back and forth.
refactor: Improve struct serialization codegen to better handle nullability and reduce the number of generated warnings.
refactor: Remove unnecessary safe-call on non-nullable prefix headers. See disallow sparse prefixHeaders
refactor: Move httpLabel required checks introduced in fix: require values for HTTP query- and queryParams-bound parameters #697 to the operation serializer.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ianbotsf · 2023-10-10T16:41:36Z

...mithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/KotlinSettings.kt

+    /**
+     * Always serialize default values even if they are set to the default
+     */
+    ALWAYS("always"),
+
+    /**
+     * Only serialize default values when they differ from the default given in the model.
+     */
+    WHEN_DIFFERENT("whenDifferent"),
+    ;


Suggestion: These KDocs are confusing to me because they use the term "default values" to (I believe) indicate "values set on fields which have the @default trait". Perhaps they would be clearer as:

Always serialize values even when they are set to the default given in the model

Only serialize values when they differ from the default given in the model

or just add one word: "modeled default"

ianbotsf · 2023-10-10T16:42:25Z

...mithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/KotlinSettings.kt

+     */
+    WHEN_DIFFERENT("whenDifferent"),
+    ;
+    override fun toString(): String = value


Question: Why override this? Does some code rely on the output of toString?

ianbotsf · 2023-10-10T16:47:10Z

...mithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/KotlinSettings.kt

+private fun checkModefromValue(value: String): CheckMode {
+    val camelCaseToMode = CheckMode.values().associateBy { it.toString().toCamelCase() }
+    return requireNotNull(camelCaseToMode[value]) { "$value is not a valid CheckMode, expected one of ${camelCaseToMode.keys}" }
+}


Correctness: This is a case-sensitive comparison. Do we want that?

I think we do not. Build files will be written mostly by humans and so I think we should be forgiving about capitalization.

I disagree. It results in an error so it's not like a human isn't going to find it and I'd rather be explicit.

ianbotsf · 2023-10-10T16:47:23Z

...mithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/KotlinSettings.kt

+        fun fromValue(value: String): DefaultValueSerializationMode =
+            values().find {
+                it.value == value
+            } ?: throw IllegalArgumentException("$value is not a valid DefaultValueSerializationMode, expected one of ${values().map { it.value }}")


Correctness: This is a case-sensitive comparison. Do we want that?

ianbotsf · 2023-10-10T16:53:07Z

...mithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/KotlinSettings.kt


        fun fromNode(node: Optional<ObjectNode>): ApiSettings = node.map {
            val visibility = Visibility.fromValue(node.get().getStringMemberOrDefault(VISIBILITY, "public"))
-            ApiSettings(visibility)
+            val checkMode = checkModefromValue(node.get().getStringMemberOrDefault(NULLABILITY_CHECK_MODE, "clientCareful"))


Style: This seems odd because it specifies as the default a string constant which isn't verified at compile time. This might be safer as getting the nullable string member value, ?.let passing that to the deserializer function, and then null-coalescing to CheckMode.CLIENT_CAREFUL.

ianbotsf · 2023-10-10T17:15:35Z

.../main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/ServiceClientConfigGenerator.kt

+        // FIXME - we only generate an endpoint provider type if we have a protocol generator defined
+        if (context.protocolGenerator != null) {


Question: Why did this change?

So that we don't have to add a hand written endpoint provider in things like waiter-tests, nullability-tests, and paginator-tests. We shouldn't be generating code that doesn't compile by default and we should likely be generating those definitions without the protocol generator.

ianbotsf · 2023-10-10T18:35:51Z

...ain/kotlin/software/amazon/smithy/kotlin/codegen/rendering/serde/SerializeStructGenerator.kt

-
-        writer.write("input.#L?.let { field(#L, it, #L) }", memberName, memberShape.descriptorName(), tsFormat)
+            val fn = serializerFn.format(memberShape, "input.$memberName")
+            writer.write("${defaultCheck}${fn}$postfix")


Nit: Braces unnecessary.

ianbotsf · 2023-10-10T18:52:14Z

...software/amazon/smithy/kotlin/codegen/rendering/protocol/HttpBindingProtocolGeneratorTest.kt

@@ -510,4 +510,74 @@ internal class SmokeTestOperationDeserializer: HttpDeserialize<SmokeTestResponse
        """
        contents.shouldContainOnlyOnceWithDiff(expected)
    }
+
+    @Test
+    fun itValidatesRequiredAndNonBlankURIBindings() {


Nit: URI → Uri

ianbotsf · 2023-10-10T19:52:39Z

tests/codegen/nullability-tests/build.gradle.kts

+    doLast {
+    }


Question: What does this do?

🤔 , nothing not sure where it came from.

ianbotsf · 2023-10-10T20:11:48Z

tests/compile/src/test/kotlin/software/amazon/smithy/kotlin/codegen/util/TestUtils.kt

-private const val SmithyVersion = "1.0"
+private const val SmithyVersion = "2.0"


Question: Where is this used?

Good question, I believe it's used in the test utils somewhere as the model version.

lauzadis · 2023-10-12T14:45:06Z

...mithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/KotlinSettings.kt

+    /**
+     * Always serialize default values even if they are set to the default
+     */
+    ALWAYS("always"),
+
+    /**
+     * Only serialize default values when they differ from the default given in the model.
+     */
+    WHEN_DIFFERENT("whenDifferent"),
+    ;


or just add one word: "modeled default"

lauzadis · 2023-10-12T14:49:34Z

...odegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/StructureGenerator.kt

+        val prefix = if (shape.isError && memberName == "message") {
+            val targetShape = model.expectShape(memberShape.target)
+            if (!targetShape.isStringShape) {
+                throw CodegenException("Message is a reserved name for exception types and cannot be used for any other property")


Should the exception refer to lowercase "message"?

lauzadis · 2023-10-12T19:59:23Z

tests/codegen/nullability-tests/build.gradle.kts

+
+plugins {
+    kotlin("jvm")
+    @Suppress("DSL_SCOPE_VIOLATION") // TODO: Remove once https://youtrack.jetbrains.com/issue/KTIJ-19369 is fixed


It's fixed in Gradle 8.1, maybe we should add that detail?

…rror correction

…erialization

…to only serializing when value differs from runtime

sonarqubecloud · 2023-10-25T14:03:53Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
23 Code Smells

No Coverage information
0.0% Duplication

aajtodd requested a review from a team as a code owner October 2, 2023 17:44

aajtodd mentioned this pull request Oct 2, 2023

feat(codegen): improve nullability of generated types awslabs/aws-sdk-kotlin#1059

Merged

aajtodd force-pushed the fix-nullability branch from 612a292 to fa93151 Compare October 2, 2023 18:01

aajtodd force-pushed the fix-nullability branch from fdad0eb to c4513c2 Compare October 10, 2023 19:39

ianbotsf reviewed Oct 10, 2023

View reviewed changes

lauzadis approved these changes Oct 12, 2023

View reviewed changes

aajtodd force-pushed the fix-nullability branch 2 times, most recently from 61d90fe to 1b7920f Compare October 19, 2023 14:24

aajtodd added 22 commits October 25, 2023 09:26

add nullability check mode as an API setting

679da34

wip nullability tests

abfe20d

refactor structure generator to handle nullability

7517229

improve when explicit null checking is required

fc291a3

fix prefix headers and unnecessary empty body null default

e9e8959

fix hashCode nullability

d144a39

fix structure serialization nullability

4f9413b

remove todo

ed3111d

improve header and query serialization nullability

39da924

add codegen integration tests covering client nullability modes and e…

d3eabef

…rror correction

fix instantiation of null struct params

8c85b86

remove unnecessary shims now that codegen is fixed

7dfc1e8

remove fixmes

8b2f6a5

treat all members as clientOptional when container is marked with @input

0b7ecf6

changelog and version bump

09d15f0

refactor http label and query validation to happen during operation s…

a7f8593

…erialization

update rules for handling default value

4bacb80

remove test that requires client mode

c381a8d

ignore nullability-tests for binary compat

73a8a6a

add plugin setting to control default value serialization and revert …

efb063c

…to only serializing when value differs from runtime

update rules to use default values only when T not T?

8642799

update test with new semantics

6cae680

aajtodd added 2 commits October 25, 2023 09:26

fix test

6862af3

feedback

fccdb86

aajtodd force-pushed the fix-nullability branch from 1b7920f to fccdb86 Compare October 25, 2023 13:39

lint

f95e935

aajtodd merged commit 4f06399 into main Oct 25, 2023
13 of 14 checks passed

aajtodd deleted the fix-nullability branch October 25, 2023 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(codegen): improve nullability of generated types #968

feat(codegen): improve nullability of generated types #968

aajtodd commented Oct 2, 2023 •

edited

Loading

ianbotsf Oct 10, 2023

lauzadis Oct 12, 2023

ianbotsf Oct 10, 2023

ianbotsf Oct 10, 2023

aajtodd Oct 11, 2023

ianbotsf Oct 12, 2023

aajtodd Oct 13, 2023

ianbotsf Oct 10, 2023

ianbotsf Oct 10, 2023

ianbotsf Oct 10, 2023

aajtodd Oct 11, 2023

ianbotsf Oct 10, 2023

ianbotsf Oct 10, 2023

ianbotsf Oct 10, 2023

aajtodd Oct 11, 2023

ianbotsf Oct 10, 2023

aajtodd Oct 11, 2023

lauzadis Oct 12, 2023

lauzadis Oct 12, 2023

lauzadis Oct 12, 2023

sonarqubecloud bot commented Oct 25, 2023

		// FIXME - we only generate an endpoint provider type if we have a protocol generator defined
		if (context.protocolGenerator != null) {

		private const val SmithyVersion = "1.0"
		private const val SmithyVersion = "2.0"

feat(codegen): improve nullability of generated types #968

feat(codegen): improve nullability of generated types #968

Conversation

aajtodd commented Oct 2, 2023 • edited Loading

Issue #

Description of changes

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

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

sonarqubecloud bot commented Oct 25, 2023

aajtodd commented Oct 2, 2023 •

edited

Loading