Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add DSL overloads, paginators, and better builder integration for DDB Mapper ops codegen #1409

Merged
merged 4 commits into from
Sep 19, 2024

Conversation

ianbotsf
Copy link
Contributor

Issue #

Part of #472

Description of changes

This change implements the TODOed and FIXMEd features of DSL overloads, pagination, and builder support for code-generated operations. Those changes required a fair amount of refactoring so you may see some classes moved/renamed without much functional difference. The key changes are in BuilderRenderer, DataTypeGenerator, OperationRenderer, and OperationsTypeRenderer.

In flagrant violation of the one-change-per-PR principle, I also turned on explicit API mode for hll-codegen since I noticed it was off when I moved some classes up from dynamodb-mapper-codegen. That mostly results in a bunch of new explicit visibility modifiers, return types, and @InternalSdkApis all over the place.

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

@ianbotsf ianbotsf requested a review from a team as a code owner September 17, 2024 22:13
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

Comment on lines 38 to 39
val loggerOutStream = KspLoggerOutputStream(logger)
System.setOut(loggerOutStream.asPrintStream())
Copy link
Member

Choose a reason for hiding this comment

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

What is this change used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was really useful in debugging deep codegen stacks where the KSP logger wasn't passed through. Regular println to stdout appear to be swallowed by KSP (or possibly Gradle?) so I needed a way to access the logger despite not having it in scope. I thought it could be useful in future debug scenarios but if we feel this doesn't merit permanent inclusion (or perhaps warrants a better solution) I can remove/update as requested.

Copy link
Member

Choose a reason for hiding this comment

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

Ok that's what I thought. I've also ran into this issue in the past, so I'm happy to have this feature, as long as we remove the println's before merging. Could you add the same thing in the high level class of the annotations processor?

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 a base class from which all of our symbol processors inherit? 😃

@@ -19,4 +21,6 @@ internal object ModelAttributes {
* For a given high-level [Structure], this attribute key identifies the associated low-level [Structure]
*/
val LowLevelStructure: AttributeKey<Structure> = AttributeKey("aws.sdk.kotlin.ddbmapper#LowLevelStructure")

val PaginationInfo: AttributeKey<PaginationMembers> = AttributeKey("aws.sdk.kotlin.ddbmapper#PaginationInfo")
Copy link
Member

Choose a reason for hiding this comment

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

nit: missing kdocs

Comment on lines 14 to 25
@InternalSdkApi
public object Hl {
public val Base: String = "aws.sdk.kotlin.hll.dynamodbmapper"
public val Annotations: String = "$Base.annotations"
public val Items: String = "$Base.items"
public val Model: String = "$Base.model"
public val Ops: String = "$Base.operations"
public val PipelineImpl: String = "$Base.pipeline.internal"
public val Values: String = "$Base.values"
public val ScalarValues: String = "$Values.scalars"
public val SmithyTypeValues: String = "$Values.smithytypes"
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we be keeping DDB mapper packages in the dynamodbmapper package?

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, moving them here was a mistake. Will fix!

response = Structure.from(declaration.returnType!!),
)

return ModelParsingPlugin.transform(op, ModelParsingPlugin::postProcessOperation)
Copy link
Member

Choose a reason for hiding this comment

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

Why does only this from function mean that an operation has been processed? What if you create an Operation with the primary constructor, should the postProcessOperation run in that case too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well we cannot modify the return value from a constructor—it always return the newly-initialized instance.

More broadly, the intent of the plugin was to modify model shapes after they've been parsed. Since that parsing happens in the from methods it seemed reasonable to limit plugin application to that spot. If we could move it to the constructor (or more likely a factory function) then we'd still have a case where plugins would execute any time we constructed the object in unit tests or other non-model cases.

private fun renderDslOp(op: Operation) {
val builderType = BuilderRenderer.builderType(op.request.type)
val generics = op.request.genericVars().asParamsList(" ").also {
println("Generics for ${op.request} = $it")
Copy link
Member

Choose a reason for hiding this comment

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

nit: stray println?

Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

Copy link

@ianbotsf ianbotsf merged commit 21f1bd5 into feat-ddb-mapper Sep 19, 2024
10 checks passed
@ianbotsf ianbotsf deleted the ddbmapper-dsls-and-paginators branch September 19, 2024 22:12
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

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 <lauzmata@amazon.com>
Co-authored-by: aws-sdk-kotlin-ci <aws-kotlin-sdk-automation@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants