Fix bug with quote deduping.

This commit is contained in:
Greyson Parrelli
2024-03-15 13:30:41 -04:00
committed by Cody Henthorne
parent 1d29b0166d
commit 011f1d592e
2 changed files with 62 additions and 11 deletions

View File

@@ -434,6 +434,43 @@ class AttachmentTableTest_deduping {
}
}
@Test
fun quotes() {
// Basic quote deduping
test {
val id1 = insertWithData(DATA_A)
val id2 = insertQuote(id1)
assertDataFilesAreTheSame(id1, id2)
assertDataHashStartMatches(id1, id2)
}
// Making sure remote fields carry
test {
val id1 = insertWithData(DATA_A)
val id2 = insertQuote(id1)
upload(id1)
assertDataFilesAreTheSame(id1, id2)
assertDataHashStartMatches(id1, id2)
assertDataHashEndMatches(id1, id2)
assertRemoteFieldsMatch(id1, id2)
}
// Making sure things work for quotes of videos, which have trickier transform properties
test {
val id1 = insertWithData(DATA_A, transformProperties = TransformProperties.forVideoTrim(1, 2))
compress(id1, DATA_A_COMPRESSED)
upload(id1)
val id2 = insertQuote(id1)
assertDataFilesAreTheSame(id1, id2)
assertDataHashEndMatches(id1, id2)
assertRemoteFieldsMatch(id1, id2)
}
}
/**
* Suite of tests around the migration where we hash all of the attachments and potentially dedupe them.
*/
@@ -678,6 +715,7 @@ class AttachmentTableTest_deduping {
val lhsInfo = SignalDatabase.attachments.getDataFileInfo(lhs)!!
val rhsInfo = SignalDatabase.attachments.getDataFileInfo(rhs)!!
assertNotNull(lhsInfo.hashStart)
assertEquals("DATA_HASH_START's did not match!", lhsInfo.hashStart, rhsInfo.hashStart)
}
@@ -685,6 +723,7 @@ class AttachmentTableTest_deduping {
val lhsInfo = SignalDatabase.attachments.getDataFileInfo(lhs)!!
val rhsInfo = SignalDatabase.attachments.getDataFileInfo(rhs)!!
assertNotNull(lhsInfo.hashEnd)
assertEquals("DATA_HASH_END's did not match!", lhsInfo.hashEnd, rhsInfo.hashEnd)
}

View File

@@ -1278,9 +1278,20 @@ class AttachmentTable(
}
}
private fun areTransformationsCompatible(newProperties: TransformProperties, potentialMatchProperties: TransformProperties, newHashStart: String, potentialMatchHashEnd: String?): Boolean {
private fun areTransformationsCompatible(
newProperties: TransformProperties,
potentialMatchProperties: TransformProperties,
newHashStart: String,
potentialMatchHashEnd: String?,
newIsQuote: Boolean
): Boolean {
// If we're starting now where another attachment finished, then it means we're forwarding an attachment.
if (newHashStart == potentialMatchHashEnd) {
// Quotes don't get transcoded or anything and are just a reference to the original attachment, so as long as the hashes match we're fine
if (newIsQuote) {
return true
}
// If the new attachment is an edited video, we can't re-use the file
if (newProperties.videoEdited) {
return false
@@ -1402,15 +1413,16 @@ class AttachmentTable(
newProperties = transformProperties,
potentialMatchProperties = existingMatch.transformProperties,
newHashStart = fileWriteResult.hash,
potentialMatchHashEnd = existingMatch.hashEnd
potentialMatchHashEnd = existingMatch.hashEnd,
newIsQuote = quote
)
}
if (hashMatch != null) {
if (fileWriteResult.hash == hashMatch.hashStart) {
Log.i(TAG, "[insertAttachmentWithData] Found that the new attachment has the same DATA_HASH_START as ${hashMatch.id}. Using all of it's fields. (MessageId: $messageId, ${attachment.uri})")
Log.i(TAG, "[insertAttachmentWithData] Found that the new attachment hash matches the DATA_HASH_START of ${hashMatch.id}. Using all of it's fields. (MessageId: $messageId, ${attachment.uri})")
} else if (fileWriteResult.hash == hashMatch.hashEnd) {
Log.i(TAG, "[insertAttachmentWithData] Found that the new attachment has the same DATA_HASH_END as ${hashMatch.id}. Using all of it's fields. (MessageId: $messageId, ${attachment.uri})")
Log.i(TAG, "[insertAttachmentWithData] Found that the new attachment hash matches the DATA_HASH_END of ${hashMatch.id}. Using all of it's fields. (MessageId: $messageId, ${attachment.uri})")
} else {
throw IllegalStateException("Should not be possible based on query.")
}
@@ -1453,12 +1465,12 @@ class AttachmentTable(
contentValues.put(MESSAGE_ID, messageId)
contentValues.put(CONTENT_TYPE, uploadTemplate?.contentType ?: attachment.contentType)
contentValues.put(TRANSFER_STATE, attachment.transferState) // Even if we have a template, we let AttachmentUploadJob have the final say so it can re-check and make sure the template is still valid
contentValues.put(CDN_NUMBER, uploadTemplate?.cdnNumber ?: attachment.cdnNumber)
contentValues.put(REMOTE_LOCATION, uploadTemplate?.remoteLocation ?: attachment.remoteLocation)
contentValues.put(REMOTE_DIGEST, uploadTemplate?.remoteDigest ?: attachment.remoteDigest)
contentValues.put(REMOTE_INCREMENTAL_DIGEST, uploadTemplate?.incrementalDigest ?: attachment.incrementalDigest)
contentValues.put(REMOTE_INCREMENTAL_DIGEST_CHUNK_SIZE, uploadTemplate?.incrementalMacChunkSize ?: attachment.incrementalMacChunkSize)
contentValues.put(REMOTE_KEY, uploadTemplate?.remoteKey ?: attachment.remoteKey)
contentValues.put(CDN_NUMBER, uploadTemplate?.cdnNumber ?: 0)
contentValues.put(REMOTE_LOCATION, uploadTemplate?.remoteLocation)
contentValues.put(REMOTE_DIGEST, uploadTemplate?.remoteDigest)
contentValues.put(REMOTE_INCREMENTAL_DIGEST, uploadTemplate?.incrementalDigest)
contentValues.put(REMOTE_INCREMENTAL_DIGEST_CHUNK_SIZE, uploadTemplate?.incrementalMacChunkSize ?: 0)
contentValues.put(REMOTE_KEY, uploadTemplate?.remoteKey)
contentValues.put(FILE_NAME, StorageUtil.getCleanFileName(attachment.fileName))
contentValues.put(FAST_PREFLIGHT_ID, attachment.fastPreflightId)
contentValues.put(VOICE_NOTE, if (attachment.voiceNote) 1 else 0)
@@ -1468,7 +1480,7 @@ class AttachmentTable(
contentValues.put(HEIGHT, uploadTemplate?.height ?: attachment.height)
contentValues.put(QUOTE, quote)
contentValues.put(CAPTION, attachment.caption)
contentValues.put(UPLOAD_TIMESTAMP, uploadTemplate?.uploadTimestamp ?: attachment.uploadTimestamp)
contentValues.put(UPLOAD_TIMESTAMP, uploadTemplate?.uploadTimestamp ?: 0)
contentValues.put(TRANSFORM_PROPERTIES, transformProperties.serialize())
if (attachment.transformProperties?.videoEdited == true) {