Paginator codegen using Kotlin Flow #557

kggilmer · 2021-12-21T01:23:27Z

Issue #

#124

Description of changes

Generate paginator extension functions for paginated operations
Use the kotlinx-coroutines flow abstraction
Reorganize compile-tests and benchmarks into common /tests/ folder
Use benchmark codegen strategy to codegen paginators and then test them

Testing done

Built against all service SDKs, verify all services compile
Write and written tests against dynamodb, ensure that functions return expected data

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

…ame. Minor cleanup and additional tests.

aajtodd

Nice!

Minor fixes/suggestions. Overall looks great

aajtodd · 2022-01-06T15:52:47Z

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

+ * https://awslabs.github.io/smithy/1.0/spec/core/behavior-traits.html#paginated-trait for details.
+ */
+class PaginatorGenerator : KotlinIntegration {
+    private val kotlinxFlowSymbol = "kotlinx.coroutines.flow.Flow".toSymbol()


fix

Move to RuntimeTypes (or similar).

Also need to add a dependency on kotlinx.coroutines. We can either do that by defining it in KotlinDependency or alternatively we can add an api("kotlinx.coroutines:$coroutinesVersion") to runtime-core/build.gradle.kts for managing the dependency (since runtime-core is basically guaranteed to always be pulled in). This may be favorable to ensure the version stays in sync with the runtime.

I created a new object to house external types, I think it makes sense to keep them separate from our own runtime types. Regarding the dependency aspect, yeah we had discussed this but then I found that everything works as is so was reluctant to add it without knowing why it's needed. I notice that the build file you mention already expresses a dependency on coroutines-core, which is where Flow and friends live. Is there something I'm missing other than switching from implementation to api?

aajtodd · 2022-01-06T15:54:37Z

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

+        val service = ctx.model.expectShape<ServiceShape>(ctx.settings.service)
+        val paginatedIndex = PaginatedIndex.of(ctx.model)
+
+        delegator.useFileWriter("Paginators.kt", "${ctx.settings.pkg.name}.paginator") { writer ->


question

Should we use paginator/waiter or paginators/waiters as the package name?

personally I don't see the value of the nested package. But as we have opted to do this, singular requires one less byte so that would be my vote.

Plural better matches conventions in the ecosystem (e.g., kotlin.sequences, kotlin.streams, kotlin.coroutines, etc.) so paginators gets my vote. We could also name it pagination in the theory that there may be additional helper classes in the future.

aajtodd · 2022-01-06T15:56:18Z

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

+                .filter { operationShape -> operationShape.hasTrait(PaginatedTrait.ID) }
+
+            paginatedOperations.forEach { paginatedOperation ->
+                val paginationInfo = paginatedIndex.getPaginationInfo(service, paginatedOperation).getOrNull()


style

Doesn't optional have orElseThrow { } ?

It does but it doesn't throw a CodegenException nor does it provide a way of setting a message. I think this is better as is.

Right no I want to keep CodegenException, it looked to me like you can supply your own

val paginationInfo = paginatedIndex.getPaginationInfo(...) .orElseThrow { CodegenException(...) }

What you have is fine though.

aajtodd · 2022-01-06T16:22:09Z

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

+        val markerLiteral = paginationInfo.inputTokenMember.defaultName()
+
+        writer.write("")
+        writer.dokka("Paginate over [${outputSymbol.name}]")


We could probably come up with more complete documentation.

just spit balling

/** * Paginate over [Client.Operation] results. * When this operation is called, a [kotlinx.coroutines.Flow] is created. Flows are lazy (cold) so no service calls are * made until the flow is collected. This also means there is no guarantee that the request is valid until then. Once * you start collecting the flow, the SDK will lazily load response pages by making service calls until there are no * pages left or the flow is cancelled. If there are errors in your request, you will see the failures only after you start * collection. * * @return a flow of [outputSymbol] */ fun Client.fooPaginated(...): Flow<..> { ... }

aajtodd · 2022-01-06T16:27:55Z

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

+                inputSymbol,
+                outputSymbol
+            ) {
+                withBlock("return flow {", "}") {


this could just be = flow { from the outer block with the function definition (L125)

aajtodd · 2022-01-06T16:42:11Z

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

+
+tasks["smithyBuildJar"].enabled = false
+
+val codegen by configurations.creating


you don't need this or any of the stuff in generateSdk that uses it. There is no custom codegen component for this project

Seems to fail at gradle configuration if I remove it.

aajtodd · 2022-01-06T16:45:55Z

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

+val codegenSourceInfo = listOf("paginator-tests").map{ CodegenSourceInfo(it) }
+
+
+val stageGeneratedSources = tasks.register("stageGeneratedSources") {


if you're leaving it in the buildDir we could just avoid any copying at all and include the projection dir directly on L52

I'll look into this in a subsequent refactor of project from kmp to jvm.

aajtodd · 2022-01-06T16:48:18Z

tests/codegen/paginator-tests/jvm/src/DefaultLambdaClient.kt

+ * each dot represents a service round trip.  Within each service round trip a list of [FunctionConfiguration]
+ * is generated including the round trip index.
+ */
+class DefaultLambdaClient(config: LambdaClient.Config) : LambdaClient {


question

Won't this conflict with the generated DefaultLambdaClient? Or is it not generating one because no protocol generator is found?

Either way we should probably rename this to TestLambdaClient or something to remove ambiguity.

Also LambdaClient.Config looks unused, no need to keep it.

Won't this conflict with the generated DefaultLambdaClient? Or is it not generating one because no protocol generator is found?

Yes. In fact the test client must be called this due to reference to the type name in LambdaClient.

LambdaClient.Config is also required for compilation to succeed

I see, can we document what is happening here because it was confusing to me at first glance. Something to the effect of The default client is not generated because no protocol generator is found/available, we supply a hand written version instead suitable for these tests

Not something in this PR but it seems like we should maybe make the generated clients not depend on DefaultLambdaClient. IDK I mean I guess in practice it should normally always be there.

Added a note to describe the situation as requested.

seems like we should maybe make the generated clients not depend on DefaultLambdaClient.

Yeah I think this is the rough edge of where a proper Smithy SDK would be designed and built.

Verified that I can address these concerns after #564 is merged.

aajtodd · 2022-01-06T16:49:37Z

tests/codegen/paginator-tests/jvm/src/DefaultLambdaClient.kt

+ */
+class DefaultLambdaClient(config: LambdaClient.Config) : LambdaClient {
+    override val config: LambdaClient.Config
+        get() = TODO("Unneeded for test")


I think we wanted to leave TODO for real todos. Move to error()

aajtodd · 2022-01-06T16:52:14Z

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

+                itemDesc.collectionLiteral
+            ) {
+                withBlock("transform() { response -> ", "}") {
+                    withBlock("response.#L?.forEach {", "}", itemDesc.itemPathLiteral) {


question

Is this correct for maps? We end up with an Entry in that case?

Yes, we get a Map.Entry. Here is a sample from apigateway:

fun Flow<GetUsageResponse>.items(): Flow<Map.Entry<String, List<List<Long>>>> = transform() { response -> response.items?.forEach { emit(it) } }

ianbotsf · 2022-01-07T15:43:14Z

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

+        val service = ctx.model.expectShape<ServiceShape>(ctx.settings.service)
+        val paginatedIndex = PaginatedIndex.of(ctx.model)
+
+        delegator.useFileWriter("Paginators.kt", "${ctx.settings.pkg.name}.paginator") { writer ->


Plural better matches conventions in the ecosystem (e.g., kotlin.sequences, kotlin.streams, kotlin.coroutines, etc.) so paginators gets my vote. We could also name it pagination in the theory that there may be additional helper classes in the future.

ianbotsf · 2022-01-07T15:58:01Z

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

+                inputSymbol,
+                outputSymbol
+            ) {
+                withBlock("flow {", "}") {


Nit: Seems like this could be on the previous line: fun #T.#LPaginated(initialRequest: #T): Flow<#T> = flow {

I think the following line will be more readable as the function names may end up quite long for some services (where the #L expands). Knowing that the body is a Flow is critical to understanding the function and if it's off screen that's harder to see.

ianbotsf · 2022-01-07T16:03:10Z

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

+                    write("var cursor: #F = null", cursorSymbol)
+                    write("var isFirstPage: Boolean = true")
+                    write("")
+                    withBlock("while (isFirstPage || (cursor?.isNotEmpty() == true)) {", "}") {


Question: The cursor symbol is detected above but isNotEmpty() seems like it can only apply to specific types. Will this always be a string? If so, do we need to go through the trouble of keeping the cursor type dynamic?

Will this always be a string?

Yes, according to the Smithy spec on the paginated trait.

If so, do we need to go through the trouble of keeping the cursor type dynamic?

This is simply using the model to generate it's specified type. What do you find troublesome, the action to retrieve the type of the member, using a variable to store that, or the notation in CodeWriter?

All of the above. In cases where we know the one-and-only type of a variable, we typically hardcode that (e.g., var isFirstPage: Boolean). The resulting codegen code is much more readable.

is cursor the next token? This is not always a string. See:

"com.amazonaws.dynamodb#Key": { "type": "map", "key": { "target": "com.amazonaws.dynamodb#AttributeName" }, "value": { "target": "com.amazonaws.dynamodb#AttributeValue" } },

which is used as a token for DynamoDb::scan

Good callout, tracking issue: #565

ianbotsf · 2022-01-07T16:07:20Z

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

+        writer.write("")
+        writer.dokka(
+            """
+                 This paginator transforms the flow returned by [${operationShape.defaultName()}Paginated] to access the nested member [${itemDesc.targetMember.defaultName(serviceShape)}]


Style: Line length > 120 chars.

ianbotsf · 2022-01-07T16:08:37Z

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

+        writer.dokka(
+            """
+                 This paginator transforms the flow returned by [${operationShape.defaultName()}Paginated] to access the nested member [${itemDesc.targetMember.defaultName(serviceShape)}]
+                 @return a [kotlinx.coroutines.flow.Flow] that can collect [${itemDesc.targetMember.defaultName(serviceShape)}]
+            """.trimIndent()
+        )
+        writer.dokka("Paginate over [${outputSymbol.name}.${itemDesc.itemLiteral}]")


Question: Why two writer.dokkas?

whoops forgot to delete the original.. ナイスキャッチ

ianbotsf · 2022-01-07T16:14:12Z

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

+            // NOTE: This does not mean these functions are callable from Java.
+            .write(
+                """@JvmName("#L#L")""",
+                outputSymbol.name.toggleFirstCharacterCase(),


Suggestion: Explicitly casting the first character to lowercase would clarify the intent here.

This?

outputSymbol.name.replaceFirstChar { if (it.isLowerCase()) it.titlecase(Locale.getDefault()) else it.toString() }

I'm confused...do we want this to be uppercase? That's not normally how Java/Kotlin methods are named. Assuming not, I think this captures the intent more clearly:

outputSymbol.name.replaceFirstChar(Char::lowercaseChar)

kggilmer · 2022-01-07T17:20:54Z

Plural better matches conventions in the ecosystem (e.g., kotlin.sequences, kotlin.streams, kotlin.coroutines, etc.) so paginators gets my vote.

I agree platform consistency wins, will change.

sonarqubecloud · 2022-01-10T18:23:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
17 Code Smells

No Coverage information
0.3% Duplication

kggilmer added 3 commits December 20, 2021 14:53

Update design doc

ae2d63d

Integrate code from dev branch

5390324

Add codegen unit test

424c8cd

kggilmer requested review from aajtodd and ianbotsf December 21, 2021 01:24

lint

c778838

kggilmer changed the title ~~Paginator codegen using Koltin Flow~~ Paginator codegen using Kotlin Flow Dec 21, 2021

kggilmer added 3 commits December 29, 2021 14:23

Update to use 'items()' for nested item pagination over using model n…

db4f97d

…ame. Minor cleanup and additional tests.

Merge branch 'main' into feat-flow-paginator

4da4faf

Fix tests from codegen updates

d7535a3

kggilmer force-pushed the feat-flow-paginator branch from 37a25e8 to d7535a3 Compare January 6, 2022 15:36

kggilmer marked this pull request as ready for review January 6, 2022 15:37

kggilmer requested a review from a team as a code owner January 6, 2022 15:37

aajtodd suggested changes Jan 6, 2022

View reviewed changes

kggilmer added 5 commits January 6, 2022 18:07

Updates to paginator codegen based on PR feedback

2efff80

Update tests for codegen changes

c8a5574

tweaks from PR feedback

586ddcf

placate the linter

aa07d17

Merge branch 'main' into feat-flow-paginator

d9dbd56

ianbotsf approved these changes Jan 7, 2022

View reviewed changes

kggilmer added 4 commits January 7, 2022 10:41

Fix build issue in codegen test dependency. Tweaks from PR feedback.

83648a3

cleanup

5c336fe

Suppress warnings in compile tests

641d3b3

Fix new bug from recent update

5df147c

aajtodd approved these changes Jan 10, 2022

View reviewed changes

kggilmer and others added 2 commits January 10, 2022 10:00

Merge branch 'main' into feat-flow-paginator

4b21e64

Add test to verify empty-string token use case

983d297

kggilmer merged commit 1197943 into main Jan 10, 2022

kggilmer deleted the feat-flow-paginator branch January 10, 2022 18:44


		tasks["smithyBuildJar"].enabled = false

		val codegen by configurations.creating

		val codegenSourceInfo = listOf("paginator-tests").map{ CodegenSourceInfo(it) }


		val stageGeneratedSources = tasks.register("stageGeneratedSources") {

Paginator codegen using Kotlin Flow #557

Paginator codegen using Kotlin Flow #557

Conversation

kggilmer commented Dec 21, 2021 • edited Loading

Issue #

Description of changes

Testing done

aajtodd left a 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

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

kggilmer Jan 7, 2022 • edited Loading

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

kggilmer commented Jan 7, 2022

sonarqubecloud bot commented Jan 10, 2022

kggilmer commented Dec 21, 2021 •

edited

Loading

kggilmer Jan 7, 2022 •

edited

Loading