Fix removal of local only unregistered accounts in storage sync

Author: AsamK

lib/src/main/java/org/asamk/signal/manager/helper/StorageHelper.java

@@ -2,6 +2,7 @@
 
 import org.asamk.signal.manager.api.GroupIdV1;
 import org.asamk.signal.manager.api.GroupIdV2;
+import org.asamk.signal.manager.api.Pair;
 import org.asamk.signal.manager.api.Profile;
 import org.asamk.signal.manager.internal.SignalDependencies;
 import org.asamk.signal.manager.storage.SignalAccount;
@@ -38,6 +39,7 @@
 import java.util.Base64;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.stream.Collectors;
@@ -211,20 +213,23 @@ private boolean readDataFromStorage(
                             remoteOnlyRecords.size());
                 }
 
-                // This logic is wrong, records should only be deleted if they're deleted remotely, not if the remote record is updated
-//                if (!idDifference.localOnlyIds().isEmpty()) {
-//                    final var updated = account.getRecipientStore()
-//                            .removeStorageIdsFromLocalOnlyUnregisteredRecipients(connection,
-//                                    idDifference.localOnlyIds());
-//
-//                    if (updated > 0) {
-//                        logger.warn(
-//                                "Found {} records that were deleted remotely but only marked unregistered locally. Removed those from local store.",
-//                                updated);
-//                    }
-//                }
-//
-                final var unknownInserts = processKnownRecords(connection, remoteOnlyRecords);
+                final var listListPair = processKnownRecords(connection, remoteOnlyRecords);
+                final var unknownInserts = listListPair.first();
+                final var updatedStorageIds = listListPair.second();
+                final var oldUnregisteredLocalOnlyIds = new HashSet<>(idDifference.localOnlyIds());
+                updatedStorageIds.forEach(oldUnregisteredLocalOnlyIds::remove);
+                if (!idDifference.localOnlyIds().isEmpty()) {
+                    final var updated = account.getRecipientStore()
+                            .removeStorageIdsFromLocalOnlyUnregisteredRecipients(connection,
+                                    oldUnregisteredLocalOnlyIds);
+
+                    if (updated > 0) {
+                        logger.warn(
+                                "Found {} records that were deleted remotely but only marked unregistered locally. Removed those from local store.",
+                                updated);
+                    }
+                }
+
                 final var unknownDeletes = idDifference.localOnlyIds()
                         .stream()
                         .filter(id -> !KNOWN_TYPES.contains(id.getType()))
@@ -480,13 +485,13 @@ private Map<RecipientId, StorageId> generateContactStorageIds(List<RecipientId>
     private Map<GroupIdV1, StorageId> generateGroupV1StorageIds(List<GroupIdV1> groupIds) {
         return groupIds.stream()
                 .collect(Collectors.toMap(recipientId -> recipientId,
-                        recipientId -> StorageId.forGroupV1(KeyUtils.createRawStorageId())));
+                        _ -> StorageId.forGroupV1(KeyUtils.createRawStorageId())));
     }
 
     private Map<GroupIdV2, StorageId> generateGroupV2StorageIds(List<GroupIdV2> groupIds) {
         return groupIds.stream()
                 .collect(Collectors.toMap(recipientId -> recipientId,
-                        recipientId -> StorageId.forGroupV2(KeyUtils.createRawStorageId())));
+                        _ -> StorageId.forGroupV2(KeyUtils.createRawStorageId())));
     }
 
     private void storeManifestLocally(
@@ -630,16 +635,17 @@ private static IdDifferenceResult findIdDifference(
         return new IdDifferenceResult(remoteOnlyKeys, localOnlyKeys, hasTypeMismatch);
     }
 
-    private List<StorageId> processKnownRecords(
+    private Pair<List<StorageId>, List<StorageId>> processKnownRecords(
             final Connection connection,
             List<SignalStorageRecord> records
     ) throws SQLException {
         final var unknownRecords = new ArrayList<StorageId>();
+        final var processedRecords = new ArrayList<StorageId>();
 
         final var accountRecordProcessor = new AccountRecordProcessor(account, connection, context.getJobExecutor());
-        final var contactRecordProcessor = new ContactRecordProcessor(account, connection, context.getJobExecutor());
         final var groupV1RecordProcessor = new GroupV1RecordProcessor(account, connection);
         final var groupV2RecordProcessor = new GroupV2RecordProcessor(account, connection);
+        final var contactRecordProcessor = new ContactRecordProcessor(account, connection, context.getJobExecutor());
 
         for (final var record : records) {
             if (record.getProto().account != null) {
@@ -662,8 +668,12 @@ private List<StorageId> processKnownRecords(
                 unknownRecords.add(record.getId());
             }
         }
+        processedRecords.addAll(accountRecordProcessor.getUpdatedStorageIds());
+        processedRecords.addAll(groupV1RecordProcessor.getUpdatedStorageIds());
+        processedRecords.addAll(groupV2RecordProcessor.getUpdatedStorageIds());
+        processedRecords.addAll(contactRecordProcessor.getUpdatedStorageIds());
 
-        return unknownRecords;
+        return new Pair<>(unknownRecords, processedRecords);
     }
 
     /**

lib/src/main/java/org/asamk/signal/manager/storage/recipients/RecipientStore.java

@@ -878,7 +878,7 @@ public void storeContact(
 
     public int removeStorageIdsFromLocalOnlyUnregisteredRecipients(
             final Connection connection,
-            final List<StorageId> storageIds
+            final Collection<StorageId> storageIds
     ) throws SQLException {
         final var sql = (
                 """

lib/src/main/java/org/asamk/signal/manager/syncStorage/DefaultStorageRecordProcessor.java

@@ -6,7 +6,9 @@
 import org.whispersystems.signalservice.api.storage.StorageId;
 
 import java.sql.SQLException;
+import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashSet;
 import java.util.Optional;
 import java.util.Set;
 import java.util.TreeSet;
@@ -24,6 +26,7 @@ abstract class DefaultStorageRecordProcessor<E extends SignalRecord<?>> implemen
 
     private static final Logger logger = LoggerFactory.getLogger(DefaultStorageRecordProcessor.class);
     private final Set<E> matchedRecords = new TreeSet<>(this);
+    private final Set<StorageId> updatedStorageIds = new HashSet<>();
 
     /**
      * One type of invalid remote data this handles is two records mapping to the same local data. We
@@ -50,6 +53,7 @@ public void process(E remote) throws SQLException {
 
         if (local.isEmpty()) {
             debug(remote.getId(), remote, "[Local Insert] No matching local record. Inserting.");
+            updatedStorageIds.add(remote.getId());
             insertLocal(remote);
             return;
         }
@@ -64,6 +68,7 @@ public void process(E remote) throws SQLException {
         matchedRecords.add(local.get());
 
         final var merged = merge(remote, local.get());
+        updatedStorageIds.add(merged.getId());
         if (!merged.equals(remote)) {
             debug(remote.getId(), remote, "[Remote Update] " + merged.describeDiff(remote));
         }
@@ -75,6 +80,10 @@ public void process(E remote) throws SQLException {
         }
     }
 
+    public Set<StorageId> getUpdatedStorageIds() {
+        return Collections.unmodifiableSet(updatedStorageIds);
+    }
+
     private void debug(StorageId i, E record, String message) {
         logger.debug("[{}][{}] {}", i, record.getClass().getSimpleName(), message);
     }