-
Notifications
You must be signed in to change notification settings - Fork 1k
Feature/dynamodb enhanced increase functional tests coverage #6946
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "type": "feature", | ||
| "category": "Amazon DynamoDB Enhanced Client", | ||
| "contributor": "", | ||
| "description": "Increase functional tests coverage on dynamodb-enhanced module" | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -318,7 +318,14 @@ private void processFlattenedNestedAttributes( | |
| } | ||
|
|
||
| for (String attrName : customMetadataObject) { | ||
| AttributeConverter<?> converter = schema.converterForAttribute(attrName); | ||
| AttributeConverter<?> converter; | ||
| try { | ||
| converter = schema.converterForAttribute(attrName); | ||
| } catch (UnsupportedOperationException e) { | ||
| // Some custom/third-party TableSchema implementations don't support converterForAttribute. | ||
| // In that case, skip timestamp insertion for this attribute instead of failing the write. | ||
| continue; | ||
| } | ||
| if (converter != null) { | ||
| insertTimestampInItemToTransform(updatedItems, | ||
| reconstructCompositeKey(path, attrName), | ||
|
|
@@ -390,7 +397,13 @@ private Map<String, AttributeValue> applyAutoGeneratedTimestampsToMap( | |
| boolean mapCopied = false; | ||
|
|
||
| for (String key : customMetadataObject) { | ||
| AttributeConverter<?> converter = nestedSchema.converterForAttribute(key); | ||
| AttributeConverter<?> converter; | ||
| try { | ||
| converter = nestedSchema.converterForAttribute(key); | ||
| } catch (UnsupportedOperationException e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do the test cases actually cover this? When I removed this check, all the tests still passed which implies we don't actually have sufficient coverage of a nested schema case that hits this. |
||
| // Nested schema can't resolve converters: skip timestamp insertion for this attribute. | ||
| continue; | ||
| } | ||
| if (converter != null) { | ||
| if (!mapCopied) { | ||
| updatedNestedMap = new HashMap<>(nestedMap); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| package software.amazon.awssdk.enhanced.dynamodb.functionaltests; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: This file, along with other new ones are missing the copyright header. |
||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import org.junit.After; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
| import software.amazon.awssdk.enhanced.dynamodb.DynamoDbAsyncTable; | ||
| import software.amazon.awssdk.enhanced.dynamodb.DynamoDbEnhancedAsyncClient; | ||
| import software.amazon.awssdk.enhanced.dynamodb.TableSchema; | ||
| import software.amazon.awssdk.enhanced.dynamodb.functionaltests.models.AtomicCounterRecord; | ||
|
|
||
| public class AsyncAtomicCounterTest extends LocalDynamoDbAsyncTestBase { | ||
| private static final TableSchema<AtomicCounterRecord> TABLE_SCHEMA = TableSchema.fromClass(AtomicCounterRecord.class); | ||
|
|
||
| private final DynamoDbEnhancedAsyncClient enhancedAsyncClient = | ||
| DynamoDbEnhancedAsyncClient.builder().dynamoDbClient(getDynamoDbAsyncClient()).build(); | ||
|
|
||
| private final DynamoDbAsyncTable<AtomicCounterRecord> mappedTable = | ||
| enhancedAsyncClient.table(getConcreteTableName("table-name"), TABLE_SCHEMA); | ||
|
|
||
| @Before | ||
| public void createTable() { | ||
| mappedTable.createTable(r -> r.provisionedThroughput(getDefaultProvisionedThroughput())).join(); | ||
| } | ||
|
|
||
| @After | ||
| public void deleteTable() { | ||
| getDynamoDbAsyncClient().deleteTable(r -> r.tableName(getConcreteTableName("table-name"))).join(); | ||
| } | ||
|
|
||
| @Test | ||
| public void repeatedUpdate_shouldIncrementCountersOnEachUpdate() { | ||
| AtomicCounterRecord record = new AtomicCounterRecord(); | ||
| record.setId("id1"); | ||
| record.setAttribute1("value"); | ||
| mappedTable.updateItem(record).join(); | ||
| mappedTable.updateItem(record).join(); | ||
| mappedTable.updateItem(record).join(); | ||
|
|
||
| AtomicCounterRecord persisted = mappedTable.getItem(r -> r.key(k -> k.partitionValue("id1"))).join(); | ||
| assertThat(persisted.getDefaultCounter()).isEqualTo(2L); | ||
| assertThat(persisted.getCustomCounter()).isEqualTo(20L); | ||
| assertThat(persisted.getDecreasingCounter()).isEqualTo(-22L); | ||
|
Comment on lines
+42
to
+44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These feel like magic numbers. Where do they come from? We should at least comment how they were derived so that when they fail in the future we can understand why |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| package software.amazon.awssdk.enhanced.dynamodb.functionaltests; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static software.amazon.awssdk.enhanced.dynamodb.extensions.AutoGeneratedUuidExtension.AttributeTags.autoGeneratedUuidAttribute; | ||
| import static software.amazon.awssdk.enhanced.dynamodb.mapper.StaticAttributeTags.primaryPartitionKey; | ||
| import static software.amazon.awssdk.enhanced.dynamodb.mapper.StaticAttributeTags.updateBehavior; | ||
|
|
||
| import java.util.UUID; | ||
| import org.junit.After; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
| import software.amazon.awssdk.enhanced.dynamodb.DynamoDbAsyncTable; | ||
| import software.amazon.awssdk.enhanced.dynamodb.DynamoDbEnhancedAsyncClient; | ||
| import software.amazon.awssdk.enhanced.dynamodb.TableSchema; | ||
| import software.amazon.awssdk.enhanced.dynamodb.extensions.AutoGeneratedUuidExtension; | ||
| import software.amazon.awssdk.enhanced.dynamodb.mapper.StaticTableSchema; | ||
| import software.amazon.awssdk.enhanced.dynamodb.mapper.UpdateBehavior; | ||
|
|
||
| public class AsyncAutoGeneratedUuidRecordTest extends LocalDynamoDbAsyncTestBase { | ||
| private static class Record { | ||
| private String id; | ||
| private String writeAlwaysUuid; | ||
| private String writeIfNotExistsUuid; | ||
| public String getId() { return id; } | ||
| public void setId(String id) { this.id = id; } | ||
| public String getWriteAlwaysUuid() { return writeAlwaysUuid; } | ||
| public void setWriteAlwaysUuid(String writeAlwaysUuid) { this.writeAlwaysUuid = writeAlwaysUuid; } | ||
| public String getWriteIfNotExistsUuid() { return writeIfNotExistsUuid; } | ||
| public void setWriteIfNotExistsUuid(String writeIfNotExistsUuid) { this.writeIfNotExistsUuid = writeIfNotExistsUuid; } | ||
| } | ||
|
|
||
| private static final TableSchema<Record> TABLE_SCHEMA = | ||
| StaticTableSchema.builder(Record.class) | ||
| .newItemSupplier(Record::new) | ||
| .addAttribute(String.class, a -> a.name("id").getter(Record::getId).setter(Record::setId) | ||
| .tags(primaryPartitionKey())) | ||
| .addAttribute(String.class, a -> a.name("writeAlwaysUuid") | ||
| .getter(Record::getWriteAlwaysUuid) | ||
| .setter(Record::setWriteAlwaysUuid) | ||
| .tags(autoGeneratedUuidAttribute())) | ||
| .addAttribute(String.class, a -> a.name("writeIfNotExistsUuid") | ||
| .getter(Record::getWriteIfNotExistsUuid) | ||
| .setter(Record::setWriteIfNotExistsUuid) | ||
| .tags(autoGeneratedUuidAttribute(), | ||
| updateBehavior(UpdateBehavior.WRITE_IF_NOT_EXISTS))) | ||
| .build(); | ||
|
|
||
| private final DynamoDbEnhancedAsyncClient enhancedAsyncClient = | ||
| DynamoDbEnhancedAsyncClient.builder() | ||
| .dynamoDbClient(getDynamoDbAsyncClient()) | ||
| .extensions(AutoGeneratedUuidExtension.create()) | ||
| .build(); | ||
|
|
||
| private final DynamoDbAsyncTable<Record> mappedTable = enhancedAsyncClient.table(getConcreteTableName("table-name"), | ||
| TABLE_SCHEMA); | ||
|
|
||
| @Before | ||
| public void createTable() { | ||
| mappedTable.createTable(r -> r.provisionedThroughput(getDefaultProvisionedThroughput())).join(); | ||
| } | ||
|
|
||
| @After | ||
| public void deleteTable() { | ||
| getDynamoDbAsyncClient().deleteTable(r -> r.tableName(getConcreteTableName("table-name"))).join(); | ||
| } | ||
|
|
||
| @Test | ||
| public void putOverwrite_withExplicitUuidValues_shouldRegenerateWriteAlwaysAndNotPreserveWriteIfNotExists() { | ||
| Record seed = new Record(); | ||
| seed.setId("id1"); | ||
| mappedTable.putItem(seed).join(); | ||
|
|
||
| Record overwrite = new Record(); | ||
| overwrite.setId("id1"); | ||
| overwrite.setWriteAlwaysUuid("manual-a"); | ||
| overwrite.setWriteIfNotExistsUuid("manual-b"); | ||
| mappedTable.putItem(overwrite).join(); | ||
| Record persisted = mappedTable.getItem(r -> r.key(k -> k.partitionValue("id1"))).join(); | ||
|
|
||
| assertThat(persisted.getWriteAlwaysUuid()).isNotEqualTo("manual-a"); | ||
| UUID.fromString(persisted.getWriteAlwaysUuid()); | ||
| assertThat(persisted.getWriteIfNotExistsUuid()).isNotEqualTo("manual-b"); | ||
| assertThat(persisted.getWriteIfNotExistsUuid()).isNotNull(); | ||
| UUID.fromString(persisted.getWriteIfNotExistsUuid()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd argue this should be a bugfix rather than a feature - I notice there are more than just test changes in the PR - we're including additional bugfixes in the AutoGeneratedTimestampRecordExtensionnced - that should be mentioned in the description as well.