Skip to content

fix: missing idempotency key should not persist any data #1201

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

Merged
merged 4 commits into from
Jun 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/utilities/idempotency.md
Original file line number Diff line number Diff line change
Expand Up @@ -708,10 +708,12 @@ By using **`withPayloadValidationJMESPath("amount")`**, we prevent this potentia

### Making idempotency key required

If you want to enforce that an idempotency key is required, you can set **`ThrowOnNoIdempotencyKey`** to `True`.
If you want to enforce that an idempotency key is required, you can set **`ThrowOnNoIdempotencyKey`** to `true`.

This means that we will throw **`IdempotencyKeyException`** if the evaluation of **`EventKeyJMESPath`** is `null`.

When set to `false` (the default), if the idempotency key is null, then the data is not persisted in the store.

=== "App.java"

```java hl_lines="9-10 13"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ private Object processIdempotency() throws Throwable {
persistenceStore.saveInProgress(data, Instant.now(), getRemainingTimeInMillis());
} catch (IdempotencyItemAlreadyExistsException iaee) {
DataRecord record = getIdempotencyRecord();
return handleForStatus(record);
if (record != null) {
return handleForStatus(record);
}
} catch (IdempotencyKeyException ike) {
throw ike;
} catch (Exception e) {
Expand All @@ -111,7 +113,7 @@ private OptionalInt getRemainingTimeInMillis() {
/**
* Retrieve the idempotency record from the persistence layer.
*
* @return the record if available
* @return the record if available, potentially null
*/
private DataRecord getIdempotencyRecord() {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,12 @@
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.Map;
import java.util.OptionalLong;
import java.util.Optional;
import java.util.OptionalInt;
import java.util.stream.Stream;
import java.util.Spliterators;
import java.util.OptionalLong;
import java.util.Spliterator;
import java.util.Spliterators;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;

import static software.amazon.lambda.powertools.core.internal.LambdaConstants.LAMBDA_FUNCTION_NAME_ENV;
Expand Down Expand Up @@ -117,8 +118,13 @@ public void saveSuccess(JsonNode data, Object result, Instant now) {
} else {
responseJson = writer.writeValueAsString(result);
}
Optional<String> hashedIdempotencyKey = getHashedIdempotencyKey(data);
if (!hashedIdempotencyKey.isPresent()) {
// missing idempotency key => non-idempotent transaction, we do not store the data, simply return
return;
}
DataRecord record = new DataRecord(
getHashedIdempotencyKey(data),
hashedIdempotencyKey.get(),
DataRecord.Status.COMPLETED,
getExpiryEpochSecond(now),
responseJson,
Expand All @@ -140,8 +146,13 @@ public void saveSuccess(JsonNode data, Object result, Instant now) {
* @param now
*/
public void saveInProgress(JsonNode data, Instant now, OptionalInt remainingTimeInMs) throws IdempotencyItemAlreadyExistsException {
String idempotencyKey = getHashedIdempotencyKey(data);
Optional<String> hashedIdempotencyKey = getHashedIdempotencyKey(data);
if (!hashedIdempotencyKey.isPresent()) {
// missing idempotency key => non-idempotent transaction, we do not store the data, simply return
return;
}

String idempotencyKey = hashedIdempotencyKey.get();
if (retrieveFromCache(idempotencyKey, now) != null) {
throw new IdempotencyItemAlreadyExistsException();
}
Expand Down Expand Up @@ -170,8 +181,13 @@ public void saveInProgress(JsonNode data, Instant now, OptionalInt remainingTime
* @param throwable The throwable thrown by the function
*/
public void deleteRecord(JsonNode data, Throwable throwable) {
String idemPotencyKey = getHashedIdempotencyKey(data);
Optional<String> hashedIdempotencyKey = getHashedIdempotencyKey(data);
if (!hashedIdempotencyKey.isPresent()) {
// missing idempotency key => non-idempotent transaction, we do not delete the data, simply return
return;
}

String idemPotencyKey = hashedIdempotencyKey.get();
LOG.debug("Function raised an exception {}. " +
"Clearing in progress record in persistence store for idempotency key: {}",
throwable.getClass(),
Expand All @@ -190,8 +206,13 @@ public void deleteRecord(JsonNode data, Throwable throwable) {
* @throws IdempotencyItemNotFoundException Exception thrown if no record exists in persistence store with the idempotency key
*/
public DataRecord getRecord(JsonNode data, Instant now) throws IdempotencyValidationException, IdempotencyItemNotFoundException {
String idemPotencyKey = getHashedIdempotencyKey(data);
Optional<String> hashedIdempotencyKey = getHashedIdempotencyKey(data);
if (!hashedIdempotencyKey.isPresent()) {
// missing idempotency key => non-idempotent transaction, we do not get the data, simply return nothing
return null;
}

String idemPotencyKey = hashedIdempotencyKey.get();
DataRecord cachedRecord = retrieveFromCache(idemPotencyKey, now);
if (cachedRecord != null) {
LOG.debug("Idempotency record found in cache with idempotency key: {}", idemPotencyKey);
Expand All @@ -211,7 +232,7 @@ public DataRecord getRecord(JsonNode data, Instant now) throws IdempotencyValida
* @param data incoming data
* @return Hashed representation of the data extracted by the jmespath expression
*/
private String getHashedIdempotencyKey(JsonNode data) {
private Optional<String> getHashedIdempotencyKey(JsonNode data) {
JsonNode node = data;

if (eventKeyJMESPath != null) {
Expand All @@ -221,13 +242,15 @@ private String getHashedIdempotencyKey(JsonNode data) {
if (isMissingIdemPotencyKey(node)) {
if (throwOnNoIdempotencyKey) {
throw new IdempotencyKeyException("No data found to create a hashed idempotency key");
} else {
LOG.warn("No data found to create a hashed idempotency key. JMESPath: {}", eventKeyJMESPath);
return Optional.empty();
}
LOG.warn("No data found to create a hashed idempotency key. JMESPath: {}", eventKeyJMESPath);
}

String hash = generateHash(node);
hash = functionName + "#" + hash;
return hash;
return Optional.of(hash);
}

private boolean isMissingIdemPotencyKey(JsonNode data) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,15 +143,15 @@ public void saveInProgress_jmespath_NotFound_shouldThrowException() {
}

@Test
public void saveInProgress_jmespath_NotFound_shouldNotThrowException() {
public void saveInProgress_jmespath_NotFound_shouldNotPersist() {
APIGatewayProxyRequestEvent event = EventLoader.loadApiGatewayRestEvent("apigw_event.json");
persistenceStore.configure(IdempotencyConfig.builder()
.withEventKeyJMESPath("unavailable")
.build(), "");
Instant now = Instant.now();
persistenceStore.saveInProgress(JsonConfig.get().getObjectMapper().valueToTree(event), now, OptionalInt.empty());
assertThat(dr.getStatus()).isEqualTo(DataRecord.Status.INPROGRESS);
assertThat(status).isEqualTo(1);
assertThat(dr).isNull();
assertThat(status).isEqualTo(-1);
}

@Test
Expand Down