fix!: overhaul client config property generation (#504)

Author: aajtodd

smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/core/KotlinWriter.kt

@@ -121,6 +121,14 @@ fun <T : CodeWriter> T.declareSection(id: SectionId, context: Map<String, Any?>
 private fun <T : CodeWriter> T.removeContext(context: Map<String, Any?>): Unit =
     context.keys.forEach { key -> removeContext(key) }
 
+/**
+ * Convenience function to get a typed value out of the context or throw if the key doesn't exist
+ * or the type is wrong
+ */
+inline fun <reified T> CodeWriter.getContextValue(key: String): T = checkNotNull(getContext(key) as? T) {
+    "Expected `$key` in CodeWriter context"
+}
+
 /**
  * Registers a [SectionWriter] given a [SectionId] to a specific writer.  This will cause the
  * [SectionWriter.write] to be called at the point in which the section is declared via

smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/core/RuntimeTypes.kt

@@ -64,6 +64,7 @@ object RuntimeTypes {
 
         object Engine {
             val HttpClientEngineConfig = runtimeSymbol("HttpClientEngineConfig", KotlinDependency.HTTP, "engine")
+            val HttpClientEngine = runtimeSymbol("HttpClientEngine", KotlinDependency.HTTP, "engine")
         }
     }
 
@@ -84,6 +85,7 @@ object RuntimeTypes {
         }
 
         object Retries {
+            val RetryStrategy = runtimeSymbol("RetryStrategy", KotlinDependency.CORE, "retries")
             object Impl {
                 val ExponentialBackoffWithJitter = runtimeSymbol("ExponentialBackoffWithJitter", KotlinDependency.CORE, "retries.impl")
                 val ExponentialBackoffWithJitterOptions = runtimeSymbol("ExponentialBackoffWithJitterOptions", KotlinDependency.CORE, "retries.impl")

smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/ClientConfigGenerator.kt

@@ -81,12 +81,17 @@ class ClientConfigGenerator(
     }
 
     private fun renderCompanionObject() {
-        if (builderReturnType != null) {
-            ctx.writer.withBlock("companion object {", "}") {
-                write("@JvmStatic")
-                write("fun fluentBuilder(): FluentBuilder = BuilderImpl()")
-                write("fun builder(): DslBuilder = BuilderImpl()")
-                write("operator fun invoke(block: DslBuilder.() -> kotlin.Unit): #T = BuilderImpl().apply(block).build()", builderReturnType)
+        ctx.writer.withBlock("companion object {", "}") {
+            write("@JvmStatic")
+            write("fun fluentBuilder(): FluentBuilder = BuilderImpl()")
+            write("")
+            if (builderReturnType != null) {
+                write(
+                    "operator fun invoke(block: DslBuilder.() -> kotlin.Unit): #T = BuilderImpl().apply(block).build()",
+                    builderReturnType
+                )
+            } else {
+                write("operator fun invoke(block: DslBuilder.() -> kotlin.Unit): #configClass.name:L = BuilderImpl().apply(block).build()")
             }
         }
     }
@@ -101,22 +106,38 @@ class ClientConfigGenerator(
             }
             ctx.writer.addImport(it.symbol)
             ctx.writer.addImportReferences(it.symbol, SymbolReference.ContextOption.USE)
+            it.additionalImports.forEach { symbol ->
+                ctx.writer.addImport(symbol)
+            }
         }
     }
 
     private fun renderImmutableProperties() {
         props.forEach { prop ->
             val override = if (prop.requiresOverride) "override " else ""
 
-            when {
-                prop.constantValue != null -> {
-                    ctx.writer.write("${override}val #1L: #2T = #3L", prop.propertyName, prop.symbol, prop.constantValue)
+            when (prop.propertyType) {
+                is ClientConfigPropertyType.SymbolDefault -> {
+                    ctx.writer.write("${override}val #1L: #2P = builder.#1L", prop.propertyName, prop.symbol)
                 }
-                prop.required -> {
-                    ctx.writer.write("${override}val #1L: #2T = builder.#1L ?: throw ClientException(\"#1L must be set\")", prop.propertyName, prop.symbol)
+                is ClientConfigPropertyType.ConstantValue -> {
+                    ctx.writer.write("${override}val #1L: #2T = #3L", prop.propertyName, prop.symbol, prop.propertyType.value)
                 }
-                else -> {
-                    ctx.writer.write("${override}val #1L: #2P = builder.#1L", prop.propertyName, prop.symbol)
+                is ClientConfigPropertyType.Required -> {
+                    ctx.writer.write(
+                        "${override}val #1L: #2T = requireNotNull(builder.#1L) { #3S }",
+                        prop.propertyName,
+                        prop.symbol,
+                        prop.propertyType.message ?: "${prop.propertyName} is a required configuration property"
+                    )
+                }
+                is ClientConfigPropertyType.RequiredWithDefault -> {
+                    ctx.writer.write(
+                        "${override}val #1L: #2T = builder.#1L ?: #3L",
+                        prop.propertyName,
+                        prop.symbol,
+                        prop.propertyType.default
+                    )
                 }
             }
         }
@@ -126,7 +147,7 @@ class ClientConfigGenerator(
         ctx.writer.write("")
             .withBlock("interface FluentBuilder {", "}") {
                 props
-                    .filter { it.constantValue == null }
+                    .filter { it.propertyType !is ClientConfigPropertyType.ConstantValue }
                     .forEach { prop ->
                         // we want the type names sans nullability (?) for arguments
                         write("fun #1L(#1L: #2L): FluentBuilder", prop.propertyName, prop.symbol.name)
@@ -139,14 +160,12 @@ class ClientConfigGenerator(
         ctx.writer.write("")
             .withBlock("interface DslBuilder {", "}") {
                 props
-                    .filter { it.constantValue == null }
+                    .filter { it.propertyType !is ClientConfigPropertyType.ConstantValue }
                     .forEach { prop ->
                         prop.documentation?.let { ctx.writer.dokka(it) }
                         write("var #L: #P", prop.propertyName, prop.symbol)
                         write("")
                     }
-                write("")
-                write("fun build(): #configClass.name:L")
             }
     }
 
@@ -155,7 +174,7 @@ class ClientConfigGenerator(
             .withBlock("internal class BuilderImpl() : FluentBuilder, DslBuilder {", "}") {
                 // override DSL properties
                 props
-                    .filter { it.constantValue == null }
+                    .filter { it.propertyType !is ClientConfigPropertyType.ConstantValue }
                     .forEach { prop ->
                         write("override var #L: #D", prop.propertyName, prop.symbol)
                     }
@@ -164,7 +183,7 @@ class ClientConfigGenerator(
                 write("")
                 write("override fun build(): #configClass.name:L = #configClass.name:L(this)")
                 props
-                    .filter { it.constantValue == null }
+                    .filter { it.propertyType !is ClientConfigPropertyType.ConstantValue }
                     .forEach { prop ->
                         // we want the type names sans nullability (?) for arguments
                         write("override fun #1L(#1L: #2L): FluentBuilder = apply { this.#1L = #1L }", prop.propertyName, prop.symbol.name)

smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/ClientConfigProperty.kt

@@ -5,7 +5,6 @@
 package software.amazon.smithy.kotlin.codegen.rendering
 
 import software.amazon.smithy.codegen.core.Symbol
-import software.amazon.smithy.codegen.core.SymbolReference
 import software.amazon.smithy.kotlin.codegen.core.*
 import software.amazon.smithy.kotlin.codegen.model.boxed
 import software.amazon.smithy.kotlin.codegen.model.buildSymbol
@@ -59,14 +58,17 @@ class ClientConfigProperty private constructor(builder: Builder) {
     val baseClass: Symbol? = builder.baseClass
 
     /**
-     * If the property is not provided in the builder then a ClientException is thrown
+     * The configuration property type. This controls how the property is constructed and rendered
      */
-    val required: Boolean = builder.required
+    val propertyType: ClientConfigPropertyType = builder.propertyType
 
     /**
-     * Specifies that the value should be populated with a constant value that cannot be overridden in the builder.
+     * Additional symbols that should be imported when this property is generated. This is useful for
+     * example when the [symbol] type has is an interface and has a default or constant value that
+     * implements that type. The default value symbol also needs imported.
      */
-    val constantValue: String? = builder.constantValue
+    val additionalImports: List<Symbol> = builder.additionalImports
+
     /**
      * Flag indicating if this property stems from some base class and needs an override modifier when rendered
      */
@@ -90,7 +92,7 @@ class ClientConfigProperty private constructor(builder: Builder) {
             name: String,
             defaultValue: Int? = null,
             documentation: String? = null,
-            baseClass: Symbol? = null
+            baseClass: Symbol? = null,
         ): ClientConfigProperty =
             builtInProperty(name, builtInSymbol("Int", defaultValue?.toString()), documentation, baseClass)
 
@@ -107,7 +109,7 @@ class ClientConfigProperty private constructor(builder: Builder) {
             name: String,
             defaultValue: Boolean? = null,
             documentation: String? = null,
-            baseClass: Symbol? = null
+            baseClass: Symbol? = null,
         ): ClientConfigProperty =
             builtInProperty(name, builtInSymbol("Boolean", defaultValue?.toString()), documentation, baseClass)
 
@@ -124,7 +126,7 @@ class ClientConfigProperty private constructor(builder: Builder) {
             name: String,
             defaultValue: String? = null,
             documentation: String? = null,
-            baseClass: Symbol? = null
+            baseClass: Symbol? = null,
         ): ClientConfigProperty =
             builtInProperty(name, builtInSymbol("String", defaultValue), documentation, baseClass)
     }
@@ -137,13 +139,49 @@ class ClientConfigProperty private constructor(builder: Builder) {
 
         var baseClass: Symbol? = null
 
-        var required: Boolean = false
-        var constantValue: String? = null
+        var propertyType: ClientConfigPropertyType = ClientConfigPropertyType.SymbolDefault
+
+        var additionalImports: List<Symbol> = emptyList()
 
         fun build(): ClientConfigProperty = ClientConfigProperty(this)
     }
 }
 
+/**
+ * Descriptor for how a configuration property is rendered when the configuration is built
+ */
+sealed class ClientConfigPropertyType {
+    /**
+     * A property type that uses the symbol type and builder symbol directly
+     */
+    object SymbolDefault : ClientConfigPropertyType()
+
+    /**
+     * Specifies that the value should be populated with a constant value that cannot be overridden in the builder.
+     * These are effectively read-only properties that will show up in the configuration type but not the builder.
+     *
+     * @param value the value to assign to the property at construction time
+     */
+    data class ConstantValue(val value: String) : ClientConfigPropertyType()
+
+    /**
+     * A configuration property that is required to be set (i.e. not null).
+     * If the property is not provided in the builder then an IllegalArgumentException is thrown
+     *
+     * @param message The exception message to throw if the property is null, if not set a message is generated
+     * automatically based on the property name
+     */
+    data class Required(val message: String? = null) : ClientConfigPropertyType()
+
+    /**
+     * A configuration property that is required but has a default value. This has the same semantics of [Required]
+     * but instead of an exception the default value will be used when not provided in the builder.
+     *
+     * @param default the value to assign if the corresponding builder property is null
+     */
+    data class RequiredWithDefault(val default: String) : ClientConfigPropertyType()
+}
+
 private fun builtInSymbol(symbolName: String, defaultValue: String?): Symbol {
     val builder = Symbol.builder()
         .name(symbolName)
@@ -156,7 +194,12 @@ private fun builtInSymbol(symbolName: String, defaultValue: String?): Symbol {
     return builder.build()
 }
 
-private fun builtInProperty(name: String, symbol: Symbol, documentation: String?, baseClass: Symbol?): ClientConfigProperty =
+private fun builtInProperty(
+    name: String,
+    symbol: Symbol,
+    documentation: String?,
+    baseClass: Symbol?,
+): ClientConfigProperty =
     ClientConfigProperty {
         this.symbol = symbol
         this.name = name
@@ -180,13 +223,10 @@ object KotlinClientRuntimeConfigProperty {
         }
 
         HttpClientEngine = ClientConfigProperty {
-            symbol = buildSymbol {
-                name = "HttpClientEngine"
-                namespace(KotlinDependency.HTTP, "engine")
-            }
+            symbol = RuntimeTypes.Http.Engine.HttpClientEngine
             baseClass = httpClientConfigSymbol
             documentation = """
-            Override the default HTTP client configuration (e.g. configure proxy behavior, concurrency, etc)    
+            Override the default HTTP client engine used to make SDK requests (e.g. configure proxy behavior, timeouts, concurrency, etc)    
             """.trimIndent()
         }
 
@@ -208,6 +248,13 @@ object KotlinClientRuntimeConfigProperty {
         }
 
         RetryStrategy = ClientConfigProperty {
+            symbol = RuntimeTypes.Core.Retries.RetryStrategy
+            name = "retryStrategy"
+            documentation = """
+                The [RetryStrategy] implementation to use for service calls. All API calls will be wrapped by the
+                strategy.
+            """.trimIndent()
+
             val retryStrategyBlock = """
                 run {
                     val strategyOptions = StandardRetryStrategyOptions.Default
@@ -216,24 +263,16 @@ object KotlinClientRuntimeConfigProperty {
                     StandardRetryStrategy(strategyOptions, tokenBucket, delayer)
                 }
             """.trimIndent()
-
-            symbol = buildSymbol {
-                name = "RetryStrategy"
-                namespace(KotlinDependency.CORE, "retries")
-                nullable = false
-                reference(RuntimeTypes.Core.Retries.Impl.StandardRetryStrategy, SymbolReference.ContextOption.USE)
-                reference(RuntimeTypes.Core.Retries.Impl.StandardRetryStrategyOptions, SymbolReference.ContextOption.USE)
-                reference(RuntimeTypes.Core.Retries.Impl.StandardRetryTokenBucket, SymbolReference.ContextOption.USE)
-                reference(RuntimeTypes.Core.Retries.Impl.StandardRetryTokenBucketOptions, SymbolReference.ContextOption.USE)
-                reference(RuntimeTypes.Core.Retries.Impl.ExponentialBackoffWithJitter, SymbolReference.ContextOption.USE)
-                reference(RuntimeTypes.Core.Retries.Impl.ExponentialBackoffWithJitterOptions, SymbolReference.ContextOption.USE)
-            }
-            name = "retryStrategy"
-            documentation = """
-                The [RetryStrategy] implementation to use for service calls. All API calls will be wrapped by the
-                strategy.
-            """.trimIndent()
-            constantValue = retryStrategyBlock
+            propertyType = ClientConfigPropertyType.ConstantValue(retryStrategyBlock)
+
+            additionalImports = listOf(
+                RuntimeTypes.Core.Retries.Impl.StandardRetryStrategy,
+                RuntimeTypes.Core.Retries.Impl.StandardRetryStrategyOptions,
+                RuntimeTypes.Core.Retries.Impl.StandardRetryTokenBucket,
+                RuntimeTypes.Core.Retries.Impl.StandardRetryTokenBucketOptions,
+                RuntimeTypes.Core.Retries.Impl.ExponentialBackoffWithJitter,
+                RuntimeTypes.Core.Retries.Impl.ExponentialBackoffWithJitterOptions,
+            )
         }
 
         SdkLogMode = ClientConfigProperty {

smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/ServiceGenerator.kt

@@ -23,7 +23,12 @@ class ServiceGenerator(private val ctx: RenderingContext<ServiceShape>) {
     /**
      * SectionId used when rendering the service interface companion object
      */
-    object ServiceInterfaceCompanionObject : SectionId
+    object ServiceInterfaceCompanionObject : SectionId {
+        /**
+         * Context key for the service symbol
+         */
+        const val ServiceSymbol = "ServiceSymbol"
+    }
 
     /**
      * SectionId used when rendering the service configuration object
@@ -58,7 +63,10 @@ class ServiceGenerator(private val ctx: RenderingContext<ServiceShape>) {
             .call {
                 // allow integrations to add additional fields to companion object or configuration
                 writer.write("")
-                writer.declareSection(ServiceInterfaceCompanionObject) {
+                writer.declareSection(
+                    ServiceInterfaceCompanionObject,
+                    context = mapOf(ServiceInterfaceCompanionObject.ServiceSymbol to serviceSymbol)
+                ) {
                     renderCompanionObject()
                 }
                 writer.write("")
@@ -85,20 +93,25 @@ class ServiceGenerator(private val ctx: RenderingContext<ServiceShape>) {
      * e.g.
      * ```
      * companion object {
-     *     fun build(block: Configuration.() -> Unit = {}): LambdaClient {
-     *         val config = Configuration().apply(block)
+     *     operator fun invoke(block: Config.DslBuilder.() -> Unit = {}): LambdaClient {
+     *         val config = Config.BuilderImpl().apply(block).build()
      *         return DefaultLambdaClient(config)
      *     }
+     *
+     *     operator fun invoke(config: Config): LambdaClient = DefaultLambdaClient(config)
      * }
      * ```
      */
     private fun renderCompanionObject() {
-        writer.openBlock("companion object {")
-            .openBlock("operator fun invoke(block: Config.DslBuilder.() -> Unit = {}): ${serviceSymbol.name} {")
-            .write("val config = Config.BuilderImpl().apply(block).build()")
-            .write("return Default${serviceSymbol.name}(config)")
-            .closeBlock("}")
-            .closeBlock("}")
+        writer.withBlock("companion object {", "}") {
+            withBlock("operator fun invoke(block: Config.DslBuilder.() -> Unit = {}): ${serviceSymbol.name} {", "}") {
+                write("val config = Config.BuilderImpl().apply(block).build()")
+                write("return Default${serviceSymbol.name}(config)")
+            }
+
+            write("")
+            write("operator fun invoke(config: Config): ${serviceSymbol.name} = Default${serviceSymbol.name}(config)")
+        }
     }
 
     private fun importExternalSymbols() {