soto-project / soto

Swift SDK for AWS that works on Linux, macOS and iOS
https://soto.codes
Apache License 2.0
868 stars 81 forks source link

Fix additionalAttributeNames in UpdateItemCodableInput by mapping key array #673

Closed Andrea-Scuderi closed 1 year ago

Andrea-Scuderi commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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