Skip to content
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

Fix additionalAttributeNames in UpdateItemCodableInput by mapping key array #673

Merged

Conversation

Andrea-Scuderi
Copy link
Contributor

#671 has introduced a new method to initialize UpdateItemCodableInput, but it has introduced an issue.

Consider the following code:

private struct AdditionalAttributes: Encodable {
        let keyName: String
        let oldUpdatedAt: String
}
    
func updateItem<T: BreezeCodable>(item: T) async throws -> T {
        var item = item
        let oldUpdatedAt = item.updatedAt ?? ""
        let date = Date()
        item.updatedAt = date.iso8601
        let attributes = AdditionalAttributes(keyName: keyName, oldUpdatedAt: oldUpdatedAt)
        let input = try DynamoDB.UpdateItemCodableInput(
            additionalAttributes: attributes,
            conditionExpression: "attribute_exists(#keyName) AND #updatedAt = :oldUpdatedAt AND #createdAt = :createdAt",
            key: [keyName],
            tableName: tableName,
            updateItem: item
        )
        let _ = try await db.updateItem(input)
        return try await readItem(key: item.key)
}

The intention of the previous code is to update the item only when the key exists and when the record to update has the same createdAt and updatedAt data stored in DynamoDB. The update will change updatedAt with a new timestamp. In this way a client that doesn't have the same item cannot update the DynamoDB table and is forced to read the item before an update. (Optimistic locking)

But it leads to the following exception:

test_updateItem(): failed: caught error: "ValidationException: Value provided in ExpressionAttributeNames unused in expressions: keys: {#oldUpdatedAt}"

To fix the issue the code has be changed to include only the key in additionalAttributeNames and avoiding to add attributes that are not used.

The client code can be fixed by using:

private struct AdditionalAttributes: Encodable {
        let oldUpdatedAt: String
}
    
func updateItem<T: BreezeCodable>(item: T) async throws -> T {
        var item = item
        let oldUpdatedAt = item.updatedAt ?? ""
        let date = Date()
        item.updatedAt = date.iso8601
        let attributes = AdditionalAttributes(oldUpdatedAt: oldUpdatedAt)
        let input = try DynamoDB.UpdateItemCodableInput(
            additionalAttributes: attributes,
            conditionExpression: "attribute_exists(#\(keyName)) AND #updatedAt = :oldUpdatedAt AND #createdAt = :createdAt",
            key: [keyName],
            tableName: tableName,
            updateItem: item
        )
        let _ = try await db.updateItem(input)
        return try await readItem(key: item.key)
}

In the Unit Tests, the optimistic locking has been tested using id as key and age as condition to validate the record.

An alternative solution could be to add an explicit parameter to additionalAttributeNames.

@adam-fowler
Copy link
Member

Swift 5.8 failed https://github.com/soto-project/soto/actions/runs/5275592595/jobs/9541238015?pr=673#step:5:8784

Maybe localstack getting it wrong though

@Andrea-Scuderi
Copy link
Contributor Author

Swift 5.8 failed https://github.com/soto-project/soto/actions/runs/5275592595/jobs/9541238015?pr=673#step:5:8784

Maybe localstack getting it wrong though

The age in the example was updated to 33 to prove that conditional update works when executed against the last update.
All seems ok.
Still in doubt if It's better to have explicit parameter for additionalAttributeNames.
For implementing optimistic locking this seems enough.

@adam-fowler
Copy link
Member

Just re-running the tests. I can't get them to fail locally with local stack.

@adam-fowler adam-fowler merged commit edfafcf into soto-project:main Jun 16, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants