Make phone number identifiers non-optional

service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java

@@ -392,7 +392,7 @@ public class AccountController {
 
     return new AccountCreationResult(account.getUuid(),
         account.getNumber(),
-        account.getPhoneNumberIdentifier().orElse(null),
+        account.getPhoneNumberIdentifier(),
         existingAccount.map(Account::isStorageSupported).orElse(false));
   }
 
@@ -606,7 +606,7 @@ public class AccountController {
   public AccountCreationResult whoAmI(@Auth AuthenticatedAccount auth) {
     return new AccountCreationResult(auth.getAccount().getUuid(),
         auth.getAccount().getNumber(),
-        auth.getAccount().getPhoneNumberIdentifier().orElse(null),
+        auth.getAccount().getPhoneNumberIdentifier(),
         auth.getAccount().isStorageSupported());
   }
 

service/src/main/java/org/whispersystems/textsecuregcm/controllers/CertificateController.java

@@ -16,7 +16,6 @@ import java.util.List;
 import java.util.Optional;
 import java.util.UUID;
 import javax.ws.rs.GET;
-import javax.ws.rs.NotFoundException;
 import javax.ws.rs.Path;
 import javax.ws.rs.PathParam;
 import javax.ws.rs.Produces;
@@ -87,7 +86,7 @@ public class CertificateController {
     List<GroupCredentials.GroupCredential> credentials = new LinkedList<>();
 
     final UUID identifier = identityType.map(String::toLowerCase).orElse("aci").equals("pni") ?
-        auth.getAccount().getPhoneNumberIdentifier().orElseThrow(NotFoundException::new) :
+        auth.getAccount().getPhoneNumberIdentifier() :
         auth.getAccount().getUuid();
 
     for (int i = startRedemptionTime; i <= endRedemptionTime; i++) {

service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java

@@ -21,7 +21,6 @@ import javax.validation.Valid;
 import javax.ws.rs.Consumes;
 import javax.ws.rs.GET;
 import javax.ws.rs.HeaderParam;
-import javax.ws.rs.NotFoundException;
 import javax.ws.rs.PUT;
 import javax.ws.rs.Path;
 import javax.ws.rs.PathParam;
@@ -192,8 +191,7 @@ public class KeysController {
       }
     }
 
-    final boolean usePhoneNumberIdentity =
-        target.getPhoneNumberIdentifier().map(pni -> pni.equals(targetUuid)).orElse(false);
+    final boolean usePhoneNumberIdentity = target.getPhoneNumberIdentifier().equals(targetUuid);
 
     Map<Long, PreKey>        preKeysByDeviceId = getLocalKeys(target, deviceId, usePhoneNumberIdentity);
     List<PreKeyResponseItem> responseItems     = new LinkedList<>();
@@ -254,7 +252,7 @@ public class KeysController {
 
   private static UUID getIdentifier(final Account account, final Optional<String> identityType) {
     return usePhoneNumberIdentity(identityType) ?
-        account.getPhoneNumberIdentifier().orElseThrow(NotFoundException::new) :
+        account.getPhoneNumberIdentifier() :
         account.getUuid();
   }
 
@@ -262,7 +260,7 @@ public class KeysController {
     final Map<Long, PreKey> preKeys;
 
     final UUID identifier = usePhoneNumberIdentity ?
-        destination.getPhoneNumberIdentifier().orElseThrow(NotFoundException::new) :
+        destination.getPhoneNumberIdentifier() :
         destination.getUuid();
 
     if (deviceIdSelector.equals("*")) {

service/src/main/java/org/whispersystems/textsecuregcm/storage/Account.java

@@ -23,7 +23,6 @@ import org.whispersystems.textsecuregcm.auth.AuthenticationCredentials;
 import org.whispersystems.textsecuregcm.auth.StoredRegistrationLock;
 import org.whispersystems.textsecuregcm.entities.AccountAttributes;
 import org.whispersystems.textsecuregcm.util.Util;
-import javax.annotation.Nullable;
 
 public class Account {
 
@@ -33,9 +32,7 @@ public class Account {
   @JsonIgnore
   private UUID uuid;
 
-  // Nullable only until initial migration is complete
   @JsonProperty("pni")
-  @Nullable
   private UUID phoneNumberIdentifier;
 
   @JsonProperty
@@ -108,11 +105,10 @@ public class Account {
     this.uuid = uuid;
   }
 
-  // Optional only until initial migration is complete
-  public Optional<UUID> getPhoneNumberIdentifier() {
+  public UUID getPhoneNumberIdentifier() {
     requireNotStale();
 
-    return Optional.ofNullable(phoneNumberIdentifier);
+    return phoneNumberIdentifier;
   }
 
   /**

service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java

@@ -108,18 +108,12 @@ public class Accounts extends AbstractDynamoDbStore {
                     .build())
             .build();
 
-        assert account.getPhoneNumberIdentifier().isPresent();
-
-        if (account.getPhoneNumberIdentifier().isEmpty()) {
-          log.error("Account {} is missing a phone number identifier", account.getUuid());
-        }
-
         TransactWriteItem phoneNumberIdentifierConstraintPut = TransactWriteItem.builder()
             .put(
                 Put.builder()
                     .tableName(phoneNumberIdentifierConstraintTableName)
                     .item(Map.of(
-                        ATTR_PNI_UUID, AttributeValues.fromUUID(account.getPhoneNumberIdentifier().get()),
+                        ATTR_PNI_UUID, AttributeValues.fromUUID(account.getPhoneNumberIdentifier()),
                         KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid())))
                     .conditionExpression(
                         "attribute_not_exists(#pni) OR (attribute_exists(#pni) AND #uuid = :uuid)")
@@ -135,12 +129,11 @@ public class Accounts extends AbstractDynamoDbStore {
         final Map<String, AttributeValue> item = new HashMap<>(Map.of(
             KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()),
             ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()),
+            ATTR_PNI_UUID, AttributeValues.fromUUID(account.getPhoneNumberIdentifier()),
             ATTR_ACCOUNT_DATA, AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)),
             ATTR_VERSION, AttributeValues.fromInt(account.getVersion()),
             ATTR_CANONICALLY_DISCOVERABLE, AttributeValues.fromBool(account.shouldBeVisibleInDirectory())));
 
-        account.getPhoneNumberIdentifier().ifPresent(pni -> item.put(ATTR_PNI_UUID, AttributeValues.fromUUID(pni)));
-
         TransactWriteItem accountPut = TransactWriteItem.builder()
             .put(Put.builder()
                 .conditionExpression("attribute_not_exists(#number) OR #number = :number")
@@ -180,7 +173,7 @@ public class Accounts extends AbstractDynamoDbStore {
             account.setUuid(UUIDUtil.fromByteBuffer(actualAccountUuid));
 
             final Account existingAccount = getByAccountIdentifier(account.getUuid()).orElseThrow();
-            account.setNumber(existingAccount.getNumber(), existingAccount.getPhoneNumberIdentifier().orElse(account.getPhoneNumberIdentifier().orElseThrow()));
+            account.setNumber(existingAccount.getNumber(), existingAccount.getPhoneNumberIdentifier());
             account.setVersion(existingAccount.getVersion());
 
             update(account);
@@ -220,7 +213,7 @@ public class Accounts extends AbstractDynamoDbStore {
   public void changeNumber(final Account account, final String number, final UUID phoneNumberIdentifier) {
     CHANGE_NUMBER_TIMER.record(() -> {
       final String originalNumber = account.getNumber();
-      final Optional<UUID> originalPni = account.getPhoneNumberIdentifier();
+      final UUID originalPni = account.getPhoneNumberIdentifier();
 
       boolean succeeded = false;
 
@@ -248,12 +241,12 @@ public class Accounts extends AbstractDynamoDbStore {
                 .build())
             .build());
 
-        originalPni.ifPresent(pni -> writeItems.add(TransactWriteItem.builder()
+        writeItems.add(TransactWriteItem.builder()
             .delete(Delete.builder()
                 .tableName(phoneNumberIdentifierConstraintTableName)
-                .key(Map.of(ATTR_PNI_UUID, AttributeValues.fromUUID(pni)))
+                .key(Map.of(ATTR_PNI_UUID, AttributeValues.fromUUID(originalPni)))
                 .build())
-            .build()));
+            .build());
 
         writeItems.add(TransactWriteItem.builder()
             .put(Put.builder()
@@ -301,7 +294,7 @@ public class Accounts extends AbstractDynamoDbStore {
         throw new IllegalArgumentException(e);
       } finally {
         if (!succeeded) {
-          account.setNumber(originalNumber, originalPni.orElse(null));
+          account.setNumber(originalNumber, originalPni);
         }
       }
     });
@@ -312,65 +305,27 @@ public class Accounts extends AbstractDynamoDbStore {
       final List<TransactWriteItem> transactWriteItems = new ArrayList<>(2);
 
       try {
-        final TransactWriteItem updateAccountWriteItem;
-
-        if (account.getPhoneNumberIdentifier().isPresent()) {
-          updateAccountWriteItem = TransactWriteItem.builder()
-              .update(Update.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")
-                  .conditionExpression("attribute_exists(#number) AND #version = :version")
-                  .expressionAttributeNames(Map.of("#number", ATTR_ACCOUNT_E164,
-                      "#data", ATTR_ACCOUNT_DATA,
-                      "#cds", ATTR_CANONICALLY_DISCOVERABLE,
-                      "#version", ATTR_VERSION,
-                      "#pni", ATTR_PNI_UUID))
-                  .expressionAttributeValues(Map.of(
-                      ":data", AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)),
-                      ":cds", AttributeValues.fromBool(account.shouldBeVisibleInDirectory()),
-                      ":version", AttributeValues.fromInt(account.getVersion()),
-                      ":version_increment", AttributeValues.fromInt(1),
-                      ":pni", AttributeValues.fromUUID(account.getPhoneNumberIdentifier().get())))
-                  .build())
-              .build();
-        } else {
-          updateAccountWriteItem = TransactWriteItem.builder()
-              .update(Update.builder()
-                  .tableName(accountsTableName)
-                  .key(Map.of(KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid())))
-                  .updateExpression("SET #data = :data, #cds = :cds ADD #version :version_increment")
-                  .conditionExpression("attribute_exists(#number) AND #version = :version")
-                  .expressionAttributeNames(Map.of("#number", ATTR_ACCOUNT_E164,
-                      "#data", ATTR_ACCOUNT_DATA,
-                      "#cds", ATTR_CANONICALLY_DISCOVERABLE,
-                      "#version", ATTR_VERSION))
-                  .expressionAttributeValues(Map.of(
-                      ":data", AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)),
-                      ":cds", AttributeValues.fromBool(account.shouldBeVisibleInDirectory()),
-                      ":version", AttributeValues.fromInt(account.getVersion()),
-                      ":version_increment", AttributeValues.fromInt(1)))
-                  .build())
-              .build();
-        }
+        final TransactWriteItem updateAccountWriteItem = TransactWriteItem.builder()
+            .update(Update.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")
+                .conditionExpression("attribute_exists(#number) AND #version = :version")
+                .expressionAttributeNames(Map.of("#number", ATTR_ACCOUNT_E164,
+                    "#data", ATTR_ACCOUNT_DATA,
+                    "#cds", ATTR_CANONICALLY_DISCOVERABLE,
+                    "#version", ATTR_VERSION,
+                    "#pni", ATTR_PNI_UUID))
+                .expressionAttributeValues(Map.of(
+                    ":data", AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)),
+                    ":cds", AttributeValues.fromBool(account.shouldBeVisibleInDirectory()),
+                    ":version", AttributeValues.fromInt(account.getVersion()),
+                    ":version_increment", AttributeValues.fromInt(1),
+                    ":pni", AttributeValues.fromUUID(account.getPhoneNumberIdentifier())))
+                .build())
+            .build();;
 
         transactWriteItems.add(updateAccountWriteItem);
-
-        // TODO Remove after initial migration to phone number identifiers
-        account.getPhoneNumberIdentifier().ifPresent(phoneNumberIdentifier -> transactWriteItems.add(
-            TransactWriteItem.builder()
-                .put(Put.builder()
-                    .tableName(phoneNumberIdentifierConstraintTableName)
-                    .item(Map.of(
-                        ATTR_PNI_UUID, AttributeValues.fromUUID(account.getPhoneNumberIdentifier().get()),
-                        KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid())))
-                    .conditionExpression("attribute_not_exists(#pni) OR (attribute_exists(#pni) AND #uuid = :uuid)")
-                    .expressionAttributeNames(Map.of("#uuid", KEY_ACCOUNT_UUID, "#pni", ATTR_PNI_UUID))
-                    .expressionAttributeValues(
-                        Map.of(":uuid", AttributeValues.fromUUID(account.getUuid())))
-                    .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD)
-                    .build())
-                .build()));
       } catch (JsonProcessingException e) {
         throw new IllegalArgumentException(e);
       }
@@ -464,12 +419,12 @@ public class Accounts extends AbstractDynamoDbStore {
 
         final List<TransactWriteItem> transactWriteItems = new ArrayList<>(List.of(phoneNumberDelete, accountDelete));
 
-        account.getPhoneNumberIdentifier().ifPresent(pni -> transactWriteItems.add(TransactWriteItem.builder()
+        transactWriteItems.add(TransactWriteItem.builder()
             .delete(Delete.builder()
                 .tableName(phoneNumberIdentifierConstraintTableName)
-                .key(Map.of(ATTR_PNI_UUID, AttributeValues.fromUUID(pni)))
+                .key(Map.of(ATTR_PNI_UUID, AttributeValues.fromUUID(account.getPhoneNumberIdentifier())))
                 .build())
-            .build()));
+            .build());
 
         TransactWriteItemsRequest request = TransactWriteItemsRequest.builder()
             .transactItems(transactWriteItems).build();

service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java

@@ -438,12 +438,9 @@ public class AccountsManager {
     usernamesManager.delete(account.getUuid());
     profilesManager.deleteAll(account.getUuid());
     keys.delete(account.getUuid());
+    keys.delete(account.getPhoneNumberIdentifier());
     messagesManager.clear(account.getUuid());
-
-    account.getPhoneNumberIdentifier().ifPresent(pni -> {
-      keys.delete(pni);
-      messagesManager.clear(pni);
-    });
+    messagesManager.clear(account.getPhoneNumberIdentifier());
 
     deleteStorageServiceDataFuture.join();
     deleteBackupServiceDataFuture.join();
@@ -471,9 +468,8 @@ public class AccountsManager {
       cacheCluster.useCluster(connection -> {
         final RedisAdvancedClusterCommands<String, String> commands = connection.sync();
 
-        account.getPhoneNumberIdentifier().ifPresent(pni ->
-            commands.set(getAccountMapKey(pni.toString()), account.getUuid().toString()));
 
+        commands.set(getAccountMapKey(account.getPhoneNumberIdentifier().toString()), account.getUuid().toString());
         commands.set(getAccountMapKey(account.getNumber()), account.getUuid().toString());
         commands.set(getAccountEntityKey(account.getUuid()), accountJson);
       });
@@ -528,11 +524,10 @@ public class AccountsManager {
 
   private void redisDelete(final Account account) {
     try (final Timer.Context ignored = redisDeleteTimer.time()) {
-      cacheCluster.useCluster(connection -> {
-        connection.sync().del(getAccountMapKey(account.getNumber()), getAccountEntityKey(account.getUuid()));
-
-        account.getPhoneNumberIdentifier().ifPresent(pni -> connection.sync().del(getAccountMapKey(pni.toString())));
-      });
+      cacheCluster.useCluster(connection -> connection.sync().del(
+          getAccountMapKey(account.getNumber()),
+          getAccountMapKey(account.getPhoneNumberIdentifier().toString()),
+          getAccountEntityKey(account.getUuid())));
     }
   }
 }