misc: add DynamoDbAttributeConverter annotation
#1751
Conversation
lauzadis
force-pushed
the
feat-attribute-converter
branch
from
5de27fa to
5a52082
Compare
lauzadis
added
the
no-changelog
|
A new generated diff is ready to view.
|
| /** | ||
| * Specifies the type of [ValueConverter] to be used when processing this attribute. | ||
| */ | ||
| public annotation class DynamoDbAttributeConverter(val converter: KClass<out ValueConverter<*>>) | ||
|
|
There was a problem hiding this comment.
Comment: I always seem to forget that annotation classes can only take KClass/Class arguments, not actual class instances. 😖 Several of the value converters (e.g., BooleanValueConverter, the various number value converters, etc.) are defined as val instances. We'll need to update those class definitions (or maybe object?) to allow users to select them in annotations. Can you either leave a FIXME somewhere and/or create a second task to handle updating the converter declarations?
| // FIXME Update to take a KClass<ItemConverter>, which will require splitting codegen modules due to a circular dependency | ||
| // FIXME Update to take a KClass<ItemConverter>? | ||
| @Target(AnnotationTarget.CLASS) | ||
| public annotation class DynamoDbItem(val converterName: String = "") |
There was a problem hiding this comment.
Comment: Yes, we'll definitely still want to do this. I think that'll be far more convenient.
| val annotation = SchemaRenderer(annotated, renderCtx) | ||
| val annotation = SchemaRenderer(logger, annotated, renderCtx) | ||
| annotation.render() |
There was a problem hiding this comment.
Question: Isn't the logger already available in renderCtx?
| kotlin { | ||
| sourceSets { | ||
| commonMain { | ||
| dependencies { | ||
| // For ValueConverter | ||
| implementation(project(":hll:dynamodb-mapper:dynamodb-mapper")) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Comment: I'd hoped to keep the dependencies clean for the annotations package but we obviously need the ValueConverter (and soon, ItemConverter) types. I wonder if we should extract a dynamodb-mapper-core-api package which contains some important interfaces but no implementations.
| assertContains( | ||
| schemaContents, | ||
| """ AttributeDescriptor( | ||
| "occupation", | ||
| Employee::occupation, | ||
| Employee::occupation::set, | ||
| org.example.OccupationConverter(), | ||
| ),""", | ||
| ) | ||
|
|
||
| // Test cross-package converter | ||
| assertContains(schemaContents, "import a.different.pkg.HealthcareConverter") | ||
| assertContains( | ||
| schemaContents, | ||
| """ AttributeDescriptor( | ||
| "healthcare", | ||
| Employee::healthcare, | ||
| Employee::healthcare::set, | ||
| a.different.pkg.HealthcareConverter(), | ||
| ),""", |
There was a problem hiding this comment.
Nit: String indentation is a little wonky.
| @property:DynamoDbAttributeConverter(OccupationConverter::class) | ||
| var occupation: Occupation = Occupation("Student", 0), | ||
|
|
||
| @property:DynamoDbAttributeConverter(HealthcareConverter::class) | ||
| var healthcare: Healthcare = Healthcare(false), |
There was a problem hiding this comment.
Question: Having to qualify the target in the annotation line seems kind of ugly and unintuitive. Is this still required if the annotation class is annotated with @Target(AnnotationTarget.PROPERTY)?
2 participants
Issue #
Addresses #1457
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.