Bug: Incorrect error message for IdempotencyAlreadyInProgressError when sortKeyAttr is in use

[mikesnare]

Expected Behavior

When an IdempotencyAlreadyInProgressError is thrown, the error message should include the hashed key information.

Current Behavior

Currently, the error message only includes the partition key, but when a sortKeyAttr has been provided in the config the partition key is a static string. The leads to meaningless errors in the log that all say there's a IdempotencyAlreadyInProgressError error for the same key.

Code snippet

// none

Steps to Reproduce

Set up idempotency and provide a sortKeyAttr in the config, then send duplicate events in quick succession. You will get errors that reference only the partition key, but the partition key is static in this scenario and is therefore not very helpful for debugging.

Possible Solution

Include both the pk/sk, or detect the current scenario better and include the more meaningful one.

Powertools for AWS Lambda (TypeScript) version

latest

AWS Lambda function runtime

20.x

Packaging format used

npm

Execution logs

[boring-cyborg]

Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

[dreamorosi]

Hi @mikesnare, thank you for opening the issue.

I have tested the case you described and the error message we emit does not include the sort key, which as you mentioned, makes the error unhelpful.

From a first look, fixing this will require a bit of effort since our abstraction didn't account for keeping track of the sort key at all.

This is to say that we might need some time and the fix might not be ready for the next release (this Friday).

I'll report back here once I have updates.

Also, copying @aws-powertools/lambda-python-core, @aws-powertools/lambda-java-core, and @aws-powertools/lambda-dotnet-core - as far as I can tell this affects all runtimes.

[phipag]

I checked the issue for Java and it looks like the Java runtime is not affected by this. The DynamoDBPersistenceStore checks whether the user specified a sortKeyAttr or not. If the user specified a sort key, it will use this as idempotencyKey for all usages across the code-base including the exception message.

• The idempotencyKey is set here based on whether a sort key is specified: https://github.com/aws-powertools/powertools-lambda-java/blob/main/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/persistence/DynamoDBPersistenceStore.java#L272
• And it is retrieved for the exception message here: https://github.com/aws-powertools/powertools-lambda-java/blob/394ab0ccfc99488fafeea4497e4e6df79cb75047/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/internal/IdempotencyHandler.java#L161-L162

[dreamorosi]

Thank you for reporting @phipag - looks like @aws-powertools/lambda-dotnet-core is doing the same thing, so it might not be affected. I wasn't able to find the same for Python but I haven't tested it.

Regardless, I wanted to ask everyone's opinion on what would be the desired state for the error message.

Current status

There is already an execution in progress with idempotency key: idempotency#idemsk-aamorosi-IdempotencyFunction-onenncmn

As we have established, today in TS (& potentially Python) we include only the primary key which is not helpful.

Option 1 - Java/.NET - SK only

If we were to follow .NET/Java's implementations we would end up with a message like this:

There is already an execution in progress with idempotency key: idemsk-aamorosi-IdempotencyFunction-onenncmn#secAC8juGo8Z7F3rbCs37w==

This is the sort key only. While it's not enough to query the item from DynamoDB by itself, knowing that the primary key is static and equivalent to the static prefix (idempotency by default) + the function name, operators would have enough to go by.

Option 2 - Include both

A third option would be to modify all implementations so they include both primary and sort key, so that the message would be something like:

There is already an execution in progress with idempotency key: idempotency#idemsk-aamorosi-IdempotencyFunction-onenncmn and sk idemsk-aamorosi-IdempotencyFunction-onenncmn#secAC8juGo8Z7F3rbCs37w==

This would be the most unambiguous of the three in my opinion, but as I was alluding to earlier, it might require us revisiting some of the internal abstractions (IdempotencyRecord) and potentially making them less generic.

---

Option 1 is less exhaustive but quick to implement. Option 2 provides all the info needed to unequivocally query an item, but it might require more effort/changes to implement.

What do we think it's the best path forward?

[mikesnare]

First of all, thank you, @dreamorosi for looking into this so quickly. As a user of the package, I don't see a choice but to go with Option 2 even if it means a delay in getting the fix out (this is the first report, so it doesn't look like it's such a serious issue that waiting on the "correct" fix would be a problem). This is an error message, and an error message should be actionable. I don't think users should need to take half the keyset and then try to reverse engineer the other half in order to find the offending records -- even if the pk is static. My 2 cents, but I appreciate whatever fix you all come up with. Thanks again!

[dreamorosi]

I agree and as I was writing the comment above I was leaning towards the same option.

Before continuing I'd like to hear the opinion of other team members as well, since they'll be implementing it in their respective runtimes.

[phipag]

I agree that Option 2 sounds like the better option from the user perspective and I am also leaning towards this option. My reason is that we cannot assume that users know the Idempotency module in so much detail to understand that the PK is fixed if a SK is enabled.

From an implementation point-of-view we need to make sure that we do not introduce a coupling between the generic interface of a persistence store and DynamoDB. We would like to keep the ability of supporting custom data stores i.e. data stores without the concept of secondary indices or sort keys. For example, I would not modify the DataRecord representation by including a sortKey parameter. But these language-specific idiomatic details can be discussed in a separate issue for the specific language.

[leandrodamascena]

Thank you for opening this issue @mikesnare! I really appreciate when customers ask us to improve the developer experience and shorten the feedback loop, as this aligns with my primary mission/vision when building Powertools.

In Python, we have the same problem of showing an incorrect/incomplete message that doesn't help customers understand the error and probably leads them to waste time on further investigation.

I haven't spent time investigating how much time I need to refactor this, but it seems like it's very simple to do. I need to add a new key idempotency_sort_key here and propagate it to the Persistence layer + error message. Also add some tests.

I agree with option 2.

[dreamorosi]

I need to add a new key idempotency_sort_key here and propagate it to the Persistence layer + error message. Also add some tests.

The main concern, that was raised by @phipag earlier today when we talked about this, is that the DataRecord abstraction is expected to support any kind of persistence layer and not just DynamoDB. Not sure how you're handling this in Python - or maybe it's not a problem - but since you're the only one to have Redis then adding a DynamoDB specific key - even if optional - would kind of break the abstraction.

[dreamorosi]

I agree that Option 2 sounds like the better option from the user perspective and I am also leaning towards this option. My reason is that we cannot assume that users know the Idempotency module in so much detail to understand that the PK is fixed if a SK is enabled.

From an implementation point-of-view we need to make sure that we do not introduce a coupling between the generic interface of a persistence store and DynamoDB. We would like to keep the ability of supporting custom data stores i.e. data stores without the concept of secondary indices or sort keys. For example, I would not modify the DataRecord representation by including a sortKey parameter. But these language-specific idiomatic details can be discussed in a separate issue for the specific language.

I just re-read this and I see that it's the same thing I said in my comment above.

---

So I think we agree (pending @hjgraca) that we should include both keys - but I also agree with @phipag and we might want to keep things more in order even though the easy answer is to add an optional parameter to the DataRecord.

I also think this is the right place to discuss this, since we're all here.

[leandrodamascena]

I need to add a new key idempotency_sort_key here and propagate it to the Persistence layer + error message. Also add some tests.

The main concern, that was raised by @phipag earlier today when we talked about this, is that the DataRecord abstraction is expected to support any kind of persistence layer and not just DynamoDB. Not sure how you're handling this in Python - or maybe it's not a problem - but since you're the only one to have Redis then adding a DynamoDB specific key - even if optional - would kind of break the abstraction.

I see a lot of value in this perspective, but adding a new optional field wouldn't break the abstraction. If we start making the DataRecord class highly specialized for specific persistence storage, then it would be the right time to create backend-specific DataRecord classes while keeping the current DataRecord as the base class. In my opinion, changing the DataRecord implementation to add new classes specified now - just for a new key - would require a lot of work for a minimal benefit. We probably won't talk about this DataRecord class for quite some time, even if we add new backends.

[dreamorosi]

I agree with you @leandrodamascena - in my mind I was thinking of doing something like .NET & Java are doing - which is replacing the value of idempotency_key with a string like f"{actual_idempotencykey} and sk {item[self.sort_key]}" or similar, so that when the error is generated it includes both.

Although now that I wrote it, I don't love the idea and I think it's a foot gun for potential future improvements/refactoring. I think having a dedicated key/object for this in the DataRecord is better.

What about (last one, I promise) instead adding a persistence_layer_keys/persistence_layer_data generic dictionary/object that can be used by any persistence layer to add storage-specific keys to the DataRecord?

class DataRecord:
    """
    Data Class for idempotency records.
    """

    def __init__(
        self,
        idempotency_key: str,
        status: str = "",
        expiry_timestamp: int | None = None,
        in_progress_expiry_timestamp: int | None = None,
        response_data: str = "",
        payload_hash: str = "",
        persistence_layer_keys: dict = {},

This way we keep it generic but also maintainable.

[phipag]

I agree @leandrodamascena that we should be careful about overengineering this for – just adding a new key.

@dreamorosi This could be an option although I am not a big fan of having arbitrary untyped dictionaries like this. I know this is commonly used in Python (and TypeScript?) to circumvent the fact that overloading of methods is not straightforward possible/idiomatic.

If we think about our goal: We would like to display information (not change code behavior) to users that help them identify a record uniquely in any given data store (e.g. PK and SK in DynamoDB) without introducing a coupling between the data store and the handling of idempotency. Therefore, my suggestion is to keep a minimal (in the case of Java / .NET unmodified) DataRecord class and create data store specific sub-classes – only if the data store needs it. All behavioral code will still reference the (static) DataRecord type while the datastore specific class will overwrite the string representation (e.g. __str__ / __repr__ in Python)

// In Java, we can make this type package private (omit public keyword) to decouple it from the actual idempotency handling
class DynamoDBDataRecord extends DataRecord {
    private final String sk;
    private final String pk;

    public DynamoDBDataRecord(String idempotencyKey, Status status, long expiryTimestamp, String responseData,
            String payloadHash, String sk, String pk) {
        super(idempotencyKey, status, expiryTimestamp, responseData, payloadHash);
        this.sk = sk;
        this.pk = pk;
    }

    @Override
    public String toString() {
        return String.format("DynamoDBDataRecord(pk='%s', sk='%s', idempotencyKey='%s')", pk, sk, getIdempotencyKey());
    }

    // HashCode, Equals ...
}

public class IdempotencyHandler {
    // The static type stays DataRecord at compile time but will be DynamoDBDataRecord at runtime.
    private Object handleForStatus(DataRecord record) {
        if (INPROGRESS.equals(record.getStatus())) {
           // ...
            throw new IdempotencyAlreadyInProgressException(
                    // Note that we print the whole record object now instead of the idempotencyKey only.
                    // This will call .toString() on DynamoDBDataRecord.
                    "Execution already in progress with idempotency key: " + record);
        }
        // ...
    }
}

Current IdempotencyHandler: https://github.com/aws-powertools/powertools-lambda-java/blob/394ab0ccfc99488fafeea4497e4e6df79cb75047/powertools-idempotency/src/main/java/software/amazon/lambda/powertools/idempotency/internal/IdempotencyHandler.java#L161-L162

[leandrodamascena]

@dreamorosi This could be an option although I am not a big fan of having arbitrary untyped dictionaries like this. I know this is commonly used in Python (and TypeScript?) to circumvent the fact that overloading of methods is not straightforward possible/idiomatic.

I understand your idea @dreamorosi, but I'm also not a fan of having this arbitrary dictionary instead of adding the parameter explicitly. We're introducing a possible workaround that will serve the purpose of everything that isn't part of the initial DataRecord implementation. I'd rather we add the field to the DataRecord now, make it explicit, and evaluate if a new change is needed.

I'm waiting for your final comments so I can open an issue in Python and move forward with the fix.

Thanks for all discussion here, I really appreciate it.

[dreamorosi]

Ok, let's agree that each runtime can implement this how they see fit, as long as the error messages are the following:

• There is already an execution in progress with idempotency key: idempotencyKey and sort key: sk (when using composite key)
• There is already an execution in progress with idempotency key: idempotencyKey

In TS I'll likely implement it the same way that Leandro suggested and add a key to IdempotencyRecord, but I understand if Java & .NET want to have a more structured OOP-based approach.

[phipag]

Agreed. I created an issue in Java: aws-powertools/powertools-lambda-java#1800

[dreamorosi]

While working on this I noticed that the same issue affects also IdempotencyItemAlreadyExistsError that is thrown by the DynamoDBPersistenceLayer here.

I think this might affect Python as well, but not Java & .NET since they don't use ReturnValuesOnConditionCheckFailure when putting the item.

[phipag]

I will adjust the error message in Java after implementing aws-powertools/powertools-lambda-java#1784.

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

[github-actions]

This is now released under v2.17.0 version!