diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java index 5f211eebd..18a797c5e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java @@ -27,6 +27,7 @@ import org.whispersystems.textsecuregcm.util.UUIDUtil; import software.amazon.awssdk.services.dynamodb.DynamoDbClient; import software.amazon.awssdk.services.dynamodb.model.AttributeValue; import software.amazon.awssdk.services.dynamodb.model.CancellationReason; +import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; import software.amazon.awssdk.services.dynamodb.model.Delete; import software.amazon.awssdk.services.dynamodb.model.GetItemRequest; import software.amazon.awssdk.services.dynamodb.model.GetItemResponse; @@ -38,6 +39,8 @@ import software.amazon.awssdk.services.dynamodb.model.TransactWriteItemsRequest; import software.amazon.awssdk.services.dynamodb.model.TransactionCanceledException; import software.amazon.awssdk.services.dynamodb.model.TransactionConflictException; import software.amazon.awssdk.services.dynamodb.model.Update; +import software.amazon.awssdk.services.dynamodb.model.UpdateItemRequest; +import software.amazon.awssdk.services.dynamodb.model.UpdateItemResponse; public class Accounts extends AbstractDynamoDbStore { @@ -302,11 +305,10 @@ public class Accounts extends AbstractDynamoDbStore { public void update(Account account) throws ContestedOptimisticLockException { UPDATE_TIMER.record(() -> { - final List transactWriteItems = new ArrayList<>(2); + final UpdateItemRequest updateItemRequest; try { - final TransactWriteItem updateAccountWriteItem = TransactWriteItem.builder() - .update(Update.builder() + updateItemRequest = UpdateItemRequest.builder() .tableName(accountsTableName) .key(Map.of(KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) .updateExpression("SET #data = :data, #cds = :cds, #pni = :pni ADD #version :version_increment") @@ -322,32 +324,21 @@ public class Accounts extends AbstractDynamoDbStore { ":version", AttributeValues.fromInt(account.getVersion()), ":version_increment", AttributeValues.fromInt(1), ":pni", AttributeValues.fromUUID(account.getPhoneNumberIdentifier()))) - .build()) - .build();; - - transactWriteItems.add(updateAccountWriteItem); + .build(); } catch (JsonProcessingException e) { throw new IllegalArgumentException(e); } try { - client.transactWriteItems(TransactWriteItemsRequest.builder() - .transactItems(transactWriteItems) - .build()); - - account.setVersion(account.getVersion() + 1); + final UpdateItemResponse response = client.updateItem(updateItemRequest); + account.setVersion(AttributeValues.getInt(response.attributes(), "V", account.getVersion() + 1)); } catch (final TransactionConflictException e) { throw new ContestedOptimisticLockException(); - } catch (final TransactionCanceledException e) { - - if (e.cancellationReasons().size() > 1 && "ConditionalCheckFailed".equals(e.cancellationReasons().get(1).code())) { - log.error("Conflicting phone number mapping exists for account {}, PNI {}", account.getUuid(), account.getPhoneNumberIdentifier()); - throw e; - } - - // We can infer an optimistic locking failure if the UUID is known + } catch (final ConditionalCheckFailedException e) { + // the exception doesn't give details about which condition failed, + // but we can infer it was an optimistic locking failure if the UUID is known throw getByAccountIdentifier(account.getUuid()).isPresent() ? new ContestedOptimisticLockException() : e; } }); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java index 217f658cf..96faa029c 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -40,6 +40,7 @@ import software.amazon.awssdk.services.dynamodb.DynamoDbClient; import software.amazon.awssdk.services.dynamodb.model.AttributeDefinition; import software.amazon.awssdk.services.dynamodb.model.AttributeValue; import software.amazon.awssdk.services.dynamodb.model.CancellationReason; +import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; import software.amazon.awssdk.services.dynamodb.model.CreateTableRequest; import software.amazon.awssdk.services.dynamodb.model.GetItemRequest; import software.amazon.awssdk.services.dynamodb.model.GetItemResponse; @@ -329,7 +330,7 @@ class AccountsTest { device = generateDevice(1); Account unknownAccount = generateAccount("+14151113333", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(device)); - assertThatThrownBy(() -> accounts.update(unknownAccount)).isInstanceOfAny(TransactionCanceledException.class); + assertThatThrownBy(() -> accounts.update(unknownAccount)).isInstanceOfAny(ConditionalCheckFailedException.class); account.setProfileName("name"); @@ -358,38 +359,14 @@ class AccountsTest { accounts = new Accounts(dynamoDbClient, dynamoDbExtension.getTableName(), NUMBER_CONSTRAINT_TABLE_NAME, PNI_CONSTRAINT_TABLE_NAME, SCAN_PAGE_SIZE); - when(dynamoDbClient.transactWriteItems(any(TransactWriteItemsRequest.class))) + when(dynamoDbClient.updateItem(any(UpdateItemRequest.class))) .thenThrow(TransactionConflictException.class); - Device device = generateDevice(1); - Account account = generateAccount("+14151112222", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(device)); + Account account = generateAccount("+14151112222", UUID.randomUUID(), UUID.randomUUID()); assertThatThrownBy(() -> accounts.update(account)).isInstanceOfAny(ContestedOptimisticLockException.class); } - @Test - // TODO Remove after initial migration is complete - void testUpdateWithMockTransactionCancellationException() { - - final DynamoDbClient dynamoDbClient = mock(DynamoDbClient.class); - accounts = new Accounts(dynamoDbClient, - dynamoDbExtension.getTableName(), NUMBER_CONSTRAINT_TABLE_NAME, PNI_CONSTRAINT_TABLE_NAME, SCAN_PAGE_SIZE); - - when(dynamoDbClient.transactWriteItems(any(TransactWriteItemsRequest.class))) - .thenThrow(TransactionCanceledException.builder() - .cancellationReasons(CancellationReason.builder() - .code("Test") - .build()) - .build()); - - when(dynamoDbClient.getItem(any(GetItemRequest.class))).thenReturn(GetItemResponse.builder().build()); - - Device device = generateDevice(1); - Account account = generateAccount("+14151112222", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(device)); - - assertThatThrownBy(() -> accounts.update(account)).isInstanceOfAny(TransactionCanceledException.class); - } - @Test void testRetrieveFrom() { List users = new ArrayList<>();