implement mapper pipeline #1266

ianbotsf · 2024-03-22T20:55:53Z

Issue #

Part of #76

Description of changes

This changes adds an implementation of a pipeline for orchestrating/executing mapper operations and interceptors. The Interceptor interface KDocs should (hopefully) describe well enough how interceptors, hooks, and the overall pipeline works...if not, leave comments so that I can fix that!

Feedback solicited

Any modified file is open for feedback except where TODOs disclaim parts of the code. Things I'd particularly like feedback on include:

The implementation of Operation: I considered and partly implemented a higher-level abstraction of this which involved modeled stages (e.g., InitializationStage, SerializationStage, etc.) which combined into a pipeline inside Operation. It felt too complex though and introduced even more generics than are present already. That's not to say it's definitively wrong, though, or that my implementation couldn't be clarified in other ways.
The ordering of the pipeline and interceptors: I designed the pipeline to have distinct steps (e.g., initialization, serialization, etc.) surrounded by phases during which interceptor hooks would run. I wanted read and modify hooks before each step and a read hook after. Interceptors will accumulate errors in read hooks but not modify hooks...those halt subsequent interceptor modify hooks and proceed to the next read hook stage. After a read stage terminates, any present exception gets thrown.

This design leads to a situation which may limit its usefulness: Any exception thrown in a modify hook, read hook, or pipeline step will not have an opportunity to be caught by a subsequent modify hook and changed. Maybe some logic could convert serialization or invocation exceptions into some result representing success?
The number/type of generics: Interceptor, all the context classes, and Operation are awash in generic type parameters now. I think they make sense but additional eyes will confirm/disconfirm.

Other minor changes

Separated Table interface into TableSpec and TableOperations. This had a convenient property of minimizing the context that interceptors and the pipeline needed to include (e.g., mocking an Operation no longer requires a mock Table implementation including all operations). It also addresses what I believe will be a future codegen concern...namely, having a dedicated space (TableOperations) in which to generate operation methods (e.g., getItem, createTable, etc.) that's separate from the hand-maintained parts of table metadata (TableSpec).
Made a mapper's config a public val on the mapper itself. This will allow callers to access values they passed in to us such as the underlying DDB client, the list of interceptors, etc. I think this is okay but let me know if there are concerns.
Promoted keyAttributeNames from an implementation detail to a public val in KeySpec. It simplifies connecting operations to the pipeline and may also be useful for introspection by users.

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

github-actions · 2024-03-22T21:02:34Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

aajtodd

Looks good overall! Nice work.

aajtodd · 2024-03-25T16:03:08Z

...pper/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/pipeline/SerializeInput.kt

+         * object
+         * @param serializeSchema The [ItemSchema] to use for serializing objects into items
+         */
+        public operator fun <T, HReq> invoke(


style: Why invoke vs a factory function

fun <T, HReq> SerializeInput(highLevelRequest: HReq, serializeScheme: ItemSchemea<T>): SerializeInput = SerializeInputImpl(highLevelRequest, serializeScheme)

No particular reason so I'm not tied to the choice. Are there any tradeoffs between invoke vs factory function?

The one that comes to mind is intellij often autocompletes/suggests Foo.invoke(...) instead of inlining it as just Foo(...)

aajtodd · 2024-03-25T16:10:19Z

...-mapper/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/pipeline/Interceptor.kt

+     * A **read-only** hook that runs _after_ the **Initialization** step
+     * @param ctx Context containing the high-level request
+     */
+    public fun readAfterInitialization(ctx: HReqContext<T, HReq>) { }


Question: What happens in "initialization"? after implies to me something happened before this but if this is the first step before anything else happens then I think readBeforeExecution was preferable.

"Initialization" refers to the Operation.doInitialize method, which delegates to the initialize constructor parameter passed in by individual operations. My strawman GetItem implementation uses initialize to create the initial HReqContext (see GetItem.kt:62). I imagine it's possible that some operations we codegen may need additional initialization steps but I cannot say for sure.

aajtodd · 2024-03-25T16:14:44Z

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

+
+        commonTest {
+            dependencies {
+                implementation(libs.mockk)


concern: mockk does not support KMP really from what I can see. This will make many tests JVM only effectively and perhaps result in some refactoring later to move test from commonTest -> jvmTest.

I think some tests being JVM only are likely ok since verification of behavior of common code in one platform source set is still pretty good coverage (any issues from that point would indicate the compiler has a bug I would think).

Good point. I think we have at least two options at the moment:

Accept that OperationTest (and perhaps others in the future) is JVM-only, move it from commonTest to jvmTest

Remove MockK and manually mock/spy dependencies as appropriate. This is doable but will result in a lot more code and may be brittle in the case of DynamoDbClient, whose members are codegenned from the model.

I favor option 1 at the moment but am open to other opinions.

Option 1 seems fine to me.

aajtodd · 2024-03-25T16:32:23Z

...amodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/pipeline/internal/PipelineStage.kt

@@ -0,0 +1,5 @@
+/*


Question: What is the purpose of this file?

😖 Accidentally included file from my aborted attempt to further abstract the pipeline. Will remove!

lauzadis · 2024-03-25T16:42:43Z

...-mapper/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/pipeline/Interceptor.kt

+ * mapper. The order in which interceptors are given in mapper config determines the order in which they will be
+ * executed:
+ * * For phases _before_ the **Low-level invocation** step, hooks will be executed _in given order_
+ * * For phases _after_ the **Low-level invocation** step, hooks will be executed _in reverse order_


What is the purpose of this reverse-order execution? It seems like it might cause confusing/unexpected behavior for users

The intent is to support predictable lifetimes for interceptor events. (Whether it's reasonable to do so is still up for debate.) For an example, suppose there are two interceptors registered for an operation, A and B in that order.

A has:

A modifyBeforeSerialization hook that normalizes timestamps in the high-level request from local times to UTC

A modifyBeforeCompletion hook that localizes timestamps in the high-level response from UTC to local times

B has:

A modifyBeforeSerialization hook that logs the high-level request

A modifyBeforeCompletion hook that logs the high-level response

The current logic would run them in this order:

A.modifyBeforeSerialization (times to UTC)

B.modifyBeforeSerialization (log request with UTC timestamp)

...

B.modifyBeforeCompletion (log response with UTC timestamp)

A.modifyBeforeCompletion (times to local)

This sequence allows interceptors to rely on the ordering of "set up" and "tear down" events (e.g., B knows that whatever A has changed on the way "down" will be undone in a predictable order on the way back "up").

I will say that this execution order differs from what we do in low-level interceptors. Those all run in registration sequence regardless of where they sit in the pipeline. This divergence may be fine or it may confuse people, although I'd say that registering custom interceptors in the low-level and high-level clients is a pretty advanced use case.

Ok that's a good example where this is nice to have. It stood out to me because it's inconsistent with the low-level interceptors, but since it's an advanced use case, I think it's ok

lauzadis · 2024-03-25T16:49:28Z

...-mapper/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/pipeline/Interceptor.kt

+/**
+ * A "universal" interceptor which acts on any type of objects, requests, and responses
+ */
+public typealias UniversalInterceptor = Interceptor<*, *, *, *, *>


nit naming / discoverability suggestion: InterceptorAny (or something else, as long as it's prefixed with Interceptor)

Ooh yes, that's much better.

lauzadis · 2024-03-25T16:50:34Z

...-mapper/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/pipeline/Interceptor.kt

+ * interceptors hooks in the same phase. Only after _all_ interceptors' read-only hooks for a given phase have completed
+ * will any exception be thrown. For example, if a mapper has two interceptors **A** and **B** registered, and **A**'s
+ * [readAfterSerialization] hook throws an exception, it will be added to the context passed to **B**'s
+ * [readAfterSerialization] hook. After **B**'s [readAfterSerialization] hook has completed, the exception be thrown


nit / missing a word: "the exception will be thrown back to the caller"

lauzadis · 2024-03-25T17:46:52Z

.../dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/pipeline/internal/Operation.kt

+    interceptors: Collection<UniversalInterceptor>,
+) {
+    private val interceptors = interceptors.map {
+        // Will cause runtime ClassCastExceptions during interceptor invocation if the types don't match. Is that ok?


I think it's ok, but why can't we have the constructor parameter interceptors be the narrower type Collection<Interceptor<T, HReq, LReq, LRes, HRes>>?

It might be able to but then the business of converting to more specific types would fall to the callers of Operation. Interceptors are registered at the mapper level right now and can apply to any operation. Consequently, the mapper-level config for interceptors is also a collection of InterceptorAny.

…eanups

sonarqubecloud · 2024-03-29T17:55:07Z

Quality Gate passed

Issues
18 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-03-29T18:00:46Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

…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>

implement mapper pipeline

e7cc576

ianbotsf requested a review from a team as a code owner March 22, 2024 20:55

aajtodd reviewed Mar 25, 2024

View reviewed changes

lauzadis approved these changes Mar 25, 2024

View reviewed changes

replace invoke operators with factory functions, small renames and cl…

ba930c0

…eanups

ianbotsf merged commit 2a844c5 into feat-ddb-mapper Mar 29, 2024

ianbotsf deleted the mapper-pipeline branch March 29, 2024 22:22

implement mapper pipeline #1266

implement mapper pipeline #1266

Uh oh!

Conversation

ianbotsf commented Mar 22, 2024

Issue #

Description of changes

Feedback solicited

Other minor changes

Uh oh!

github-actions bot commented Mar 22, 2024

Uh oh!

aajtodd 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!

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!

sonarqubecloud bot commented Mar 29, 2024

Quality Gate passed

Uh oh!

github-actions bot commented Mar 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants