From 6a71d369e20969548fcd78b63ff5f3e16fd4c818 Mon Sep 17 00:00:00 2001 From: Chris Eager <79161849+eager-signal@users.noreply.github.com> Date: Tue, 21 Sep 2021 15:25:16 -0700 Subject: [PATCH] More Accounts cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove `AccountStore` * Clean up `AccountsDynamoDb#delete` * Rename `AccountsDynamoDb` → `Accounts` * Remove unused configuration * Move Accounts scan page size to static configuration * Remove disabled tests and related methods --- .../WhisperServerConfiguration.java | 36 --- .../textsecuregcm/WhisperServerService.java | 15 +- .../AccountsDynamoDbConfiguration.java | 10 + ...ccountsDynamoDbMigrationConfiguration.java | 15 -- .../dynamic/DynamicConfiguration.java | 7 - .../storage/AccountDatabaseCrawler.java | 4 +- .../textsecuregcm/storage/AccountStore.java | 17 -- .../{AccountsDynamoDb.java => Accounts.java} | 98 ++++--- .../storage/AccountsManager.java | 56 ++-- .../workers/DeleteUserCommand.java | 14 +- .../SetUserDiscoverabilityCommand.java | 16 +- .../dynamic/DynamicConfigurationTest.java | 23 -- ...AccountDatabaseCrawlerIntegrationTest.java | 4 - ...ConcurrentModificationIntegrationTest.java | 61 ++--- ...ntsDynamoDbTest.java => AccountsTest.java} | 130 +++++----- .../tests/storage/AccountsManagerTest.java | 239 ++++-------------- 16 files changed, 232 insertions(+), 513 deletions(-) delete mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicAccountsDynamoDbMigrationConfiguration.java delete mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountStore.java rename service/src/main/java/org/whispersystems/textsecuregcm/storage/{AccountsDynamoDb.java => Accounts.java} (86%) rename service/src/test/java/org/whispersystems/textsecuregcm/storage/{AccountsDynamoDbTest.java => AccountsTest.java} (79%) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java index 4de9003ab..79c87e89f 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java @@ -109,11 +109,6 @@ public class WhisperServerConfiguration extends Configuration { @JsonProperty private AccountDatabaseCrawlerConfiguration accountDatabaseCrawler; - @NotNull - @Valid - @JsonProperty - private AccountDatabaseCrawlerConfiguration dynamoDbMigrationCrawler; - @NotNull @Valid @JsonProperty @@ -149,21 +144,6 @@ public class WhisperServerConfiguration extends Configuration { @JsonProperty private AccountsDynamoDbConfiguration accountsDynamoDb; - @Valid - @NotNull - @JsonProperty - private DynamoDbConfiguration migrationDeletedAccountsDynamoDb; - - @Valid - @NotNull - @JsonProperty - private DynamoDbConfiguration migrationMismatchedAccountsDynamoDb; - - @Valid - @NotNull - @JsonProperty - private DynamoDbConfiguration migrationRetryAccountsDynamoDb; - @Valid @NotNull @JsonProperty @@ -376,10 +356,6 @@ public class WhisperServerConfiguration extends Configuration { return accountDatabaseCrawler; } - public AccountDatabaseCrawlerConfiguration getDynamoDbMigrationCrawlerConfiguration() { - return dynamoDbMigrationCrawler; - } - public MessageCacheConfiguration getMessageCacheConfiguration() { return messageCache; } @@ -408,18 +384,6 @@ public class WhisperServerConfiguration extends Configuration { return accountsDynamoDb; } - public DynamoDbConfiguration getMigrationDeletedAccountsDynamoDbConfiguration() { - return migrationDeletedAccountsDynamoDb; - } - - public DynamoDbConfiguration getMigrationMismatchedAccountsDynamoDbConfiguration() { - return migrationMismatchedAccountsDynamoDb; - } - - public DynamoDbConfiguration getMigrationRetryAccountsDynamoDbConfiguration() { - return migrationRetryAccountsDynamoDb; - } - public DeletedAccountsDynamoDbConfiguration getDeletedAccountsDynamoDbConfiguration() { return deletedAccountsDynamoDb; } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index f02fa0e62..a37811c60 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -154,7 +154,7 @@ import org.whispersystems.textsecuregcm.storage.AccountCleaner; import org.whispersystems.textsecuregcm.storage.AccountDatabaseCrawler; import org.whispersystems.textsecuregcm.storage.AccountDatabaseCrawlerCache; import org.whispersystems.textsecuregcm.storage.AccountDatabaseCrawlerListener; -import org.whispersystems.textsecuregcm.storage.AccountsDynamoDb; +import org.whispersystems.textsecuregcm.storage.Accounts; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.ContactDiscoveryWriter; import org.whispersystems.textsecuregcm.storage.DeletedAccounts; @@ -337,10 +337,10 @@ public class WhisperServerService extends Application 0) { - logger.info("Sleeping {}ms", sleepIntervalMs); + logger.debug("Sleeping {}ms", sleepIntervalMs); sleepWhileRunning(sleepIntervalMs); } } finally { @@ -135,7 +135,7 @@ public class AccountDatabaseCrawler implements Managed, Runnable { cacheLastUuid(Optional.empty()); cache.setAccelerated(false); } else { - logger.info("Processing chunk"); + logger.debug("Processing chunk"); try { for (AccountDatabaseCrawlerListener listener : listeners) { listener.timeAndProcessCrawlChunk(fromUuid, chunkAccounts.getAccounts()); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountStore.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountStore.java deleted file mode 100644 index 065c97bf6..000000000 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountStore.java +++ /dev/null @@ -1,17 +0,0 @@ -package org.whispersystems.textsecuregcm.storage; - -import java.util.Optional; -import java.util.UUID; - -public interface AccountStore { - - boolean create(Account account); - - void update(Account account) throws ContestedOptimisticLockException; - - Optional get(String number); - - Optional get(UUID uuid); - - void delete(final UUID uuid); -} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsDynamoDb.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java similarity index 86% rename from service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsDynamoDb.java rename to service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java index 527c16e98..fddd558d1 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsDynamoDb.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java @@ -38,7 +38,7 @@ import software.amazon.awssdk.services.dynamodb.model.TransactionConflictExcepti import software.amazon.awssdk.services.dynamodb.model.UpdateItemRequest; import software.amazon.awssdk.services.dynamodb.model.UpdateItemResponse; -public class AccountsDynamoDb extends AbstractDynamoDbStore implements AccountStore { +public class Accounts extends AbstractDynamoDbStore { // uuid, primary key static final String KEY_ACCOUNT_UUID = "U"; @@ -56,25 +56,28 @@ public class AccountsDynamoDb extends AbstractDynamoDbStore implements AccountSt private final String phoneNumbersTableName; private final String accountsTableName; - private static final Timer CREATE_TIMER = Metrics.timer(name(AccountsDynamoDb.class, "create")); - private static final Timer UPDATE_TIMER = Metrics.timer(name(AccountsDynamoDb.class, "update")); - private static final Timer GET_BY_NUMBER_TIMER = Metrics.timer(name(AccountsDynamoDb.class, "getByNumber")); - private static final Timer GET_BY_UUID_TIMER = Metrics.timer(name(AccountsDynamoDb.class, "getByUuid")); - private static final Timer GET_ALL_FROM_START_TIMER = Metrics.timer(name(AccountsDynamoDb.class, "getAllFrom")); - private static final Timer GET_ALL_FROM_OFFSET_TIMER = Metrics.timer(name(AccountsDynamoDb.class, "getAllFromOffset")); - private static final Timer DELETE_TIMER = Metrics.timer(name(AccountsDynamoDb.class, "delete")); + private final int scanPageSize; + + private static final Timer CREATE_TIMER = Metrics.timer(name(Accounts.class, "create")); + private static final Timer UPDATE_TIMER = Metrics.timer(name(Accounts.class, "update")); + private static final Timer GET_BY_NUMBER_TIMER = Metrics.timer(name(Accounts.class, "getByNumber")); + private static final Timer GET_BY_UUID_TIMER = Metrics.timer(name(Accounts.class, "getByUuid")); + private static final Timer GET_ALL_FROM_START_TIMER = Metrics.timer(name(Accounts.class, "getAllFrom")); + private static final Timer GET_ALL_FROM_OFFSET_TIMER = Metrics.timer(name(Accounts.class, "getAllFromOffset")); + private static final Timer DELETE_TIMER = Metrics.timer(name(Accounts.class, "delete")); - public AccountsDynamoDb(DynamoDbClient client, String accountsTableName, String phoneNumbersTableName) { + public Accounts(DynamoDbClient client, String accountsTableName, String phoneNumbersTableName, + final int scanPageSize) { super(client); this.client = client; this.phoneNumbersTableName = phoneNumbersTableName; this.accountsTableName = accountsTableName; + this.scanPageSize = scanPageSize; } - @Override public boolean create(Account account) { return CREATE_TIMER.record(() -> { @@ -115,7 +118,7 @@ public class AccountsDynamoDb extends AbstractDynamoDbStore implements AccountSt } if ("TransactionConflict".equals(accountCancellationReason.code())) { - // this should only happen during concurrent update()s for an account migration + // this should only happen if two clients manage to make concurrent create() calls throw new ContestedOptimisticLockException(); } @@ -164,7 +167,6 @@ public class AccountsDynamoDb extends AbstractDynamoDbStore implements AccountSt .build(); } - @Override public void update(Account account) throws ContestedOptimisticLockException { UPDATE_TIMER.record(() -> { UpdateItemRequest updateItemRequest; @@ -207,8 +209,6 @@ public class AccountsDynamoDb extends AbstractDynamoDbStore implements AccountSt }); } - - @Override public Optional get(String number) { return GET_BY_NUMBER_TIMER.record(() -> { @@ -220,7 +220,7 @@ public class AccountsDynamoDb extends AbstractDynamoDbStore implements AccountSt return Optional.ofNullable(response.item()) .map(item -> item.get(KEY_ACCOUNT_UUID)) .map(uuid -> accountByUuid(uuid)) - .map(AccountsDynamoDb::fromItem); + .map(Accounts::fromItem); }); } @@ -233,29 +233,52 @@ public class AccountsDynamoDb extends AbstractDynamoDbStore implements AccountSt return r.item().isEmpty() ? null : r.item(); } - @Override public Optional get(UUID uuid) { return GET_BY_UUID_TIMER.record(() -> Optional.ofNullable(accountByUuid(AttributeValues.fromUUID(uuid))) - .map(AccountsDynamoDb::fromItem)); + .map(Accounts::fromItem)); } - @Override public void delete(UUID uuid) { - DELETE_TIMER.record(() -> delete(uuid, true)); + DELETE_TIMER.record(() -> { + + Optional maybeAccount = get(uuid); + + maybeAccount.ifPresent(account -> { + + TransactWriteItem phoneNumberDelete = TransactWriteItem.builder() + .delete(Delete.builder() + .tableName(phoneNumbersTableName) + .key(Map.of(ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()))) + .build()) + .build(); + + TransactWriteItem accountDelete = TransactWriteItem.builder() + .delete(Delete.builder() + .tableName(accountsTableName) + .key(Map.of(KEY_ACCOUNT_UUID, AttributeValues.fromUUID(uuid))) + .build()) + .build(); + + TransactWriteItemsRequest request = TransactWriteItemsRequest.builder() + .transactItems(phoneNumberDelete, accountDelete).build(); + + client.transactWriteItems(request); + }); + }); } - public AccountCrawlChunk getAllFrom(final UUID from, final int maxCount, final int pageSize) { + public AccountCrawlChunk getAllFrom(final UUID from, final int maxCount) { final ScanRequest.Builder scanRequestBuilder = ScanRequest.builder() - .limit(pageSize) + .limit(scanPageSize) .exclusiveStartKey(Map.of(KEY_ACCOUNT_UUID, AttributeValues.fromUUID(from))); return scanForChunk(scanRequestBuilder, maxCount, GET_ALL_FROM_OFFSET_TIMER); } - public AccountCrawlChunk getAllFromStart(final int maxCount, final int pageSize) { + public AccountCrawlChunk getAllFromStart(final int maxCount) { final ScanRequest.Builder scanRequestBuilder = ScanRequest.builder() - .limit(pageSize); + .limit(scanPageSize); return scanForChunk(scanRequestBuilder, maxCount, GET_ALL_FROM_START_TIMER); } @@ -266,39 +289,12 @@ public class AccountsDynamoDb extends AbstractDynamoDbStore implements AccountSt final List accounts = timer.record(() -> scan(scanRequestBuilder.build(), maxCount) .stream() - .map(AccountsDynamoDb::fromItem) + .map(Accounts::fromItem) .collect(Collectors.toList())); return new AccountCrawlChunk(accounts, accounts.size() > 0 ? accounts.get(accounts.size() - 1).getUuid() : null); } - private void delete(UUID uuid, boolean saveInDeletedAccountsTable) { - - Optional maybeAccount = get(uuid); - - maybeAccount.ifPresent(account -> { - - TransactWriteItem phoneNumberDelete = TransactWriteItem.builder() - .delete(Delete.builder() - .tableName(phoneNumbersTableName) - .key(Map.of(ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()))) - .build()) - .build(); - - TransactWriteItem accountDelete = TransactWriteItem.builder() - .delete(Delete.builder() - .tableName(accountsTableName) - .key(Map.of(KEY_ACCOUNT_UUID, AttributeValues.fromUUID(uuid))) - .build()) - .build(); - - TransactWriteItemsRequest request = TransactWriteItemsRequest.builder() - .transactItems(phoneNumberDelete, accountDelete).build(); - - client.transactWriteItems(request); - }); - } - private static String extractCancellationReasonCodes(final TransactionCanceledException exception) { return exception.cancellationReasons().stream() .map(CancellationReason::code) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java index 9f85a605e..1c6122e42 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -18,15 +18,12 @@ import io.lettuce.core.cluster.api.sync.RedisAdvancedClusterCommands; import io.micrometer.core.instrument.Metrics; import io.micrometer.core.instrument.Tags; import java.io.IOException; -import java.util.Arrays; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; -import java.util.stream.Collectors; -import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.whispersystems.textsecuregcm.auth.AuthenticationCredentials; @@ -65,7 +62,7 @@ public class AccountsManager { private final Logger logger = LoggerFactory.getLogger(AccountsManager.class); - private final AccountsDynamoDb accountsDynamoDb; + private final Accounts accounts; private final FaultTolerantRedisCluster cacheCluster; private final DeletedAccountsManager deletedAccountsManager; private final DirectoryQueue directoryQueue; @@ -78,8 +75,6 @@ public class AccountsManager { private final SecureBackupClient secureBackupClient; private final ObjectMapper mapper; - private final DynamicConfigurationManager dynamicConfigurationManager; - public enum DeletionReason { ADMIN_DELETED("admin"), EXPIRED ("expired"), @@ -92,7 +87,7 @@ public class AccountsManager { } } - public AccountsManager(AccountsDynamoDb accountsDynamoDb, FaultTolerantRedisCluster cacheCluster, + public AccountsManager(Accounts accounts, FaultTolerantRedisCluster cacheCluster, final DeletedAccountsManager deletedAccountsManager, final DirectoryQueue directoryQueue, final KeysDynamoDb keysDynamoDb, final MessagesManager messagesManager, @@ -100,22 +95,20 @@ public class AccountsManager { final ProfilesManager profilesManager, final StoredVerificationCodeManager pendingAccounts, final SecureStorageClient secureStorageClient, - final SecureBackupClient secureBackupClient, - final DynamicConfigurationManager dynamicConfigurationManager) { - this.accountsDynamoDb = accountsDynamoDb; - this.cacheCluster = cacheCluster; + final SecureBackupClient secureBackupClient) { + this.accounts = accounts; + this.cacheCluster = cacheCluster; this.deletedAccountsManager = deletedAccountsManager; - this.directoryQueue = directoryQueue; - this.keysDynamoDb = keysDynamoDb; + this.directoryQueue = directoryQueue; + this.keysDynamoDb = keysDynamoDb; this.messagesManager = messagesManager; this.usernamesManager = usernamesManager; - this.profilesManager = profilesManager; + this.profilesManager = profilesManager; this.pendingAccounts = pendingAccounts; this.secureStorageClient = secureStorageClient; this.secureBackupClient = secureBackupClient; this.mapper = SystemMapper.getMapper(); - this.dynamicConfigurationManager = dynamicConfigurationManager; } public Account create(final String number, @@ -336,15 +329,11 @@ public class AccountsManager { } public AccountCrawlChunk getAllFromDynamo(int length) { - final int maxPageSize = dynamicConfigurationManager.getConfiguration().getAccountsDynamoDbMigrationConfiguration() - .getDynamoCrawlerScanPageSize(); - return accountsDynamoDb.getAllFromStart(length, maxPageSize); + return accounts.getAllFromStart(length); } public AccountCrawlChunk getAllFromDynamo(UUID uuid, int length) { - final int maxPageSize = dynamicConfigurationManager.getConfiguration().getAccountsDynamoDbMigrationConfiguration() - .getDynamoCrawlerScanPageSize(); - return accountsDynamoDb.getAllFrom(uuid, length, maxPageSize); + return accounts.getAllFrom(uuid, length); } public void delete(final Account account, final DeletionReason deletionReason) throws InterruptedException { @@ -445,38 +434,23 @@ public class AccountsManager { } private Optional dynamoGet(String number) { - return accountsDynamoDb.get(number); + return accounts.get(number); } private Optional dynamoGet(UUID uuid) { - return accountsDynamoDb.get(uuid); + return accounts.get(uuid); } private boolean dynamoCreate(Account account) { - return accountsDynamoDb.create(account); + return accounts.create(account); } private void dynamoUpdate(Account account) { - accountsDynamoDb.update(account); + accounts.update(account); } private void dynamoDelete(final Account account) { - accountsDynamoDb.delete(account.getUuid()); + accounts.delete(account.getUuid()); } - // TODO delete - @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - @Deprecated - public Optional compareAccounts(final Optional maybePrimaryAccount, - final Optional maybeSecondaryAccount) { - return Optional.empty(); - } - - private String getAbbreviatedCallChain(final StackTraceElement[] stackTrace) { - return Arrays.stream(stackTrace) - .filter(stackTraceElement -> stackTraceElement.getClassName().contains("org.whispersystems")) - .filter(stackTraceElement -> !(stackTraceElement.getClassName().endsWith("AccountsManager") && stackTraceElement.getMethodName().contains("compare"))) - .map(stackTraceElement -> StringUtils.substringAfterLast(stackTraceElement.getClassName(), ".") + ":" + stackTraceElement.getMethodName()) - .collect(Collectors.joining(" -> ")); - } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java index 780f74d8c..1b216910b 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java @@ -33,7 +33,7 @@ import org.whispersystems.textsecuregcm.securebackup.SecureBackupClient; import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient; import org.whispersystems.textsecuregcm.sqs.DirectoryQueue; import org.whispersystems.textsecuregcm.storage.Account; -import org.whispersystems.textsecuregcm.storage.AccountsDynamoDb; +import org.whispersystems.textsecuregcm.storage.Accounts; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.AccountsManager.DeletionReason; import org.whispersystems.textsecuregcm.storage.DeletedAccounts; @@ -150,9 +150,10 @@ public class DeleteUserCommand extends EnvironmentCommand account = accountsManager.get(user); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/SetUserDiscoverabilityCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/SetUserDiscoverabilityCommand.java index e2d8c9c6d..4a0fce48e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/SetUserDiscoverabilityCommand.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/SetUserDiscoverabilityCommand.java @@ -32,7 +32,7 @@ import org.whispersystems.textsecuregcm.securebackup.SecureBackupClient; import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient; import org.whispersystems.textsecuregcm.sqs.DirectoryQueue; import org.whispersystems.textsecuregcm.storage.Account; -import org.whispersystems.textsecuregcm.storage.AccountsDynamoDb; +import org.whispersystems.textsecuregcm.storage.Accounts; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.DeletedAccounts; import org.whispersystems.textsecuregcm.storage.DeletedAccountsManager; @@ -153,10 +153,10 @@ public class SetUserDiscoverabilityCommand extends EnvironmentCommand maybeAccount; diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfigurationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfigurationTest.java index cf6f5cc50..052ee6df0 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfigurationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfigurationTest.java @@ -313,29 +313,6 @@ class DynamicConfigurationTest { } } - @Test - void testParseAccountsDynamoDbMigrationConfiguration() throws JsonProcessingException { - { - final String emptyConfigYaml = "test: true"; - final DynamicConfiguration emptyConfig = - DynamicConfigurationManager.parseConfiguration(emptyConfigYaml).orElseThrow(); - - assertEquals(10, emptyConfig.getAccountsDynamoDbMigrationConfiguration().getDynamoCrawlerScanPageSize()); - } - - { - final String accountsDynamoDbMigrationConfig = - "accountsDynamoDbMigration:\n" - + " dynamoCrawlerScanPageSize: 5000"; - - final DynamicAccountsDynamoDbMigrationConfiguration config = - DynamicConfigurationManager.parseConfiguration(accountsDynamoDbMigrationConfig).orElseThrow() - .getAccountsDynamoDbMigrationConfiguration(); - - assertEquals(5000, config.getDynamoCrawlerScanPageSize()); - } - } - @Test void testParseLimits() throws JsonProcessingException { { diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountDatabaseCrawlerIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountDatabaseCrawlerIntegrationTest.java index 01fc6ab09..60c23c7cf 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountDatabaseCrawlerIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountDatabaseCrawlerIntegrationTest.java @@ -33,8 +33,6 @@ public class AccountDatabaseCrawlerIntegrationTest extends AbstractRedisClusterT private AccountsManager accountsManager; private AccountDatabaseCrawlerListener listener; - private DynamicConfigurationManager dynamicConfigurationManager; - private AccountDatabaseCrawler accountDatabaseCrawler; private static final int CHUNK_SIZE = 1; @@ -50,8 +48,6 @@ public class AccountDatabaseCrawlerIntegrationTest extends AbstractRedisClusterT accountsManager = mock(AccountsManager.class); listener = mock(AccountDatabaseCrawlerListener.class); - dynamicConfigurationManager = mock(DynamicConfigurationManager.class); - when(firstAccount.getUuid()).thenReturn(FIRST_UUID); when(secondAccount.getUuid()).thenReturn(SECOND_UUID); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java index 24cc4292e..707fc64a0 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java @@ -15,11 +15,7 @@ import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; -import com.opentable.db.postgres.embedded.LiquibasePreparer; -import com.opentable.db.postgres.junit5.EmbeddedPostgresExtension; -import com.opentable.db.postgres.junit5.PreparedDbExtension; import io.lettuce.core.cluster.api.sync.RedisAdvancedClusterCommands; import java.io.IOException; import java.time.Instant; @@ -38,7 +34,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.mockito.ArgumentCaptor; import org.whispersystems.textsecuregcm.auth.AuthenticationCredentials; -import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; import org.whispersystems.textsecuregcm.entities.AccountAttributes; import org.whispersystems.textsecuregcm.entities.SignedPreKey; import org.whispersystems.textsecuregcm.securebackup.SecureBackupClient; @@ -55,23 +50,22 @@ import software.amazon.awssdk.services.dynamodb.model.ScalarAttributeType; class AccountsManagerConcurrentModificationIntegrationTest { - @RegisterExtension - static PreparedDbExtension db = EmbeddedPostgresExtension.preparedDatabase(LiquibasePreparer.forClasspathLocation("accountsdb.xml")); - private static final String ACCOUNTS_TABLE_NAME = "accounts_test"; private static final String NUMBERS_TABLE_NAME = "numbers_test"; + private static final int SCAN_PAGE_SIZE = 1; + @RegisterExtension static DynamoDbExtension dynamoDbExtension = DynamoDbExtension.builder() .tableName(ACCOUNTS_TABLE_NAME) - .hashKey(AccountsDynamoDb.KEY_ACCOUNT_UUID) + .hashKey(Accounts.KEY_ACCOUNT_UUID) .attributeDefinition(AttributeDefinition.builder() - .attributeName(AccountsDynamoDb.KEY_ACCOUNT_UUID) + .attributeName(Accounts.KEY_ACCOUNT_UUID) .attributeType(ScalarAttributeType.B) .build()) .build(); - private AccountsDynamoDb accountsDynamoDb; + private Accounts accounts; private AccountsManager accountsManager; @@ -86,11 +80,11 @@ class AccountsManagerConcurrentModificationIntegrationTest { CreateTableRequest createNumbersTableRequest = CreateTableRequest.builder() .tableName(NUMBERS_TABLE_NAME) .keySchema(KeySchemaElement.builder() - .attributeName(AccountsDynamoDb.ATTR_ACCOUNT_E164) + .attributeName(Accounts.ATTR_ACCOUNT_E164) .keyType(KeyType.HASH) .build()) .attributeDefinitions(AttributeDefinition.builder() - .attributeName(AccountsDynamoDb.ATTR_ACCOUNT_E164) + .attributeName(Accounts.ATTR_ACCOUNT_E164) .attributeType(ScalarAttributeType.S) .build()) .provisionedThroughput(DynamoDbExtension.DEFAULT_PROVISIONED_THROUGHPUT) @@ -99,18 +93,14 @@ class AccountsManagerConcurrentModificationIntegrationTest { dynamoDbExtension.getDynamoDbClient().createTable(createNumbersTableRequest); } - accountsDynamoDb = new AccountsDynamoDb( + accounts = new Accounts( dynamoDbExtension.getDynamoDbClient(), dynamoDbExtension.getTableName(), - NUMBERS_TABLE_NAME - ); + NUMBERS_TABLE_NAME, + SCAN_PAGE_SIZE); { - final DynamicConfigurationManager dynamicConfigurationManager = mock(DynamicConfigurationManager.class); - - DynamicConfiguration dynamicConfiguration = new DynamicConfiguration(); - when(dynamicConfigurationManager.getConfiguration()).thenReturn(dynamicConfiguration); - + //noinspection unchecked commands = mock(RedisAdvancedClusterCommands.class); final DeletedAccountsManager deletedAccountsManager = mock(DeletedAccountsManager.class); @@ -122,7 +112,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { }).when(deletedAccountsManager).lockAndTake(anyString(), any()); accountsManager = new AccountsManager( - accountsDynamoDb, + accounts, RedisClusterHelper.buildMockRedisCluster(commands), deletedAccountsManager, mock(DirectoryQueue.class), @@ -132,8 +122,8 @@ class AccountsManagerConcurrentModificationIntegrationTest { mock(ProfilesManager.class), mock(StoredVerificationCodeManager.class), mock(SecureStorageClient.class), - mock(SecureBackupClient.class), - dynamicConfigurationManager); + mock(SecureBackupClient.class) + ); } } @@ -186,12 +176,12 @@ class AccountsManagerConcurrentModificationIntegrationTest { modifyAccount(uuid, account -> account.setUnidentifiedAccessKey(unidentifiedAccessKey)), modifyAccount(uuid, account -> account.setRegistrationLock(credentials.getHashedAuthenticationToken(), credentials.getSalt())), modifyAccount(uuid, account -> account.setUnrestrictedUnidentifiedAccess(unrestrictedUnidentifiedAccess)), - modifyDevice(uuid, Device.MASTER_ID, device-> device.setLastSeen(lastSeen)), - modifyDevice(uuid, Device.MASTER_ID, device-> device.setName("deviceName")) + modifyDevice(uuid, Device.MASTER_ID, device -> device.setLastSeen(lastSeen)), + modifyDevice(uuid, Device.MASTER_ID, device -> device.setName("deviceName")) ).join(); - final Account managerAccount = accountsManager.get(uuid).get(); - final Account dynamoAccount = accountsDynamoDb.get(uuid).get(); + final Account managerAccount = accountsManager.get(uuid).orElseThrow(); + final Account dynamoAccount = accounts.get(uuid).orElseThrow(); final Account redisAccount = getLastAccountFromRedisMock(commands); @@ -200,10 +190,9 @@ class AccountsManagerConcurrentModificationIntegrationTest { new Pair<>("dynamo", dynamoAccount), new Pair<>("redis", redisAccount) ).forEach(pair -> - verifyAccount(pair.first(), pair.second(), profileName, avatar, discoverableByPhoneNumber, - currentProfileVersion, identityKey, unidentifiedAccessKey, pin, registrationLock, - unrestrictedUnidentifiedAccess, lastSeen) - ); + verifyAccount(pair.first(), pair.second(), profileName, avatar, discoverableByPhoneNumber, + currentProfileVersion, identityKey, unidentifiedAccessKey, pin, registrationLock, + unrestrictedUnidentifiedAccess, lastSeen)); } private Account getLastAccountFromRedisMock(RedisAdvancedClusterCommands commands) throws IOException { @@ -220,9 +209,9 @@ class AccountsManagerConcurrentModificationIntegrationTest { () -> assertEquals(profileName, account.getProfileName()), () -> assertEquals(avatar, account.getAvatar()), () -> assertEquals(discoverableByPhoneNumber, account.isDiscoverableByPhoneNumber()), - () -> assertEquals(currentProfileVersion, account.getCurrentProfileVersion().get()), + () -> assertEquals(currentProfileVersion, account.getCurrentProfileVersion().orElseThrow()), () -> assertEquals(identityKey, account.getIdentityKey()), - () -> assertArrayEquals(unidentifiedAccessKey, account.getUnidentifiedAccessKey().get()), + () -> assertArrayEquals(unidentifiedAccessKey, account.getUnidentifiedAccessKey().orElseThrow()), () -> assertTrue(account.getRegistrationLock().verify(clientRegistrationLock)), () -> assertEquals(unrestrictedUnidentifiedAcces, account.isUnrestrictedUnidentifiedAccess()) ); @@ -231,7 +220,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { private CompletableFuture modifyAccount(final UUID uuid, final Consumer accountMutation) { return CompletableFuture.runAsync(() -> { - final Account account = accountsManager.get(uuid).get(); + final Account account = accountsManager.get(uuid).orElseThrow(); accountsManager.update(account, accountMutation); }, mutationExecutor); } @@ -239,7 +228,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { private CompletableFuture modifyDevice(final UUID uuid, final long deviceId, final Consumer deviceMutation) { return CompletableFuture.runAsync(() -> { - final Account account = accountsManager.get(uuid).get(); + final Account account = accountsManager.get(uuid).orElseThrow(); accountsManager.updateDevice(account, deviceId, deviceMutation); }, mutationExecutor); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsDynamoDbTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java similarity index 79% rename from service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsDynamoDbTest.java rename to service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java index dc32c9f46..5d6430805 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsDynamoDbTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -45,33 +45,35 @@ import software.amazon.awssdk.services.dynamodb.model.TransactWriteItemsRequest; import software.amazon.awssdk.services.dynamodb.model.TransactionConflictException; import software.amazon.awssdk.services.dynamodb.model.UpdateItemRequest; -class AccountsDynamoDbTest { +class AccountsTest { private static final String ACCOUNTS_TABLE_NAME = "accounts_test"; private static final String NUMBERS_TABLE_NAME = "numbers_test"; + private static final int SCAN_PAGE_SIZE = 1; + @RegisterExtension static DynamoDbExtension dynamoDbExtension = DynamoDbExtension.builder() .tableName(ACCOUNTS_TABLE_NAME) - .hashKey(AccountsDynamoDb.KEY_ACCOUNT_UUID) + .hashKey(Accounts.KEY_ACCOUNT_UUID) .attributeDefinition(AttributeDefinition.builder() - .attributeName(AccountsDynamoDb.KEY_ACCOUNT_UUID) + .attributeName(Accounts.KEY_ACCOUNT_UUID) .attributeType(ScalarAttributeType.B) .build()) .build(); - private AccountsDynamoDb accountsDynamoDb; + private Accounts accounts; @BeforeEach void setupAccountsDao() { CreateTableRequest createNumbersTableRequest = CreateTableRequest.builder() .tableName(NUMBERS_TABLE_NAME) .keySchema(KeySchemaElement.builder() - .attributeName(AccountsDynamoDb.ATTR_ACCOUNT_E164) + .attributeName(Accounts.ATTR_ACCOUNT_E164) .keyType(KeyType.HASH) .build()) .attributeDefinitions(AttributeDefinition.builder() - .attributeName(AccountsDynamoDb.ATTR_ACCOUNT_E164) + .attributeName(Accounts.ATTR_ACCOUNT_E164) .attributeType(ScalarAttributeType.S) .build()) .provisionedThroughput(DynamoDbExtension.DEFAULT_PROVISIONED_THROUGHPUT) @@ -79,11 +81,11 @@ class AccountsDynamoDbTest { dynamoDbExtension.getDynamoDbClient().createTable(createNumbersTableRequest); - this.accountsDynamoDb = new AccountsDynamoDb( + this.accounts = new Accounts( dynamoDbExtension.getDynamoDbClient(), dynamoDbExtension.getTableName(), - NUMBERS_TABLE_NAME - ); + NUMBERS_TABLE_NAME, + SCAN_PAGE_SIZE); } @Test @@ -91,12 +93,12 @@ class AccountsDynamoDbTest { Device device = generateDevice (1 ); Account account = generateAccount("+14151112222", UUID.randomUUID(), Collections.singleton(device)); - boolean freshUser = accountsDynamoDb.create(account); + boolean freshUser = accounts.create(account); assertThat(freshUser).isTrue(); verifyStoredState("+14151112222", account.getUuid(), account, true); - freshUser = accountsDynamoDb.create(account); + freshUser = accounts.create(account); assertThat(freshUser).isTrue(); verifyStoredState("+14151112222", account.getUuid(), account, true); @@ -110,7 +112,7 @@ class AccountsDynamoDbTest { Account account = generateAccount("+14151112222", UUID.randomUUID(), devices); - accountsDynamoDb.create(account); + accounts.create(account); verifyStoredState("+14151112222", account.getUuid(), account, true); } @@ -131,11 +133,11 @@ class AccountsDynamoDbTest { UUID uuidSecond = UUID.randomUUID(); Account accountSecond = generateAccount("+14152221111", uuidSecond, devicesSecond); - accountsDynamoDb.create(accountFirst); - accountsDynamoDb.create(accountSecond); + accounts.create(accountFirst); + accounts.create(accountSecond); - Optional retrievedFirst = accountsDynamoDb.get("+14151112222"); - Optional retrievedSecond = accountsDynamoDb.get("+14152221111"); + Optional retrievedFirst = accounts.get("+14151112222"); + Optional retrievedSecond = accounts.get("+14152221111"); assertThat(retrievedFirst.isPresent()).isTrue(); assertThat(retrievedSecond.isPresent()).isTrue(); @@ -143,8 +145,8 @@ class AccountsDynamoDbTest { verifyStoredState("+14151112222", uuidFirst, retrievedFirst.get(), accountFirst); verifyStoredState("+14152221111", uuidSecond, retrievedSecond.get(), accountSecond); - retrievedFirst = accountsDynamoDb.get(uuidFirst); - retrievedSecond = accountsDynamoDb.get(uuidSecond); + retrievedFirst = accounts.get(uuidFirst); + retrievedSecond = accounts.get(uuidSecond); assertThat(retrievedFirst.isPresent()).isTrue(); assertThat(retrievedSecond.isPresent()).isTrue(); @@ -159,27 +161,27 @@ class AccountsDynamoDbTest { UUID firstUuid = UUID.randomUUID(); Account account = generateAccount("+14151112222", firstUuid, Collections.singleton(device)); - accountsDynamoDb.create(account); + accounts.create(account); verifyStoredState("+14151112222", account.getUuid(), account, true); account.setProfileName("name"); - accountsDynamoDb.update(account); + accounts.update(account); UUID secondUuid = UUID.randomUUID(); device = generateDevice(1); account = generateAccount("+14151112222", secondUuid, Collections.singleton(device)); - final boolean freshUser = accountsDynamoDb.create(account); + final boolean freshUser = accounts.create(account); assertThat(freshUser).isFalse(); verifyStoredState("+14151112222", firstUuid, account, true); device = generateDevice(1); Account invalidAccount = generateAccount("+14151113333", firstUuid, Collections.singleton(device)); - assertThatThrownBy(() -> accountsDynamoDb.create(invalidAccount)); + assertThatThrownBy(() -> accounts.create(invalidAccount)); } @Test @@ -187,18 +189,18 @@ class AccountsDynamoDbTest { Device device = generateDevice (1 ); Account account = generateAccount("+14151112222", UUID.randomUUID(), Collections.singleton(device)); - accountsDynamoDb.create(account); + accounts.create(account); device.setName("foobar"); - accountsDynamoDb.update(account); + accounts.update(account); - Optional retrieved = accountsDynamoDb.get("+14151112222"); + Optional retrieved = accounts.get("+14151112222"); assertThat(retrieved.isPresent()).isTrue(); verifyStoredState("+14151112222", account.getUuid(), retrieved.get(), account); - retrieved = accountsDynamoDb.get(account.getUuid()); + retrieved = accounts.get(account.getUuid()); assertThat(retrieved.isPresent()).isTrue(); verifyStoredState("+14151112222", account.getUuid(), account, true); @@ -206,11 +208,11 @@ class AccountsDynamoDbTest { device = generateDevice(1); Account unknownAccount = generateAccount("+14151113333", UUID.randomUUID(), Collections.singleton(device)); - assertThatThrownBy(() -> accountsDynamoDb.update(unknownAccount)).isInstanceOfAny(ConditionalCheckFailedException.class); + assertThatThrownBy(() -> accounts.update(unknownAccount)).isInstanceOfAny(ConditionalCheckFailedException.class); account.setProfileName("name"); - accountsDynamoDb.update(account); + accounts.update(account); assertThat(account.getVersion()).isEqualTo(2); @@ -218,12 +220,12 @@ class AccountsDynamoDbTest { account.setVersion(1); - assertThatThrownBy(() -> accountsDynamoDb.update(account)).isInstanceOfAny(ContestedOptimisticLockException.class); + assertThatThrownBy(() -> accounts.update(account)).isInstanceOfAny(ContestedOptimisticLockException.class); account.setVersion(2); account.setProfileName("name2"); - accountsDynamoDb.update(account); + accounts.update(account); verifyStoredState("+14151112222", account.getUuid(), account, true); } @@ -232,8 +234,8 @@ class AccountsDynamoDbTest { void testUpdateWithMockTransactionConflictException() { final DynamoDbClient dynamoDbClient = mock(DynamoDbClient.class); - accountsDynamoDb = new AccountsDynamoDb(dynamoDbClient, - dynamoDbExtension.getTableName(), NUMBERS_TABLE_NAME); + accounts = new Accounts(dynamoDbClient, + dynamoDbExtension.getTableName(), NUMBERS_TABLE_NAME, SCAN_PAGE_SIZE); when(dynamoDbClient.updateItem(any(UpdateItemRequest.class))) .thenThrow(TransactionConflictException.class); @@ -241,7 +243,7 @@ class AccountsDynamoDbTest { Device device = generateDevice(1); Account account = generateAccount("+14151112222", UUID.randomUUID(), Collections.singleton(device)); - assertThatThrownBy(() -> accountsDynamoDb.update(account)).isInstanceOfAny(ContestedOptimisticLockException.class); + assertThatThrownBy(() -> accounts.update(account)).isInstanceOfAny(ContestedOptimisticLockException.class); } @Test @@ -251,12 +253,12 @@ class AccountsDynamoDbTest { for (int i = 1; i <= 100; i++) { Account account = generateAccount("+1" + String.format("%03d", i), UUID.randomUUID()); users.add(account); - accountsDynamoDb.create(account); + accounts.create(account); } users.sort((account, t1) -> UUIDComparator.staticCompare(account.getUuid(), t1.getUuid())); - AccountCrawlChunk retrieved = accountsDynamoDb.getAllFromStart(10, 1); + AccountCrawlChunk retrieved = accounts.getAllFromStart(10); assertThat(retrieved.getAccounts().size()).isEqualTo(10); for (int i = 0; i < retrieved.getAccounts().size(); i++) { @@ -273,7 +275,7 @@ class AccountsDynamoDbTest { } for (int j = 0; j < 9; j++) { - retrieved = accountsDynamoDb.getAllFrom(retrieved.getLastUuid().orElseThrow(), 10, 1); + retrieved = accounts.getAllFrom(retrieved.getLastUuid().orElseThrow(), 10); assertThat(retrieved.getAccounts().size()).isEqualTo(10); for (int i = 0; i < retrieved.getAccounts().size(); i++) { @@ -295,33 +297,36 @@ class AccountsDynamoDbTest { @Test void testDelete() { - final Device deletedDevice = generateDevice (1); - final Account deletedAccount = generateAccount("+14151112222", UUID.randomUUID(), Collections.singleton(deletedDevice)); - final Device retainedDevice = generateDevice (1); - final Account retainedAccount = generateAccount("+14151112345", UUID.randomUUID(), Collections.singleton(retainedDevice)); + final Device deletedDevice = generateDevice(1); + final Account deletedAccount = generateAccount("+14151112222", UUID.randomUUID(), + Collections.singleton(deletedDevice)); + final Device retainedDevice = generateDevice(1); + final Account retainedAccount = generateAccount("+14151112345", UUID.randomUUID(), + Collections.singleton(retainedDevice)); - accountsDynamoDb.create(deletedAccount); - accountsDynamoDb.create(retainedAccount); + accounts.create(deletedAccount); + accounts.create(retainedAccount); - assertThat(accountsDynamoDb.get(deletedAccount.getUuid())).isPresent(); - assertThat(accountsDynamoDb.get(retainedAccount.getUuid())).isPresent(); + assertThat(accounts.get(deletedAccount.getUuid())).isPresent(); + assertThat(accounts.get(retainedAccount.getUuid())).isPresent(); - accountsDynamoDb.delete(deletedAccount.getUuid()); + accounts.delete(deletedAccount.getUuid()); - assertThat(accountsDynamoDb.get(deletedAccount.getUuid())).isNotPresent(); + assertThat(accounts.get(deletedAccount.getUuid())).isNotPresent(); - verifyStoredState(retainedAccount.getNumber(), retainedAccount.getUuid(), accountsDynamoDb.get(retainedAccount.getUuid()).get(), retainedAccount); + verifyStoredState(retainedAccount.getNumber(), retainedAccount.getUuid(), + accounts.get(retainedAccount.getUuid()).get(), retainedAccount); { final Account recreatedAccount = generateAccount(deletedAccount.getNumber(), UUID.randomUUID(), Collections.singleton(generateDevice(1))); - final boolean freshUser = accountsDynamoDb.create(recreatedAccount); + final boolean freshUser = accounts.create(recreatedAccount); assertThat(freshUser).isTrue(); - assertThat(accountsDynamoDb.get(recreatedAccount.getUuid())).isPresent(); + assertThat(accounts.get(recreatedAccount.getUuid())).isPresent(); verifyStoredState(recreatedAccount.getNumber(), recreatedAccount.getUuid(), - accountsDynamoDb.get(recreatedAccount.getUuid()).get(), recreatedAccount); + accounts.get(recreatedAccount.getUuid()).get(), recreatedAccount); } } @@ -330,12 +335,12 @@ class AccountsDynamoDbTest { Device device = generateDevice (1 ); Account account = generateAccount("+14151112222", UUID.randomUUID(), Collections.singleton(device)); - accountsDynamoDb.create(account); + accounts.create(account); - Optional retrieved = accountsDynamoDb.get("+11111111"); + Optional retrieved = accounts.get("+11111111"); assertThat(retrieved.isPresent()).isFalse(); - retrieved = accountsDynamoDb.get(UUID.randomUUID()); + retrieved = accounts.get(UUID.randomUUID()); assertThat(retrieved.isPresent()).isFalse(); } @@ -357,7 +362,7 @@ class AccountsDynamoDbTest { when(client.updateItem(any(UpdateItemRequest.class))) .thenThrow(RuntimeException.class); - AccountsDynamoDb accounts = new AccountsDynamoDb(client, ACCOUNTS_TABLE_NAME, NUMBERS_TABLE_NAME); + Accounts accounts = new Accounts(client, ACCOUNTS_TABLE_NAME, NUMBERS_TABLE_NAME, SCAN_PAGE_SIZE); Account account = generateAccount("+14151112222", UUID.randomUUID()); try { @@ -397,13 +402,13 @@ class AccountsDynamoDbTest { UUID uuid = UUID.randomUUID(); Account account = generateAccount("+14151112222", uuid, Collections.singleton(device)); account.setDiscoverableByPhoneNumber(false); - accountsDynamoDb.create(account); + accounts.create(account); verifyStoredState("+14151112222", account.getUuid(), account, false); account.setDiscoverableByPhoneNumber(true); - accountsDynamoDb.update(account); + accounts.update(account); verifyStoredState("+14151112222", account.getUuid(), account, true); account.setDiscoverableByPhoneNumber(false); - accountsDynamoDb.update(account); + accounts.update(account); verifyStoredState("+14151112222", account.getUuid(), account, false); } @@ -433,20 +438,21 @@ class AccountsDynamoDbTest { final GetItemResponse get = db.getItem(GetItemRequest.builder() .tableName(dynamoDbExtension.getTableName()) - .key(Map.of(AccountsDynamoDb.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(uuid))) + .key(Map.of(Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(uuid))) .consistentRead(true) .build()); if (get.hasItem()) { - String data = new String(get.item().get(AccountsDynamoDb.ATTR_ACCOUNT_DATA).b().asByteArray(), StandardCharsets.UTF_8); + String data = new String(get.item().get(Accounts.ATTR_ACCOUNT_DATA).b().asByteArray(), StandardCharsets.UTF_8); assertThat(data).isNotEmpty(); - assertThat(AttributeValues.getInt(get.item(), AccountsDynamoDb.ATTR_VERSION, -1)) + assertThat(AttributeValues.getInt(get.item(), Accounts.ATTR_VERSION, -1)) .isEqualTo(expecting.getVersion()); - assertThat(AttributeValues.getBool(get.item(), AccountsDynamoDb.ATTR_CANONICALLY_DISCOVERABLE, !canonicallyDiscoverable)).isEqualTo(canonicallyDiscoverable); + assertThat(AttributeValues.getBool(get.item(), Accounts.ATTR_CANONICALLY_DISCOVERABLE, + !canonicallyDiscoverable)).isEqualTo(canonicallyDiscoverable); - Account result = AccountsDynamoDb.fromItem(get.item()); + Account result = Accounts.fromItem(get.item()); verifyStoredState(number, uuid, result, expecting); } else { throw new AssertionError("No data"); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java index 6053411ad..c99b04172 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java @@ -6,9 +6,7 @@ package org.whispersystems.textsecuregcm.tests.storage; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertSame; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; @@ -26,20 +24,17 @@ import static org.mockito.Mockito.when; import io.lettuce.core.RedisException; import io.lettuce.core.cluster.api.sync.RedisAdvancedClusterCommands; -import java.io.IOException; import java.util.HashSet; import java.util.Optional; import java.util.UUID; import java.util.function.Consumer; import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; -import org.mockito.ArgumentCaptor; import org.mockito.stubbing.Answer; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; import org.whispersystems.textsecuregcm.entities.AccountAttributes; @@ -48,7 +43,7 @@ import org.whispersystems.textsecuregcm.securebackup.SecureBackupClient; import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient; import org.whispersystems.textsecuregcm.sqs.DirectoryQueue; import org.whispersystems.textsecuregcm.storage.Account; -import org.whispersystems.textsecuregcm.storage.AccountsDynamoDb; +import org.whispersystems.textsecuregcm.storage.Accounts; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.ContestedOptimisticLockException; import org.whispersystems.textsecuregcm.storage.DeletedAccountsManager; @@ -60,12 +55,11 @@ import org.whispersystems.textsecuregcm.storage.MessagesManager; import org.whispersystems.textsecuregcm.storage.ProfilesManager; import org.whispersystems.textsecuregcm.storage.StoredVerificationCodeManager; import org.whispersystems.textsecuregcm.storage.UsernamesManager; -import org.whispersystems.textsecuregcm.tests.util.JsonHelpers; import org.whispersystems.textsecuregcm.tests.util.RedisClusterHelper; class AccountsManagerTest { - private AccountsDynamoDb accountsDynamoDb; + private Accounts accounts; private DeletedAccountsManager deletedAccountsManager; private DirectoryQueue directoryQueue; private DynamicConfigurationManager dynamicConfigurationManager; @@ -86,7 +80,7 @@ class AccountsManagerTest { @BeforeEach void setup() throws InterruptedException { - accountsDynamoDb = mock(AccountsDynamoDb.class); + accounts = mock(Accounts.class); deletedAccountsManager = mock(DeletedAccountsManager.class); directoryQueue = mock(DirectoryQueue.class); dynamicConfigurationManager = mock(DynamicConfigurationManager.class); @@ -107,7 +101,7 @@ class AccountsManagerTest { }).when(deletedAccountsManager).lockAndTake(anyString(), any()); accountsManager = new AccountsManager( - accountsDynamoDb, + accounts, RedisClusterHelper.buildMockRedisCluster(commands), deletedAccountsManager, directoryQueue, @@ -117,8 +111,8 @@ class AccountsManagerTest { profilesManager, mock(StoredVerificationCodeManager.class), mock(SecureStorageClient.class), - mock(SecureBackupClient.class), - dynamicConfigurationManager); + mock(SecureBackupClient.class) + ); } @Test @@ -138,7 +132,7 @@ class AccountsManagerTest { verify(commands, times(1)).get(eq("Account3::" + uuid)); verifyNoMoreInteractions(commands); - verifyNoInteractions(accountsDynamoDb); + verifyNoInteractions(accounts); } @Test @@ -157,7 +151,7 @@ class AccountsManagerTest { verify(commands, times(1)).get(eq("Account3::" + uuid)); verifyNoMoreInteractions(commands); - verifyNoInteractions(accountsDynamoDb); + verifyNoInteractions(accounts); } @@ -168,7 +162,7 @@ class AccountsManagerTest { Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); when(commands.get(eq("AccountMap::+14152222222"))).thenReturn(null); - when(accountsDynamoDb.get(eq("+14152222222"))).thenReturn(Optional.of(account)); + when(accounts.get(eq("+14152222222"))).thenReturn(Optional.of(account)); Optional retrieved = accountsManager.get("+14152222222"); @@ -180,8 +174,8 @@ class AccountsManagerTest { verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); verifyNoMoreInteractions(commands); - verify(accountsDynamoDb, times(1)).get(eq("+14152222222")); - verifyNoMoreInteractions(accountsDynamoDb); + verify(accounts, times(1)).get(eq("+14152222222")); + verifyNoMoreInteractions(accounts); } @Test @@ -191,7 +185,7 @@ class AccountsManagerTest { Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); when(commands.get(eq("Account3::" + uuid))).thenReturn(null); - when(accountsDynamoDb.get(eq(uuid))).thenReturn(Optional.of(account)); + when(accounts.get(eq(uuid))).thenReturn(Optional.of(account)); Optional retrieved = accountsManager.get(uuid); @@ -203,8 +197,8 @@ class AccountsManagerTest { verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); verifyNoMoreInteractions(commands); - verify(accountsDynamoDb, times(1)).get(eq(uuid)); - verifyNoMoreInteractions(accountsDynamoDb); + verify(accounts, times(1)).get(eq(uuid)); + verifyNoMoreInteractions(accounts); } @Test @@ -213,7 +207,7 @@ class AccountsManagerTest { Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); when(commands.get(eq("AccountMap::+14152222222"))).thenThrow(new RedisException("Connection lost!")); - when(accountsDynamoDb.get(eq("+14152222222"))).thenReturn(Optional.of(account)); + when(accounts.get(eq("+14152222222"))).thenReturn(Optional.of(account)); Optional retrieved = accountsManager.get("+14152222222"); @@ -225,18 +219,17 @@ class AccountsManagerTest { verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); verifyNoMoreInteractions(commands); - verify(accountsDynamoDb, times(1)).get(eq("+14152222222")); - verifyNoMoreInteractions(accountsDynamoDb); + verify(accounts, times(1)).get(eq("+14152222222")); + verifyNoMoreInteractions(accounts); } @Test void testGetAccountByUuidBrokenCache() { - final boolean dynamoEnabled = true; UUID uuid = UUID.randomUUID(); Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); when(commands.get(eq("Account3::" + uuid))).thenThrow(new RedisException("Connection lost!")); - when(accountsDynamoDb.get(eq(uuid))).thenReturn(Optional.of(account)); + when(accounts.get(eq(uuid))).thenReturn(Optional.of(account)); Optional retrieved = accountsManager.get(uuid); @@ -248,76 +241,8 @@ class AccountsManagerTest { verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); verifyNoMoreInteractions(commands); - verify(accountsDynamoDb, times(1)).get(eq(uuid)); - verifyNoMoreInteractions(accountsDynamoDb); - } - - // TODO delete - @Disabled("migration specific") - @Test - void testUpdate_dynamoDbMigration() throws IOException { - - UUID uuid = UUID.randomUUID(); - Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); - - when(commands.get(eq("Account3::" + uuid))).thenReturn(null); - // database fetches should always return new instances - when(accountsDynamoDb.get(uuid)).thenReturn( - Optional.of(new Account("+14152222222", uuid, new HashSet<>(), new byte[16]))); - when(accountsDynamoDb.get(uuid)).thenReturn( - Optional.of(new Account("+14152222222", uuid, new HashSet<>(), new byte[16]))); - doAnswer(ACCOUNT_UPDATE_ANSWER).when(accountsDynamoDb).update(any(Account.class)); - - Account updatedAccount = accountsManager.update(account, a -> a.setProfileName("name")); - - assertThrows(AssertionError.class, account::getProfileName, "Account passed to update() should be stale"); - - assertNotSame(updatedAccount, account); - - verify(accountsDynamoDb, times(1)).update(account); - verifyNoMoreInteractions(accountsDynamoDb); - - ArgumentCaptor argumentCaptor = ArgumentCaptor.forClass(Account.class); - verify(accountsDynamoDb, times(1)).update(argumentCaptor.capture()); - assertEquals(uuid, argumentCaptor.getValue().getUuid()); - verify(accountsDynamoDb, times(1)).get(uuid); - verifyNoMoreInteractions(accountsDynamoDb); - - ArgumentCaptor redisSetArgumentCapture = ArgumentCaptor.forClass(String.class); - - verify(commands, times(2)).set(anyString(), redisSetArgumentCapture.capture()); - - Account accountCached = JsonHelpers.fromJson(redisSetArgumentCapture.getAllValues().get(1), Account.class); - - // uuid is @JsonIgnore, so we need to set it for compareAccounts to work - accountCached.setUuid(uuid); - - assertEquals(Optional.empty(), - accountsManager.compareAccounts(Optional.of(updatedAccount), Optional.of(accountCached))); - } - - // TODO delete - @Disabled("migration specific") - @Test - void testUpdate_dynamoMissing() { - UUID uuid = UUID.randomUUID(); - Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); - - when(commands.get(eq("Account3::" + uuid))).thenReturn(null); - when(accountsDynamoDb.get(uuid)).thenReturn(Optional.empty()); - doAnswer(ACCOUNT_UPDATE_ANSWER).when(accountsDynamoDb).update(any()); - - Account updatedAccount = accountsManager.update(account, a -> { - }); - - verify(accountsDynamoDb, times(1)).update(account); - verifyNoMoreInteractions(accountsDynamoDb); - - verify(accountsDynamoDb, never()).update(account); - verify(accountsDynamoDb, times(1)).get(uuid); - verifyNoMoreInteractions(accountsDynamoDb); - - assertEquals(1, updatedAccount.getVersion()); + verify(accounts, times(1)).get(eq(uuid)); + verifyNoMoreInteractions(accounts); } @Test @@ -327,52 +252,51 @@ class AccountsManagerTest { when(commands.get(eq("Account3::" + uuid))).thenReturn(null); - when(accountsDynamoDb.get(uuid)).thenReturn( + when(accounts.get(uuid)).thenReturn( Optional.of(new Account("+14152222222", uuid, new HashSet<>(), new byte[16]))); doThrow(ContestedOptimisticLockException.class) .doAnswer(ACCOUNT_UPDATE_ANSWER) - .when(accountsDynamoDb).update(any()); + .when(accounts).update(any()); - when(accountsDynamoDb.get(uuid)).thenReturn( + when(accounts.get(uuid)).thenReturn( Optional.of(new Account("+14152222222", uuid, new HashSet<>(), new byte[16]))); doThrow(ContestedOptimisticLockException.class) .doAnswer(ACCOUNT_UPDATE_ANSWER) - .when(accountsDynamoDb).update(any()); + .when(accounts).update(any()); account = accountsManager.update(account, a -> a.setProfileName("name")); assertEquals(1, account.getVersion()); assertEquals("name", account.getProfileName()); - verify(accountsDynamoDb, times(1)).get(uuid); - verify(accountsDynamoDb, times(2)).update(any()); - verifyNoMoreInteractions(accountsDynamoDb); + verify(accounts, times(1)).get(uuid); + verify(accounts, times(2)).update(any()); + verifyNoMoreInteractions(accounts); } @Test void testUpdate_dynamoOptimisticLockingFailureDuringCreate() { - UUID uuid = UUID.randomUUID(); - Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); + UUID uuid = UUID.randomUUID(); + Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); when(commands.get(eq("Account3::" + uuid))).thenReturn(null); - when(accountsDynamoDb.get(uuid)).thenReturn(Optional.empty()) - .thenReturn(Optional.of(account)); - when(accountsDynamoDb.create(any())).thenThrow(ContestedOptimisticLockException.class); + when(accounts.get(uuid)).thenReturn(Optional.empty()) + .thenReturn(Optional.of(account)); + when(accounts.create(any())).thenThrow(ContestedOptimisticLockException.class); - accountsManager.update(account, a -> {}); + accountsManager.update(account, a -> { + }); - verify(accountsDynamoDb, times(1)).update(account); - verifyNoMoreInteractions(accountsDynamoDb); + verify(accounts, times(1)).update(account); + verifyNoMoreInteractions(accounts); } @Test void testUpdateDevice() { - assertEquals(Optional.empty(), accountsManager.compareAccounts(Optional.empty(), Optional.empty())); - final UUID uuid = UUID.randomUUID(); Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); - when(accountsDynamoDb.get(uuid)).thenReturn( + when(accounts.get(uuid)).thenReturn( Optional.of(new Account("+14152222222", uuid, new HashSet<>(), new byte[16]))); assertTrue(account.getDevices().isEmpty()); @@ -400,90 +324,15 @@ class AccountsManagerTest { verify(unknownDeviceUpdater, never()).accept(any(Device.class)); } - // TODO delete - @Disabled("migration specific") - @Test - void testCompareAccounts() throws Exception { - assertEquals(Optional.empty(), accountsManager.compareAccounts(Optional.empty(), Optional.empty())); - - final UUID uuidA = UUID.randomUUID(); - final Account a1 = new Account("+14152222222", uuidA, new HashSet<>(), new byte[16]); - - assertEquals(Optional.of("primaryMissing"), accountsManager.compareAccounts(Optional.empty(), Optional.of(a1))); - - final Account a2 = new Account("+14152222222", uuidA, new HashSet<>(), new byte[16]); - - assertEquals(Optional.empty(), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - - { - Device device1 = new Device(); - device1.setId(1L); - - a1.addDevice(device1); - - assertEquals(Optional.of("devices"), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - - Device device2 = new Device(); - device2.setId(1L); - - a2.addDevice(device2); - - assertEquals(Optional.empty(), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - - device1.setLastSeen(1L); - - assertEquals(Optional.empty(), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - - device1.setName("name"); - - assertEquals(Optional.of("devices"), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - - device1.setName(null); - - device1.setSignedPreKey(new SignedPreKey(1L, "123", "456")); - device2.setSignedPreKey(new SignedPreKey(2L, "123", "456")); - - assertEquals(Optional.of("masterDeviceSignedPreKey"), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - - device1.setSignedPreKey(null); - device2.setSignedPreKey(null); - - assertEquals(Optional.empty(), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - - device1.setApnId("123"); - Thread.sleep(5); - device2.setApnId("123"); - - assertEquals(Optional.empty(), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - - a1.removeDevice(1L); - a2.removeDevice(1L); - - assertEquals(Optional.empty(), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - } - - assertEquals(Optional.empty(), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - - a1.setVersion(1); - - assertEquals(Optional.of("version"), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - - a2.setVersion(1); - - a2.setProfileName("name"); - - assertEquals(Optional.of("profileName"), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); - } - @Test void testCreateFreshAccount() throws InterruptedException { - when(accountsDynamoDb.create(any())).thenReturn(true); + when(accounts.create(any())).thenReturn(true); final String e164 = "+18005550123"; final AccountAttributes attributes = new AccountAttributes(false, 0, null, null, true, null); accountsManager.create(e164, "password", null, attributes); - verify(accountsDynamoDb).create(argThat(account -> e164.equals(account.getNumber()))); + verify(accounts).create(argThat(account -> e164.equals(account.getNumber()))); verifyNoInteractions(keys); verifyNoInteractions(messagesManager); verifyNoInteractions(profilesManager); @@ -493,7 +342,7 @@ class AccountsManagerTest { void testReregisterAccount() throws InterruptedException { final UUID existingUuid = UUID.randomUUID(); - when(accountsDynamoDb.create(any())).thenAnswer(invocation -> { + when(accounts.create(any())).thenAnswer(invocation -> { invocation.getArgument(0, Account.class).setUuid(existingUuid); return false; }); @@ -502,7 +351,7 @@ class AccountsManagerTest { final AccountAttributes attributes = new AccountAttributes(false, 0, null, null, true, null); accountsManager.create(e164, "password", null, attributes); - verify(accountsDynamoDb).create( + verify(accounts).create( argThat(account -> e164.equals(account.getNumber()) && existingUuid.equals(account.getUuid()))); verify(keys).delete(existingUuid); verify(messagesManager).clear(existingUuid); @@ -519,13 +368,13 @@ class AccountsManagerTest { return null; }).when(deletedAccountsManager).lockAndTake(anyString(), any()); - when(accountsDynamoDb.create(any())).thenReturn(true); + when(accounts.create(any())).thenReturn(true); final String e164 = "+18005550123"; final AccountAttributes attributes = new AccountAttributes(false, 0, null, null, true, null); accountsManager.create(e164, "password", null, attributes); - verify(accountsDynamoDb).create( + verify(accounts).create( argThat(account -> e164.equals(account.getNumber()) && recentlyDeletedUuid.equals(account.getUuid()))); verifyNoInteractions(keys); verifyNoInteractions(messagesManager); @@ -596,7 +445,7 @@ class AccountsManagerTest { accountsManager.updateDeviceLastSeen(account, device, updatedLastSeen); assertEquals(expectUpdate ? updatedLastSeen : initialLastSeen, device.getLastSeen()); - verify(accountsDynamoDb, expectUpdate ? times(1) : never()).update(account); + verify(accounts, expectUpdate ? times(1) : never()).update(account); } @SuppressWarnings("unused")