Use failIfThrows in some Attachments code, remove unused code

2025-12-30 11:21:26 -08:00 · 2025-12-30 11:21:26 -08:00 · d5dca601a6
commit d5dca601a6
parent b12335de09
12 changed files with 84 additions and 71 deletions
--- 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
--- 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
    }
 }
--- 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)
+            }
        }
    }

--- 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
--- 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
--- 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
--- 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),
+        )
    }
 }

--- 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.
--- 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
+                }
            }
        }
    }
--- 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
--- 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
--- 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)")
        }
    }