Skip to content

Conversation

@lauzadis
Copy link
Member

@lauzadis lauzadis commented Sep 6, 2024

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
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

val operations = getOperations(resolver)

val codegenFactory = CodeGeneratorFactory(codeGenerator, logger)
val codegenFactory = CodeGeneratorFactory(codeGenerator, logger) // FIXME Pass dependencies
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI: I could not figure out how to get a list of dependencies for the ops code generator, I kept getting null values when trying to find the file which contains DynamoDbClient

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

Copy link
Contributor

@ianbotsf ianbotsf left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just a few small things left.

Comment on lines 59 to 67
// If the property OR any of its arguments reference the annotated type
(propName == name) ||
(
propType.arguments.any { arg ->
val argType = arg.type?.resolve()
val argName = requireNotNull(argType?.declaration?.qualifiedName).asString()
argName == name
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Parentheses around propType.arguments.any call are unnecessary

Comment on lines 163 to 174
/**
* Renders a ValueConverter for the [KSType].
*
* Note: The ValueConverter(s) will be rendered without a newline in order to support deep recursion.
* Callers are responsible for adding a newline after the top-level invocation of this function.
*/
private fun KSType.renderValueConverter(writer: SchemaRenderer) {
writer.apply {
val ksType = this@renderValueConverter
val type = Type.from(ksType).copy(genericArgs = listOf())

when {
Copy link
Contributor

Choose a reason for hiding this comment

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

Style: I can see how this works recursively and is better than trying to bundle multiple actual types into a single #T param but an in-class extension method that takes its own class instance as an argument feels awkward. Could this just be a regular instance method that takes the type as an argument:

private fun renderValueConverter(type: KSType)

// calling code
renderValueConverter(prop.type.resolve())

private fun KSType.renderValueConverter(writer: SchemaRenderer) {
writer.apply {
val ksType = this@renderValueConverter
val type = Type.from(ksType).copy(genericArgs = listOf())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: I see you're removing the generics from ksType in order to use an equality check with non-parameterized map/list types from Types.Kotlin.Collections. That works but it's not obvious at first read why you're stripping the generics. It might be cleaner to define an extension method Type.isGenericFor(other: Type) which performs an equality check on name, package, and nullability.

Comment on lines 44 to 52
if (!listCharArray.zip(other.listCharArray).all { (a, b) -> a.contentEquals(b) }) return false
for (i in listCharArray.indices) {
if (!listCharArray[i].contentEquals(other.listCharArray[i])) return false
}
if (listChar != other.listChar) return false
if (listByte != other.listByte) return false
if (listByteArray.size != other.listByteArray.size) return false
if (!listByteArray.zip(other.listByteArray).all { (a, b) -> a.contentEquals(b) }) return false
for (i in listByteArray.indices) {
if (!listByteArray[i].contentEquals(other.listByteArray[i])) return false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Correctness: These updated List<*Array> comparisons still miss cases where other.list*Array is longer than this.list*Array. I suggest:

if (listCharArray.size != other.listCharArray.size) return false
if (!listCharArray.zip(other.listCharArray).all { (a, b) -> a.contentEquals(b) }) return falseif (listByteArray.size != other.listByteArray.size) return false
if (!listByteArray.zip(other.listByteArray).all { (a, b) -> a.contentEquals(b) }) return false

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that's what I had in the beginning. I think I missed it / forgot to mention after your first comment, but I was always checking the size of the lists

#1399 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wow, my eyes totally skipped past that. I apologize. What you originally had was fine.

Comment on lines 11 to 14
public object ItemToValueConverter : ValueConverter<Item> {
override fun convertFrom(to: AttributeValue): Item = to.asM().toItem()
override fun convertTo(from: Item): AttributeValue = AttributeValue.M(from)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: KDocs

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

@lauzadis lauzadis merged commit 15a9472 into feat-ddb-mapper Sep 19, 2024
10 of 11 checks passed
ianbotsf added a commit that referenced this pull request Oct 29, 2024
…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 <[email protected]>
Co-authored-by: aws-sdk-kotlin-ci <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants