diff --git a/app/src/androidTest/java/org/thoughtcrime/securesms/database/AttachmentTableTest.kt b/app/src/androidTest/java/org/thoughtcrime/securesms/database/AttachmentTableTest.kt index 6895d57ab7..d339ed1722 100644 --- a/app/src/androidTest/java/org/thoughtcrime/securesms/database/AttachmentTableTest.kt +++ b/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, diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentTable.java b/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentTable.java index 426752de41..a2d052b574 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentTable.java +++ b/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 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()) { - Log.i(TAG, "[setAttachmentData] Deleted original file. " + dataInfo.file); + if (!dataInfo.file.equals(sharedDataInfo.get().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); - Log.d(TAG, "Wrote part to file: " + dataInfo.file.getAbsolutePath()); + DataInfo storeDataInfo = storeAttachmentStream(PartAuthority.getAttachmentStream(context, attachment.getUri())); + 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; - private final long length; - private final byte[] random; - private final String hash; + final File file; + final long length; + final byte[] random; + 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;