Prevent attachment send of duplicate data with different transforms from failing.

app/src/androidTest/java/org/thoughtcrime/securesms/database/AttachmentTableTest.kt

@@ -9,10 +9,13 @@ import org.junit.Before
 import org.junit.Ignore
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.thoughtcrime.securesms.attachments.AttachmentId
 import org.thoughtcrime.securesms.attachments.UriAttachment
 import org.thoughtcrime.securesms.mms.MediaStream
 import org.thoughtcrime.securesms.mms.SentMediaQuality
 import org.thoughtcrime.securesms.providers.BlobProvider
+import org.thoughtcrime.securesms.testing.assertIs
+import org.thoughtcrime.securesms.testing.assertIsNot
 import org.thoughtcrime.securesms.util.MediaUtil
 import java.util.Optional
 
@@ -92,6 +95,45 @@ class AttachmentTableTest {
     assertNotEquals(attachment1Info, attachment2Info)
   }
 
+  /**
+   * Given: A previous attachment and two pre-upload attachments with the same data but different transform properties (standard and high).
+   *
+   * When changing content of standard pre-upload attachment to match pre-existing attachment
+   *
+   * Then update standard pre-upload attachment to match previous attachment, do not update high pre-upload attachment, and do
+   * not delete shared pre-upload uri from disk as it is still being used by the high pre-upload attachment.
+   */
+  @Test
+  fun doNotDeleteDedupedFileIfUsedByAnotherAttachmentWithADifferentTransformProperties() {
+    // GIVEN
+    val uncompressData = byteArrayOf(1, 2, 3, 4, 5)
+    val compressedData = byteArrayOf(1, 2, 3)
+
+    val blobUncompressed = BlobProvider.getInstance().forData(uncompressData).createForSingleSessionInMemory()
+
+    val previousAttachment = createAttachment(1, BlobProvider.getInstance().forData(compressedData).createForSingleSessionInMemory(), AttachmentTable.TransformProperties.empty())
+    val previousDatabaseAttachmentId: AttachmentId = SignalDatabase.attachments.insertAttachmentsForMessage(1, listOf(previousAttachment), emptyList()).values.first()
+
+    val standardQualityPreUpload = createAttachment(1, blobUncompressed, AttachmentTable.TransformProperties.empty())
+    val standardDatabaseAttachment = SignalDatabase.attachments.insertAttachmentForPreUpload(standardQualityPreUpload)
+
+    val highQualityPreUpload = createAttachment(1, blobUncompressed, AttachmentTable.TransformProperties.forSentMediaQuality(Optional.empty(), SentMediaQuality.HIGH))
+    val highDatabaseAttachment = SignalDatabase.attachments.insertAttachmentForPreUpload(highQualityPreUpload)
+
+    // WHEN
+    SignalDatabase.attachments.updateAttachmentData(standardDatabaseAttachment, createMediaStream(compressedData), false)
+
+    // THEN
+    val previousInfo = SignalDatabase.attachments.getAttachmentDataFileInfo(previousDatabaseAttachmentId, AttachmentTable.DATA)!!
+    val standardInfo = SignalDatabase.attachments.getAttachmentDataFileInfo(standardDatabaseAttachment.attachmentId, AttachmentTable.DATA)!!
+    val highInfo = SignalDatabase.attachments.getAttachmentDataFileInfo(highDatabaseAttachment.attachmentId, AttachmentTable.DATA)!!
+
+    assertNotEquals(standardInfo, highInfo)
+    standardInfo.file assertIs previousInfo.file
+    highInfo.file assertIsNot standardInfo.file
+    highInfo.file.exists() assertIs true
+  }
+
   private fun createAttachment(id: Long, uri: Uri, transformProperties: AttachmentTable.TransformProperties): UriAttachment {
     return UriAttachmentBuilder.build(
       id,

app/src/main/java/org/thoughtcrime/securesms/database/AttachmentTable.java

@@ -20,7 +20,6 @@ import android.content.ContentValues;
 import android.content.Context;
 import android.database.Cursor;
 import android.media.MediaDataSource;
-import android.net.Uri;
 import android.text.TextUtils;
 import android.util.Pair;
 
@@ -612,6 +611,20 @@ public class AttachmentTable extends DatabaseTable {
     return new DataUsageResult(quoteRows);
   }
 
+  /**
+   * Check if data file is in use by another attachment row with a different hash. Rows with the same data and has
+   * will be fixed in a later call to {@link #updateAttachmentAndMatchingHashes(SQLiteDatabase, AttachmentId, String, ContentValues)}.
+   */
+  private boolean isAttachmentFileUsedByOtherAttachments(@Nullable AttachmentId attachmentId, @NonNull DataInfo dataInfo) {
+    if (attachmentId == null) {
+      return false;
+    }
+
+    return SQLiteDatabaseExtensionsKt.exists(getReadableDatabase(), TABLE_NAME)
+                                     .where(DATA + " = ? AND " + DATA_HASH + " != ?", dataInfo.file.getAbsolutePath(), dataInfo.hash)
+                                     .run();
+  }
+
   public void insertAttachmentsForPlaceholder(long mmsId, @NonNull AttachmentId attachmentId, @NonNull InputStream inputStream)
       throws MmsException
   {
@@ -1192,8 +1205,14 @@ public class AttachmentTable extends DatabaseTable {
     Optional<DataInfo> sharedDataInfo = findDuplicateDataFileInfo(db, dataInfo.hash, attachmentId);
     if (sharedDataInfo.isPresent()) {
       Log.i(TAG, "[setAttachmentData] Duplicate data file found! " + sharedDataInfo.get().file.getAbsolutePath());
-      if (!dataInfo.file.equals(sharedDataInfo.get().file) && dataInfo.file.delete()) {
+      if (!dataInfo.file.equals(sharedDataInfo.get().file)) {
-        Log.i(TAG, "[setAttachmentData] Deleted original file. " + dataInfo.file);
+        if (isAttachmentFileUsedByOtherAttachments(attachmentId, dataInfo)) {
+          Log.i(TAG, "[setAttachmentData] Original file still in use by another attachment with a different hash.");
+        } else if (dataInfo.file.delete()) {
+          Log.i(TAG, "[setAttachmentData] Deleted original file. " + dataInfo.file);
+        } else {
+          Log.w(TAG, "[setAttachmentData] Original file could not be deleted.");
+        }
       }
       return sharedDataInfo.get();
     } else {
@@ -1367,8 +1386,9 @@ public class AttachmentTable extends DatabaseTable {
       long           uniqueId        = System.currentTimeMillis();
 
       if (attachment.getUri() != null) {
-        dataInfo = deduplicateAttachment(storeAttachmentStream(PartAuthority.getAttachmentStream(context, attachment.getUri())), attachmentId);
+        DataInfo storeDataInfo = storeAttachmentStream(PartAuthority.getAttachmentStream(context, attachment.getUri()));
-        Log.d(TAG, "Wrote part to file: " + dataInfo.file.getAbsolutePath());
+        Log.d(TAG, "Wrote part to file: " + storeDataInfo.file.getAbsolutePath());
+        dataInfo = deduplicateAttachment(storeDataInfo, attachmentId);
       }
 
       Attachment template = attachment;
@@ -1516,10 +1536,10 @@ public class AttachmentTable extends DatabaseTable {
 
   @VisibleForTesting
   static class DataInfo {
-    private final File   file;
+    final File   file;
-    private final long   length;
+    final long   length;
-    private final byte[] random;
+    final byte[] random;
-    private final String hash;
+    final String hash;
 
     private DataInfo(File file, long length, byte[] random, String hash) {
       this.file   = file;
@@ -1528,7 +1548,8 @@ public class AttachmentTable extends DatabaseTable {
       this.hash   = hash;
     }
 
-    @Override public boolean equals(Object o) {
+    @Override
+    public boolean equals(Object o) {
       if (this == o) return true;
       if (o == null || getClass() != o.getClass()) return false;
       final DataInfo dataInfo = (DataInfo) o;
@@ -1538,7 +1559,8 @@ public class AttachmentTable extends DatabaseTable {
              Objects.equals(hash, dataInfo.hash);
     }
 
-    @Override public int hashCode() {
+    @Override
+    public int hashCode() {
       int result = Objects.hash(file, length, hash);
       result = 31 * result + Arrays.hashCode(random);
       return result;