From d5dca601a60dfb2f54068bd6f8ede08fc66931dc Mon Sep 17 00:00:00 2001 From: Sasha Weiss Date: Tue, 30 Dec 2025 11:21:26 -0800 Subject: [PATCH] Use failIfThrows in some Attachments code, remove unused code --- .../AttachmentOffloadingManager.swift | 4 ++- .../BackupAttachmentUploadScheduler.swift | 28 +++++++-------- .../BackupAttachmentUploadStore.swift | 16 ++++++--- .../BackupAttachmentUploadStoreTests.swift | 20 +++++------ .../BackupListMediaManagerTests.swift | 4 +-- .../AttachmentManagerImpl.swift | 6 ++-- .../AttachmentReference.swift | 18 +++++----- .../V2/AttachmentStore/AttachmentStore.swift | 2 +- .../AttachmentStore/AttachmentStoreImpl.swift | 35 ++++++++++++------- .../AttachmentStoreTests.swift | 4 +-- .../AttachmentDownloadManagerImpl.swift | 14 ++++---- .../Upload/AttachmentUploadManager.swift | 4 +-- 12 files changed, 84 insertions(+), 71 deletions(-) diff --git a/SignalServiceKit/Backups/Attachments/AttachmentOffloadingManager.swift b/SignalServiceKit/Backups/Attachments/AttachmentOffloadingManager.swift index 68fd62b888..52174676f2 100644 --- a/SignalServiceKit/Backups/Attachments/AttachmentOffloadingManager.swift +++ b/SignalServiceKit/Backups/Attachments/AttachmentOffloadingManager.swift @@ -488,6 +488,8 @@ public class AttachmentOffloadingManagerImpl: AttachmentOffloadingManager { } } +// MARK: - + extension AttachmentStore { func fetchMostRecentReference( @@ -496,7 +498,7 @@ extension AttachmentStore { ) throws -> AttachmentReference { var mostRecentReference: AttachmentReference? var maxMessageTimestamp: UInt64 = 0 - try self.enumerateAllReferences( + self.enumerateAllReferences( toAttachmentId: attachmentId, tx: tx ) { reference, stop in diff --git a/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadScheduler.swift b/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadScheduler.swift index c920e46243..1cc54ae8b5 100644 --- a/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadScheduler.swift +++ b/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadScheduler.swift @@ -55,7 +55,7 @@ public protocol BackupAttachmentUploadScheduler { file: StaticString?, function: StaticString?, line: UInt? - ) throws + ) } extension BackupAttachmentUploadScheduler { @@ -85,8 +85,8 @@ extension BackupAttachmentUploadScheduler { file: StaticString? = #file, function: StaticString? = #function, line: UInt? = #line - ) throws { - try enqueueIfNeededWithOwner( + ) { + enqueueIfNeededWithOwner( attachment, owner: owner, tx: tx, @@ -139,7 +139,7 @@ public class BackupAttachmentUploadSchedulerImpl: BackupAttachmentUploadSchedule return false } - let highestPriorityEligibleOwner = try? self.highestPriorityEligibleOwner( + let highestPriorityEligibleOwner = self.highestPriorityEligibleOwner( attachment, tx: tx ) @@ -153,7 +153,7 @@ public class BackupAttachmentUploadSchedulerImpl: BackupAttachmentUploadSchedule file: StaticString? = #file, function: StaticString? = #function, line: UInt? = #line - ) throws { + ) { // Before we fetch references, check if the attachment is // eligible to begin with. guard let stream = attachment.asStream() else { @@ -177,7 +177,7 @@ public class BackupAttachmentUploadSchedulerImpl: BackupAttachmentUploadSchedule } guard - let uploadOwnerType = try highestPriorityEligibleOwner(attachment, tx: tx) + let uploadOwnerType = highestPriorityEligibleOwner(attachment, tx: tx) else { if let file, let function, let line { Logger.info("No eligible owners; skipping enqueue of \(attachment.id) from \(file) \(line): \(function)") @@ -187,7 +187,7 @@ public class BackupAttachmentUploadSchedulerImpl: BackupAttachmentUploadSchedule if mode != .thumbnailOnly { if eligibility.needsUploadFullsize { - try backupAttachmentUploadStore.enqueue( + backupAttachmentUploadStore.enqueue( stream, owner: uploadOwnerType, fullsize: true, @@ -202,7 +202,7 @@ public class BackupAttachmentUploadSchedulerImpl: BackupAttachmentUploadSchedule } if mode != .fullsizeOnly { if eligibility.needsUploadThumbnail { - try backupAttachmentUploadStore.enqueue( + backupAttachmentUploadStore.enqueue( stream, owner: uploadOwnerType, fullsize: false, @@ -224,7 +224,7 @@ public class BackupAttachmentUploadSchedulerImpl: BackupAttachmentUploadSchedule file: StaticString? = #file, function: StaticString? = #function, line: UInt? = #line - ) throws { + ) { guard let stream = attachment.asStream() else { if let file, let function, let line { Logger.info("Skipping enqueue of non-stream \(attachment.id) from \(file) \(line): \(function)") @@ -260,7 +260,7 @@ public class BackupAttachmentUploadSchedulerImpl: BackupAttachmentUploadSchedule } if eligibility.needsUploadFullsize { - try backupAttachmentUploadStore.enqueue( + backupAttachmentUploadStore.enqueue( stream, owner: uploadOwnerType, fullsize: true, @@ -270,7 +270,7 @@ public class BackupAttachmentUploadSchedulerImpl: BackupAttachmentUploadSchedule Logger.info("Skipping enqueue of fullsize \(attachment.id) from \(file) \(line): \(function)") } if eligibility.needsUploadThumbnail { - try backupAttachmentUploadStore.enqueue( + backupAttachmentUploadStore.enqueue( stream, owner: uploadOwnerType, fullsize: false, @@ -368,11 +368,11 @@ public class BackupAttachmentUploadSchedulerImpl: BackupAttachmentUploadSchedule private func highestPriorityEligibleOwner( _ attachment: Attachment, tx: DBReadTransaction - ) throws -> QueuedBackupAttachmentUpload.OwnerType? { + ) -> QueuedBackupAttachmentUpload.OwnerType? { // Backup uploads are prioritized by attachment owner. Find the highest // priority owner to use. var uploadOwnerType: QueuedBackupAttachmentUpload.OwnerType? - try attachmentStore.enumerateAllReferences( + attachmentStore.enumerateAllReferences( toAttachmentId: attachment.id, tx: tx ) { reference, _ in @@ -443,7 +443,7 @@ open class BackupAttachmentUploadSchedulerMock: BackupAttachmentUploadScheduler file: StaticString?, function: StaticString?, line: UInt? - ) throws { + ) { // Do nothing } } diff --git a/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStore.swift b/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStore.swift index cb9dd3bd8e..9ed4dbf417 100644 --- a/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStore.swift +++ b/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStore.swift @@ -26,7 +26,7 @@ public class BackupAttachmentUploadStore { file: StaticString? = #file, function: StaticString? = #function, line: UInt? = #line - ) throws { + ) { if let file, let function, let line { Logger.info("Enqueuing \(attachment.id) fullsize? \(fullsize) from \(file) \(line): \(function)") } @@ -51,10 +51,12 @@ public class BackupAttachmentUploadStore { ) ?? .max), ) - let existingRecord = try QueuedBackupAttachmentUpload + let existingRecordQuery = QueuedBackupAttachmentUpload .filter(Column(QueuedBackupAttachmentUpload.CodingKeys.attachmentRowId) == attachment.id) .filter(Column(QueuedBackupAttachmentUpload.CodingKeys.isFullsize) == fullsize) - .fetchOne(db) + let existingRecord = failIfThrows { + try existingRecordQuery.fetchOne(db) + } if var existingRecord { // Only update if done or the new one has higher priority; otherwise leave untouched. @@ -65,11 +67,15 @@ public class BackupAttachmentUploadStore { if shouldUpdate { existingRecord.highestPriorityOwnerType = newRecord.highestPriorityOwnerType existingRecord.state = newRecord.state - try existingRecord.update(db) + failIfThrows { + try existingRecord.update(db) + } } } else { // If there's no existing record, insert and we're done. - try newRecord.insert(db) + failIfThrows { + try newRecord.insert(db) + } } } diff --git a/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStoreTests.swift b/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStoreTests.swift index 77c6399a68..f482a0ee96 100644 --- a/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStoreTests.swift +++ b/SignalServiceKit/Backups/Attachments/BackupAttachmentUploadStoreTests.swift @@ -37,8 +37,8 @@ class BackupAttachmentUploadStoreTests: XCTestCase { timestamp: 1234, tx: tx ) - try store.enqueue( - Attachment(record: attachmentRecord).asStream()!, + store.enqueue( + try Attachment(record: attachmentRecord).asStream()!, owner: reference.owner.asEligibleUploadOwnerType, fullsize: true, tx: tx @@ -65,8 +65,8 @@ class BackupAttachmentUploadStoreTests: XCTestCase { timestamp: 5678, tx: tx ) - try store.enqueue( - Attachment(record: attachmentRecord).asStream()!, + store.enqueue( + try Attachment(record: attachmentRecord).asStream()!, owner: reference.owner.asEligibleUploadOwnerType, fullsize: true, tx: tx @@ -169,8 +169,8 @@ class BackupAttachmentUploadStoreTests: XCTestCase { return try AttachmentReference(record: referenceRecord) } }() - try store.enqueue( - Attachment(record: attachmentRecord).asStream()!, + store.enqueue( + try Attachment(record: attachmentRecord).asStream()!, owner: reference.owner.asEligibleUploadOwnerType, fullsize: true, tx: tx @@ -260,14 +260,14 @@ class BackupAttachmentUploadStoreTests: XCTestCase { tx: tx ) // Enqueue both fullsize and thumbnail - try store.enqueue( - Attachment(record: attachmentRecord).asStream()!, + store.enqueue( + try Attachment(record: attachmentRecord).asStream()!, owner: reference.owner.asEligibleUploadOwnerType, fullsize: true, tx: tx ) - try store.enqueue( - Attachment(record: attachmentRecord).asStream()!, + store.enqueue( + try Attachment(record: attachmentRecord).asStream()!, owner: reference.owner.asEligibleUploadOwnerType, fullsize: false, tx: tx diff --git a/SignalServiceKit/Backups/Attachments/BackupListMediaManagerTests.swift b/SignalServiceKit/Backups/Attachments/BackupListMediaManagerTests.swift index 7a35f1fcf4..2f160f9ae9 100644 --- a/SignalServiceKit/Backups/Attachments/BackupListMediaManagerTests.swift +++ b/SignalServiceKit/Backups/Attachments/BackupListMediaManagerTests.swift @@ -364,8 +364,8 @@ public class BackupListMediaManagerTests { } if scheduleUpload { - try! backupAttachmentUploadStore.enqueue( - Attachment(record: attachmentRecord).asStream()!, + backupAttachmentUploadStore.enqueue( + try! Attachment(record: attachmentRecord).asStream()!, owner: .threadWallpaper, fullsize: true, tx: tx diff --git a/SignalServiceKit/Messages/Attachments/V2/AttachmentManager/AttachmentManagerImpl.swift b/SignalServiceKit/Messages/Attachments/V2/AttachmentManager/AttachmentManagerImpl.swift index 39601eeff6..1ea39bb6c4 100644 --- a/SignalServiceKit/Messages/Attachments/V2/AttachmentManager/AttachmentManagerImpl.swift +++ b/SignalServiceKit/Messages/Attachments/V2/AttachmentManager/AttachmentManagerImpl.swift @@ -718,7 +718,7 @@ public class AttachmentManagerImpl: AttachmentManager { tx: tx ) { - try backupAttachmentUploadScheduler.enqueueIfNeededWithOwner( + backupAttachmentUploadScheduler.enqueueIfNeededWithOwner( attachment, owner: owner, tx: tx @@ -825,7 +825,7 @@ public class AttachmentManagerImpl: AttachmentManager { // Just hold all refs in memory; there shouldn't in practice be // so many pointers to the same attachment. var references = [AttachmentReference]() - try self.attachmentStore.enumerateAllReferences( + self.attachmentStore.enumerateAllReferences( toAttachmentId: attachment.id, tx: tx ) { reference, _ in @@ -1015,7 +1015,7 @@ public class AttachmentManagerImpl: AttachmentManager { // and whether to actually upload, but let it know about every new // stream created. if let newAttachmentOwner { - try backupAttachmentUploadScheduler.enqueueIfNeededWithOwner( + backupAttachmentUploadScheduler.enqueueIfNeededWithOwner( existingAttachment, owner: newAttachmentOwner, tx: tx diff --git a/SignalServiceKit/Messages/Attachments/V2/AttachmentReference/AttachmentReference.swift b/SignalServiceKit/Messages/Attachments/V2/AttachmentReference/AttachmentReference.swift index bf3fd95d8b..844883bf45 100644 --- a/SignalServiceKit/Messages/Attachments/V2/AttachmentReference/AttachmentReference.swift +++ b/SignalServiceKit/Messages/Attachments/V2/AttachmentReference/AttachmentReference.swift @@ -40,7 +40,7 @@ public class AttachmentReference { self.attachmentRowId = record.attachmentRowId self.sourceFilename = record.sourceFilename self.sourceUnencryptedByteCount = record.sourceUnencryptedByteCount - self.sourceMediaSizePixels = try Self.buildSourceMediaSizePixels( + self.sourceMediaSizePixels = Self.buildSourceMediaSizePixels( sourceMediaWidthPixels: record.sourceMediaWidthPixels, sourceMediaHeightPixels: record.sourceMediaHeightPixels ) @@ -51,7 +51,7 @@ public class AttachmentReference { self.attachmentRowId = record.attachmentRowId self.sourceFilename = record.sourceFilename self.sourceUnencryptedByteCount = record.sourceUnencryptedByteCount - self.sourceMediaSizePixels = try Self.buildSourceMediaSizePixels( + self.sourceMediaSizePixels = Self.buildSourceMediaSizePixels( sourceMediaWidthPixels: record.sourceMediaWidthPixels, sourceMediaHeightPixels: record.sourceMediaHeightPixels ) @@ -68,7 +68,7 @@ public class AttachmentReference { private static func buildSourceMediaSizePixels( sourceMediaWidthPixels: UInt32?, sourceMediaHeightPixels: UInt32? - ) throws -> CGSize? { + ) -> CGSize? { guard let sourceMediaWidthPixels, let sourceMediaHeightPixels @@ -80,13 +80,11 @@ public class AttachmentReference { ) return nil } - guard - let sourceMediaWidthPixels = Int(exactly: sourceMediaWidthPixels), - let sourceMediaHeightPixels = Int(exactly: sourceMediaHeightPixels) - else { - throw OWSAssertionError("Invalid pixel size") - } - return CGSize(width: sourceMediaWidthPixels, height: sourceMediaHeightPixels) + // Safe Int conversion: all UInt32 can be an Int on 64b systems + return CGSize( + width: Int(sourceMediaWidthPixels), + height: Int(sourceMediaHeightPixels), + ) } } diff --git a/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStore.swift b/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStore.swift index 88fa47f772..d7b4489b40 100644 --- a/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStore.swift +++ b/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStore.swift @@ -60,7 +60,7 @@ public protocol AttachmentStore { toAttachmentId: Attachment.IDType, tx: DBReadTransaction, block: (AttachmentReference, _ stop: inout Bool) -> Void - ) throws + ) /// Return all attachments that are themselves quoted replies /// of another attachment; provide the original attachment they point to. diff --git a/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreImpl.swift b/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreImpl.swift index 6416ad78fb..c34162f12a 100644 --- a/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreImpl.swift +++ b/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreImpl.swift @@ -182,9 +182,9 @@ public class AttachmentStoreImpl: AttachmentStore { toAttachmentId attachmentId: Attachment.IDType, tx: DBReadTransaction, block: (AttachmentReference, _ stop: inout Bool) -> Void - ) throws { - try AttachmentReference.recordTypes.forEach { recordType in - try enumerateReferences( + ) { + AttachmentReference.recordTypes.forEach { recordType in + enumerateReferences( attachmentId: attachmentId, recordType: recordType, tx: tx, @@ -198,17 +198,26 @@ public class AttachmentStoreImpl: AttachmentStore { recordType: RecordType.Type, tx: DBReadTransaction, block: (AttachmentReference, _ stop: inout Bool) -> Void - ) throws { - let cursor = try recordType - .filter(recordType.attachmentRowIdColumn == attachmentId) - .fetchCursor(tx.database) + ) { + failIfThrows { + let cursor = try recordType + .filter(recordType.attachmentRowIdColumn == attachmentId) + .fetchCursor(tx.database) - var stop = false - while let record = try cursor.next() { - let reference = try record.asReference() - block(reference, &stop) - if stop { - break + var stop = false + while let record = try cursor.next() { + let reference: AttachmentReference + do { + reference = try record.asReference() + } catch { + owsFailDebug("Failed to create AttachmentReference! \(error)") + continue + } + + block(reference, &stop) + if stop { + break + } } } } diff --git a/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreTests.swift b/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreTests.swift index c4fd8c2f3c..37614a7514 100644 --- a/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreTests.swift +++ b/SignalServiceKit/Messages/Attachments/V2/AttachmentStore/AttachmentStoreTests.swift @@ -367,8 +367,8 @@ class AttachmentStoreTests: XCTestCase { // Check that we enumerate all the ids we created for the original attachment's id. var enumeratedCount = 0 - try db.read { tx in - try attachmentStore.enumerateAllReferences( + db.read { tx in + attachmentStore.enumerateAllReferences( toAttachmentId: attachmentId, tx: tx, block: { reference, _ in diff --git a/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadManagerImpl.swift b/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadManagerImpl.swift index 046b541852..88e8b3ea4d 100644 --- a/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadManagerImpl.swift +++ b/SignalServiceKit/Messages/Attachments/V2/Downloads/AttachmentDownloadManagerImpl.swift @@ -933,7 +933,7 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager { ) async -> Bool { let installedSticker: InstalledSticker? = db.read { tx in var stickerMetadata: AttachmentReference.Owner.MessageSource.StickerMetadata? - try? attachmentStore.enumerateAllReferences( + attachmentStore.enumerateAllReferences( toAttachmentId: record.attachmentId, tx: tx, block: { reference, stop in @@ -1097,7 +1097,7 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager { return db.read { tx in var downloadability: Downloadability? - try? self.attachmentStore.enumerateAllReferences( + self.attachmentStore.enumerateAllReferences( toAttachmentId: record.attachmentId, tx: tx ) { reference, stop in @@ -2166,7 +2166,7 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager { // Just hold all refs in memory; there shouldn't in practice be // so many pointers to the same attachment. var references = [AttachmentReference]() - try self.attachmentStore.enumerateAllReferences( + self.attachmentStore.enumerateAllReferences( toAttachmentId: attachmentId, tx: tx ) { reference, _ in @@ -2222,7 +2222,7 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager { } var references = [AttachmentReference]() - try self.attachmentStore.enumerateAllReferences( + self.attachmentStore.enumerateAllReferences( toAttachmentId: attachmentId, tx: tx ) { reference, _ in @@ -2412,9 +2412,9 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager { ) .filter({ $0.asStream() == nil }) - let references = try thumbnailAttachments.flatMap { attachment in + let references = thumbnailAttachments.flatMap { attachment in var refs = [AttachmentReference]() - try self.attachmentStore.enumerateAllReferences( + self.attachmentStore.enumerateAllReferences( toAttachmentId: attachment.id, tx: tx ) { ref, _ in @@ -2587,7 +2587,7 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager { } func touchAllOwners(attachmentId: Attachment.IDType, tx: DBWriteTransaction) { - try? self.attachmentStore.enumerateAllReferences( + self.attachmentStore.enumerateAllReferences( toAttachmentId: attachmentId, tx: tx ) { reference, _ in diff --git a/SignalServiceKit/Upload/AttachmentUploadManager.swift b/SignalServiceKit/Upload/AttachmentUploadManager.swift index 9921bf3472..fb6c41a54c 100644 --- a/SignalServiceKit/Upload/AttachmentUploadManager.swift +++ b/SignalServiceKit/Upload/AttachmentUploadManager.swift @@ -1167,7 +1167,7 @@ public actor AttachmentUploadManagerImpl: AttachmentUploadManager { ) do { - try self.attachmentStore.enumerateAllReferences( + self.attachmentStore.enumerateAllReferences( toAttachmentId: attachmentStream.attachment.id, tx: tx ) { attachmentReference, _ in @@ -1198,8 +1198,6 @@ public actor AttachmentUploadManagerImpl: AttachmentUploadManager { break } } - } catch { - Logger.error("Failed to enumerate references: \(error)") } }