From 14261386a5650c7a22ef5b42e38bc4a4497fc508 Mon Sep 17 00:00:00 2001 From: Scott Gerring Date: Wed, 12 Jul 2023 17:24:14 +0200 Subject: [PATCH 1/5] Idemp should be fixed, test isnt --- .../persistence/DynamoDBPersistenceStore.java | 3 ++- .../persistence/DynamoDBPersistenceStoreTest.java | 13 ++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java b/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java index 47ddf4c5c..1776ac38a 100644 --- a/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java +++ b/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java @@ -152,6 +152,7 @@ public void putRecord(DataRecord record, Instant now) throws IdempotencyItemAlre Map expressionAttributeValues = Stream.of( new AbstractMap.SimpleEntry<>(":now", AttributeValue.builder().n(String.valueOf(now.getEpochSecond())).build()), + new AbstractMap.SimpleEntry<>(":now_milliseconds", AttributeValue.builder().n(String.valueOf(now.getEpochSecond() * 1000)).build()), new AbstractMap.SimpleEntry<>(":inprogress", AttributeValue.builder().s(INPROGRESS.toString()).build()) ).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); @@ -160,7 +161,7 @@ public void putRecord(DataRecord record, Instant now) throws IdempotencyItemAlre PutItemRequest.builder() .tableName(tableName) .item(item) - .conditionExpression("attribute_not_exists(#id) OR #expiry < :now OR (attribute_exists(#in_progress_expiry) AND #in_progress_expiry < :now AND #status = :inprogress)") + .conditionExpression("attribute_not_exists(#id) OR #expiry < :now OR (attribute_exists(#in_progress_expiry) AND #in_progress_expiry < :now_milliseconds AND #status = :inprogress)") .expressionAttributeNames(expressionAttributeNames) .expressionAttributeValues(expressionAttributeValues) .build() diff --git a/powertools-idempotency/src/test/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStoreTest.java b/powertools-idempotency/src/test/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStoreTest.java index 86e35cd33..793ccfa46 100644 --- a/powertools-idempotency/src/test/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStoreTest.java +++ b/powertools-idempotency/src/test/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStoreTest.java @@ -168,21 +168,20 @@ public void putRecord_shouldThrowIdempotencyItemAlreadyExistsException_IfRecordA // WHEN: call putRecord long expiry2 = now.plus(3600, ChronoUnit.SECONDS).getEpochSecond(); - assertThatThrownBy(() -> dynamoDBPersistenceStore.putRecord( + dynamoDBPersistenceStore.putRecord( new DataRecord("key", DataRecord.Status.INPROGRESS, expiry2, - null, + "Fake Data 2", null - ), now) - ).isInstanceOf(IdempotencyItemAlreadyExistsException.class); + ), now); - // THEN: item was not updated, retrieve the initial one + // THEN: item was update updated, retrieve the new one Map itemInDb = client.getItem(GetItemRequest.builder().tableName(TABLE_NAME).key(key).build()).item(); assertThat(itemInDb).isNotNull(); assertThat(itemInDb.get("status").s()).isEqualTo("INPROGRESS"); - assertThat(itemInDb.get("expiration").n()).isEqualTo(String.valueOf(expiry)); - assertThat(itemInDb.get("data").s()).isEqualTo("Fake Data"); + assertThat(itemInDb.get("expiration").n()).isEqualTo(String.valueOf(expiry2)); + assertThat(itemInDb.get("data").s()).isEqualTo("Fake Data 2"); } From f4baaa37a6bedceafa92e11ca4379cbdff2611a8 Mon Sep 17 00:00:00 2001 From: Scott Gerring Date: Thu, 13 Jul 2023 08:55:30 +0200 Subject: [PATCH 2/5] Fix some of the tests. Need to go through the rest still --- .../DynamoDBPersistenceStoreTest.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/powertools-idempotency/src/test/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStoreTest.java b/powertools-idempotency/src/test/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStoreTest.java index 793ccfa46..768da2eaa 100644 --- a/powertools-idempotency/src/test/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStoreTest.java +++ b/powertools-idempotency/src/test/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStoreTest.java @@ -95,7 +95,7 @@ public void putRecord_shouldCreateRecordInDynamoDB_IfLambdaWasInProgressAndTimed Map item = new HashMap<>(key); Instant now = Instant.now(); long expiry = now.plus(30, ChronoUnit.SECONDS).getEpochSecond(); - long progressExpiry = now.minus(30, ChronoUnit.SECONDS).getEpochSecond(); + long progressExpiry = now.minus(30, ChronoUnit.SECONDS).toEpochMilli(); item.put("expiration", AttributeValue.builder().n(String.valueOf(expiry)).build()); item.put("status", AttributeValue.builder().s(DataRecord.Status.INPROGRESS.toString()).build()); item.put("data", AttributeValue.builder().s("Fake Data").build()); @@ -152,14 +152,14 @@ public void putRecord_shouldThrowIdempotencyItemAlreadyExistsException_IfRecordA } @Test - public void putRecord_shouldThrowIdempotencyItemAlreadyExistsException_IfRecordAlreadyExistAndProgressNotExpiredAfterLambdaTimedOut() { + public void putRecord_shouldBlockUpdate_IfRecordAlreadyExistAndProgressNotExpiredAfterLambdaTimedOut() { key = Collections.singletonMap("id", AttributeValue.builder().s("key").build()); // GIVEN: Insert a fake item with same id Map item = new HashMap<>(key); Instant now = Instant.now(); long expiry = now.plus(30, ChronoUnit.SECONDS).getEpochSecond(); // not expired - long progressExpiry = now.plus(30, ChronoUnit.SECONDS).getEpochSecond(); // not expired + long progressExpiry = now.plus(30, ChronoUnit.SECONDS).toEpochMilli(); // not expired item.put("expiration", AttributeValue.builder().n(String.valueOf(expiry)).build()); item.put("status", AttributeValue.builder().s(DataRecord.Status.INPROGRESS.toString()).build()); item.put("data", AttributeValue.builder().s("Fake Data").build()); @@ -168,20 +168,21 @@ public void putRecord_shouldThrowIdempotencyItemAlreadyExistsException_IfRecordA // WHEN: call putRecord long expiry2 = now.plus(3600, ChronoUnit.SECONDS).getEpochSecond(); - dynamoDBPersistenceStore.putRecord( + assertThatThrownBy(() -> dynamoDBPersistenceStore.putRecord( new DataRecord("key", DataRecord.Status.INPROGRESS, expiry2, "Fake Data 2", null - ), now); + ), now)) + .isInstanceOf(IdempotencyItemAlreadyExistsException.class); - // THEN: item was update updated, retrieve the new one + // THEN: item was not updated, retrieve the initial one Map itemInDb = client.getItem(GetItemRequest.builder().tableName(TABLE_NAME).key(key).build()).item(); assertThat(itemInDb).isNotNull(); assertThat(itemInDb.get("status").s()).isEqualTo("INPROGRESS"); - assertThat(itemInDb.get("expiration").n()).isEqualTo(String.valueOf(expiry2)); - assertThat(itemInDb.get("data").s()).isEqualTo("Fake Data 2"); + assertThat(itemInDb.get("expiration").n()).isEqualTo(String.valueOf(expiry)); + assertThat(itemInDb.get("data").s()).isEqualTo("Fake Data"); } From 5cba9080b9c0041f75aa587be1d57077e16dbeef Mon Sep 17 00:00:00 2001 From: Scott Gerring Date: Thu, 13 Jul 2023 12:47:58 +0200 Subject: [PATCH 3/5] Add some docs --- .../idempotency/persistence/DataRecord.java | 21 +++++++++++++++++++ .../persistence/DynamoDBPersistenceStore.java | 13 ++++++++++++ 2 files changed, 34 insertions(+) diff --git a/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DataRecord.java b/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DataRecord.java index 934ec3d09..0d5438cd1 100644 --- a/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DataRecord.java +++ b/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DataRecord.java @@ -26,9 +26,30 @@ public class DataRecord { private final String idempotencyKey; private final String status; + + /** + * This field is control how long the result of the idempotent + * event is cached. It is stored in _seconds since epoch_. + * + * DynamoDB's TTL mechanism is used to remove the record once the + * expiry has been reached, and subsequent execution of the request + * will be permitted. The user must configure this on their table. + */ private final long expiryTimestamp; private final String responseData; private final String payloadHash; + + /** + * The in-progress field is set to the remaining lambda execution time + * when the record is created. + * This field is stored in _milliseconds since epoch_. + * + * This ensures that: + * + * 1/ other concurrently executing requests are blocked from starting + * 2/ if a lambda times out, subsequent requests will be allowed again, despite + * the fact that the idempotency record is already in the table + */ private final OptionalLong inProgressExpiryTimestamp; public DataRecord(String idempotencyKey, Status status, long expiryTimestamp, String responseData, String payloadHash) { diff --git a/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java b/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java index 1776ac38a..79594d095 100644 --- a/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java +++ b/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java @@ -126,6 +126,19 @@ public DataRecord getRecord(String idempotencyKey) throws IdempotencyItemNotFoun return itemToRecord(response.item()); } + /** + * Store's the given idempotency record in the DDB store. If there + * is an existing record that has expired - either due to the + * cache expiry or due to the in_progress_expiry - the record + * will be overwritten and the idempotent operation can continue. + * + * Note: This method writes only expiry and status information - not + * the results of the operation itself. + * + * @param record DataRecord instance to store + * @param now + * @throws IdempotencyItemAlreadyExistsException + */ @Override public void putRecord(DataRecord record, Instant now) throws IdempotencyItemAlreadyExistsException { Map item = new HashMap<>(getKey(record.getIdempotencyKey())); From 40390f4312d0b5b65114fb15b02b1421367ef335 Mon Sep 17 00:00:00 2001 From: Scott Gerring Date: Thu, 13 Jul 2023 16:15:18 +0200 Subject: [PATCH 4/5] Update powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DataRecord.java MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jérôme Van Der Linden <117538+jeromevdl@users.noreply.github.com> --- .../lambda/powertools/idempotency/persistence/DataRecord.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DataRecord.java b/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DataRecord.java index 0d5438cd1..54001c449 100644 --- a/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DataRecord.java +++ b/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DataRecord.java @@ -28,7 +28,7 @@ public class DataRecord { private final String status; /** - * This field is control how long the result of the idempotent + * This field is controlling how long the result of the idempotent * event is cached. It is stored in _seconds since epoch_. * * DynamoDB's TTL mechanism is used to remove the record once the From b975178fc1e20bf65f49171f900090098d97d4a3 Mon Sep 17 00:00:00 2001 From: Scott Gerring Date: Thu, 13 Jul 2023 16:19:57 +0200 Subject: [PATCH 5/5] Address review comment --- .../idempotency/persistence/DynamoDBPersistenceStore.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java b/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java index 79594d095..6b5d0fcb2 100644 --- a/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java +++ b/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java @@ -165,7 +165,7 @@ public void putRecord(DataRecord record, Instant now) throws IdempotencyItemAlre Map expressionAttributeValues = Stream.of( new AbstractMap.SimpleEntry<>(":now", AttributeValue.builder().n(String.valueOf(now.getEpochSecond())).build()), - new AbstractMap.SimpleEntry<>(":now_milliseconds", AttributeValue.builder().n(String.valueOf(now.getEpochSecond() * 1000)).build()), + new AbstractMap.SimpleEntry<>(":now_milliseconds", AttributeValue.builder().n(String.valueOf(now.toEpochMilli())).build()), new AbstractMap.SimpleEntry<>(":inprogress", AttributeValue.builder().s(INPROGRESS.toString()).build()) ).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));