feat: add `@DynamoDbIgnore` and `@DynamoDbItemConverter` annotations #1402

lauzadis · 2024-09-10T14:36:27Z

Issue #

Description of changes

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

github-actions · 2024-09-10T14:51:33Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

ianbotsf · 2024-09-10T21:32:02Z

...ain/kotlin/aws/sdk/kotlin/hll/dynamodbmapper/codegen/annotations/rendering/SchemaRenderer.kt

 * @param classDeclaration the [KSClassDeclaration] of the class
 * @param ctx the [RenderContext] of the renderer
 */
+@OptIn(KspExperimental::class)


Question: What do we need this for? Can we narrow the scope of this opt-in?

It's needed for KSP's isAnnotationPresent function, it can be scoped down

ianbotsf · 2024-09-10T21:34:31Z

...ain/kotlin/aws/sdk/kotlin/hll/dynamodbmapper/codegen/annotations/rendering/SchemaRenderer.kt

+    private val properties = classDeclaration.getAllProperties()
+        .filterNot { it.modifiers.contains(Modifier.PRIVATE) }
+        .filterNot { it.isAnnotationPresent(DynamoDbIgnore::class) }


Style: Fluent call chaining should generally separate each member (e.g., getAllProperties) onto its own line. Additionally, stacking identical collection operations (e.g., filterNot) with different arguments is generally unnecessary and overly verbose.

private val properties = classDeclaration .getAllProperties() .filterNot { it.modifiers.contains(Modifier.PRIVATE) || it.isAnnotationPresent(DynamoDbIgnore::class) }

ianbotsf · 2024-09-10T21:36:38Z

...pper/dynamodb-mapper-annotations/common/src/aws/sdk/kotlin/hll/dynamodbmapper/Annotations.kt

+/**
+ * Specifies that a custom ItemConverter should be used to convert this class/interface.
+ * @param qualifiedName The fully qualified name of the item converter.
+ */
+@Target(AnnotationTarget.CLASS)
+public annotation class DynamoDbItemConverter(val qualifiedName: String)


Question: Does having a separate annotation offer additional benefit over a converter: String? field on DynamoDbItem?

No, there's not any specific benefit I was going for with this design choice. I'm open to making it a parameter on DynamoDbItem, I think anything works as long as we document it properly.

I did this refactor, the only problem was that annotation parameters cannot be nullable (String?) so I made it a String defaulted to the empty string

ianbotsf · 2024-09-10T21:39:36Z

...ain/kotlin/aws/sdk/kotlin/hll/dynamodbmapper/codegen/annotations/rendering/SchemaRenderer.kt

+    private val itemConverter: Type
+        get() = run {
+            val qualifiedName = classDeclaration.getAnnotationsByType(DynamoDbItemConverter::class).singleOrNull()?.qualifiedName
+
+            return qualifiedName?.let {
+                val pkg = qualifiedName.substringBeforeLast(".")
+                val shortName = qualifiedName.removePrefix("$pkg.")
+                TypeRef(pkg, shortName)
+            } ?: TypeRef(ctx.pkg, "${className}Converter")
+        }


Question: Does this need to be calculated on each call? Could it be calculated once instead?

private val itemConverter: Type = run { val qualifiedName = classDeclaration.getAnnotationsByType(DynamoDbItemConverter::class).singleOrNull()?.qualifiedName return qualifiedName?.let { val pkg = qualifiedName.substringBeforeLast(".") val shortName = qualifiedName.removePrefix("$pkg.") TypeRef(pkg, shortName) } ?: TypeRef(ctx.pkg, "${className}Converter") }

Yes it should probably only be calculated once

github-actions · 2024-09-11T14:59:45Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

0marperez · 2024-09-11T15:29:22Z

...lugin/src/test/kotlin/aws/sdk/kotlin/hll/dynamodbmapper/plugins/SchemaGeneratorPluginTest.kt

+        val result = runner.build()
+        assertContains(setOf(TaskOutcome.SUCCESS, TaskOutcome.UP_TO_DATE), result.task(":build")?.outcome)
+
+        // Schema should not exist


small nit: this comment makes it seem like the assertion below should be false

good catch, when I was first writing this test I thought the schema file would not be generated because there's no item converter needed, but we still need to render the schema

0marperez · 2024-09-11T16:02:30Z

...pper-schema-generator-plugin/src/test/resources/custom-item-converter/CustomItemConverter.kt

+            IntConverter,
+        ),
+        AttributeDescriptor(
+            "myCustomFirstName",


nit: naming here is differs from the data class property i.e. givenName and firstName

that's intentional to show what a custom item converter might do

sonarqubecloud · 2024-09-12T18:52:59Z

Quality Gate failed

Failed conditions
14.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

github-actions · 2024-09-12T18:56:15Z

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>

lauzadis added 2 commits September 10, 2024 10:35

Add DynamoDbIgnore and DynamoDbItemConverter

42efb77

ktlint

c8c44db

lauzadis added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Sep 10, 2024

lauzadis requested a review from a team as a code owner September 10, 2024 14:36

lauzadis changed the title ~~feat: add DynamoDbIgnore and DynamoDbItemConverter~~ feat: add DynamoDbIgnore and DynamoDbItemConverter annotations Sep 10, 2024

lauzadis added 2 commits September 10, 2024 10:37

update kdoc

81ba731

Remove extra opt-in

8cc97bd

lauzadis changed the title ~~feat: add DynamoDbIgnore and DynamoDbItemConverter annotations~~ feat: add @DynamoDbIgnore and @DynamoDbItemConverter annotations Sep 10, 2024

ianbotsf approved these changes Sep 10, 2024

View reviewed changes

lauzadis added 4 commits September 11, 2024 10:33

Calculate itemConverter once

fe6323c

Remove class-level optin to KspExperimental

db5b3fb

Fluent style call chaining

af4c868

Addressing PR feedback

b8c4df6

latest

2736ffd

0marperez approved these changes Sep 11, 2024

View reviewed changes

lauzadis added 3 commits September 12, 2024 14:43

add a FIXME

9fd0ac6

Remove GSIs

c677e2f

Remove misleading comment

a471469

lauzadis merged commit b08e24d into feat-ddb-mapper Sep 12, 2024

lauzadis deleted the feat-more-annotations branch September 12, 2024 19:24

feat: add @DynamoDbIgnore and @DynamoDbItemConverter annotations #1402

feat: add @DynamoDbIgnore and @DynamoDbItemConverter annotations #1402

Uh oh!

Conversation

lauzadis commented Sep 10, 2024

Issue #

Description of changes

Uh oh!

github-actions bot commented Sep 10, 2024

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!

github-actions bot commented Sep 11, 2024

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 Sep 12, 2024

Quality Gate failed

Uh oh!

github-actions bot commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat: add `@DynamoDbIgnore` and `@DynamoDbItemConverter` annotations #1402

feat: add `@DynamoDbIgnore` and `@DynamoDbItemConverter` annotations #1402