initial implementation of codegen for low-level operations/types #1357

ianbotsf · 2024-07-10T23:42:04Z

Issue #

Part of #76

Description of changes

At long last, the wait for codegenned mapper operations and high-level types is ~~done~~ started! This change removes the handwritten toy implementation of a getItem operation and now generates the operation glue code for the following DDB ops:

deleteItem
getItem
putItem
query*
scan*

* These operations should be paginated but the codegen doesn't handle that yet. It's left as a FIXME in the code for follow-up.

PR flight plan

⚠️ There's a lot to take in here, especially if you're unfamiliar with KSP. I recommend taking a short break to learn some basics about KSP before getting started with the review.

Recommended order of review:

Start with the new ddb-mapper-ops-codegen module, in particular:
- HighLevelOpsProcessor which is the main entry point for KSP. Everything else in this module is called somehow from this point.
- The model package is for data types for describing the low- & high-level operations, structures, and types
- The core package contains rudimentary code writing utilities such as indentation, blocks, imports, and template processing. These are good candidates for commonizing somewhere in the future, since we'll likely need similar functionality for DDB mapper annotation processing and possibly other HLLs in the future.
- The rendering package contains structured codegen for rendering the operations, convenience functions, and types
Next, hit up the dynamodb-mapper module, in particular:
- build.gradle.kts has been beefed up with the new KSP application of ddb-mapper-ops-codegen. Some unpleasant (and hopefully temporary) hacks were necessary to get KSP to work in both JVM+Native builds and JVM-only builds.
- The new unit tests in the operations package which use DynamoDB Local in lieu of mocking or making actual calls to DDB

Sample codegen output

Understanding the codegen may be easier if you see the generated code. All code is generated into the hll/ddb-mapper/dynamodb-mapper/build/generated/ksp/common/commonMain/kotlin directory.

GetItem operation

// Code generated by ddb-mapper-ops-codegen. DO NOT EDIT!

package aws.sdk.kotlin.hll.dynamodbmapper.operations

import aws.sdk.kotlin.hll.dynamodbmapper.TableSpec
import aws.sdk.kotlin.hll.dynamodbmapper.items.ItemSchema
import aws.sdk.kotlin.hll.dynamodbmapper.model.toItem
import aws.sdk.kotlin.hll.dynamodbmapper.pipeline.internal.HReqContextImpl
import aws.sdk.kotlin.hll.dynamodbmapper.pipeline.internal.MapperContextImpl
import aws.sdk.kotlin.hll.dynamodbmapper.pipeline.internal.Operation
import aws.sdk.kotlin.services.dynamodb.model.ConsumedCapacity
import aws.sdk.kotlin.services.dynamodb.model.ReturnConsumedCapacity
import kotlin.Boolean
import kotlin.String
import aws.sdk.kotlin.services.dynamodb.model.GetItemRequest as LowLevelGetItemRequest
import aws.sdk.kotlin.services.dynamodb.model.GetItemResponse as LowLevelGetItemResponse

public interface GetItemRequest<T> {
    public companion object { }

    public val consistentRead: Boolean?
    public val key: T?
    public val returnConsumedCapacity: ReturnConsumedCapacity?
}

private data class GetItemRequestImpl<T>(
    override val consistentRead: Boolean?,
    override val key: T?,
    override val returnConsumedCapacity: ReturnConsumedCapacity?,
): GetItemRequest<T>

public fun <T> GetItemRequest(
    consistentRead: Boolean?,
    key: T?,
    returnConsumedCapacity: ReturnConsumedCapacity?,
): GetItemRequest<T> = GetItemRequestImpl(
    consistentRead,
    key,
    returnConsumedCapacity,
)

private fun <T> GetItemRequest<T>.convert(
    tableName: String?, 
    schema: ItemSchema<T>,
) = LowLevelGetItemRequest {
    consistentRead = this@convert.consistentRead
    returnConsumedCapacity = this@convert.returnConsumedCapacity
    this@convert.key?.let { key = schema.converter.toItem(it, schema.keyAttributeNames) }
    this.tableName = tableName
}

public interface GetItemResponse<T> {
    public companion object { }

    public val consumedCapacity: ConsumedCapacity?
    public val item: T?
}

private data class GetItemResponseImpl<T>(
    override val consumedCapacity: ConsumedCapacity?,
    override val item: T?,
): GetItemResponse<T>

public fun <T> GetItemResponse(
    consumedCapacity: ConsumedCapacity?,
    item: T?,
): GetItemResponse<T> = GetItemResponseImpl(
    consumedCapacity,
    item,
)

private fun <T> LowLevelGetItemResponse.convert(schema: ItemSchema<T>) = GetItemResponse<T>(
    consumedCapacity = this@convert.consumedCapacity,
    item = this@convert.item?.toItem()?.let(schema.converter::fromItem),
)

internal fun <T> getItemOperation(table: TableSpec<T>) = Operation(
    initialize = { hReq: GetItemRequest<T> -> HReqContextImpl(hReq, table.schema, MapperContextImpl(table, "GetItem")) },
    serialize = { hReq, schema -> hReq.convert(table.name, schema) },
    lowLevelInvoke = table.mapper.client::getItem,
    deserialize = LowLevelGetItemResponse::convert,
    interceptors = table.mapper.config.interceptors,
)

Table operations interface/implementation

// Code generated by ddb-mapper-ops-codegen. DO NOT EDIT!

package aws.sdk.kotlin.hll.dynamodbmapper.operations

import aws.sdk.kotlin.hll.dynamodbmapper.TableSpec

/**
 * Provides access to operations on a particular table, which will invoke low-level operations after
 * mapping objects to items and vice versa
 * @param T The type of objects which will be read from and/or written to this table
 */
public interface TableOperations<T> {
    public suspend fun deleteItem(request: DeleteItemRequest<T>): DeleteItemResponse<T>
    public suspend fun getItem(request: GetItemRequest<T>): GetItemResponse<T>
    public suspend fun putItem(request: PutItemRequest<T>): PutItemResponse<T>
    public suspend fun query(request: QueryRequest<T>): QueryResponse<T>
    public suspend fun scan(request: ScanRequest<T>): ScanResponse<T>
}

internal class TableOperationsImpl<T>(private val tableSpec: TableSpec<T>) : TableOperations<T> {
    override suspend fun deleteItem(request: DeleteItemRequest<T>) =
        deleteItemOperation(tableSpec).execute(request)
    
    override suspend fun getItem(request: GetItemRequest<T>) =
        getItemOperation(tableSpec).execute(request)
    
    override suspend fun putItem(request: PutItemRequest<T>) =
        putItemOperation(tableSpec).execute(request)
    
    override suspend fun query(request: QueryRequest<T>) =
        queryOperation(tableSpec).execute(request)
    
    override suspend fun scan(request: ScanRequest<T>) =
        scanOperation(tableSpec).execute(request)
    
}

Known work remaining

Several TODOs and FIXMEs are to be found right now. At the very least I know we'll need to:

Replace function-style builders (e.g., for request/response creation) with DSL-style builders (similar to what we use in the low-level clients)
Render DSL-style extension methods for operations (similar to what we do for low-level operations)
Give KSP clearer indications of which output files depend on which input files. Right now, KSP can't detect which updates should trigger re-generating which code so it generates all the code on every build.
Generate paginated operations (e.g., Query, Scan, etc.) correctly

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

lauzadis · 2024-07-11T18:55:04Z

...ops-codegen/jvm/src/aws/sdk/kotlin/hll/dynamodbmapper/codegen/model/MemberCodegenBehavior.kt

+            member.type in attrMapTypes -> if (member.name == "key") MapKeys else MapAll
+            member.isTableName -> Hoist
+            else -> PassThrough
+        }.also { println("  ${member.name} is $it") }


nit: stray println

lauzadis · 2024-07-11T18:56:07Z

.../ddb-mapper-ops-codegen/jvm/src/aws/sdk/kotlin/hll/dynamodbmapper/codegen/model/Structure.kt

+        }
+    }
+
+    val hAttributes = llStructure.attributes + (ModelAttributes.LowLevelStructure to llStructure)


nit/naming: hlAttributes

lauzadis · 2024-07-11T20:52:09Z

hll/ddb-mapper/dynamodb-mapper/build.gradle.kts

+    // Start by invoking the JVM-only KSP configuration
+    dependencies.kspJvm(project(":hll:ddb-mapper:ddb-mapper-ops-codegen"))
+
+    // Then we need to move the generated source from jvm to common. Gradle lacks a move task so that means a copy...


Gradle lacks a move task so that means a copy...

Have you tried the File.renameTo() API?

🤦‍♂️ No, that's precisely what I was looking for and failed to find. I'll try it out and see.

lauzadis · 2024-07-11T21:23:51Z

hll/ddb-mapper/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/items/KeySpec.kt

 */
 public sealed interface KeySpec<in K> {
+    /**
+     * A [KeySpec] which for a [kotlin.ByteArray]-typed field


nit: unnecessary word "which", applies to other KDocs in this class

lauzadis · 2024-07-12T13:54:37Z

...ops-codegen/jvm/src/aws/sdk/kotlin/hll/dynamodbmapper/codegen/model/MemberCodegenBehavior.kt

+    data object MapAll : MemberCodegenBehavior
+    data object MapKeys : MemberCodegenBehavior
+    data object Drop : MemberCodegenBehavior
+    data object Hoist : MemberCodegenBehavior // FIXME Note sure this is useful...get rid of Hoist?


question: why is this only used for tableName members?

Hoist indicates that the field shouldn't appear in the request/response structure itself but should be "hoisted" to a different location in the overall API. In the case of tableName, that's required before you can even invoke an operation:

val table = mapper.getTable("the-table-name", ...) table.getItem(...)

As the FIXME notes, I'm not entirely sure this is a meaningful codegen behavior. Right now the effect is the same as Drop in that it's not included in the generated high-level structures. The presence of tableName in other APIs is a result of hand-written code, not codegen. I've left it here because it may be useful for non-table-centric operations such as BatchWriteItem, in which multiple table names may be specified.

I suppose the YAGNI principle dictates that this code shouldn't exist until there's a clear planned/actual use for it so perhaps I should just clean it up for now. 🤔

Wait no, I did actually make use of it! 😅 It's used when rendering the convert operation that turns low-level requests into high-level requests:

// generated code private fun <T> GetItemRequest<T>.convert( tableName: String?, schema: ItemSchema<T>, ) = LowLevelGetItemRequest { consistentRead = this@convert.consistentRead returnConsumedCapacity = this@convert.returnConsumedCapacity this@convert.key?.let { key = schema.converter.toItem(it, schema.keyAttributeNames) } this.tableName = tableName }

In that generated code the name/type tableName: String are derived from the low-level structure, not hand-written. So I believe I'll keep it as-is for now and remove the FIXME.

lauzadis · 2024-07-12T14:07:33Z

...db-mapper/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/internal/TableImpl.kt

+        Table.CompositeKey<T, PK, SK>,
+        TableSpec.CompositeKey<T, PK, SK> by specImpl,
+        TableOperations<T> by opsImpl {
+        override suspend fun getItem(partitionKey: PK, sortKey: SK) = TODO("Not yet implemented")


question: should these override suspend fun getItem be removed now that operations are codegenerated?

No, they should stay here and get implemented because they're convenience overloads of the codegenned method. This will allow users to do something simple like table.getItem(123) instead of table.getItem { key = SomeItem(id = 123) }. We may add other hand-written convenience overloads for codegenned methods but I knew for sure at least this one would be useful.

lauzadis · 2024-07-12T14:10:24Z

...pper/dynamodb-mapper/common/test/aws/sdk/kotlin/hll/dynamodbmapper/testutils/DdbLocalTest.kt

+                port = DDB_LOCAL_PORT
+            }
+
+            region = "us-west-2" // FIXME


question: What is the FIXME here? It seems like DDB Local requires a region:

The AWS SDKs for DynamoDB require that your application configuration specify an access key value and an AWS Region value. Unless you're using the -sharedDb or the -inMemory option, DynamoDB uses these values to name the local database file. These values don't have to be valid AWS values to run locally. However, you might find it convenient to use valid values so that you can run your code in the cloud later by changing the endpoint you're using.
https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/DynamoDBLocal.UsageNotes.html

Maybe we can update it to DUMMY like the other unnecessary parameters

lauzadis · 2024-07-12T14:11:30Z

...ynamodb-mapper/jvm/test/aws/sdk/kotlin/hll/dynamodbmapper/pipeline/internal/OperationTest.kt


 private const val TABLE_NAME = "foo-table"

+// FIXME Should be in commonTest but mockk is JVM-only and finding a good KMP mocking library is hard


Is it possible/useful to rewrite these tests and remove the mockk dependency? Do we need to use a mock to test this functionality?

Technically we never need a mocking framework to test functionality, they're merely convenient for stubbing out mock objects quickly and with minimal boilerplate. In this case, I'm using mockk to mock DynamoDbMapper (which will eventually have dozens of codegenned operations on it) and to spy on Interceptor (which also has many methods) and verify invocation order. All that is achievable with hand-written mocks but would increase the size of the test code.

This test shouldn't be platform-specific but also the behavior isn't platform-specific either—it's all common code. Thus, the risk of it working differently on non-JVM platforms is pretty low. All things being equal, I'd definitely prefer it in commonTest but since the tradeoff is more boilerplate for marginal test utility, I think I prefer leaving it here until we can find a good KMP mocking library.

What do you think?

I think it's fine to keep it JVM-only since the code is all common. We have other tests set up like this IIRC.

lauzadis · 2024-07-12T14:13:07Z

hll/ddb-mapper/ddb-mapper-ops-codegen/build.gradle.kts

Is there a reason for the naming inconsistencies between modules like ddb-mapper-* and dynamodb-mapper-*?

There's a reason, yes, but I'm not sure if it's a good one or not: modules named dynamodb-* are intended to be released publicly as Maven artifacts. Modules named ddb-* are intended to remain as build details and should not be released as Maven artifacts. It's a quick way to see/remember which modules our users will interact with vs not.

How does that sound to you? Should we make the naming more consistent?

Oh ok. I'm open to either way but I think keeping them consistent would be better. I'm guessing we'll have a
clear mechanism for deciding which modules should be published (ignoreProjects = listOf(...)) or something like that?

lauzadis · 2024-07-12T14:14:58Z

...b-mapper-ops-codegen/jvm/src/aws/sdk/kotlin/hll/dynamodbmapper/codegen/core/CodeGenerator.kt

+
+    override fun persist() {
+        val content = buildString {
+            appendLine("// Code generated by ddb-mapper-ops-codegen. DO NOT EDIT!")


For a second I thought you fixed #314 until I realized this was hand-written 😃

Yep, totally separate codegen I'm afraid. #314 still stands.

0marperez

style: k docs on some of the classes would be helpful

0marperez · 2024-07-15T16:11:05Z

...ops-codegen/jvm/src/aws/sdk/kotlin/hll/dynamodbmapper/codegen/rendering/OperationRenderer.kt

+
+        withBlock("internal fun <T> #L(table: #T) = #T(", ")", factoryName, Types.tableSpec("T"), Types.Operation) {
+            write(
+                "initialize = { hReq: #T -> #T(hReq, table.schema, #T(table, #S)) },",


style: highLevelReq over hReq/hlReq for readability

lauzadis

Nice, so many docs!

lauzadis · 2024-07-17T21:28:34Z

hll/build.gradle.kts

 val libraries = libs

 subprojects {
+    println("Subproject $this needsKmpConfigured? $needsKmpConfigured")


nit: stray println?

Yep, should've removed that.

sonarqubecloud · 2024-07-18T16:57:25Z

Quality Gate passed

Issues
26 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.7% Duplication on New Code

See analysis details on SonarCloud

…lin (#1451) * initial poc commit of DynamoDB Mapper (#1232) * add support for Mapper initialization (#1237) * implement mapper pipeline (#1266) * initial implementation of codegen for low-level operations/types (#1357) * initial implementation of secondary index support (#1375) * Create new codegen module and refactor annotation processor to use it (#1382) * feat: add Schema generator Gradle plugin (#1385) * Fix plugin test package * add attribute converters for "standard" values (#1381) * fix: schema generator plugin test module (#1394) * feat: annotation processor codegen configuration (#1392) * feat: add `@DynamoDbIgnore` annotation (#1402) * DDB Mapper filter expressions (runtime components) (#1401) * feat: basic annotation processing (#1399) * add DSL overloads, paginators, and better builder integration for DDB Mapper ops codegen (#1409) * chore: split dynamodb-mapper-codegen into two modules (#1414) * emit DDB_MAPPER business metric (#1426) * feat: setup DynamoDbMapper publication (#1419) * DDB Mapper filter expressions (codegen components) (#1424) * correct docs * mark every HLL/DDBM API experimental (#1428) * fix accidental inclusion of expression attribute members in high-level DynamoDB Mapper requests (#1432) * Upgrade to latest build plugin version * fix: various issues found during testing (#1450) * chore: update Athena changelog notes for 1.3.57 (2024-10-18) release (#1449) * feat: update AWS API models * feat: update AWS service endpoints metadata * chore: release 1.3.60 * chore: bump snapshot version to 1.3.61-SNAPSHOT * feat: initial release of Developer Preview of DynamoDB Mapper for Kotlin * Fix Kotlin gradle-plugin version * fix: ddb mapper tests (#1453) * Bump build plugin version --------- Co-authored-by: Matas <lauzmata@amazon.com> Co-authored-by: aws-sdk-kotlin-ci <aws-kotlin-sdk-automation@amazon.com>

initial implementation of codegen for low-level operations/types

3c7cc0d

ianbotsf requested a review from a team as a code owner July 10, 2024 23:42

lauzadis approved these changes Jul 12, 2024

View reviewed changes

ianbotsf added 2 commits July 12, 2024 18:40

fixes from PR feedback

4e5d44f

bump smithy-kotlin version

2ed34d9

0marperez approved these changes Jul 15, 2024

View reviewed changes

rename ddb-* projects to dynamodb-*; add KDocs in a bunch of places

39220e8

lauzadis approved these changes Jul 17, 2024

View reviewed changes

remove stray println

64a5541

ianbotsf merged commit b31e851 into feat-ddb-mapper Jul 18, 2024

ianbotsf deleted the mapper-op-codegen/final branch July 18, 2024 17:36


		private const val TABLE_NAME = "foo-table"

		// FIXME Should be in commonTest but mockk is JVM-only and finding a good KMP mocking library is hard

initial implementation of codegen for low-level operations/types #1357

initial implementation of codegen for low-level operations/types #1357

Uh oh!

Conversation

ianbotsf commented Jul 10, 2024

Issue #

Description of changes

PR flight plan

Sample codegen output

Known work remaining

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!

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!

0marperez left a comment

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!

sonarqubecloud bot commented Jul 18, 2024

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants