Make ACI's optional on ContactRecords.

app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt

@@ -795,19 +795,19 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da
     if (id < 0) {
       Log.w(TAG, "[applyStorageSyncContactInsert] Failed to insert. Possibly merging.")
       if (FeatureFlags.phoneNumberPrivacy()) {
-        recipientId = getAndPossiblyMergePnpVerified(if (insert.aci.isValid) insert.aci else null, insert.pni.orElse(null), insert.number.orElse(null))
+        recipientId = getAndPossiblyMergePnpVerified(insert.aci.orNull(), insert.pni.orNull(), insert.number.orNull())
       } else {
-        recipientId = getAndPossiblyMerge(if (insert.aci.isValid) insert.aci else null, insert.number.orElse(null))
+        recipientId = getAndPossiblyMerge(insert.aci.orNull(), insert.number.orNull())
       }
       db.update(TABLE_NAME, values, ID_WHERE, SqlUtil.buildArgs(recipientId))
     } else {
       recipientId = RecipientId.from(id)
     }
 
-    if (insert.identityKey.isPresent && insert.aci.isValid) {
+    if (insert.identityKey.isPresent && insert.aci.isPresent) {
       try {
         val identityKey = IdentityKey(insert.identityKey.get(), 0)
-        identities.updateIdentityAfterSync(insert.aci.toString(), recipientId, identityKey, StorageSyncModels.remoteToLocalIdentityStatus(insert.identityState))
+        identities.updateIdentityAfterSync(insert.aci.get().toString(), recipientId, identityKey, StorageSyncModels.remoteToLocalIdentityStatus(insert.identityState))
       } catch (e: InvalidKeyException) {
         Log.w(TAG, "Failed to process identity key during insert! Skipping.", e)
       }
@@ -836,9 +836,9 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da
 
       Log.w(TAG, "[applyStorageSyncContactUpdate] Found user $recipientId. Possibly merging.")
       if (FeatureFlags.phoneNumberPrivacy()) {
-        recipientId = getAndPossiblyMergePnpVerified(if (update.new.aci.isValid) update.new.aci else null, update.new.pni.orElse(null), update.new.number.orElse(null))
+        recipientId = getAndPossiblyMergePnpVerified(update.new.aci.orElse(null), update.new.pni.orElse(null), update.new.number.orElse(null))
       } else {
-        recipientId = getAndPossiblyMerge(if (update.new.aci.isValid) update.new.aci else null, update.new.number.orElse(null))
+        recipientId = getAndPossiblyMerge(update.new.aci.orElse(null), update.new.number.orElse(null))
       }
 
       Log.w(TAG, "[applyStorageSyncContactUpdate] Merged into $recipientId")
@@ -855,9 +855,9 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da
 
     try {
       val oldIdentityRecord = identityStore.getIdentityRecord(recipientId)
-      if (update.new.identityKey.isPresent && update.new.aci.isValid) {
+      if (update.new.identityKey.isPresent && update.new.aci.isPresent) {
         val identityKey = IdentityKey(update.new.identityKey.get(), 0)
-        identities.updateIdentityAfterSync(update.new.aci.toString(), recipientId, identityKey, StorageSyncModels.remoteToLocalIdentityStatus(update.new.identityState))
+        identities.updateIdentityAfterSync(update.new.aci.get().toString(), recipientId, identityKey, StorageSyncModels.remoteToLocalIdentityStatus(update.new.identityState))
       }
 
       val newIdentityRecord = identityStore.getIdentityRecord(recipientId)
@@ -3872,9 +3872,7 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da
       val systemName = ProfileName.fromParts(contact.systemGivenName.orElse(null), contact.systemFamilyName.orElse(null))
       val username = contact.username.orElse(null)
 
-      if (contact.aci.isValid) {
+      put(ACI_COLUMN, contact.aci.orElse(null)?.toString())
-        put(ACI_COLUMN, contact.aci.toString())
-      }
 
       if (FeatureFlags.phoneNumberPrivacy()) {
         put(PNI_COLUMN, contact.pni.orElse(null)?.toString())
@@ -3905,14 +3903,14 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da
       put(UNREGISTERED_TIMESTAMP, contact.unregisteredTimestamp)
       if (contact.unregisteredTimestamp > 0L) {
         put(REGISTERED, RegisteredState.NOT_REGISTERED.id)
-      } else if (contact.aci.isValid) {
+      } else if (contact.aci.isPresent) {
         put(REGISTERED, RegisteredState.REGISTERED.id)
       } else {
         Log.w(TAG, "Contact is marked as registered, but has no serviceId! Can't locally mark registered. (Phone: ${contact.number.orElse("null")}, Username: ${username?.isNotEmpty()})")
       }
 
       if (isInsert) {
-        put(AVATAR_COLOR, AvatarColorHash.forAddress(contact.aci.toString(), contact.number.orNull()).serialize())
+        put(AVATAR_COLOR, AvatarColorHash.forAddress(contact.aci.map { it.toString() }.or(contact.pni.map { it.toString() }).orNull(), contact.number.orNull()).serialize())
       }
     }
   }

app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java

@@ -80,9 +80,9 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
         continue;
       }
 
-      if (remoteRecord.getUnregisteredTimestamp() > 0 && remoteRecord.getAci() != null && !remoteRecord.getPni().isPresent() && !remoteRecord.getNumber().isPresent()) {
+      if (remoteRecord.getUnregisteredTimestamp() > 0 && remoteRecord.getAci().isPresent() && remoteRecord.getPni().isEmpty() && remoteRecord.getNumber().isEmpty()) {
         unregisteredAciOnly.add(remoteRecord);
-      } else if (remoteRecord.getAci() != null && remoteRecord.getAci().equals(remoteRecord.getPni().orElse(null))) {
+      } else if (remoteRecord.getAci().isEmpty()) {
         pniE164Only.add(remoteRecord);
       }
     }
@@ -120,20 +120,20 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
 
   /**
    * Error cases:
-   * - You can't have a contact record without an address component.
+   * - You can't have a contact record without an ACI or PNI.
    * - You can't have a contact record for yourself. That should be an account record.
    *
    * Note: This method could be written more succinctly, but the logs are useful :)
    */
   @Override
   boolean isInvalid(@NonNull SignalContactRecord remote) {
-    if (remote.getAci() == null) {
+    boolean hasAci = remote.getAci().isPresent() && remote.getAci().get().isValid();
-      Log.w(TAG, "No address on the ContentRecord -- marking as invalid.");
+    boolean hasPni = remote.getPni().isPresent() && remote.getPni().get().isValid();
+
+    if (!hasAci && !hasPni) {
+      Log.w(TAG, "Found a ContactRecord with neither an ACI nor a PNI -- marking as invalid.");
       return true;
-    } else if (remote.getAci().isUnknown()) {
+    } else if (selfAci != null && selfAci.equals(remote.getAci().orElse(null)) ||
-      Log.w(TAG, "Found a ContactRecord without a UUID -- marking as invalid.");
-      return true;
-    } else if (remote.getAci().equals(selfAci) ||
                (selfPni != null && selfPni.equals(remote.getPni().orElse(null))) ||
                (selfE164 != null && remote.getNumber().isPresent() && remote.getNumber().get().equals(selfE164)))
     {
@@ -153,7 +153,7 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
       remote = remote.withoutPni();
     }
 
-    Optional<RecipientId> found = recipientTable.getByAci(remote.getAci());
+    Optional<RecipientId> found = remote.getAci().isPresent() ? recipientTable.getByAci(remote.getAci().get()) : Optional.empty();
 
     if (found.isEmpty() && remote.getNumber().isPresent()) {
       found = recipientTable.getByE164(remote.getNumber().get());
@@ -200,7 +200,7 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
     byte[]        identityKey;
 
     if ((remote.getIdentityState() != local.getIdentityState() && remote.getIdentityKey().isPresent()) ||
-        (remote.getIdentityKey().isPresent() && !local.getIdentityKey().isPresent()))
+        (remote.getIdentityKey().isPresent() && local.getIdentityKey().isEmpty()))
     {
       identityState = remote.getIdentityState();
       identityKey   = remote.getIdentityKey().get();
@@ -209,9 +209,9 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
       identityKey   = local.getIdentityKey().orElse(null);
     }
 
-    if (identityKey != null && remote.getIdentityKey().isPresent() && !Arrays.equals(identityKey, remote.getIdentityKey().get())) {
+    if (local.getAci().isPresent() && identityKey != null && remote.getIdentityKey().isPresent() && !Arrays.equals(identityKey, remote.getIdentityKey().get())) {
-      Log.w(TAG, "The local and remote identity keys do not match for " + local.getAci() + ". Enqueueing a profile fetch.");
+      Log.w(TAG, "The local and remote identity keys do not match for " + local.getAci().orElse(null) + ". Enqueueing a profile fetch.");
-      RetrieveProfileJob.enqueue(Recipient.trustedPush(local.getAci(), local.getPni().orElse(null), local.getNumber().orElse(null)).getId());
+      RetrieveProfileJob.enqueue(Recipient.trustedPush(local.getAci().get(), local.getPni().orElse(null), local.getNumber().orElse(null)).getId());
     }
 
     PNI    pni;
@@ -250,7 +250,7 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
     }
 
     byte[]               unknownFields         = remote.serializeUnknownFields();
-    ACI                  aci                   = local.getAci() == ACI.UNKNOWN ? remote.getAci() : local.getAci();
+    ACI                  aci                   = local.getAci().isEmpty() ? remote.getAci().orElse(null) : local.getAci().get();
     byte[]               profileKey            = OptionalUtil.or(remote.getProfileKey(), local.getProfileKey()).orElse(null);
     String               username              = OptionalUtil.or(remote.getUsername(), local.getUsername()).orElse("");
     boolean              blocked               = remote.isBlocked();
@@ -324,7 +324,7 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
 
   private static boolean doParamsMatch(@NonNull SignalContactRecord contact,
                                        @Nullable byte[] unknownFields,
-                                       @NonNull ServiceId serviceId,
+                                       @NonNull ACI aci,
                                        @Nullable PNI pni,
                                        @Nullable String e164,
                                        @NonNull String profileGivenName,
@@ -346,7 +346,7 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
                                        boolean hidden)
   {
     return Arrays.equals(contact.serializeUnknownFields(), unknownFields) &&
-           Objects.equals(contact.getAci(), serviceId) &&
+           Objects.equals(contact.getAci().orElse(null), aci) &&
            Objects.equals(contact.getPni().orElse(null), pni) &&
            Objects.equals(contact.getNumber().orElse(null), e164) &&
            Objects.equals(contact.getProfileGivenName().orElse(""), profileGivenName) &&

app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java

@@ -164,7 +164,7 @@ public final class StorageSyncValidations {
       if (insert.getContact().isPresent()) {
         SignalContactRecord contact = insert.getContact().get();
 
-        if (self.requireAci().equals(contact.getAci()) ||
+        if (self.requireAci().equals(contact.getAci().orElse(null)) ||
             self.requirePni().equals(contact.getPni().orElse(null)) ||
             self.requireE164().equals(contact.getNumber().orElse("")))
         {

app/src/test/java/org/thoughtcrime/securesms/storage/ContactRecordProcessorTest.kt

@@ -69,7 +69,7 @@ class ContactRecordProcessorTest {
   }
 
   @Test
-  fun `isInvalid, missing serviceId, true`() {
+  fun `isInvalid, missing ACI and PNI, true`() {
     // GIVEN
     val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable)
 
@@ -84,6 +84,24 @@ class ContactRecordProcessorTest {
     assertTrue(result)
   }
 
+  @Test
+  fun `isInvalid, unknown ACI and PNI, true`() {
+    // GIVEN
+    val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable)
+
+    val record = buildRecord {
+      setAci(ACI.UNKNOWN.toString())
+      setPni(PNI.UNKNOWN.toString())
+      setE164(E164_B)
+    }
+
+    // WHEN
+    val result = subject.isInvalid(record)
+
+    // THEN
+    assertTrue(result)
+  }
+
   @Test
   fun `isInvalid, e164 matches self, true`() {
     // GIVEN

libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java

@@ -26,7 +26,7 @@ public final class SignalContactRecord implements SignalRecord {
   private final ContactRecord proto;
   private final boolean       hasUnknownFields;
 
-  private final ACI              aci;
+  private final Optional<ACI>    aci;
   private final Optional<PNI>    pni;
   private final Optional<String> e164;
   private final Optional<String> profileGivenName;
@@ -42,7 +42,7 @@ public final class SignalContactRecord implements SignalRecord {
     this.id                = id;
     this.proto             = proto;
     this.hasUnknownFields  = ProtoUtil.hasUnknownFields(proto);
-    this.aci               = ACI.parseOrUnknown(proto.getAci());
+    this.aci               = OptionalUtil.absentIfEmpty(proto.getAci()).map(ACI::parseOrNull);
     this.pni               = OptionalUtil.absentIfEmpty(proto.getPni()).map(PNI::parseOrNull);
     this.e164              = OptionalUtil.absentIfEmpty(proto.getE164());
     this.profileGivenName  = OptionalUtil.absentIfEmpty(proto.getGivenName());
@@ -173,7 +173,7 @@ public final class SignalContactRecord implements SignalRecord {
     return hasUnknownFields ? proto.toByteArray() : null;
   }
 
-  public ACI getAci() {
+  public Optional<ACI> getAci() {
     return aci;
   }
 
@@ -181,6 +181,16 @@ public final class SignalContactRecord implements SignalRecord {
     return pni;
   }
 
+  public Optional<? extends ServiceId> getServiceId() {
+    if (aci.isPresent()) {
+      return aci;
+    } else if (pni.isPresent()) {
+      return pni;
+    } else {
+      return Optional.empty();
+    }
+  }
+
   public Optional<String> getNumber() {
     return e164;
   }