Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 5 additions & 0 deletions .changes/13a47747-197a-4b88-bc3b-393af5f5127a.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"id": "13a47747-197a-4b88-bc3b-393af5f5127a",
"type": "bugfix",
"description": "Correctly generate paginators for item type names which collide with other used types (e.g., an item type `com.foo.Flow` which conflicts with `kotlinx.coroutines.flow.Flow`)"
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,7 @@ import software.amazon.smithy.codegen.core.CodegenException
import software.amazon.smithy.codegen.core.Symbol
import software.amazon.smithy.codegen.core.SymbolReference
import software.amazon.smithy.kotlin.codegen.KotlinSettings
import software.amazon.smithy.kotlin.codegen.core.CodegenContext
import software.amazon.smithy.kotlin.codegen.core.ExternalTypes
import software.amazon.smithy.kotlin.codegen.core.KotlinDelegator
import software.amazon.smithy.kotlin.codegen.core.KotlinWriter
import software.amazon.smithy.kotlin.codegen.core.defaultName
import software.amazon.smithy.kotlin.codegen.core.withBlock
import software.amazon.smithy.kotlin.codegen.core.*
import software.amazon.smithy.kotlin.codegen.integration.KotlinIntegration
import software.amazon.smithy.kotlin.codegen.lang.KotlinTypes
import software.amazon.smithy.kotlin.codegen.model.*
Expand Down Expand Up @@ -54,7 +49,7 @@ class PaginatorGenerator : KotlinIntegration {
paginatedOperations.forEach { paginatedOperation ->
val paginationInfo = paginatedIndex.getPaginationInfo(service, paginatedOperation).getOrNull()
?: throw CodegenException("Unexpectedly unable to get PaginationInfo from $service $paginatedOperation")
val paginationItemInfo = getItemDescriptorOrNull(paginationInfo, ctx)
val paginationItemInfo = getItemDescriptorOrNull(paginationInfo, ctx, writer)

renderPaginatorForOperation(ctx, writer, paginatedOperation, paginationInfo, paginationItemInfo)
}
Expand Down Expand Up @@ -264,7 +259,11 @@ private data class ItemDescriptor(
/**
* Return an [ItemDescriptor] if model supplies, otherwise null
*/
private fun getItemDescriptorOrNull(paginationInfo: PaginationInfo, ctx: CodegenContext): ItemDescriptor? {
private fun getItemDescriptorOrNull(
paginationInfo: PaginationInfo,
ctx: CodegenContext,
writer: KotlinWriter,
): ItemDescriptor? {
val itemMemberId = paginationInfo.itemsMemberPath?.lastOrNull()?.target ?: return null

val itemLiteral = paginationInfo.itemsMemberPath!!.last()!!.defaultName()
Expand All @@ -273,15 +272,18 @@ private fun getItemDescriptorOrNull(paginationInfo: PaginationInfo, ctx: Codegen
val isSparse = itemMember.isSparse
val (collectionLiteral, targetMember) = when (itemMember) {
is MapShape -> {
val symbol = ctx.symbolProvider.toSymbol(itemMember)
val entryExpression = symbol.expectProperty(SymbolProperty.ENTRY_EXPRESSION) as String
entryExpression to itemMember
val keySymbol = ctx.symbolProvider.toSymbol(itemMember.key)
val valueSymbol = ctx.symbolProvider.toSymbol(itemMember.value)
val valueSuffix = if (isSparse || valueSymbol.isNullable) "?" else ""
val elementExpression = writer.format("Map.Entry<#T, #T#L>", keySymbol, valueSymbol, valueSuffix)
elementExpression to itemMember
}
is CollectionShape -> {
val target = ctx.model.expectShape(itemMember.member.target)
val symbol = ctx.symbolProvider.toSymbol(target)
val literal = symbol.name + if (symbol.isNullable || isSparse) "?" else ""
literal to target
val suffix = if (isSparse || symbol.isNullable) "?" else ""
val elementExpression = writer.format("#T#L", symbol, suffix)
elementExpression to target
}
else -> error("Unexpected shape type ${itemMember.type}")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,130 @@ class PaginatorGeneratorTest {
actual.shouldContainOnlyOnceWithDiff(expectedImports)
}

@Test
fun testRenderPaginatorWithItemRequiringFqName() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Fq -> FullyQualified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about just Full instead?

@Test
fun testRenderPaginatorWithItemRequiringFullName() {

val testModelWithItems = """
namespace com.test

use aws.protocols#restJson1

service FlowService {
operations: [ListFlows]
}

@paginated(
inputToken: "Marker",
outputToken: "NextMarker",
pageSize: "MaxItems",
items: "Flows"
)
@readonly
@http(method: "GET", uri: "/flows", code: 200)
operation ListFlows {
input: ListFlowsRequest,
output: ListFlowsResponse
}

structure ListFlowsRequest {
@httpQuery("FlowVersion")
FlowVersion: String,
@httpQuery("Marker")
Marker: String,
@httpQuery("MasterRegion")
MasterRegion: String,
@httpQuery("MaxItems")
MaxItems: Integer
}

structure ListFlowsResponse {
Flows: FlowList,
NextMarker: String
}

list FlowList {
member: Flow
}

structure Flow {
Name: String
}
""".toSmithyModel()
val testContextWithItems = testModelWithItems.newTestContext("FlowService", "com.test")

val codegenContextWithItems = object : CodegenContext {
override val model: Model = testContextWithItems.generationCtx.model
override val symbolProvider: SymbolProvider = testContextWithItems.generationCtx.symbolProvider
override val settings: KotlinSettings = testContextWithItems.generationCtx.settings
override val protocolGenerator: ProtocolGenerator = testContextWithItems.generator
override val integrations: List<KotlinIntegration> = testContextWithItems.generationCtx.integrations
}

val unit = PaginatorGenerator()
unit.writeAdditionalFiles(codegenContextWithItems, testContextWithItems.generationCtx.delegator)

testContextWithItems.generationCtx.delegator.flushWriters()
val testManifest = testContextWithItems.generationCtx.delegator.fileManifest as MockManifest
val actual = testManifest.expectFileString("src/main/kotlin/com/test/paginators/Paginators.kt")

val expectedCode = """
/**
* Paginate over [ListFlowsResponse] 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.
* @param initialRequest A [ListFlowsRequest] to start pagination
* @return A [kotlinx.coroutines.flow.Flow] that can collect [ListFlowsResponse]
*/
public fun TestClient.listFlowsPaginated(initialRequest: ListFlowsRequest = ListFlowsRequest { }): kotlinx.coroutines.flow.Flow<ListFlowsResponse> =
Copy link
Contributor

Choose a reason for hiding this comment

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

question: These will only be fully qualified kotlinx.coroutines.flow.Flow when there's an import collision, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, although typically kotlinx.coroutines.flow.Flow will be referenced first in our codegen and so a conflicting symbol like com.foo.Flow would actually be the one which gets the fully-qualified name in code. This test case shows it the other way around because the very first paginator is the one which has the collision and the model shape is referenced before kotlinx.coroutines.flow.Flow.

flow {
var cursor: kotlin.String? = initialRequest.marker
var hasNextPage: Boolean = true

while (hasNextPage) {
val req = initialRequest.copy {
this.marker = cursor
}
val result = [email protected](req)
cursor = result.nextMarker
hasNextPage = cursor?.isNotEmpty() == true
emit(result)
}
}

/**
* Paginate over [ListFlowsResponse] 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.
* @param block A builder block used for DSL-style invocation of the operation
* @return A [kotlinx.coroutines.flow.Flow] that can collect [ListFlowsResponse]
*/
public fun TestClient.listFlowsPaginated(block: ListFlowsRequest.Builder.() -> Unit): kotlinx.coroutines.flow.Flow<ListFlowsResponse> =
listFlowsPaginated(ListFlowsRequest.Builder().apply(block).build())

/**
* This paginator transforms the flow returned by [listFlowsPaginated]
* to access the nested member [Flow]
* @return A [kotlinx.coroutines.flow.Flow] that can collect [Flow]
*/
@JvmName("listFlowsResponseFlow")
public fun kotlinx.coroutines.flow.Flow<ListFlowsResponse>.flows(): kotlinx.coroutines.flow.Flow<Flow> =
transform() { response ->
response.flows?.forEach {
emit(it)
}
}
""".trimIndent()

actual.shouldContainOnlyOnceWithDiff(expectedCode)
}

@Test
fun testRenderPaginatorWithSparseItem() {
val testModelWithItems = """
Expand Down
Loading