refactor: provide structure to client configuration #778

aajtodd · 2023-01-13T15:21:22Z

Issue #

N/A

Description of changes

This PR refactors client configuration to provide a more structured approach to the way it is generated.

Proposes a new convention doc for client configuration that describes how new configurations should be added and in general how they are used by the SDK. This isn't a normal design doc but a convention/guidelines doc.
Define new builder interfaces for specific domain configurations (e.g. HttpClientConfig.Builder, TracingClientConfig.Builder, etc). Previously these configuration interfaces were inherited on the generated Config type but not on the configuration builder. This is pre-requisite for runtime plugins to work because without it every Config.Builder generated shares nothing with any other client. This prevents any kind of common code (e.g. like a plugin) to configure a service client without knowing the concrete type of client.
Rename ServiceGenerator to ServiceClientGenerator
Refactor ClientConfigGenerator into an abstract base class that makes less assumptions about the type of configuration being generated.
Generate a builder for service clients
Introduce new runtime types that generated service client companion objects inherit from. This moves the DSL operator invoke function into the runtime.
Introduce an abstract base class for service client builders. This will allow us to inject code into all clients instantiated (e.g. load plugins using SPI is one use case).
Moved a few things around (e.g. everything in core config package moved to the client subpackage).
Refactors RuntimeTypes to reduce boilerplate
Refactor our section generator abstraction to be type safe when getting section keys

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

…t package

ianbotsf · 2023-01-17T18:10:31Z

docs/design/client-configuration.md

@@ -0,0 +1,336 @@
+# Client Configuration
+
+* **Type**: Convention


Question: What's the difference between a convention and a design? Isn't convention merely ex-post facto design?

Aren't we basically in "ex-post facto" phase at this point w.r.t to config?

We certainly are. I suppose my point was this feels like a design even if it's being written after the fact. It's even in the docs/design directory. I'm not sure what I would do differently after reading this document knowing it's a "convention" vs a "design".

If there's a clear difference in your mind we can leave it but this wording felt novel and conspicuous.

ianbotsf · 2023-01-17T18:12:43Z

docs/design/client-configuration.md

+This section describes general conventions and guidelines to be used for service client configuration. See the section
+below for concrete example of these conventions in practice.


Nit: "for a concrete example"

ianbotsf · 2023-01-17T18:17:49Z

docs/design/client-configuration.md

+This section describes general conventions and guidelines to be used for service client configuration. See the section
+below for concrete example of these conventions in practice.
+
+* All generated service clients inherit from `SdkClientConfig.Builder`


Correctness: This statement isn't true. Perhaps this was meant to be one/all of the following:

All generated service clients implement SdkClient

All generated service client configs implement SdkClientConfig

All generated service client config builders implement SdkClientConfig.Builder

ianbotsf · 2023-01-17T18:25:52Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/client/IdempotencyTokenProvider.kt

 * SPDX-License-Identifier: Apache-2.0
 */
-package aws.smithy.kotlin.runtime.config
+package aws.smithy.kotlin.runtime.client


Correctness: Looks like the src classes changed packages but not the test class (and as a result the unit test no longer runs for me).

ianbotsf · 2023-01-17T18:28:43Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/client/IdempotencyTokenProvider.kt

-    public val idempotencyTokenProvider: IdempotencyTokenProvider?
+    public val idempotencyTokenProvider: IdempotencyTokenProvider
+
+    public interface Builder {


Nit: Missing KDocs on public interface.

ianbotsf · 2023-01-17T21:25:52Z

.../main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/util/AbstractConfigGenerator.kt

+ * Re-usable base class for generating some type that only contains configuration.
+ * e.g. roughly something shaped like below
+ *
+ * ```


Nit: ```kotlin

I don't think this matters for KDoc FWIW

Oh great, nevermind then.

ianbotsf · 2023-01-17T21:40:49Z

.../main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/util/AbstractConfigGenerator.kt

+        val formattedBaseClasses = if (baseClasses.isNotEmpty()) ": $baseClasses" else ""
+        writer.openBlock("public class #configClass.name:L private constructor(builder: Builder)$formattedBaseClasses {")


Nit: Missing a space before :. Ideally the class spec should look like:

public class FooConfig private constructor(builder: Builder) : BaseClass1, BaseClass2

ianbotsf · 2023-01-17T21:42:12Z

.../main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/util/AbstractConfigGenerator.kt

+        writer.openBlock("public class #configClass.name:L private constructor(builder: Builder)$formattedBaseClasses {")
+            .call { renderImmutableProperties(sortedProps, writer) }
+            .call { renderCompanionObject(writer) }
+            .call { renderBuilder(sortedProps, writer) }
+            .closeBlock("}")


Nit: Can replace openBlock/closeBlock with withBlock.

ianbotsf · 2023-01-17T21:44:58Z

.../main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/util/AbstractConfigGenerator.kt

+        val baseClasses = props
+            .mapNotNull { it.builderBaseClass?.name }
+            .sorted()
+            .toSet()
+            .joinToString(", ")
+
+        val formattedBaseClasses = if (baseClasses.isNotEmpty()) ": $baseClasses" else ""


Suggestion: Duplicated code, consider extracting to helper method.

It's nearly duplicated, uses different field from the property though. I'll take a look at cleaning it up.

ianbotsf · 2023-01-17T21:47:05Z

...degen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/util/ConfigProperty.kt

+ *
+ * e.g.
+ *
+ * ```


Nit: ```kotlin

lauzadis

Looks good! I just had some non-blocking comments.

lauzadis · 2023-01-18T21:53:10Z

docs/design/client-configuration.md

+## Client Creation Patterns
+
+This section describes in more detail how service clients are created and configured using the `BazClient` described
+in [example service client](#example-service-client) section. These behaviors are a combination of runtime types and


nit: "in the [example service client]"

lauzadis · 2023-01-18T21:55:29Z

docs/design/client-configuration.md

+```kotlin
+// explicit using DSL syntax inherited from companion `SdkClientFactory`
+val c1 = BazClient { // this: BazClient.Config.Builder
+    sdkLogMode = SdkLogMode.LogRequests


nit: the SdkLogMode enum value is LogRequest instead of LogRequests

lauzadis · 2023-01-18T22:10:18Z

...en/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/ServiceClientGenerator.kt

+
        /**
-         * Context key for the service symbol
+         * SectionId used when rendering the service client builder


nit: consider adding brackets around [SectionId] which adds a link to the definition, helpful when working in an IDE

ianbotsf · 2023-01-19T17:24:18Z

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

    private fun clientContextConfigProps(trait: ClientContextParamsTrait): List<ConfigProperty> = buildList {
-        trait.parameters.forEach { (k, v) ->
-            add(
+        trait
+            .parameters
+            .map { (k, v) ->


Correctness: Remove buildList. As written, this always returns an empty list.

NICE catch!

sonarqubecloud · 2023-01-19T19:06:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
24 Code Smells

No Coverage information
0.0% Duplication

aajtodd added 13 commits January 9, 2023 16:27

refactor: introduce client and config builders; relocate all to clien…

18d69d3

…t package

add builders for tracign and http configs

107d0b2

make section data typed; add new service client builder

fc9f077

fix base classes and minor codegen fixes

a8e299a

refactor config property generation into something reusable

d235ed9

refactor service config generator to work with new base class

e9e6cbc

cleanup runtime types

569cac8

fix service config generation to match runtime

276aecf

add JvmStatic annotation

5b7fd4d

fix visibility

b24b375

fix JvmStatic import and various docs

3747bf7

add convention doc

33f290b

add changelog

cce979c

aajtodd requested a review from a team as a code owner January 13, 2023 15:21

aajtodd mentioned this pull request Jan 13, 2023

refactor: track upstream client configuration changes aws/aws-sdk-kotlin#814

Merged

ianbotsf requested changes Jan 17, 2023

View reviewed changes

aajtodd added 4 commits January 18, 2023 10:47

cleanup

9798b1e

fix missing imports

60ee381

fix protocol tests

c6e2eee

duplicate docs

4a046a3

lauzadis approved these changes Jan 18, 2023

View reviewed changes

cleanup

93a2496

ianbotsf requested changes Jan 19, 2023

View reviewed changes

lucix-aws approved these changes Jan 19, 2023

View reviewed changes

fix context config props

691684b

ianbotsf approved these changes Jan 19, 2023

View reviewed changes

aajtodd merged commit 36e612c into main Jan 20, 2023

aajtodd deleted the refactor-client-config branch January 20, 2023 13:21

		@@ -0,0 +1,336 @@
		# Client Configuration

		* Type: Convention

		This section describes general conventions and guidelines to be used for service client configuration. See the section
		below for concrete example of these conventions in practice.

		val formattedBaseClasses = if (baseClasses.isNotEmpty()) ": $baseClasses" else ""
		writer.openBlock("public class #configClass.name:L private constructor(builder: Builder)$formattedBaseClasses {")

refactor: provide structure to client configuration #778

refactor: provide structure to client configuration #778

Uh oh!

Conversation

aajtodd commented Jan 13, 2023

Issue #

Description of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lauzadis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jan 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants