Add support for strategy-based UUID auto-generation (@DynamoDbAutoGeneratedUuid) - #6373
Conversation
519acfe to
fa35bfc
Compare
586dbf5 to
1c0b19f
Compare
335e532 to
f3a3ad1
Compare
fb82197 to
ede74c8
Compare
marcusvoltolim
left a comment
There was a problem hiding this comment.
No tests with @DynamoDbSortKey
|
Hello @marcusvoltolim, Thank you very much for your review. I have added tests for
Regarding your comment about I have updated the tests, ticket description, and PR description to clearly reflect this. Could you please take another look when you have a chance? Thank you! |
|
Hi @anasatirbasa , I read through the PR, I have a few concerns I wanted to raise with the entire team before providing feedback. Will update you soon. |
|
Hi @anasatirbasa, thanks for the wait. I have reviewed this PR with the team and have a few points that we would ideally like to fix. I agree that introducing this separate annotation would solve the problem in an "opt in" non invasive way. Considerations:
If both
This creates unpredictable behavior. If this is the case, can we please add some tests that cover it, and throw an exception when both annotations (new and current) are applied to the same field?
In v1,
but also, misleadingly suggests using We will likely ask you to add more test coverage similar to other extensions test coverage, but I will provide more concrete about testing gaps in the near future after another review. Thanks, |
RanVaknin
left a comment
There was a problem hiding this comment.
Please refer to my comments on the PR.
Hello, @RanVaknin! Thank you very much for your feedback! I am in progress with the changes. |
3ff8f87 to
7cec4a2
Compare
|
Hello @RanVaknin, Thank you very much for the feedback! I've addressed all three points you raised: 1. Documentation Updates for UpdateBehavior.WRITE_IF_NOT_EXISTSI've updated the documentation in both
2. Conflicting Annotations PreventionYour assumption about unpredictable behavior based on extension load order was correct. I've implemented bidirectional conflict detection:
Both extensions now prevent conflicting annotations regardless of load order, ensuring predictable behavior. 3. Test Coverage for Conflicting BehaviorI've added tests across multiple test classes: AutoGeneratedKeyExtensionTest:
AutoGeneratedUuidExtensionTest:
AutoGeneratedKeyRecordTest (functional):
AutoGeneratedUuidRecordTest (functional):
ConflictingAnnotationsTest (dedicated):
All tests verify that |
|
Hi @anasatirbasa, Thanks for the follow up. I'll review and get back to you asap. Also a request for the future; please do not squash the commits, it makes it difficult to review the particular changes requested :) Thanks 🙏 |
|
Can you fix the javadoc here? (this is incorrect javadoc that we already have in the SDK) : We want to clarify that The rest of the PR looks good. Can we add functional tests for Thanks, |
|
Hello @shetsa-amzn @amzn-erdemkemer, I have added functional tests covering composite primary and secondary index keys scenarios, The tests were added in AutoGeneratedKeyCompositeGsiTest.java and validate correct UUID generation and update behavior for composite keys across root and flattened attributes. Bean Structure:BeanWithMixedCompositeGsi class includes: 1. Primary Key:
2. Composite GSI Keys (Root):
3. Flattened Composite Keys:
Test Scenarios:1. putItem_whenKeysNotPopulated_generatesNewUuids: 2. putItem_whenKeysAlreadyPopulated_preservesExistingUuids: 3. updateItem_respectsUpdateBehavior:
4. batchWrite_whenKeysNotAlreadyPopulated_generatesNewUuids: 5. batchWrite_whenKeysAlreadyPopulated_preservesExistingUuids: 6. transactWrite_whenKeysNotAlreadyPopulated_generatesNewUuids: 7. transactWrite_whenKeysAlreadyPopulated_preservesExistingUuids: |
…erated-key-annotation' into feature/define-dynamo-db-autogenerated-key-annotation
LeeroyHannigan
left a comment
There was a problem hiding this comment.
I do not see any tests for multi-attribute keys for secondary indexes? Have you tested this works, can you add a functional test case.
Hello @LeeroyHannigan, Functional coverage for multi-attribute (composite) secondary index keys are present in AutoGeneratedKeyCompositeGsiTest.java. Bean Structure Used in the Tests: The test bean (BeanWithMixedCompositeGsi) is structured to cover:
1) Composite Primary Key @DynamoDbPartitionKey
@DynamoDbAutoGeneratedKey
public String getId()
@DynamoDbSortKey
@DynamoDbAutoGeneratedKey
public String getSort()Behavior:
2) Composite GSI Keys (Root Attributes): Example for partition key parts: @DynamoDbAutoGeneratedKey
@DynamoDbUpdateBehavior(UpdateBehavior.WRITE_ALWAYS)
@DynamoDbSecondaryPartitionKey(indexNames = {...}, order = FIRST)
public String getRootPartitionKey1()
@DynamoDbAutoGeneratedKey
@DynamoDbUpdateBehavior(UpdateBehavior.WRITE_IF_NOT_EXISTS)
@DynamoDbSecondaryPartitionKey(indexNames = {...}, order = SECOND)
public String getRootPartitionKey2()Example for sort key parts: @DynamoDbAutoGeneratedKey
@DynamoDbUpdateBehavior(UpdateBehavior.WRITE_ALWAYS)
@DynamoDbSecondarySortKey(indexNames = {...}, order = FIRST)
public String getRootSecondaryKey1()
@DynamoDbAutoGeneratedKey
@DynamoDbUpdateBehavior(UpdateBehavior.WRITE_IF_NOT_EXISTS)
@DynamoDbSecondarySortKey(indexNames = {...}, order = SECOND)
public String getRootSecondaryKey2()Behavior:
3) Composite GSI Keys (Flattened Attributes): The bean also extends the composite keys using a flattened object: @DynamoDbFlatten
public FlattenedKeys getFlattenedKeys()Inside: @DynamoDbAutoGeneratedKey
@DynamoDbUpdateBehavior(UpdateBehavior.WRITE_ALWAYS)
@DynamoDbSecondaryPartitionKey(indexNames = {...}, order = THIRD)
public String getFlattenedPartitionKey1()This means the composite GSI key is built across both root and flattened attributes. What the Tests Cover: The following operations are tested:
Verified behavior:
These tests confirm that multi-attribute secondary index keys (including This comment can be checked for more context regarding the tests scenarios added in AutoGeneratedKeyCompositeGsiTest.java. |
|
|
Hi @anasatirbasa, Thanks for your patience on this. I know it's been a long review cycle and I appreciate the work you've put in. I want to revisit something I should have caught earlier in the review process, and I apologize for not raising it sooner. While looking at this more closely with the team, I went back to the v1 SDK to understand how it originally handled this. In v1, the design was:
So This raises the question, what if instead of introducing a new annotation and extension, we added a @DynamoDbAutoGeneratedUuid(strategy = DynamoDbAutoGenerateStrategy.CREATE)With the default set to ALWAYS, this would be backwards compatible. Existing code using the annotation without parameters would behave exactly as it does today. Customers who want the "only generate when null" behavior would just set Here are the considerations that made me think we should take a step back and reconsider the existing design in favor of the proposed alternative:
I realize this is a significant pivot from the current approach, and I'm sorry for not connecting these dots earlier. Thanks, |
…erated-key-annotation' into feature/define-dynamo-db-autogenerated-key-annotation
| import software.amazon.awssdk.annotations.SdkPublicApi; | ||
|
|
||
| /** | ||
| * Strategy used by {@link DynamoDbAutoGeneratedUuid} to decide when UUID values are generated. |
There was a problem hiding this comment.
In the v1 SDK this strategy enum is shared between other annotations like @DynamoDBAutoGeneratedTimestamp that do not represent a UUID. Can we make the javadoc here generic?
| } | ||
|
|
||
| private boolean isMissingValue(AttributeValue currentValue) { | ||
| return currentValue == null || Boolean.TRUE.equals(currentValue.nul()) || "".equals(currentValue.s()); |
There was a problem hiding this comment.
v1 did not treat an empty string as a "missing value". Whether its the right behavior is arguable. I think it's better if we stick to the existing v1 behavior of treating an empty string as a not missing.
RanVaknin
left a comment
There was a problem hiding this comment.
Thanks for all the work you have done 👍
I created a surface API review doc and reviewed it with the Java team. It's looking good.
I just left a few small comments on things needed fixing.
Additionally, can we add the following test coverage?
- Test CREATE on a non key attribute. Every CREATE test uses a key. Non keys flow through a different path (update expressions, not the key map). Please add a functional test with a plain non key String attribute annotated strategy = CREATE: generated when absent on put, preserved when present on update, generated when absent on update.
- Test
updateItemwithignoreNulls(true)+ CREATE. WithignoreNulls(true)and a null CREATE field, the attribute is omitted from the map, so CREATE should regenerate (potentially overwriting a stored value). Can you add a functional test checking the behavior? If its a silent ovewrite we should add javadoc to reflect this edge case. - Test the DynamoDB NUL missing value case. isMissingValue treats null, NUL, and "" as missing. We cover absent case and empty string case (which needs to be removed), but we dont cover the NUL case.
Lastly, the current SDK build fails because of a Japicmp error:
[ERROR] Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.15.6:cmp (default) on project dynamodb-enhanced: There is at least one incompatibility: software.amazon.awssdk.enhanced.dynamodb.extensions.annotations.DynamoDbAutoGeneratedUuid.strategy():METHOD_ABSTRACT_ADDED_TO_CLASS
The newly added strategy was flagged by japicmp as a byte code change, but because it has a default value it should be safe. Can you please just add an exclusion to services-custom/dynamodb-enhanced/pom.xml:
<plugin>
<groupId>com.github.siom79.japicmp</groupId>
<artifactId>japicmp-maven-plugin</artifactId>
<configuration>
<parameter>
<excludes>
<exclude>software.amazon.awssdk.enhanced.dynamodb.extensions.annotations.DynamoDbAutoGeneratedUuid#strategy()</exclude>
</excludes>
</parameter>
</configuration>
</plugin>…-annotation # Conflicts: # services-custom/dynamodb-enhanced/src/test/java/software/amazon/awssdk/enhanced/dynamodb/extensions/AutoGeneratedUuidExtensionTest.java
|
Hello @RanVaknin, Thanks for the review. I've addressed the comments:
I also added few more scenarios:
|
| * {@code @DynamoDbAutoGeneratedUuid} usage. | ||
| * Use {@link DynamoDbAutoGenerateStrategy#CREATE} when you want to generate only if the value is missing. | ||
| * Use {@link DynamoDbAutoGenerateStrategy#CREATE} when you want to generate only if the value is missing | ||
| * (absent from the write item map or DynamoDB {@code NULL}). An empty string is treated as present and is preserved. |
There was a problem hiding this comment.
nit: This adds unnecassary info about the implementation. The following is sufficient:
* Use {@link DynamoDbAutoGenerateStrategy#CREATE} when you want to generate only if the value is missing
* (absent from the write item map or DynamoDB {@code NULL}).| /** | ||
| * Strategy used by {@link DynamoDbAutoGeneratedUuid} to decide when UUID values are generated. | ||
| * Strategy used to decide when auto-generated attribute values are produced. | ||
| * <p> |
There was a problem hiding this comment.
"attribute values" here is misleading, because its colliding with AttributeValue which is a real enhanced client container class.
The rest is an embellishment with implementation details that we don't need.
proposed:
/**
* Strategy used to decide when a new value is generated for an annotated attribute
*/| * Generate a UUID only when the value is missing. | ||
| * Missing means the value is absent, DynamoDB {@code NULL}, or an empty string. | ||
| * Generate a value only when the current value is missing. | ||
| * Missing means the value is absent from the write item map or is DynamoDB {@code NULL}. |
There was a problem hiding this comment.
We should remove line 35 This explanation couples the enum to how the autogenerated UUID extension works. We might want to redefine what "missing" means when we implement other extensions that use this strategy enum.



Description
Added support for strategy-based UUID auto-generation on
@DynamoDbAutoGeneratedUuid, aligned with legacy V1 semantics from@DynamoDBGeneratedUuid(DynamoDBAutoGenerateStrategy).The V2 annotation now supports:
strategy = ALWAYS(default): generate a new UUID on every writestrategy = CREATE: generate only when the attribute is missingMissing means the value is absent from the write item map or is DynamoDB NULL. An empty string is treated as present and is preserved (V1 behavior).
This provides the V1 "create-only" behavior without introducing a separate key annotation or extension.
Important Rules
@DynamoDbAutoGeneratedUuidis valid only forStringattributes.strategy = CREATEcan be used on both key and non-key attributes.strategy = ALWAYSpreserves existing V2 behavior (regenerate on write), so backward compatibility is maintained for existing users of@DynamoDbAutoGeneratedUuid.CREATEinspects the write item map after mapping, not the value already stored in DynamoDB. WithupdateItemandignoreNulls(true), a null CREATE field is omitted from the map, so a new UUID is generated and silently overwrites any existing stored value.UpdateBehavior Notes
@DynamoDbUpdateBehaviorstill applies only toUpdateItemexpression behavior (notPutItem).Primary keys (PK/SK):
handled as key attributes in request construction; update behavior does not change primary-key immutability semantics.
Secondary index keys / non-key attributes:
update behavior (
WRITE_ALWAYS/WRITE_IF_NOT_EXISTS) influences how generated values are written in update expressions.Motivation and Context
This PR remains related to issue #5497, but the implementation direction changed:
@DynamoDbAutoGeneratedKeyAPI, this PR extends the existing@DynamoDbAutoGeneratedUuidwith a strategy.ALWAYSvsCREATE).ALWAYS.strategy = CREATE.This avoids API duplication and removes annotation-selection ambiguity.
Modifications
DynamoDbAutoGenerateStrategyenum with:ALWAYSCREATE@DynamoDbAutoGeneratedUuidwith astrategyparameter (defaultALWAYS).AutoGeneratedUuidExtensionto apply per-attribute strategy at write time.AutoGeneratedUuidTagto propagate annotation strategy into table metadata.Stringonly) and strategy behavior validation paths.ignoreNulls(true)overwrite edge case on the annotation and extension.DynamoDbAutoGeneratedUuid#strategy()(METHOD_ABSTRACT_ADDED_TO_CLASSwith a default value).@DynamoDbAutoGeneratedUuid(strategy = CREATE)for create-only semantics.Testing
ALWAYSbehavior (regenerate on writes)CREATEbehavior (generate only when missing; preserve existing values including empty string)AutoGeneratedUuidRecordTest; no extra test class)updateItem(ignoreNulls(true))+ CREATE silent overwrite@DynamoDbUpdateBehavior@DynamoDbAutoGeneratedUuidusers.Test Coverage on modified classes:
Test Coverage Checklist
@DynamoDbAutoGeneratedUuiddefaults to ALWAYSTypes of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License