update rules for handling default value

smithy-lang · Oct 2, 2023 · 38b6eae · 38b6eae
1 parent 7423c66
commit 38b6eae
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 10 deletions.
diff --git a/...odegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/core/KotlinSymbolProvider.kt b/...odegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/core/KotlinSymbolProvider.kt
@@ -13,9 +13,7 @@ import software.amazon.smithy.kotlin.codegen.utils.dq
 import software.amazon.smithy.model.Model
 import software.amazon.smithy.model.knowledge.NullableIndex
 import software.amazon.smithy.model.shapes.*
-import software.amazon.smithy.model.traits.ClientOptionalTrait
 import software.amazon.smithy.model.traits.DefaultTrait
-import software.amazon.smithy.model.traits.InputTrait
 import software.amazon.smithy.model.traits.StreamingTrait
 import java.util.logging.Logger
 
@@ -183,12 +181,11 @@ class KotlinSymbolProvider(private val model: Model, private val settings: Kotli
         val targetSymbol = toSymbol(targetShape)
             .toBuilder()
             .apply {
-                if (nullableIndex.isMemberNullable(shape, settings.api.nullabilityCheckMode)) nullable()
+                val isNullable = nullableIndex.isMemberNullable(shape, settings.api.nullabilityCheckMode)
+                if (isNullable) nullable()
 
-                // @clientOptional supersedes @default
-                val container = model.expectShape(shape.container)
-                val isClientOptional = shape.hasTrait<ClientOptionalTrait>() || container.hasTrait<InputTrait>()
-                if (!isClientOptional) {
+                // only use @default if type is `T` (not `T?`) or marked `@required` (in which case it will be serialized anyway)
+                if (!isNullable || shape.isRequired) {
                     shape.getTrait<DefaultTrait>()?.let {
                         defaultValue(it.getDefaultValue(targetShape))
                     }

diff --git a/...-codegen/src/test/kotlin/software/amazon/smithy/kotlin/codegen/core/SymbolProviderTest.kt b/...-codegen/src/test/kotlin/software/amazon/smithy/kotlin/codegen/core/SymbolProviderTest.kt
@@ -369,6 +369,26 @@ class SymbolProviderTest {
 
     @Test
     fun `@clientOptional overrides @default`() {
+        val model = """
+        structure MyStruct {
+            @clientOptional
+            @default("Foo")
+            quux: QuuxType
+        }
+
+        string QuuxType
+        """.prependNamespaceAndService().toSmithyModel()
+
+        val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
+        val member = model.expectShape<MemberShape>("com.test#MyStruct\$quux")
+        val memberSymbol = provider.toSymbol(member)
+        assertEquals("kotlin", memberSymbol.namespace)
+        assertTrue(memberSymbol.isNullable)
+        assertEquals("null", memberSymbol.defaultValue())
+    }
+
+    @Test
+    fun `required @clientOptional with @default`() {
         val model = """
         structure MyStruct {
             @required
@@ -380,6 +400,27 @@ class SymbolProviderTest {
         string QuuxType
         """.prependNamespaceAndService().toSmithyModel()
 
+        val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
+        val member = model.expectShape<MemberShape>("com.test#MyStruct\$quux")
+        val memberSymbol = provider.toSymbol(member)
+        assertEquals("kotlin", memberSymbol.namespace)
+        // still nullable due to `@clientOptional` but we use the default due to `@required`
+        assertTrue(memberSymbol.isNullable)
+        assertEquals("\"Foo\"", memberSymbol.defaultValue())
+    }
+
+    @Test
+    fun `@input with default`() {
+        val model = """
+        @input
+        structure MyStruct {
+            @default(2)
+            quux: QuuxType
+        }
+        
+        long QuuxType
+        """.prependNamespaceAndService().toSmithyModel()
+
         val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
         val member = model.expectShape<MemberShape>("com.test#MyStruct\$quux")
         val memberSymbol = provider.toSymbol(member)
@@ -389,7 +430,7 @@ class SymbolProviderTest {
     }
 
     @Test
-    fun `@input`() {
+    fun `@input with required`() {
         val model = """
         @input
         structure MyStruct {
@@ -405,8 +446,10 @@ class SymbolProviderTest {
         val member = model.expectShape<MemberShape>("com.test#MyStruct\$quux")
         val memberSymbol = provider.toSymbol(member)
         assertEquals("kotlin", memberSymbol.namespace)
+
+        // still nullable due to `@input` but we can use the default due to `@required`
         assertTrue(memberSymbol.isNullable)
-        assertEquals("null", memberSymbol.defaultValue())
+        assertEquals("2L", memberSymbol.defaultValue())
     }
 
     @Test

diff --git a/tests/codegen/nullability-tests/model/nullability.smithy b/tests/codegen/nullability-tests/model/nullability.smithy
@@ -17,9 +17,12 @@ operation SayHello {
 structure TestInput {
     nested: TestStruct,
 
-    @required
     @default("hammer")
     tay: String
+
+    @required
+    @default("ball")
+    lep: String
 }
 
 structure TestStruct {
@@ -61,6 +64,11 @@ structure TestStruct {
     @required
     @clientOptional
     clientOptionalValue: Integer
+
+    @required
+    @default("model default")
+    @clientOptional
+    clientOptionalWithDefault: String
 }
 
 enum Enum {

diff --git a/...s/codegen/nullability-tests/src/test/kotlin/smithy/kotlin/nullability/DefaultValueTest.kt b/...s/codegen/nullability-tests/src/test/kotlin/smithy/kotlin/nullability/DefaultValueTest.kt
@@ -5,6 +5,7 @@
 package smithy.kotlin.nullability
 
 import kotlin.test.Test
+import kotlin.test.assertEquals
 import kotlin.test.assertNull
 
 class DefaultValueTest {
@@ -14,7 +15,9 @@ class DefaultValueTest {
             .build()
 
         // all members of structure marked with `@input` are implicitly `@clientOptional`
+        // which means they are nullable. We ignore `@default` unless it's marked `@required`
         assertNull(actual.tay)
+        assertEquals("ball", actual.lep)
     }
 
     @Test
@@ -23,6 +26,8 @@ class DefaultValueTest {
             .build()
 
         // all members of structure marked with `@input` are implicitly `@clientOptional`
+        // which means they are nullable. We ignore `@default` unless it's marked `@required`
         assertNull(actual.tay)
+        assertEquals("ball", actual.lep)
     }
 }
diff --git a/...odegen/nullability-tests/src/test/kotlin/smithy/kotlin/nullability/ErrorCorrectionTest.kt b/...odegen/nullability-tests/src/test/kotlin/smithy/kotlin/nullability/ErrorCorrectionTest.kt
@@ -37,6 +37,9 @@ class ErrorCorrectionTest {
         assertEquals(emptyList(), actual.nestedListValue)
         assertEquals(emptyMap(), actual.mapValue)
 
+        // should still be nullable due to `@clientOptional` but due to `@required` we actually use the `@default`
+        assertEquals("model default", actual.clientOptionalWithDefault)
+
         // error correction should apply recursively
         assertEquals("", actual.nested.a)
 
@@ -71,6 +74,9 @@ class ErrorCorrectionTest {
         assertEquals(emptyList(), actual.nestedListValue)
         assertEquals(emptyMap(), actual.mapValue)
 
+        // should still be nullable due to `@clientOptional` but due to `@required` we actually use the `@default`
+        assertEquals("model default", actual.clientOptionalWithDefault)
+
         // nested struct and union values should be null for client careful mode
         assertNull(actual.nested)
         assertNull(actual.union)