Consolidate shouldConsume logic

This commit is contained in:
Max Radermacher 2025-12-10 14:05:29 -06:00 committed by GitHub
parent c712942889
commit 145446f80a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
20 changed files with 40 additions and 50 deletions

View File

@ -17,7 +17,7 @@ enum SignalAttachmentCloner {
filename: attachment.reference.sourceFilename
)
let decryptedDataSource = DataSourcePath(fileUrl: decryptedCopyUrl, shouldDeleteOnDeallocation: true)
let decryptedDataSource = DataSourcePath(fileUrl: decryptedCopyUrl, ownership: .owned)
decryptedDataSource.sourceFilename = attachment.reference.sourceFilename
let signalAttachment: SignalAttachment

View File

@ -806,7 +806,7 @@ extension ConversationViewController: UIDocumentPickerDelegate {
return
}
let dataSource = DataSourcePath(fileUrl: url, shouldDeleteOnDeallocation: false)
let dataSource = DataSourcePath(fileUrl: url, ownership: .owned)
dataSource.sourceFilename = filename
let contentTypeIdentifier = (resourceValues?.contentType ?? .data).identifier

View File

@ -27,7 +27,7 @@ extension VoiceMessageSendableDraft {
func prepareAttachment() throws -> PreviewableAttachment {
let attachmentUrl = try prepareForSending()
let dataSource = DataSourcePath(fileUrl: attachmentUrl, shouldDeleteOnDeallocation: true)
let dataSource = DataSourcePath(fileUrl: attachmentUrl, ownership: .owned)
dataSource.sourceFilename = userVisibleFilename(currentDate: Date())
return PreviewableAttachment(rawValue: try SignalAttachment.voiceMessageAttachment(dataSource: dataSource, dataUTI: UTType.mpeg4Audio.identifier))

View File

@ -510,7 +510,7 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect
let consumableFilePath = OWSFileSystem.temporaryFilePath(fileExtension: assetFileExtension)
try FileManager.default.copyItem(atPath: assetFilePath, toPath: consumableFilePath)
let dataSource = DataSourcePath(filePath: consumableFilePath, shouldDeleteOnDeallocation: false)
let dataSource = DataSourcePath(filePath: consumableFilePath, ownership: .owned)
let attachment = try SignalAttachment.attachment(dataSource: dataSource, dataUTI: assetTypeIdentifier)
attachment.isLoopingVideo = attachment.isVideo

View File

@ -831,7 +831,7 @@ class CameraCaptureSession: NSObject {
private func handleVideoRecording(at outputUrl: URL) throws {
AssertIsOnMainThread()
let dataSource = DataSourcePath(fileUrl: outputUrl, shouldDeleteOnDeallocation: true)
let dataSource = DataSourcePath(fileUrl: outputUrl, ownership: .owned)
let attachment = try SignalAttachment.videoAttachment(
dataSource: dataSource,
dataUTI: UTType.mpeg4Movie.identifier,

View File

@ -14,7 +14,7 @@ class SignalAttachmentTest: SignalBaseTest {
func testMetadataStrippingDoesNotChangeOrientation(url: URL) throws {
let size = (try? DataImageSource.forPath(url.path).imageMetadata()?.pixelSize) ?? .zero
let dataSource = DataSourcePath(fileUrl: url, shouldDeleteOnDeallocation: false)
let dataSource = DataSourcePath(fileUrl: url, ownership: .borrowed)
let attachment = try SignalAttachment.imageAttachment(
dataSource: dataSource,
dataUTI: UTType.jpeg.identifier
@ -52,7 +52,7 @@ class SignalAttachmentTest: SignalBaseTest {
func testRemoveMetadataFromPng() throws {
let testBundle = Bundle(for: Self.self)
let url = testBundle.url(forResource: "test-png-with-metadata", withExtension: "png")!
let dataSource = DataSourcePath(fileUrl: url, shouldDeleteOnDeallocation: false)
let dataSource = DataSourcePath(fileUrl: url, ownership: .borrowed)
XCTAssertEqual(
try pngChunkTypes(data: dataSource.readData()),
["IHDR", "PLTE", "sRGB", "tIME", "tEXt", "IDAT", "IEND"],

View File

@ -134,7 +134,7 @@ public struct SendableAttachment {
}
let segments = try segmentFileUrls.map { url in
let dataSource = DataSourcePath(fileUrl: url, shouldDeleteOnDeallocation: true)
let dataSource = DataSourcePath(fileUrl: url, ownership: .owned)
// [15M] TODO: This doesn't transfer all SignalAttachment fields.
let attachment = try SignalAttachment.videoAttachment(dataSource: dataSource, dataUTI: self.dataUTI)
return Self(nonImagePreviewableAttachment: PreviewableAttachment(rawValue: attachment))

View File

@ -20,7 +20,6 @@ extension SendableAttachment {
public func forSending(attachmentContentValidator: any AttachmentContentValidator) async throws -> ForSending {
let dataSource = try await attachmentContentValidator.validateContents(
sendableAttachment: self,
shouldConsume: true,
shouldUseDefaultFilename: true,
)
return ForSending(

View File

@ -848,7 +848,7 @@ public class SignalAttachment: CustomDebugStringConvertible {
throw .couldNotConvertImage
}
let outputDataSource = DataSourcePath(fileUrl: tempFileUrl, shouldDeleteOnDeallocation: false)
let outputDataSource = DataSourcePath(fileUrl: tempFileUrl, ownership: .owned)
let outputFileSize: UInt64
do {
outputFileSize = try outputDataSource.readLength()
@ -1085,7 +1085,7 @@ public class SignalAttachment: CustomDebugStringConvertible {
mp4Filename = nil
}
let dataSource = DataSourcePath(fileUrl: exportURL, shouldDeleteOnDeallocation: true)
let dataSource = DataSourcePath(fileUrl: exportURL, ownership: .owned)
dataSource.sourceFilename = mp4Filename
let endTime = MonotonicDate()

View File

@ -506,7 +506,7 @@ extension OWSSyncManager: SyncManagerProtocol, SyncManagerProtocolSwift {
return
}
let dataSource = DataSourcePath(fileUrl: result.syncFileUrl, shouldDeleteOnDeallocation: true)
let dataSource = DataSourcePath(fileUrl: result.syncFileUrl, ownership: .owned)
let uploadResult = try await DependenciesBridge.shared.attachmentUploadManager.uploadTransientAttachment(dataSource: dataSource)
let message = SSKEnvironment.shared.databaseStorageRef.read { tx in

View File

@ -484,7 +484,7 @@ public class LinkAndSyncManagerImpl: LinkAndSyncManager {
) async throws(PrimaryLinkNSyncError) -> Upload.Result<Upload.LinkNSyncUploadMetadata> {
do {
return try await attachmentUploadManager.uploadLinkNSyncAttachment(
dataSource: DataSourcePath(fileUrl: metadata.fileUrl, shouldDeleteOnDeallocation: true),
dataSource: DataSourcePath(fileUrl: metadata.fileUrl, ownership: .owned),
progress: progress
)
} catch {

View File

@ -66,12 +66,8 @@ public protocol AttachmentContentValidator {
/// Returns a PendingAttachment with validated contents, ready to be inserted.
/// Note the content type may be `invalid`; we can still create an Attachment from these.
/// Errors are thrown if data reading/parsing fails.
///
/// - Parameter shouldConsume: If true, the source file will be deleted and the DataSource
/// consumed after validation is complete; otherwise the source file will be left as-is.
func validateContents(
dataSource: DataSourcePath,
shouldConsume: Bool,
mimeType: String,
renderingFlag: AttachmentReference.RenderingFlag,
sourceFilename: String?
@ -189,12 +185,10 @@ extension AttachmentContentValidator {
public func validateContents(
sendableAttachment: SendableAttachment,
shouldConsume: Bool,
shouldUseDefaultFilename: Bool,
) async throws -> AttachmentDataSource {
return try await validateContents(
dataSource: sendableAttachment.dataSource,
shouldConsume: shouldConsume,
mimeType: sendableAttachment.mimeType,
renderingFlag: sendableAttachment.renderingFlag,
sourceFilename: sendableAttachment.sourceFilename?.rawValue ?? (shouldUseDefaultFilename ? sendableAttachment.defaultFilename : nil),
@ -203,14 +197,12 @@ extension AttachmentContentValidator {
public func validateContents(
dataSource: DataSourcePath,
shouldConsume: Bool,
mimeType: String,
renderingFlag: AttachmentReference.RenderingFlag,
sourceFilename: String?
) async throws -> AttachmentDataSource {
return await .from(pendingAttachment: try self.validateContents(
dataSource: dataSource,
shouldConsume: shouldConsume,
mimeType: mimeType,
renderingFlag: renderingFlag,
sourceFilename: sourceFilename

View File

@ -28,7 +28,6 @@ public class AttachmentContentValidatorImpl: AttachmentContentValidator {
public func validateContents(
dataSource: DataSourcePath,
shouldConsume: Bool,
mimeType: String,
renderingFlag: AttachmentReference.RenderingFlag,
sourceFilename: String?
@ -46,11 +45,7 @@ public class AttachmentContentValidatorImpl: AttachmentContentValidator {
sourceFilename: sourceFilename
)]
).values.first!
if shouldConsume {
try dataSource.consumeAndDelete()
}
try dataSource.consumeAndDeleteIfNecessary()
return pendingAttachment
}

View File

@ -13,7 +13,6 @@ open class AttachmentContentValidatorMock: AttachmentContentValidator {
open func validateContents(
dataSource: DataSourcePath,
shouldConsume: Bool,
mimeType: String,
renderingFlag: AttachmentReference.RenderingFlag,
sourceFilename: String?

View File

@ -1928,8 +1928,7 @@ public class AttachmentDownloadManagerImpl: AttachmentDownloadManager {
}
return try await attachmentValidator.validateContents(
dataSource: DataSourcePath(fileUrl: stickerDataUrl, shouldDeleteOnDeallocation: false),
shouldConsume: false,
dataSource: DataSourcePath(fileUrl: stickerDataUrl, ownership: .borrowed),
mimeType: mimeType.rawValue,
renderingFlag: .borderless,
sourceFilename: nil

View File

@ -52,7 +52,6 @@ public class OnboardingStoryManagerStoryMessageFactory {
) async throws -> AttachmentDataSource {
return try await DependenciesBridge.shared.attachmentContentValidator.validateContents(
dataSource: dataSource,
shouldConsume: true,
mimeType: mimeType,
renderingFlag: .default,
sourceFilename: nil
@ -548,7 +547,7 @@ public class SystemStoryManager: SystemStoryManagerProtocol {
}
let dataSource = DataSourcePath(
fileUrl: resultUrl,
shouldDeleteOnDeallocation: CurrentAppContext().isRunningTests.negated
ownership: CurrentAppContext().isRunningTests ? .borrowed : .owned,
)
return try await storyMessageFactory.validateAttachmentContents(
dataSource: dataSource,

View File

@ -6,21 +6,30 @@
import Foundation
public class DataSourcePath {
public init(fileUrl: URL, shouldDeleteOnDeallocation: Bool) {
owsPrecondition(fileUrl.isFileURL)
self.fileUrl = fileUrl
self.shouldDeleteOnDeallocation = shouldDeleteOnDeallocation
public enum Ownership {
/// The `DataSourcePath` owns this URL and may consume it.
case owned
/// The `DataSourcePath` is borrowing a reference to this file and must not
/// touch it.
case borrowed
}
public convenience init(filePath: String, shouldDeleteOnDeallocation: Bool) {
public init(fileUrl: URL, ownership: Ownership) {
owsPrecondition(fileUrl.isFileURL)
self.fileUrl = fileUrl
self.ownership = ownership
}
public convenience init(filePath: String, ownership: Ownership) {
let fileUrl = URL(fileURLWithPath: filePath)
self.init(fileUrl: fileUrl, shouldDeleteOnDeallocation: shouldDeleteOnDeallocation)
self.init(fileUrl: fileUrl, ownership: ownership)
}
public convenience init(writingTempFileData: Data, fileExtension: String) throws {
let fileUrl = OWSFileSystem.temporaryFileUrl(fileExtension: fileExtension, isAvailableWhileDeviceLocked: true)
try writingTempFileData.write(to: fileUrl, options: .completeFileProtectionUntilFirstUserAuthentication)
self.init(fileUrl: fileUrl, shouldDeleteOnDeallocation: true)
self.init(fileUrl: fileUrl, ownership: .owned)
}
public convenience init(writingSyncMessageData: Data) throws {
@ -28,10 +37,9 @@ public class DataSourcePath {
}
deinit {
if shouldDeleteOnDeallocation && !isConsumed.get() {
// In the ObjC code this would fire into a dispatch queue
if ownership == .owned && !isConsumed.get() {
do {
try FileManager.default.removeItem(at: fileUrl)
try OWSFileSystem.deleteFileIfExists(url: fileUrl)
} catch {
owsFailDebug("DataSourcePath could not delete file: \(fileUrl), \(error)")
}
@ -39,7 +47,7 @@ public class DataSourcePath {
}
public let fileUrl: URL
private let shouldDeleteOnDeallocation: Bool
private let ownership: Ownership
private let isConsumed = AtomicBool(false, lock: .init())
private var _sourceFilename: String?
@ -63,9 +71,11 @@ public class DataSourcePath {
return UInt64(try fileUrl.resourceValues(forKeys: [.fileSizeKey]).fileSize!)
}
public func consumeAndDelete() throws {
public func consumeAndDeleteIfNecessary() throws {
owsAssertDebug(isConsumed.tryToSetFlag())
try OWSFileSystem.deleteFileIfExists(url: fileUrl)
if ownership == .owned {
try OWSFileSystem.deleteFileIfExists(url: fileUrl)
}
}
public func imageSource() throws -> any OWSImageSource {

View File

@ -766,7 +766,7 @@ public final class AttachmentApprovalViewController: UIPageViewController, UIPag
guard let dataUTI = MimeTypeUtil.utiTypeForFileExtension(fileExtension) else {
throw OWSAssertionError("Missing dataUTI.")
}
let dataSource = DataSourcePath(fileUrl: fileUrl, shouldDeleteOnDeallocation: true)
let dataSource = DataSourcePath(fileUrl: fileUrl, ownership: .owned)
// Rewrite the filename's extension to reflect the output file format.
var filename: String? = attachmentApprovalItem.attachment.rawValue.dataSource.sourceFilename?.filterFilename()
if let sourceFilename = attachmentApprovalItem.attachment.rawValue.dataSource.sourceFilename?.filterFilename() {

View File

@ -159,7 +159,6 @@ public class AttachmentMultisend {
for attachment in sendableAttachments {
let dataSource: AttachmentDataSource = try await deps.attachmentValidator.validateContents(
sendableAttachment: attachment,
shouldConsume: true,
shouldUseDefaultFilename: false,
)
try results.append(.init(
@ -182,7 +181,6 @@ public class AttachmentMultisend {
// doesn't segment.
originalDataSource = try await deps.attachmentValidator.validateContents(
sendableAttachment: segmentingResult.original,
shouldConsume: true,
shouldUseDefaultFilename: false,
)
} else {
@ -197,7 +195,6 @@ public class AttachmentMultisend {
for segment in segments {
let dataSource: AttachmentDataSource = try await deps.attachmentValidator.validateContents(
sendableAttachment: segment,
shouldConsume: true,
shouldUseDefaultFilename: false,
)
segmentedDataSources.append(dataSource)

View File

@ -379,7 +379,7 @@ public struct TypedItemProvider {
let copiedUrl = OWSFileSystem.temporaryFileUrl(fileExtension: fileUrl.pathExtension)
try FileManager.default.copyItem(at: fileUrl, to: copiedUrl)
let dataSource = DataSourcePath(fileUrl: copiedUrl, shouldDeleteOnDeallocation: true)
let dataSource = DataSourcePath(fileUrl: copiedUrl, ownership: .owned)
dataSource.sourceFilename = fileUrl.lastPathComponent
let dataUTI = MimeTypeUtil.utiTypeForFileExtension(fileUrl.pathExtension) ?? defaultTypeIdentifier