diff --git a/app/src/androidTest/assets/images/sample_image.png b/app/src/androidTest/assets/images/sample_image.png new file mode 100644 index 0000000000..0031fe6b1d Binary files /dev/null and b/app/src/androidTest/assets/images/sample_image.png differ 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 d339ed1722..746e429dcb 100644 --- a/app/src/androidTest/java/org/thoughtcrime/securesms/database/AttachmentTableTest.kt +++ b/app/src/androidTest/java/org/thoughtcrime/securesms/database/AttachmentTableTest.kt @@ -134,6 +134,40 @@ class AttachmentTableTest { highInfo.file.exists() assertIs true } + /** + * Given: Three pre-upload attachments with the same data but different transform properties (1x standard and 2x high). + * + * When inserting content of high pre-upload attachment. + * + * Then do not deduplicate with standard pre-upload attachment, but do deduplicate second high insert. + */ + @Test + fun doNotDedupedFileIfUsedByAnotherAttachmentWithADifferentTransformProperties() { + // GIVEN + val uncompressData = byteArrayOf(1, 2, 3, 4, 5) + val blobUncompressed = BlobProvider.getInstance().forData(uncompressData).createForSingleSessionInMemory() + + val standardQualityPreUpload = createAttachment(1, blobUncompressed, AttachmentTable.TransformProperties.empty()) + val standardDatabaseAttachment = SignalDatabase.attachments.insertAttachmentForPreUpload(standardQualityPreUpload) + + // WHEN + val highQualityPreUpload = createAttachment(1, blobUncompressed, AttachmentTable.TransformProperties.forSentMediaQuality(Optional.empty(), SentMediaQuality.HIGH)) + val highDatabaseAttachment = SignalDatabase.attachments.insertAttachmentForPreUpload(highQualityPreUpload) + + val secondHighQualityPreUpload = createAttachment(1, blobUncompressed, AttachmentTable.TransformProperties.forSentMediaQuality(Optional.empty(), SentMediaQuality.HIGH)) + val secondHighDatabaseAttachment = SignalDatabase.attachments.insertAttachmentForPreUpload(secondHighQualityPreUpload) + + // THEN + val standardInfo = SignalDatabase.attachments.getAttachmentDataFileInfo(standardDatabaseAttachment.attachmentId, AttachmentTable.DATA)!! + val highInfo = SignalDatabase.attachments.getAttachmentDataFileInfo(highDatabaseAttachment.attachmentId, AttachmentTable.DATA)!! + val secondHighInfo = SignalDatabase.attachments.getAttachmentDataFileInfo(secondHighDatabaseAttachment.attachmentId, AttachmentTable.DATA)!! + + highInfo.file assertIsNot standardInfo.file + secondHighInfo.file assertIs highInfo.file + standardInfo.file.exists() assertIs true + 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/androidTest/java/org/thoughtcrime/securesms/jobs/AttachmentCompressionJobTest.kt b/app/src/androidTest/java/org/thoughtcrime/securesms/jobs/AttachmentCompressionJobTest.kt new file mode 100644 index 0000000000..3a05feabb6 --- /dev/null +++ b/app/src/androidTest/java/org/thoughtcrime/securesms/jobs/AttachmentCompressionJobTest.kt @@ -0,0 +1,84 @@ +/* + * Copyright 2023 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.jobs + +import android.net.Uri +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.signal.core.util.StreamUtil +import org.thoughtcrime.securesms.attachments.UriAttachment +import org.thoughtcrime.securesms.database.AttachmentTable +import org.thoughtcrime.securesms.database.SignalDatabase +import org.thoughtcrime.securesms.database.UriAttachmentBuilder +import org.thoughtcrime.securesms.dependencies.ApplicationDependencies +import org.thoughtcrime.securesms.jobmanager.Job +import org.thoughtcrime.securesms.mms.SentMediaQuality +import org.thoughtcrime.securesms.providers.BlobProvider +import org.thoughtcrime.securesms.testing.SignalActivityRule +import org.thoughtcrime.securesms.testing.assertIs +import org.thoughtcrime.securesms.util.MediaUtil +import java.util.Optional +import java.util.concurrent.CountDownLatch + +@RunWith(AndroidJUnit4::class) +class AttachmentCompressionJobTest { + + @get:Rule + val harness = SignalActivityRule() + + @Test + fun testCompressionJobsWithDifferentTransformPropertiesCompleteSuccessfully() { + val imageBytes: ByteArray = InstrumentationRegistry.getInstrumentation().context.resources.assets.open("images/sample_image.png").use { + StreamUtil.readFully(it) + } + + val blob = BlobProvider.getInstance().forData(imageBytes).createForSingleSessionOnDisk(ApplicationDependencies.getApplication()) + + val firstPreUpload = createAttachment(1, blob, AttachmentTable.TransformProperties.empty()) + val firstDatabaseAttachment = SignalDatabase.attachments.insertAttachmentForPreUpload(firstPreUpload) + + val firstCompressionJob: AttachmentCompressionJob = AttachmentCompressionJob.fromAttachment(firstDatabaseAttachment, false, -1) + + var secondCompressionJob: AttachmentCompressionJob? = null + var firstJobResult: Job.Result? = null + var secondJobResult: Job.Result? = null + + val secondJobLatch = CountDownLatch(1) + val jobThread = Thread { + firstCompressionJob.setContext(ApplicationDependencies.getApplication()) + firstJobResult = firstCompressionJob.run() + + secondJobLatch.await() + + secondCompressionJob!!.setContext(ApplicationDependencies.getApplication()) + secondJobResult = secondCompressionJob!!.run() + } + + jobThread.start() + val secondPreUpload = createAttachment(1, blob, AttachmentTable.TransformProperties.forSentMediaQuality(Optional.empty(), SentMediaQuality.HIGH)) + val secondDatabaseAttachment = SignalDatabase.attachments.insertAttachmentForPreUpload(secondPreUpload) + secondCompressionJob = AttachmentCompressionJob.fromAttachment(secondDatabaseAttachment, false, -1) + + secondJobLatch.countDown() + + jobThread.join() + + firstJobResult!!.isSuccess assertIs true + secondJobResult!!.isSuccess assertIs true + } + + private fun createAttachment(id: Long, uri: Uri, transformProperties: AttachmentTable.TransformProperties): UriAttachment { + return UriAttachmentBuilder.build( + id, + uri = uri, + contentType = MediaUtil.IMAGE_JPEG, + transformProperties = transformProperties + ) + } +} diff --git a/app/src/androidTest/java/org/thoughtcrime/securesms/testing/TestUtils.kt b/app/src/androidTest/java/org/thoughtcrime/securesms/testing/TestUtils.kt index 3a62757904..542672cc17 100644 --- a/app/src/androidTest/java/org/thoughtcrime/securesms/testing/TestUtils.kt +++ b/app/src/androidTest/java/org/thoughtcrime/securesms/testing/TestUtils.kt @@ -1,11 +1,19 @@ package org.thoughtcrime.securesms.testing +import android.database.Cursor +import android.util.Base64 import org.hamcrest.MatcherAssert.assertThat import org.hamcrest.Matchers.hasSize import org.hamcrest.Matchers.`is` import org.hamcrest.Matchers.not import org.hamcrest.Matchers.notNullValue import org.hamcrest.Matchers.nullValue +import org.signal.core.util.logging.Log +import org.signal.core.util.readToList +import org.signal.core.util.select +import org.thoughtcrime.securesms.database.MessageTable +import org.thoughtcrime.securesms.database.SignalDatabase +import org.thoughtcrime.securesms.util.MessageTableTestUtils import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit import java.util.concurrent.TimeoutException @@ -53,3 +61,29 @@ fun CountDownLatch.awaitFor(duration: Duration) { throw TimeoutException("Latch await took longer than ${duration.inWholeMilliseconds}ms") } } + +fun dumpTableToLogs(tag: String = "TestUtils", table: String) { + dumpTable(table).forEach { Log.d(tag, it.toString()) } +} + +fun dumpTable(table: String): List>> { + return SignalDatabase.rawDatabase + .select() + .from(table) + .run() + .readToList { cursor -> + val map: List> = cursor.columnNames.map { column -> + val index = cursor.getColumnIndex(column) + var data: String? = when (cursor.getType(index)) { + Cursor.FIELD_TYPE_BLOB -> Base64.encodeToString(cursor.getBlob(index), 0) + else -> cursor.getString(index) + } + if (table == MessageTable.TABLE_NAME && column == MessageTable.TYPE) { + data = MessageTableTestUtils.typeColumnToString(cursor.getLong(index)) + } + + column to data + } + map + } +} 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 a2d052b574..e7ccf9b1ad 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentTable.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentTable.java @@ -35,6 +35,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import org.json.JSONArray; import org.json.JSONException; +import org.signal.core.util.CursorExtensionsKt; import org.signal.core.util.CursorUtil; import org.signal.core.util.SQLiteDatabaseExtensionsKt; import org.signal.core.util.SetUtil; @@ -612,7 +613,7 @@ public class AttachmentTable extends DatabaseTable { } /** - * Check if data file is in use by another attachment row with a different hash. Rows with the same data and has + * Check if data file is in use by another attachment row with a different hash. Rows with the same data and hash * will be fixed in a later call to {@link #updateAttachmentAndMatchingHashes(SQLiteDatabase, AttachmentId, String, ContentValues)}. */ private boolean isAttachmentFileUsedByOtherAttachments(@Nullable AttachmentId attachmentId, @NonNull DataInfo dataInfo) { @@ -638,7 +639,7 @@ public class AttachmentTable extends DatabaseTable { database.beginTransaction(); try { - dataInfo = deduplicateAttachment(dataInfo, attachmentId); + dataInfo = deduplicateAttachment(dataInfo, attachmentId, placeholder != null ? placeholder.getTransformProperties() : TransformProperties.empty()); if (oldInfo != null) { updateAttachmentDataHash(database, oldInfo.hash, dataInfo); } @@ -891,7 +892,7 @@ public class AttachmentTable extends DatabaseTable { database.beginTransaction(); try { - dataInfo = deduplicateAttachment(dataInfo, databaseAttachment.getAttachmentId()); + dataInfo = deduplicateAttachment(dataInfo, databaseAttachment.getAttachmentId(), databaseAttachment.getTransformProperties()); ContentValues contentValues = new ContentValues(); contentValues.put(SIZE, dataInfo.length); @@ -1131,7 +1132,7 @@ public class AttachmentTable extends DatabaseTable { { SQLiteDatabase database = databaseHelper.getSignalReadableDatabase(); - try (Cursor cursor = database.query(TABLE_NAME, new String[] { dataType, SIZE, DATA_RANDOM, DATA_HASH }, PART_ID_WHERE, attachmentId.toStrings(), null, null, null)) { + try (Cursor cursor = database.query(TABLE_NAME, new String[] { dataType, SIZE, DATA_RANDOM, DATA_HASH, TRANSFORM_PROPERTIES }, PART_ID_WHERE, attachmentId.toStrings(), null, null, null)) { if (cursor != null && cursor.moveToFirst()) { if (cursor.isNull(cursor.getColumnIndexOrThrow(dataType))) { return null; @@ -1140,7 +1141,8 @@ public class AttachmentTable extends DatabaseTable { return new DataInfo(new File(cursor.getString(cursor.getColumnIndexOrThrow(dataType))), cursor.getLong(cursor.getColumnIndexOrThrow(SIZE)), cursor.getBlob(cursor.getColumnIndexOrThrow(DATA_RANDOM)), - cursor.getString(cursor.getColumnIndexOrThrow(DATA_HASH))); + cursor.getString(cursor.getColumnIndexOrThrow(DATA_HASH)), + TransformProperties.parse(CursorUtil.requireString(cursor, TRANSFORM_PROPERTIES))); } else { return null; } @@ -1171,7 +1173,7 @@ public class AttachmentTable extends DatabaseTable { } /** - * Reads the entire stream and saves to disk. If you need to deduplicate attachments, call {@link #deduplicateAttachment(DataInfo, AttachmentId)} + * Reads the entire stream and saves to disk. If you need to deduplicate attachments, call {@link #deduplicateAttachment(DataInfo, AttachmentId, TransformProperties)} * afterwards and use the {@link DataInfo} returned by it instead. */ private @NonNull DataInfo storeAttachmentStream(@NonNull File destination, @NonNull InputStream in) throws MmsException { @@ -1189,69 +1191,74 @@ public class AttachmentTable extends DatabaseTable { throw new IllegalStateException("Couldn't rename " + tempFile.getPath() + " to " + destination.getPath()); } - return new DataInfo(destination, length, out.first, hash); + return new DataInfo(destination, length, out.first, hash, null); } catch (IOException | NoSuchAlgorithmException e) { throw new MmsException(e); } } - private @NonNull DataInfo deduplicateAttachment(@NonNull DataInfo dataInfo, @Nullable AttachmentId attachmentId) throws MmsException { + private @NonNull DataInfo deduplicateAttachment(@NonNull DataInfo dataInfo, + @Nullable AttachmentId attachmentId, + @NonNull TransformProperties transformProperties) + { SQLiteDatabase db = databaseHelper.getSignalWritableDatabase(); if (!db.inTransaction()) { throw new IllegalStateException("Must be in a transaction!"); } - 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)) { - 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); + List sharedDataInfos = findDuplicateDataFileInfos(db, dataInfo.hash, attachmentId); + for (DataInfo sharedDataInfo : sharedDataInfos) { + if (dataInfo.file.equals(sharedDataInfo.file)) { + continue; + } + + boolean isUsedElsewhere = isAttachmentFileUsedByOtherAttachments(attachmentId, dataInfo); + boolean isSameQuality = transformProperties.sentMediaQuality == sharedDataInfo.transformProperties.sentMediaQuality; + + Log.i(TAG, "[deduplicateAttachment] Potential duplicate data file found. usedElsewhere: " + isUsedElsewhere + " sameQuality: " + isSameQuality + " otherFile: " + sharedDataInfo.file.getAbsolutePath()); + + if (!isSameQuality) { + continue; + } + + if (!isUsedElsewhere) { + if (dataInfo.file.delete()) { + Log.i(TAG, "[deduplicateAttachment] Deleted original file. " + dataInfo.file); } else { - Log.w(TAG, "[setAttachmentData] Original file could not be deleted."); + Log.w(TAG, "[deduplicateAttachment] Original file could not be deleted."); } } - return sharedDataInfo.get(); - } else { - Log.i(TAG, "[setAttachmentData] No matching attachment data found. " + dataInfo.file.getAbsolutePath()); + + return sharedDataInfo; } + Log.i(TAG, "[deduplicateAttachment] No acceptable matching attachment data found. " + dataInfo.file.getAbsolutePath()); return dataInfo; } - private static @NonNull Optional findDuplicateDataFileInfo(@NonNull SQLiteDatabase database, - @NonNull String hash, - @Nullable AttachmentId excludedAttachmentId) + private static @NonNull List findDuplicateDataFileInfos(@NonNull SQLiteDatabase database, + @NonNull String hash, + @Nullable AttachmentId excludedAttachmentId) { if (!database.inTransaction()) { throw new IllegalArgumentException("Must be in a transaction!"); } Pair selectorArgs = buildSharedFileSelectorArgs(hash, excludedAttachmentId); - try (Cursor cursor = database.query(TABLE_NAME, - new String[]{DATA, DATA_RANDOM, SIZE, TRANSFORM_PROPERTIES}, - selectorArgs.first, - selectorArgs.second, - null, - null, - null, - "1")) - { - if (cursor == null || !cursor.moveToFirst()) return Optional.empty(); - - if (cursor.getCount() > 0) { - DataInfo dataInfo = new DataInfo(new File(CursorUtil.requireString(cursor, DATA)), - CursorUtil.requireLong(cursor, SIZE), - CursorUtil.requireBlob(cursor, DATA_RANDOM), - hash); - return Optional.of(dataInfo); - } else { - return Optional.empty(); - } - } + return CursorExtensionsKt.readToList(database.query(TABLE_NAME, + new String[] { DATA, DATA_RANDOM, SIZE, TRANSFORM_PROPERTIES }, + selectorArgs.first, + selectorArgs.second, + null, + null, + null, + null), + cursor -> new DataInfo(new File(CursorUtil.requireString(cursor, DATA)), + CursorUtil.requireLong(cursor, SIZE), + CursorUtil.requireBlob(cursor, DATA_RANDOM), + hash, + TransformProperties.parse(CursorUtil.requireString(cursor, TRANSFORM_PROPERTIES)))); } private static Pair buildSharedFileSelectorArgs(@NonNull String newHash, @@ -1388,27 +1395,31 @@ public class AttachmentTable extends DatabaseTable { if (attachment.getUri() != null) { DataInfo storeDataInfo = storeAttachmentStream(PartAuthority.getAttachmentStream(context, attachment.getUri())); Log.d(TAG, "Wrote part to file: " + storeDataInfo.file.getAbsolutePath()); - dataInfo = deduplicateAttachment(storeDataInfo, attachmentId); + dataInfo = deduplicateAttachment(storeDataInfo, attachmentId, attachment.getTransformProperties()); } Attachment template = attachment; + boolean useTemplateUpload = false; if (dataInfo != null && dataInfo.hash != null) { - Attachment possibleTemplate = findTemplateAttachment(dataInfo.hash); + List possibleTemplates = findTemplateAttachments(dataInfo.hash); - if (possibleTemplate != null) { - Log.i(TAG, "Found a duplicate attachment upon insertion. Using it as a template."); - template = possibleTemplate; + for (Attachment possibleTemplate : possibleTemplates) { + useTemplateUpload = possibleTemplate.getUploadTimestamp() > attachment.getUploadTimestamp() && + possibleTemplate.getTransferState() == TRANSFER_PROGRESS_DONE && + possibleTemplate.getTransformProperties().shouldSkipTransform() && + possibleTemplate.getDigest() != null && + !attachment.getTransformProperties().isVideoEdited() && + possibleTemplate.getTransformProperties().sentMediaQuality == attachment.getTransformProperties().getSentMediaQuality(); + + if (useTemplateUpload) { + Log.i(TAG, "Found a duplicate attachment upon insertion. Using it as a template."); + template = possibleTemplate; + break; + } } } - boolean useTemplateUpload = template.getUploadTimestamp() > attachment.getUploadTimestamp() && - template.getTransferState() == TRANSFER_PROGRESS_DONE && - template.getTransformProperties().shouldSkipTransform() && - template.getDigest() != null && - !attachment.getTransformProperties().isVideoEdited() && - template.getTransformProperties().sentMediaQuality == attachment.getTransformProperties().getSentMediaQuality(); - ContentValues contentValues = new ContentValues(); contentValues.put(MMS_ID, mmsId); contentValues.put(CONTENT_TYPE, template.getContentType()); @@ -1450,7 +1461,7 @@ public class AttachmentTable extends DatabaseTable { contentValues.put(DATA, dataInfo.file.getAbsolutePath()); contentValues.put(SIZE, dataInfo.length); contentValues.put(DATA_RANDOM, dataInfo.random); - if (attachment.getTransformProperties().isVideoEdited() || attachment.getTransformProperties().sentMediaQuality != template.getTransformProperties().getSentMediaQuality()) { + if (attachment.getTransformProperties().isVideoEdited()) { contentValues.putNull(DATA_HASH); } else { contentValues.put(DATA_HASH, dataInfo.hash); @@ -1478,17 +1489,11 @@ public class AttachmentTable extends DatabaseTable { return attachmentId; } - private @Nullable DatabaseAttachment findTemplateAttachment(@NonNull String dataHash) { + private @NonNull List findTemplateAttachments(@NonNull String dataHash) { String selection = DATA_HASH + " = ?"; String[] args = new String[] { dataHash }; - try (Cursor cursor = databaseHelper.getSignalWritableDatabase().query(TABLE_NAME, null, selection, args, null, null, null)) { - if (cursor != null && cursor.moveToFirst()) { - return getAttachments(cursor).get(0); - } - } - - return null; + return CursorExtensionsKt.readToList(databaseHelper.getSignalReadableDatabase().query(TABLE_NAME, null, selection, args, null, null, null), this::getAttachment); } @WorkerThread @@ -1536,16 +1541,18 @@ public class AttachmentTable extends DatabaseTable { @VisibleForTesting static class DataInfo { - final File file; - final long length; - final byte[] random; - final String hash; + final File file; + final long length; + final byte[] random; + final String hash; + final TransformProperties transformProperties; - private DataInfo(File file, long length, byte[] random, String hash) { - this.file = file; - this.length = length; - this.random = random; - this.hash = hash; + private DataInfo(File file, long length, byte[] random, String hash, TransformProperties transformProperties) { + this.file = file; + this.length = length; + this.random = random; + this.hash = hash; + this.transformProperties = transformProperties; } @Override @@ -1556,12 +1563,13 @@ public class AttachmentTable extends DatabaseTable { return length == dataInfo.length && Objects.equals(file, dataInfo.file) && Arrays.equals(random, dataInfo.random) && - Objects.equals(hash, dataInfo.hash); + Objects.equals(hash, dataInfo.hash) && + Objects.equals(transformProperties, dataInfo.transformProperties); } @Override public int hashCode() { - int result = Objects.hash(file, length, hash); + int result = Objects.hash(file, length, hash, transformProperties); result = 31 * result + Arrays.hashCode(random); return result; } diff --git a/donations/lib/src/main/java/org/signal/donations/StripeApi.kt b/donations/lib/src/main/java/org/signal/donations/StripeApi.kt index 74365a33a1..887c4bb290 100644 --- a/donations/lib/src/main/java/org/signal/donations/StripeApi.kt +++ b/donations/lib/src/main/java/org/signal/donations/StripeApi.kt @@ -14,7 +14,6 @@ import okhttp3.FormBody import okhttp3.OkHttpClient import okhttp3.Request import okhttp3.Response -import okio.ByteString import okio.ByteString.Companion.encodeUtf8 import org.json.JSONObject import org.signal.core.util.logging.Log