Skip to content
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.

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

feat: support Smithy default trait #857

Merged
merged 38 commits into from
Jun 16, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
0f155c2
Check for DefaultTrait and update nullability index check mode to `CL…
lauzadis May 23, 2023
51f8dbb
Add a few tests
lauzadis May 23, 2023
1aff46c
Add a few more tests
lauzadis May 23, 2023
4aebfe7
Full test suite passing
lauzadis May 23, 2023
528a21d
ktlint
lauzadis May 24, 2023
24f6c67
Remove duplicated default trait parsing
lauzadis May 24, 2023
292dc53
Add a changelog
lauzadis May 24, 2023
1284dd2
Suppress warning
lauzadis May 24, 2023
de93d87
Update streaming blob test to IDLv2 (making member required)
lauzadis May 24, 2023
fc2ff86
Revert
lauzadis May 24, 2023
10cb7b0
Special-case BlobShape empty-string default value
lauzadis May 24, 2023
345b411
Support defaulting empty string
lauzadis May 24, 2023
09719fe
Generic support for string node
lauzadis May 24, 2023
2f84ed7
Test null override
lauzadis May 25, 2023
6b5e7e4
Rename `boxed` to `nullable`
lauzadis May 25, 2023
cf9d353
Handle null default precedence
lauzadis May 25, 2023
e66c579
ktlint
lauzadis May 25, 2023
42ad105
Don't remove default value key, just put `null`
lauzadis May 25, 2023
ccff67e
Merge branch 'main' of github.com:awslabs/smithy-kotlin into feat-def…
lauzadis May 25, 2023
84f69c7
refactor to `when`
lauzadis May 25, 2023
f419475
Merge branch 'main' of github.com:awslabs/smithy-kotlin into feat-def…
lauzadis May 26, 2023
eb084e9
Update KDocs
lauzadis May 30, 2023
781ac4f
Set default value for numbers, update Float's default value to 0f
lauzadis May 30, 2023
ea1fbc1
Merge branch 'main' of github.com:awslabs/smithy-kotlin into feat-def…
lauzadis Jun 5, 2023
3a638e1
Add a default value type
lauzadis Jun 5, 2023
939ce26
ktlint
lauzadis Jun 5, 2023
4a3afb5
Rename box->nullable
lauzadis Jun 6, 2023
8a51691
Revert nullability index check mode
lauzadis Jun 7, 2023
defa0ce
Fix test
lauzadis Jun 7, 2023
aee25a0
Fix test
lauzadis Jun 7, 2023
42bbd66
Merge branch 'main' of github.com:awslabs/smithy-kotlin into feat-def…
lauzadis Jun 7, 2023
367d8d5
Rename boxed->nullable
lauzadis Jun 7, 2023
80e2ce8
Use `String.dq()`
lauzadis Jun 7, 2023
f8ccaf9
Default to Smithy IDLv2 in tests
lauzadis Jun 7, 2023
844b970
Add extension method for default value type
lauzadis Jun 7, 2023
137a013
Add kdocs for DefaultValueType enum
lauzadis Jun 7, 2023
1988148
Merge branch 'main' of github.com:awslabs/smithy-kotlin into feat-def…
lauzadis Jun 16, 2023
c0ca0ee
Assert isNullable, add tests for `@clientOptional` and `@input`
lauzadis Jun 16, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changes/d3ec877c-68c6-4c21-881f-8767d1f87e1c.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"id": "d3ec877c-68c6-4c21-881f-8767d1f87e1c",
"type": "feature",
"description": "Support Smithy default trait",
"issues": [
"https://github.com/awslabs/smithy-kotlin/issues/718"
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import software.amazon.smithy.kotlin.codegen.model.*
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.DefaultTrait
import software.amazon.smithy.model.traits.SparseTrait
import software.amazon.smithy.model.traits.StreamingTrait
import java.util.logging.Logger
Expand Down Expand Up @@ -79,10 +80,14 @@ class KotlinSymbolProvider(private val model: Model, private val settings: Kotli

override fun doubleShape(shape: DoubleShape): Symbol = numberShape(shape, "Double", "0.0")

private fun numberShape(shape: Shape, typeName: String, defaultValue: String = "0"): Symbol =
createSymbolBuilder(shape, typeName, namespace = "kotlin")
.defaultValue(defaultValue)
.build()
private fun numberShape(shape: Shape, typeName: String, defaultValue: String = "0"): Symbol {
val symbol = createSymbolBuilder(shape, typeName, namespace = "kotlin").build()
return if (!symbol.properties.containsKey(SymbolProperty.DEFAULT_VALUE_KEY)) {
symbol.toBuilder().defaultValue(defaultValue).build()
} else {
symbol
}
}

override fun bigIntegerShape(shape: BigIntegerShape?): Symbol = createBigSymbol(shape, "BigInteger")

Expand Down Expand Up @@ -172,11 +177,22 @@ class KotlinSymbolProvider(private val model: Model, private val settings: Kotli
val targetShape =
model.getShape(shape.target).orElseThrow { CodegenException("Shape not found: ${shape.target}") }

val targetSymbol = if (nullableIndex.isMemberNullable(shape, NullableIndex.CheckMode.CLIENT_ZERO_VALUE_V1_NO_INPUT)) {
var targetSymbol = if (nullableIndex.isMemberNullable(shape, NullableIndex.CheckMode.CLIENT)) {
toSymbol(targetShape).toBuilder().boxed().build()
} else {
toSymbol(targetShape)
}

targetSymbol = shape.getTrait<DefaultTrait>()?.let {
val builder = targetSymbol.toBuilder()
val defaultValue = it.getDefaultValue(targetShape)
builder.defaultValue(defaultValue)
if (defaultValue != "null") {
builder.unboxed()
}
builder.build()
} ?: targetSymbol
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The var reference here is unnecessary. You're already dealing with builders is several places so I suggest just keeping it as a builder until you've applied all the customizations necessary:

val targetSymbol = toSymbol(targetShape)
    .toBuilder()
    .apply {
        if (nullableIndex.isMemberNullable(shape, NullableIndex.CheckMode.CLIENT)) boxed()

        shape.getTrait<DefaultTrait>()?.getDefaultValue(targetShape)?.let {
            defaultValue(it)
            if (it != "null") unboxed()
        }
    }
    .build()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Looks like we box when the nullable index says to but then unbox if the default isn't null. Is that right? What happens if these two things conflict?


// figure out if we are referencing an event stream or not.
// NOTE: unlike blob streams we actually re-use the target (union) shape which is why we can't do this
// when visiting a unionShape() like we can for blobShape()
Expand All @@ -197,6 +213,21 @@ class KotlinSymbolProvider(private val model: Model, private val settings: Kotli
}
}

private fun DefaultTrait.getDefaultValue(targetShape: Shape): String =
if (toNode().toString() == "null" || (targetShape is BlobShape && toNode().toString() == "")) {
"null"
} else if (toNode().isNumberNode) {
getDefaultValueForNumber(targetShape, toNode().toString())
} else if (toNode().isArrayNode) {
"listOf()"
} else if (toNode().isObjectNode) {
"mapOf()"
} else if (toNode().isStringNode) {
"\"${toNode()}\""
} else {
toNode().toString()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Prefer when over if/else if/.../else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1


override fun timestampShape(shape: TimestampShape?): Symbol {
val dependency = KotlinDependency.CORE
return createSymbolBuilder(shape, "Instant", boxed = true)
Expand Down Expand Up @@ -253,6 +284,13 @@ class KotlinSymbolProvider(private val model: Model, private val settings: Kotli
return builder
}

private fun getDefaultValueForNumber(shape: Shape, value: String) = when (shape) {
is LongShape -> "${value}L"
is FloatShape -> if (value.matches("[0-9]*\\.[0-9]+".toRegex())) "${value}f" else "$value.0f"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is the decimal check necessary? The f type specifier should mean that a decimal component is not required (i.e., 5f and 5.0f are identical).

is DoubleShape -> if (value.matches("[0-9]*\\.[0-9]+".toRegex())) value else "$value.0"
else -> value
}

/**
* Creates a symbol builder for the shape with the given type name in a child namespace relative
* to the root namespace e.g. `relativeNamespace = bar` with a root namespace of `foo` would set
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ fun Symbol.defaultValue(defaultBoxed: String? = "null"): String? {
*/
fun Symbol.Builder.boxed(): Symbol.Builder = apply { putProperty(SymbolProperty.BOXED_KEY, true) }

/**
* Mark a symbol as not being boxed
*/
fun Symbol.Builder.unboxed(): Symbol.Builder = apply { removeProperty(SymbolProperty.BOXED_KEY) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Should we be keeping the notion of boxed/unboxed in our codebase at this point? It seems like the nullability index should be preferred at this point, should we rename this or change the way we represent symbol nullability?


/**
* Set the default value used when formatting the symbol
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,246 @@ class SymbolProviderTest {
else -> typeName
}

@Test
fun `can read default trait from member`() {
val modeledDefault = "5"
val expectedDefault = "5L"

val model = """
structure MyStruct {
@default($modeledDefault)
foo: MyFoo
}

long MyFoo
""".prependNamespaceAndService(version = "2").toSmithyModel()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Should version = "2" be the default?


val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
val member = model.expectShape<MemberShape>("com.test#MyStruct\$foo")
val memberSymbol = provider.toSymbol(member)
assertEquals("kotlin", memberSymbol.namespace)
assertEquals(expectedDefault, memberSymbol.defaultValue())
}

@Test
fun `can read default trait from target`() {
val modeledDefault = "2500"
val expectedDefault = "2500L"

val model = """
structure MyStruct {
@default($modeledDefault)
foo: MyFoo
}

@default($modeledDefault)
long MyFoo
""".prependNamespaceAndService(version = "2").toSmithyModel()

val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
val member = model.expectShape<MemberShape>("com.test#MyStruct\$foo")
val memberSymbol = provider.toSymbol(member)
assertEquals("kotlin", memberSymbol.namespace)
assertEquals(expectedDefault, memberSymbol.defaultValue())
}

@Test
fun `can override default trait from root-level shape`() {
val modeledDefault = "2500"
val overriddenDefault = "null"

val model = """
structure MyStruct {
@default($overriddenDefault)
foo: RootLevelShape
}

@default($modeledDefault)
long RootLevelShape
""".prependNamespaceAndService(version = "2").toSmithyModel()

val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
val member = model.expectShape<MemberShape>("com.test#MyStruct\$foo")
val memberSymbol = provider.toSymbol(member)
assertEquals("kotlin", memberSymbol.namespace)
assertEquals(overriddenDefault, memberSymbol.defaultValue())
}

@ParameterizedTest(name = "{index} ==> ''can default simple {0} type''")
@CsvSource(
"long,100,100L",
"integer,5,5",
"short,32767,32767",
"float,3.14159,3.14159f",
"double,2.71828,2.71828",
"byte,10,10",
"string,\"hello\",\"hello\"",
"blob,\"abcdefg\",\"abcdefg\"",
"boolean,true,true",
"bigInteger,5,5",
"bigDecimal,9.0123456789,9.0123456789",
"timestamp,1684869901,1684869901",
)
fun `can default simple types`(typeName: String, modeledDefault: String, expectedDefault: String) {
val model = """
structure MyStruct {
@default($modeledDefault)
foo: Shape
}

@default($modeledDefault)
$typeName Shape
""".prependNamespaceAndService(version = "2").toSmithyModel()

val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
val member = model.expectShape<MemberShape>("com.test#MyStruct\$foo")
val memberSymbol = provider.toSymbol(member)
assertEquals(expectedDefault, memberSymbol.defaultValue())
}

@Test
fun `can default empty string`() {
val model = """
structure MyStruct {
@default("")
foo: myString
}
string myString
""".prependNamespaceAndService(version = "2").toSmithyModel()

val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
val member = model.expectShape<MemberShape>("com.test#MyStruct\$foo")
val memberSymbol = provider.toSymbol(member)
assertEquals("\"\"", memberSymbol.defaultValue())
}

@Test
fun `can default enum type`() {
val model = """
structure MyStruct {
@default("club")
foo: Suit
}

enum Suit {
@enumValue("diamond")
DIAMOND

@enumValue("club")
CLUB

@enumValue("heart")
HEART

@enumValue("spade")
SPADE
}
""".prependNamespaceAndService(version = "2").toSmithyModel()

val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
val member = model.expectShape<MemberShape>("com.test#MyStruct\$foo")
val memberSymbol = provider.toSymbol(member)
assertEquals("\"club\"", memberSymbol.defaultValue())
}

@Test
fun `can default int enum type`() {
val model = """
structure MyStruct {
@default(2)
foo: Season
}

intEnum Season {
SPRING = 1
SUMMER = 2
FALL = 3
WINTER = 4
}
""".prependNamespaceAndService(version = "2").toSmithyModel()

val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
val member = model.expectShape<MemberShape>("com.test#MyStruct\$foo")
val memberSymbol = provider.toSymbol(member)
assertEquals("2", memberSymbol.defaultValue())
}

@ParameterizedTest(name = "{index} ==> ''can default document with {0} type''")
@CsvSource(
"null,null,null",
"boolean,true,true",
"boolean,false,false",
"string,\"hello\",\"hello\"",
"long,100,100",
"integer,5,5",
"short,32767,32767",
"float,3.14159,3.14159",
"double,2.71828,2.71828",
"byte,10,10",
"list,[],listOf()",
"map,{},mapOf()",
)
@Suppress("UNUSED_PARAMETER") // using the first parameter in the test name, but compiler doesn't acknowledge that
fun `can default document type`(typeName: String, modeledDefault: String, expectedDefault: String) {
val model = """
structure MyStruct {
@default($modeledDefault)
foo: MyDocument
}

document MyDocument
""".prependNamespaceAndService(version = "2").toSmithyModel()

val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
val member = model.expectShape<MemberShape>("com.test#MyStruct\$foo")
val memberSymbol = provider.toSymbol(member)
assertEquals(expectedDefault, memberSymbol.defaultValue())
}

@Test
fun `can default list type`() {
val model = """
structure MyStruct {
@default([])
foo: MyStringList
}

list MyStringList {
member: MyString
}

string MyString
""".prependNamespaceAndService(version = "2").toSmithyModel()

val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
val member = model.expectShape<MemberShape>("com.test#MyStruct\$foo")
val memberSymbol = provider.toSymbol(member)
assertEquals("listOf()", memberSymbol.defaultValue())
}

@Test
fun `can default map type`() {
val model = """
structure MyStruct {
@default({})
foo: MyStringToIntegerMap
}

map MyStringToIntegerMap {
key: MyString
value: MyInteger
}

string MyString
integer MyInteger
""".prependNamespaceAndService(version = "2").toSmithyModel()

val provider: SymbolProvider = KotlinCodegenPlugin.createSymbolProvider(model)
val member = model.expectShape<MemberShape>("com.test#MyStruct\$foo")
val memberSymbol = provider.toSymbol(member)
assertEquals("mapOf()", memberSymbol.defaultValue())
}

@Test
fun `can read box trait from member`() {
val model = """
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -379,12 +379,12 @@ class StructureGeneratorTest {
val expected = """
public class FooRequest private constructor(builder: Builder) {
public val bar: kotlin.String? = requireNotNull(builder.bar) { "A non-null value must be provided for bar" }
public val baz: kotlin.Int? = requireNotNull(builder.baz) { "A non-null value must be provided for baz" }
public val baz: kotlin.Int = requireNotNull(builder.baz) { "A non-null value must be provided for baz" }
public val corge: kotlin.String? = builder.corge
public val garply: kotlin.String? = requireNotNull(builder.garply) { "A non-null value must be provided for garply" }
.apply { require(isNotBlank()) { "A non-blank value must be provided for garply" } }
public val grault: kotlin.String? = requireNotNull(builder.grault) { "A non-null value must be provided for grault" }
public val quux: kotlin.Boolean? = requireNotNull(builder.quux) { "A non-null value must be provided for quux" }
public val quux: kotlin.Boolean = requireNotNull(builder.quux) { "A non-null value must be provided for quux" }
public val qux: kotlin.String? = builder.qux
""".formatForTest(indent = "")
generated.shouldContainOnlyOnceWithDiff(expected)
Expand Down