A number of additional merging fixes

signalapp · Aug 10, 2022 · 269d170 · 269d170
1 parent ccc8954
commit 269d170
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 7 deletions.
diff --git a/protos/SignalStorage.proto b/protos/SignalStorage.proto
@@ -73,6 +73,7 @@ message ContactRecord {
 
   optional string        serviceUuid         = 1;
   optional string        serviceE164         = 2;
+  optional string        pni                 = 15;
   optional bytes         profileKey          = 3;
   optional bytes         identityKey         = 4;
   optional IdentityState identityState       = 5;
@@ -85,6 +86,7 @@ message ContactRecord {
   optional bool          markedUnread        = 12;
   optional uint64        mutedUntilTimestamp = 13;
   optional bool          hideStory           = 14;
+  // Next ID: 16
 }
 
 message GroupV1Record {

diff --git a/ts/ConversationController.ts b/ts/ConversationController.ts
@@ -664,7 +664,6 @@ export class ConversationController {
     log.info('checkForConflicts: starting...');
     const byUuid = Object.create(null);
     const byE164 = Object.create(null);
-    const byPni = Object.create(null);
     const byGroupV2Id = Object.create(null);
     // We also want to find duplicate GV1 IDs. You might expect to see a "byGroupV1Id" map
     //   here. Instead, we check for duplicates on the derived GV2 ID.
@@ -706,18 +705,18 @@ export class ConversationController {
       }
 
       if (pni) {
-        const existing = byPni[pni];
+        const existing = byUuid[pni];
         if (!existing) {
-          byPni[pni] = conversation;
+          byUuid[pni] = conversation;
         } else {
           log.warn(`checkForConflicts: Found conflict with pni ${pni}`);
 
-          // Keep the newer one if it has a uuid, otherwise keep existing
-          if (conversation.get('uuid')) {
+          // Keep the newer one if it has additional data, otherwise keep existing
+          if (conversation.get('e164') || conversation.get('pni')) {
             // Keep new one
             // eslint-disable-next-line no-await-in-loop
             await this.combineConversations(conversation, existing);
-            byPni[pni] = conversation;
+            byUuid[pni] = conversation;
           } else {
             // Keep existing - note that this applies if neither had an e164
             // eslint-disable-next-line no-await-in-loop
@@ -860,10 +859,21 @@ export class ConversationController {
         }
       });
 
+      if (obsolete.get('isPinned')) {
+        obsolete.unpin();
+
+        if (!current.get('isPinned')) {
+          current.pin();
+        }
+      }
+
       const obsoleteId = obsolete.get('id');
       const obsoleteUuid = obsolete.getUuid();
       const currentId = current.get('id');
-      log.warn(`${logId}: Combining two conversations...`);
+      log.warn(
+        `${logId}: Combining two conversations -`,
+        `old: ${obsolete.idForLogging()} -> new: ${current.idForLogging()}`
+      );
 
       if (conversationType === 'private' && obsoleteUuid) {
         if (!current.get('profileKey') && obsolete.get('profileKey')) {
@@ -956,6 +966,8 @@ export class ConversationController {
       this._conversations.remove(obsolete);
       this._conversations.resetLookups();
 
+      current.captureChange('combineConversations');
+
       log.warn(`${logId}: Complete!`);
     });
   }

diff --git a/ts/models/conversations.ts b/ts/models/conversations.ts
@@ -1880,6 +1880,7 @@ export class ConversationModel extends window.Backbone
 
       window.Signal.Data.updateConversation(this.attributes);
       this.trigger('idUpdated', this, 'e164', oldValue);
+      this.captureChange('updateE164');
     }
   }
 
@@ -1889,6 +1890,7 @@ export class ConversationModel extends window.Backbone
       this.set('uuid', uuid ? UUID.cast(uuid.toLowerCase()) : undefined);
       window.Signal.Data.updateConversation(this.attributes);
       this.trigger('idUpdated', this, 'uuid', oldValue);
+      this.captureChange('updateUuid');
     }
   }
 
@@ -1908,6 +1910,7 @@ export class ConversationModel extends window.Backbone
 
       window.Signal.Data.updateConversation(this.attributes);
       this.trigger('idUpdated', this, 'pni', oldValue);
+      this.captureChange('updatePni');
     }
   }
 

diff --git a/ts/services/storageRecordOps.ts b/ts/services/storageRecordOps.ts
@@ -137,6 +137,10 @@ export async function toContactRecord(
   if (e164) {
     contactRecord.serviceE164 = e164;
   }
+  const pni = conversation.get('pni');
+  if (pni) {
+    contactRecord.pni = pni;
+  }
   const profileKey = conversation.get('profileKey');
   if (profileKey) {
     contactRecord.profileKey = Bytes.fromBase64(String(profileKey));
@@ -849,6 +853,7 @@ export async function mergeContactRecord(
 
   const e164 = dropNull(contactRecord.serviceE164);
   const uuid = dropNull(contactRecord.serviceUuid);
+  const pni = dropNull(contactRecord.pni);
 
   // All contacts must have UUID
   if (!uuid) {
@@ -866,13 +871,28 @@ export async function mergeContactRecord(
   const conversation = window.ConversationController.maybeMergeContacts({
     aci: uuid,
     e164,
+    pni,
     reason: 'mergeContactRecord',
   });
 
   if (!conversation) {
     throw new Error(`No conversation for ${storageID}`);
   }
 
+  // We're going to ignore this; it's likely a PNI-only contact we've already merged
+  if (conversation.get('uuid') !== uuid) {
+    log.warn(
+      `mergeContactRecord: ${conversation.idForLogging()} ` +
+        `with storageId ${conversation.get('storageID')} ` +
+        `had uuid that didn't match provided uuid ${uuid}`
+    );
+    return {
+      hasConflict: false,
+      shouldDrop: true,
+      details: [],
+    };
+  }
+
   let needsProfileFetch = false;
   if (contactRecord.profileKey && contactRecord.profileKey.length > 0) {
     needsProfileFetch = await conversation.setProfileKey(