From 185035784c973642430da46cbb22a6385396d6d8 Mon Sep 17 00:00:00 2001 From: Sasha Weiss Date: Mon, 1 Jun 2026 12:09:38 -0700 Subject: [PATCH] Treat images with `image/gif` MIME types as "GIFs" in the Media Gallery --- .../MediaGallery/MediaGallery.swift | 14 ++-- .../BackupAttachmentDownloadStoreTests.swift | 1 + .../BackupAttachmentUploadStoreTests.swift | 1 + .../AttachmentManagerImpl.swift | 10 ++- .../AttachmentReference+Owner.swift | 29 +++++++- .../AttachmentStoreTests.swift | 4 + .../AttachmentDownloadManagerImpl.swift | 21 +++++- .../AttachmentDownloadStoreTests.swift | 1 + .../V2/Mocks/MockAttachmentReference.swift | 2 + ...tachmentReference+ConstructionParams.swift | 7 ++ .../Records/AttachmentReference+Records.swift | 11 +++ .../Storage/Database/GRDBSchemaMigrator.swift | 66 +++++++++++++++++ .../MediaGalleryAttachmentFinder.swift | 7 +- .../MediaGalleryAttachmentFinderTest.swift | 73 ++++++++++++++++++- .../Database/GRDBSchemaMigratorTest.swift | 72 ++++++++++++++++++ 15 files changed, 295 insertions(+), 24 deletions(-) diff --git a/Signal/src/ViewControllers/MediaGallery/MediaGallery.swift b/Signal/src/ViewControllers/MediaGallery/MediaGallery.swift index d22f596e91..158fa85a3a 100644 --- a/Signal/src/ViewControllers/MediaGallery/MediaGallery.swift +++ b/Signal/src/ViewControllers/MediaGallery/MediaGallery.swift @@ -117,16 +117,12 @@ struct MediaGalleryItem: switch referencedAttachment.attachment.contentType { case .video: return renderingFlag == .shouldLoop - case .file, .image, .audio: - return false - } - } - - var isImage: Bool { - switch referencedAttachment.attachment.contentType { case .image: - return true - case .file, .video, .audio: + // Important that this remains synced with the conditions for the + // MessageAttachmentReference/isGifCategory computed column, which + // powers the MediaGalleryAttachmentFinder "GIFs" query. + return mimeType == MimeType.imageGif.rawValue + case .file, .audio: return false } } diff --git a/SignalServiceKit/Backups/Attachments/BackupAttachmentDownloadStoreTests.swift b/SignalServiceKit/Backups/Attachments/BackupAttachmentDownloadStoreTests.swift index d8e709eb50..614433f52d 100644 --- a/SignalServiceKit/Backups/Attachments/BackupAttachmentDownloadStoreTests.swift +++ b/SignalServiceKit/Backups/Attachments/BackupAttachmentDownloadStoreTests.swift @@ -280,6 +280,7 @@ class BackupAttachmentDownloadStoreTests: XCTestCase { receivedAtTimestamp: timestamp, threadRowId: threadRowId, contentType: attachment.contentType, + mimeType: attachment.mimeType, isPastEditRevision: false, )), ) diff --git a/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStoreTests.swift b/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStoreTests.swift index 743069e96b..502c205b51 100644 --- a/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStoreTests.swift +++ b/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStoreTests.swift @@ -354,6 +354,7 @@ class BackupAttachmentUploadStoreTests: XCTestCase { receivedAtTimestamp: timestamp, threadRowId: threadRowId, contentType: attachmentRecord.contentType, + mimeType: attachmentRecord.mimeType, isPastEditRevision: false, )), ) diff --git a/SignalServiceKit/Messages/Attachments/V2/AttachmentManager/AttachmentManagerImpl.swift b/SignalServiceKit/Messages/Attachments/V2/AttachmentManager/AttachmentManagerImpl.swift index dda32aff80..84ee0e24d5 100644 --- a/SignalServiceKit/Messages/Attachments/V2/AttachmentManager/AttachmentManagerImpl.swift +++ b/SignalServiceKit/Messages/Attachments/V2/AttachmentManager/AttachmentManagerImpl.swift @@ -218,6 +218,7 @@ public class AttachmentManagerImpl: AttachmentManager { knownIdInOwner: knownIdFromProto, renderingFlag: .fromProto(proto), contentType: contentType, + mimeType: mimeType, // This should be unset for newly-incoming attachments, but it's // still technically in the proto definition. caption: proto.hasCaption ? proto.caption : nil, @@ -415,6 +416,7 @@ public class AttachmentManagerImpl: AttachmentManager { knownIdInOwner: knownIdFromProto, renderingFlag: ownedProto.renderingFlag, contentType: contentType, + mimeType: mimeType, // Restored legacy attachments might have a caption. caption: proto.hasCaption ? proto.caption : nil, ), @@ -568,6 +570,7 @@ public class AttachmentManagerImpl: AttachmentManager { knownIdInOwner: .none, renderingFlag: existingAttachmentMetadata.renderingFlag, contentType: existingAttachment.contentType, + mimeType: existingAttachment.mimeType, ) let referenceParams = AttachmentReference.ConstructionParams( owner: owner, @@ -587,6 +590,7 @@ public class AttachmentManagerImpl: AttachmentManager { knownIdInOwner: .none, renderingFlag: pendingAttachment.renderingFlag, contentType: pendingAttachment.contentType, + mimeType: pendingAttachment.mimeType, ) let referenceParams = AttachmentReference.ConstructionParams( owner: owner, @@ -751,7 +755,10 @@ public class AttachmentManagerImpl: AttachmentManager { tx: tx, ) let newOwnerParams = AttachmentReference.ConstructionParams( - owner: reference.owner.forReassignmentWithContentType(pendingAttachment.contentType), + owner: reference.owner.forReassignmentWithContentType( + pendingAttachment.contentType, + mimeType: pendingAttachment.mimeType, + ), sourceFilename: reference.sourceFilename, sourceUnencryptedByteCount: reference.sourceUnencryptedByteCount, sourceMediaSizePixels: reference.sourceMediaSizePixels, @@ -1030,6 +1037,7 @@ public class AttachmentManagerImpl: AttachmentManager { knownIdInOwner: .none, renderingFlag: originalAttachmentSource.renderingFlag, contentType: thumbnailContentType, + mimeType: thumbnailMimeType, ), sourceFilename: originalAttachmentSource.sourceFilename, sourceUnencryptedByteCount: originalAttachmentSource.sourceUnencryptedByteCount, diff --git a/SignalServiceKit/Messages/Attachments/V2/AttachmentReference/AttachmentReference+Owner.swift b/SignalServiceKit/Messages/Attachments/V2/AttachmentReference/AttachmentReference+Owner.swift index 79ad25b1b6..2cd0e96fad 100644 --- a/SignalServiceKit/Messages/Attachments/V2/AttachmentReference/AttachmentReference+Owner.swift +++ b/SignalServiceKit/Messages/Attachments/V2/AttachmentReference/AttachmentReference+Owner.swift @@ -101,6 +101,10 @@ extension AttachmentReference { /// referenced attachment, duplicated here for query purposes. public let contentType: Attachment.ContentType + /// Mirrors the value of ``Attachment/mimeType``, for the + /// referenced attachment, duplicated here for query purposes. + public let mimeType: String + /// True if the owning message's ``TSEditState`` is `pastRevision`. public let isPastEditRevision: Bool @@ -109,12 +113,14 @@ extension AttachmentReference { receivedAtTimestamp: UInt64, threadRowId: Int64, contentType: Attachment.ContentType, + mimeType: String, isPastEditRevision: Bool, ) { self.messageRowId = messageRowId self.receivedAtTimestamp = receivedAtTimestamp self.threadRowId = threadRowId self.contentType = contentType + self.mimeType = mimeType self.isPastEditRevision = isPastEditRevision } } @@ -141,6 +147,7 @@ extension AttachmentReference { receivedAtTimestamp: UInt64, threadRowId: Int64, contentType: Attachment.ContentType, + mimeType: String, isPastEditRevision: Bool, caption: String?, renderingFlag: RenderingFlag, @@ -158,6 +165,7 @@ extension AttachmentReference { receivedAtTimestamp: receivedAtTimestamp, threadRowId: threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: isPastEditRevision, ) } @@ -172,6 +180,7 @@ extension AttachmentReference { receivedAtTimestamp: UInt64, threadRowId: Int64, contentType: Attachment.ContentType, + mimeType: String, isPastEditRevision: Bool, renderingFlag: RenderingFlag, ) { @@ -181,6 +190,7 @@ extension AttachmentReference { receivedAtTimestamp: receivedAtTimestamp, threadRowId: threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: isPastEditRevision, ) } @@ -196,6 +206,7 @@ extension AttachmentReference { receivedAtTimestamp: UInt64, threadRowId: Int64, contentType: Attachment.ContentType, + mimeType: String, isPastEditRevision: Bool, stickerPackId: Data, stickerId: UInt32, @@ -207,6 +218,7 @@ extension AttachmentReference { receivedAtTimestamp: receivedAtTimestamp, threadRowId: threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: isPastEditRevision, ) } @@ -356,6 +368,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: record.receivedAtTimestamp, threadRowId: record.threadRowId, contentType: record.contentType, + mimeType: record.mimeType, isPastEditRevision: record.ownerIsPastEditRevision, caption: record.caption, renderingFlag: try .init(rawValue: record.renderingFlag), @@ -369,6 +382,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: record.receivedAtTimestamp, threadRowId: record.threadRowId, contentType: record.contentType, + mimeType: record.mimeType, isPastEditRevision: record.ownerIsPastEditRevision, ))) case .linkPreview: @@ -377,6 +391,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: record.receivedAtTimestamp, threadRowId: record.threadRowId, contentType: record.contentType, + mimeType: record.mimeType, isPastEditRevision: record.ownerIsPastEditRevision, ))) case .quotedReplyAttachment: @@ -385,6 +400,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: record.receivedAtTimestamp, threadRowId: record.threadRowId, contentType: record.contentType, + mimeType: record.mimeType, isPastEditRevision: record.ownerIsPastEditRevision, renderingFlag: try .init(rawValue: record.renderingFlag), ))) @@ -400,6 +416,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: record.receivedAtTimestamp, threadRowId: record.threadRowId, contentType: record.contentType, + mimeType: record.mimeType, isPastEditRevision: record.ownerIsPastEditRevision, stickerPackId: stickerPackId, stickerId: stickerId, @@ -410,6 +427,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: record.receivedAtTimestamp, threadRowId: record.threadRowId, contentType: record.contentType, + mimeType: record.mimeType, isPastEditRevision: record.ownerIsPastEditRevision, ))) } @@ -449,7 +467,10 @@ extension AttachmentReference.Owner { /// When we go from a pointer to a stream (e.g. by downloading) and find another attachment with the same plaintext hash, /// we instead reassign the pointer's references to that existing attachment. When we do so, we need to update their contentType /// to match the new/old attachment (theyre the same plaintext hash so same content type). - public func forReassignmentWithContentType(_ contentType: Attachment.ContentType) -> Self { + public func forReassignmentWithContentType( + _ contentType: Attachment.ContentType, + mimeType: String, + ) -> Self { switch self { case .message(let messageSource): return .message({ @@ -460,6 +481,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, caption: metadata.caption, renderingFlag: metadata.renderingFlag, @@ -473,6 +495,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, )) case .linkPreview(let metadata): @@ -481,6 +504,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, )) case .quotedReply(let metadata): @@ -489,6 +513,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, renderingFlag: metadata.renderingFlag, )) @@ -498,6 +523,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, stickerPackId: metadata.stickerPackId, stickerId: metadata.stickerId, @@ -508,6 +534,7 @@ extension AttachmentReference.Owner { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, )) } diff --git a/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreTests.swift b/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreTests.swift index 59baec415f..2fcc7a65b5 100644 --- a/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreTests.swift +++ b/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreTests.swift @@ -439,6 +439,7 @@ class AttachmentStoreTests: XCTestCase { receivedAtTimestamp: .random(in: 0..<9999999), threadRowId: threadRowId, contentType: .image, + mimeType: "image/png", isPastEditRevision: false, caption: nil, renderingFlag: .default, @@ -471,6 +472,7 @@ class AttachmentStoreTests: XCTestCase { receivedAtTimestamp: .random(in: 0..<9999999), threadRowId: threadRowId, contentType: .image, + mimeType: "image/png", isPastEditRevision: false, stickerPackId: packId, stickerId: stickerId, @@ -515,6 +517,7 @@ class AttachmentStoreTests: XCTestCase { receivedAtTimestamp: .random(in: 0..<9999999), threadRowId: threadRowId, contentType: .image, + mimeType: "image/png", isPastEditRevision: false, caption: nil, renderingFlag: .default, @@ -555,6 +558,7 @@ class AttachmentStoreTests: XCTestCase { receivedAtTimestamp: .random(in: 0..<9999999), threadRowId: threadRowId, contentType: .image, + mimeType: "image/png", isPastEditRevision: false, stickerPackId: packId, stickerId: stickerId, diff --git a/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadManagerImpl.swift b/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadManagerImpl.swift index 00b56e21fc..cb1eddbd22 100644 --- a/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadManagerImpl.swift +++ b/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadManagerImpl.swift @@ -2269,7 +2269,10 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager { tx: tx, ) let newOwnerParams = AttachmentReference.ConstructionParams( - owner: reference.owner.forReassignmentWithContentType(pendingAttachment.contentType), + owner: reference.owner.forReassignmentWithContentType( + pendingAttachment.contentType, + mimeType: pendingAttachment.mimeType, + ), sourceFilename: reference.sourceFilename, sourceUnencryptedByteCount: reference.sourceUnencryptedByteCount, sourceMediaSizePixels: reference.sourceMediaSizePixels, @@ -2372,7 +2375,10 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager { } let referenceParams = AttachmentReference.ConstructionParams( - owner: firstReference.owner.forReassignmentWithContentType(pendingAttachment.contentType), + owner: firstReference.owner.forReassignmentWithContentType( + pendingAttachment.contentType, + mimeType: pendingAttachment.mimeType, + ), sourceFilename: firstReference.sourceFilename, sourceUnencryptedByteCount: pendingAttachment.unencryptedByteCount, sourceMediaSizePixels: pendingAttachment.mediaPixelSize, @@ -2467,6 +2473,7 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager { let newOwnerParams = AttachmentReference.ConstructionParams( owner: reference.owner.forReassignmentWithContentType( newAttachmentStream.contentType, + mimeType: newAttachmentStream.mimeType, ), sourceFilename: reference.sourceFilename, sourceUnencryptedByteCount: reference.sourceUnencryptedByteCount, @@ -2542,7 +2549,10 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager { } let referenceParams = AttachmentReference.ConstructionParams( - owner: firstReference.owner.forReassignmentWithContentType(pendingThumbnailAttachment.contentType), + owner: firstReference.owner.forReassignmentWithContentType( + pendingThumbnailAttachment.contentType, + mimeType: pendingThumbnailAttachment.mimeType, + ), sourceFilename: firstReference.sourceFilename, sourceUnencryptedByteCount: pendingThumbnailAttachment.unencryptedByteCount, sourceMediaSizePixels: pendingThumbnailAttachment.mediaPixelSize, @@ -2631,7 +2641,10 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager { tx: tx, ) let newOwnerParams = AttachmentReference.ConstructionParams( - owner: reference.owner.forReassignmentWithContentType(pendingThumbnailAttachment.contentType), + owner: reference.owner.forReassignmentWithContentType( + pendingThumbnailAttachment.contentType, + mimeType: pendingThumbnailAttachment.mimeType, + ), sourceFilename: reference.sourceFilename, sourceUnencryptedByteCount: reference.sourceUnencryptedByteCount, sourceMediaSizePixels: reference.sourceMediaSizePixels, diff --git a/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadStoreTests.swift b/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadStoreTests.swift index 21ff222ab0..4e51ffb6cd 100644 --- a/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadStoreTests.swift +++ b/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadStoreTests.swift @@ -281,6 +281,7 @@ class AttachmentDownloadStoreTests: XCTestCase { receivedAtTimestamp: interaction.receivedAtTimestamp, threadRowId: thread.sqliteRowId!, contentType: attachmentParams.contentType, + mimeType: attachmentParams.mimeType, isPastEditRevision: false, caption: nil, renderingFlag: .default, diff --git a/SignalServiceKit/Messages/Attachments/V2/Mocks/MockAttachmentReference.swift b/SignalServiceKit/Messages/Attachments/V2/Mocks/MockAttachmentReference.swift index 1d48aa5c8e..1648a02a2c 100644 --- a/SignalServiceKit/Messages/Attachments/V2/Mocks/MockAttachmentReference.swift +++ b/SignalServiceKit/Messages/Attachments/V2/Mocks/MockAttachmentReference.swift @@ -45,6 +45,7 @@ extension AttachmentReference.ConstructionParams { receivedAtTimestamp: receivedAtTimestamp, threadRowId: threadRowId, contentType: attachmentRecord.contentType, + mimeType: attachmentRecord.mimeType, isPastEditRevision: isPastEditRevision, caption: caption, renderingFlag: renderingFlag, @@ -76,6 +77,7 @@ extension AttachmentReference.ConstructionParams { receivedAtTimestamp: receivedAtTimestamp, threadRowId: threadRowId, contentType: contentType, + mimeType: MimeType.applicationOctetStream.rawValue, isPastEditRevision: isPastEditRevision, stickerPackId: stickerPackId, stickerId: stickerId, diff --git a/SignalServiceKit/Messages/Attachments/V2/Records/AttachmentReference+ConstructionParams.swift b/SignalServiceKit/Messages/Attachments/V2/Records/AttachmentReference+ConstructionParams.swift index a66ad2afc7..8e23e1b478 100644 --- a/SignalServiceKit/Messages/Attachments/V2/Records/AttachmentReference+ConstructionParams.swift +++ b/SignalServiceKit/Messages/Attachments/V2/Records/AttachmentReference+ConstructionParams.swift @@ -61,6 +61,7 @@ extension AttachmentReference { knownIdInOwner: KnownIdInOwner, renderingFlag: AttachmentReference.RenderingFlag, contentType: Attachment.ContentType, + mimeType: String, caption: String? = nil, ) -> AttachmentReference.Owner { switch self { @@ -70,6 +71,7 @@ extension AttachmentReference { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, // We ignore captions in modern instances. caption: caption, @@ -90,6 +92,7 @@ extension AttachmentReference { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, ))) case .messageLinkPreview(let metadata): @@ -98,6 +101,7 @@ extension AttachmentReference { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, ))) case .quotedReplyAttachment(let metadata): @@ -106,6 +110,7 @@ extension AttachmentReference { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, renderingFlag: renderingFlag, ))) @@ -115,6 +120,7 @@ extension AttachmentReference { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, stickerPackId: metadata.stickerPackId, stickerId: metadata.stickerId, @@ -125,6 +131,7 @@ extension AttachmentReference { receivedAtTimestamp: metadata.receivedAtTimestamp, threadRowId: metadata.threadRowId, contentType: contentType, + mimeType: mimeType, isPastEditRevision: metadata.isPastEditRevision, ))) case .storyMessageMedia(let metadata): diff --git a/SignalServiceKit/Messages/Attachments/V2/Records/AttachmentReference+Records.swift b/SignalServiceKit/Messages/Attachments/V2/Records/AttachmentReference+Records.swift index 1a6c80923f..be329a3a5b 100644 --- a/SignalServiceKit/Messages/Attachments/V2/Records/AttachmentReference+Records.swift +++ b/SignalServiceKit/Messages/Attachments/V2/Records/AttachmentReference+Records.swift @@ -26,6 +26,10 @@ extension AttachmentReference { /// The underlying database column for this is nullable, but in practice /// must always contain a valid non-NULL value. let contentType: Attachment.ContentType + /// - Important + /// The underlying database column for this is nullable, but in practice + /// must always contain a non-NULL value. + let mimeType: String let renderingFlag: UInt32 let idInMessage: String? let orderInMessage: UInt32? @@ -48,6 +52,7 @@ extension AttachmentReference { case attachmentRowId case receivedAtTimestamp case contentType + case mimeType case renderingFlag case idInMessage case orderInMessage @@ -116,6 +121,7 @@ extension AttachmentReference { self.ownerRowId = metadata.messageRowId self._receivedAtTimestamp = DBUInt64(wrappedValue: metadata.receivedAtTimestamp) self.contentType = metadata.contentType + self.mimeType = metadata.mimeType self.renderingFlag = UInt32(metadata.renderingFlag.rawValue) self.idInMessage = metadata.idInOwner?.uuidString self.orderInMessage = metadata.orderInMessage @@ -129,6 +135,7 @@ extension AttachmentReference { self.ownerRowId = metadata.messageRowId self._receivedAtTimestamp = DBUInt64(wrappedValue: metadata.receivedAtTimestamp) self.contentType = metadata.contentType + self.mimeType = metadata.mimeType self.renderingFlag = UInt32(AttachmentReference.RenderingFlag.default.rawValue) self.idInMessage = nil self.orderInMessage = nil @@ -143,6 +150,7 @@ extension AttachmentReference { self.ownerRowId = metadata.messageRowId self._receivedAtTimestamp = DBUInt64(wrappedValue: metadata.receivedAtTimestamp) self.contentType = metadata.contentType + self.mimeType = metadata.mimeType self.renderingFlag = UInt32(AttachmentReference.RenderingFlag.default.rawValue) self.idInMessage = nil self.orderInMessage = nil @@ -157,6 +165,7 @@ extension AttachmentReference { self.ownerRowId = metadata.messageRowId self._receivedAtTimestamp = DBUInt64(wrappedValue: metadata.receivedAtTimestamp) self.contentType = metadata.contentType + self.mimeType = metadata.mimeType self.renderingFlag = UInt32(metadata.renderingFlag.rawValue) self.idInMessage = nil self.orderInMessage = nil @@ -171,6 +180,7 @@ extension AttachmentReference { self.ownerRowId = metadata.messageRowId self._receivedAtTimestamp = DBUInt64(wrappedValue: metadata.receivedAtTimestamp) self.contentType = metadata.contentType + self.mimeType = metadata.mimeType self.renderingFlag = UInt32(AttachmentReference.RenderingFlag.default.rawValue) self.idInMessage = nil self.orderInMessage = nil @@ -185,6 +195,7 @@ extension AttachmentReference { self.ownerRowId = metadata.messageRowId self._receivedAtTimestamp = DBUInt64(wrappedValue: metadata.receivedAtTimestamp) self.contentType = metadata.contentType + self.mimeType = metadata.mimeType self.renderingFlag = UInt32(AttachmentReference.RenderingFlag.default.rawValue) self.idInMessage = nil self.orderInMessage = nil diff --git a/SignalServiceKit/Storage/Database/GRDBSchemaMigrator.swift b/SignalServiceKit/Storage/Database/GRDBSchemaMigrator.swift index 99cbaa1845..d9a039785f 100644 --- a/SignalServiceKit/Storage/Database/GRDBSchemaMigrator.swift +++ b/SignalServiceKit/Storage/Database/GRDBSchemaMigrator.swift @@ -330,6 +330,7 @@ public class GRDBSchemaMigrator { case addOrphanedAttachmentTimestamp case migrateSecureValueRecovery case wipeCachedSVRBAuthCredentials + case addMimeTypeToMessageAttachmentReference // NOTE: Every time we add a migration id, consider // incrementing grdbSchemaVersionLatest. @@ -5178,6 +5179,11 @@ public class GRDBSchemaMigrator { return .success(()) } + migrator.registerMigration(.addMimeTypeToMessageAttachmentReference) { tx in + try Self.addMimeTypeToMessageAttachmentReference(tx: tx) + return .success(()) + } + // MARK: - Schema Migration Insertion Point } @@ -5537,6 +5543,66 @@ public class GRDBSchemaMigrator { // MARK: - Migrations + static func addMimeTypeToMessageAttachmentReference(tx: DBWriteTransaction) throws { + try tx.database.alter(table: "MessageAttachmentReference") { table in + // In practice, this column will be NOT NULL. However, we can't + // declare it as such without providing a default, and we can't add + // a CHECK constraint or similar until we've backfilled it below. + table.add(column: "mimeType", .text) + + // New virtual columns and corresponding indices to efficiently + // support the compound queries we need for media gallery Photos and + // GIFs filters. + // + // isGifsCategory: "is a looping video, or is image/gif" + // isPhotosCategory: "is a photo that's not covered by isGifsCategory" + table.addColumn(literal: """ + isGifsCategory AS ( + (contentType = 3 AND renderingFlag = 3) + OR (contentType = 2 AND mimeType = 'image/gif') + ) VIRTUAL + """) + table.addColumn(literal: """ + isPhotosCategory AS ( + contentType = 2 AND mimeType != 'image/gif' + ) VIRTUAL + """) + } + + try tx.database.execute(sql: """ + UPDATE MessageAttachmentReference + SET mimeType = ( + SELECT mimeType FROM Attachment + WHERE Attachment.id = MessageAttachmentReference.attachmentRowId + ) + """) + + try tx.database.create( + index: "message_attachment_reference_media_gallery_gifs_index", + on: "MessageAttachmentReference", + columns: [ + "threadRowId", + "ownerType", + "receivedAtTimestamp", + "ownerRowId", + "orderInMessage", + ], + condition: Column("isGifsCategory") == true, + ) + try tx.database.create( + index: "message_attachment_reference_media_gallery_photos_index", + on: "MessageAttachmentReference", + columns: [ + "threadRowId", + "ownerType", + "receivedAtTimestamp", + "ownerRowId", + "orderInMessage", + ], + condition: Column("isPhotosCategory") == true, + ) + } + static func createStoryContextAssociatedData(tx transaction: DBWriteTransaction) throws { try transaction.database.create(table: StoryContextAssociatedData.databaseTableName) { table in table.autoIncrementedPrimaryKey(StoryContextAssociatedData.columnName(.id)) diff --git a/SignalServiceKit/Storage/MediaGallery/MediaGalleryAttachmentFinder.swift b/SignalServiceKit/Storage/MediaGallery/MediaGalleryAttachmentFinder.swift index 682e071afc..3ae92679a9 100644 --- a/SignalServiceKit/Storage/MediaGallery/MediaGalleryAttachmentFinder.swift +++ b/SignalServiceKit/Storage/MediaGallery/MediaGalleryAttachmentFinder.swift @@ -305,16 +305,13 @@ public struct MediaGalleryAttachmentFinder { case .otherFiles: query = query.filter(literal: "isInvalidOrFileContentType = \(true)") case .gifs: - query = query - .filter(contentTypeColumn == Attachment.ContentType.video.rawValue) - .filter(renderingFlagColumn == AttachmentReference.RenderingFlag.shouldLoop.rawValue) + query = query.filter(literal: "isGifsCategory = \(true)") case .videos: query = query .filter(contentTypeColumn == Attachment.ContentType.video.rawValue) .filter(renderingFlagColumn != AttachmentReference.RenderingFlag.shouldLoop.rawValue) case .photos: - query = query - .filter(contentTypeColumn == Attachment.ContentType.image.rawValue) + query = query.filter(literal: "isPhotosCategory = \(true)") case .voiceMessages: query = query .filter(contentTypeColumn == Attachment.ContentType.audio.rawValue) diff --git a/SignalServiceKit/Storage/MediaGallery/MediaGalleryAttachmentFinderTest.swift b/SignalServiceKit/Storage/MediaGallery/MediaGalleryAttachmentFinderTest.swift index 668cd1c0f6..9c5381cd11 100644 --- a/SignalServiceKit/Storage/MediaGallery/MediaGalleryAttachmentFinderTest.swift +++ b/SignalServiceKit/Storage/MediaGallery/MediaGalleryAttachmentFinderTest.swift @@ -128,6 +128,67 @@ class MediaGalleryAttachmentFinderTest: SSKBaseTest { XCTAssertEqual(results[0].orderInMessage, 4) } + func testFiltersDistinguishGifsAndPhotos() throws { + let (thread, messageRowId) = insertThreadAndInteraction() + let threadRowId = thread.sqliteRowId! + + // Four representative cases the gifs/photos virtual columns must distinguish: + // - image/jpeg → photo + // - image/gif → gif (regardless of renderingFlag) + // - looping mp4 (renderingFlag=shouldLoop) → gif + // - non-looping mp4 → video + let jpegId = insertAttachment( + messageRowId: messageRowId, + threadRowId: threadRowId, + receivedAtTimestamp: 100, + mimeType: "image/jpeg", + orderInMessage: 0, + ) + let gifFileId = insertAttachment( + messageRowId: messageRowId, + threadRowId: threadRowId, + receivedAtTimestamp: 200, + mimeType: "image/gif", + orderInMessage: 1, + ) + let loopingMp4Id = insertAttachment( + messageRowId: messageRowId, + threadRowId: threadRowId, + receivedAtTimestamp: 300, + mimeType: "video/mp4", + orderInMessage: 2, + renderingFlag: .shouldLoop, + ) + let nonLoopingMp4Id = insertAttachment( + messageRowId: messageRowId, + threadRowId: threadRowId, + receivedAtTimestamp: 400, + mimeType: "video/mp4", + orderInMessage: 3, + ) + + func attachmentRowIds(filter: AllMediaFilter) throws -> Set { + let finder = MediaGalleryAttachmentFinder(threadId: threadRowId, filter: filter) + let query = finder.galleryItemQuery( + in: nil, + excluding: [], + offset: 0, + ascending: true, + ) + return try db.read { tx in + Set(try query.fetchAll(tx.database).map(\.attachmentRowId)) + } + } + + XCTAssertEqual(try attachmentRowIds(filter: .gifs), [gifFileId, loopingMp4Id]) + XCTAssertEqual(try attachmentRowIds(filter: .photos), [jpegId]) + XCTAssertEqual(try attachmentRowIds(filter: .videos), [nonLoopingMp4Id]) + XCTAssertEqual( + try attachmentRowIds(filter: .allPhotoVideoCategory), + [jpegId, gifFileId, loopingMp4Id, nonLoopingMp4Id], + ) + } + // MARK: - Index Usage func testAllQueriesUseIndex() throws { @@ -227,9 +288,11 @@ class MediaGalleryAttachmentFinderTest: SSKBaseTest { // * we use all the columns up to the ordering columns // * we DONT use expensive B trees for ordering let allowedQueryPlans: [String] = [ - "SEARCH MessageAttachmentReference USING INDEX message_attachment_reference_media_gallery_single_content_type_index (threadRowId=? AND ownerType=? AND contentType=?", - "SEARCH MessageAttachmentReference USING INDEX message_attachment_reference_media_gallery_visualMedia_content_type_index (threadRowId=? AND ownerType=? AND isVisualMediaContentType=?", - "SEARCH MessageAttachmentReference USING INDEX message_attachment_reference_media_gallery_fileOrInvalid_content_type_index (threadRowId=? AND ownerType=? AND isInvalidOrFileContentType=?", + "SEARCH MessageAttachmentReference USING INDEX message_attachment_reference_media_gallery_single_content_type_index", + "SEARCH MessageAttachmentReference USING INDEX message_attachment_reference_media_gallery_visualMedia_content_type_index", + "SEARCH MessageAttachmentReference USING INDEX message_attachment_reference_media_gallery_fileOrInvalid_content_type_index", + "SEARCH MessageAttachmentReference USING INDEX message_attachment_reference_media_gallery_gifs_index", + "SEARCH MessageAttachmentReference USING INDEX message_attachment_reference_media_gallery_photos_index", ] XCTAssert(queryPlan.allSatisfy { queryPlan in for allowedQueryPlan in allowedQueryPlans { @@ -270,6 +333,7 @@ class MediaGalleryAttachmentFinderTest: SSKBaseTest { mimeType: String, orderInMessage: UInt32, isViewOnce: Bool = false, + renderingFlag: AttachmentReference.RenderingFlag = .default, ) -> Attachment.IDType { db.write { tx in var attachmentRecord = Attachment.Record.mockStream( @@ -285,9 +349,10 @@ class MediaGalleryAttachmentFinderTest: SSKBaseTest { receivedAtTimestamp: receivedAtTimestamp, threadRowId: threadRowId, contentType: attachment.contentType, + mimeType: attachment.mimeType, isPastEditRevision: false, caption: nil, - renderingFlag: .default, + renderingFlag: renderingFlag, orderInMessage: orderInMessage, idInOwner: nil, isViewOnce: isViewOnce, diff --git a/SignalServiceKit/tests/Storage/Database/GRDBSchemaMigratorTest.swift b/SignalServiceKit/tests/Storage/Database/GRDBSchemaMigratorTest.swift index a2f2ac3b5e..88a83f5f93 100644 --- a/SignalServiceKit/tests/Storage/Database/GRDBSchemaMigratorTest.swift +++ b/SignalServiceKit/tests/Storage/Database/GRDBSchemaMigratorTest.swift @@ -1570,4 +1570,76 @@ struct GRDBSchemaMigratorTest { } #expect(Set(nonEmptyCollections) == ["SVR.Potential"]) } + + @Test + func testAddMimeTypeToMessageAttachmentReference() throws { + let databaseQueue = DatabaseQueue() + try databaseQueue.write { db in + try db.execute(sql: """ + CREATE TABLE Attachment ( + id INTEGER PRIMARY KEY, + mimeType TEXT NOT NULL + ); + CREATE TABLE MessageAttachmentReference ( + ownerType INTEGER NOT NULL, + ownerRowId INTEGER NOT NULL, + attachmentRowId INTEGER NOT NULL REFERENCES Attachment(id) ON DELETE CASCADE, + receivedAtTimestamp INTEGER NOT NULL, + contentType INTEGER, + renderingFlag INTEGER NOT NULL, + threadRowId INTEGER NOT NULL, + orderInMessage INTEGER + ); + """) + // Cases the new virtual columns must distinguish: + // 1 = still photo (image/jpeg) + // 2 = image/gif file + // 3 = looping mp4 (renderingFlag=shouldLoop) + // 4 = regular mp4 video + try db.execute(sql: """ + INSERT INTO Attachment (id, mimeType) VALUES + (1, 'image/jpeg'), + (2, 'image/gif'), + (3, 'video/mp4'), + (4, 'video/mp4'); + INSERT INTO MessageAttachmentReference + (ownerType, ownerRowId, attachmentRowId, receivedAtTimestamp, contentType, renderingFlag, threadRowId, orderInMessage) VALUES + (0, 100, 1, 1000, 2, 0, 999, 0), + (0, 101, 2, 1001, 2, 0, 999, 0), + (0, 102, 3, 1002, 3, 3, 999, 0), + (0, 103, 4, 1003, 3, 0, 999, 0); + """) + + let tx = DBWriteTransaction(database: db) + defer { tx.finalizeTransaction() } + try GRDBSchemaMigrator.addMimeTypeToMessageAttachmentReference(tx: tx) + } + + // Backfill copied mimeType from each row's Attachment. + let backfilled = try databaseQueue.read { db in + try Row.fetchAll(db, sql: """ + SELECT attachmentRowId, mimeType, isGifsCategory, isPhotosCategory + FROM MessageAttachmentReference + ORDER BY attachmentRowId + """) + } + #expect(backfilled.map { $0["mimeType"] } == [ + "image/jpeg", + "image/gif", + "video/mp4", + "video/mp4", + ]) + #expect(backfilled.map { $0["isGifsCategory"] } == [ + false, // jpeg + true, // image/gif + true, // looping mp4 + false, // regular mp4 + ]) + #expect(backfilled.map { $0["isPhotosCategory"] } == [ + true, // jpeg + false, // image/gif + false, // looping mp4 + false, // regular mp4 + ]) + } }