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 2 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
@@ -0,0 +1,12 @@
package software.amazon.lambda.powertools.idempotency.exceptions;

/**
* Exception that occurs when the Idepmpotency Key is null or could not be found with the provided JMESPath
*/
public class NullIdempotencyKeyException extends Exception {
private static final long serialVersionUID = 5115004524004542891L;

public NullIdempotencyKeyException(String message) {
super(message);
}
}
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 @@ -21,10 +21,7 @@
import org.slf4j.LoggerFactory;
import software.amazon.awssdk.utils.StringUtils;
import software.amazon.lambda.powertools.idempotency.IdempotencyConfig;
import software.amazon.lambda.powertools.idempotency.exceptions.IdempotencyItemAlreadyExistsException;
import software.amazon.lambda.powertools.idempotency.exceptions.IdempotencyItemNotFoundException;
import software.amazon.lambda.powertools.idempotency.exceptions.IdempotencyKeyException;
import software.amazon.lambda.powertools.idempotency.exceptions.IdempotencyValidationException;
import software.amazon.lambda.powertools.idempotency.exceptions.*;
import software.amazon.lambda.powertools.idempotency.internal.cache.LRUCache;
import software.amazon.lambda.powertools.utilities.JsonConfig;

Expand All @@ -34,12 +31,8 @@
import java.security.NoSuchAlgorithmException;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.Map;
import java.util.OptionalLong;
import java.util.OptionalInt;
import java.util.*;
import java.util.stream.Stream;
import java.util.Spliterators;
import java.util.Spliterator;
import java.util.stream.StreamSupport;

import static software.amazon.lambda.powertools.core.internal.LambdaConstants.LAMBDA_FUNCTION_NAME_ENV;
Expand Down Expand Up @@ -130,6 +123,9 @@ public void saveSuccess(JsonNode data, Object result, Instant now) {
} catch (JsonProcessingException e) {
// TODO : throw ?
throw new RuntimeException("Error while serializing the response", e);
} catch (NullIdempotencyKeyException e) {
// missing idempotency key => non-idempotent transaction, we do not store the data, simply return
return;
}
}

Expand All @@ -140,7 +136,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);
String idempotencyKey;
try {
idempotencyKey = getHashedIdempotencyKey(data);
} catch (NullIdempotencyKeyException e) {
// missing idempotency key => non-idempotent transaction, we do not store the data, simply return
return;
}

if (retrieveFromCache(idempotencyKey, now) != null) {
throw new IdempotencyItemAlreadyExistsException();
Expand Down Expand Up @@ -170,7 +172,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);
String idemPotencyKey;
try {
idemPotencyKey = getHashedIdempotencyKey(data);
} catch (NullIdempotencyKeyException e) {
// missing idempotency key => non-idempotent transaction, we do not delete the data, simply return
return;
}

LOG.debug("Function raised an exception {}. " +
"Clearing in progress record in persistence store for idempotency key: {}",
Expand All @@ -190,7 +198,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);
String idemPotencyKey;
try {
idemPotencyKey = getHashedIdempotencyKey(data);
} catch (NullIdempotencyKeyException e) {
// missing idempotency key => non-idempotent transaction, we do not get the data, simply return nothing
return null;
}

DataRecord cachedRecord = retrieveFromCache(idemPotencyKey, now);
if (cachedRecord != null) {
Expand All @@ -211,7 +225,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 String getHashedIdempotencyKey(JsonNode data) throws NullIdempotencyKeyException {
JsonNode node = data;

if (eventKeyJMESPath != null) {
Expand All @@ -221,8 +235,10 @@ 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);
throw new NullIdempotencyKeyException("No data found to create a hashed idempotency key");
}
LOG.warn("No data found to create a hashed idempotency key. JMESPath: {}", eventKeyJMESPath);
}

String hash = generateHash(node);
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