Background image upload for products without attaching images to the products #15277
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,164 @@ | ||
| import Foundation | ||
|
|
||
| public protocol MediaUploadSessionManagerDelegate: AnyObject { | ||
| func mediaUploadSessionManager(_ manager: MediaUploadSessionManager, | ||
| didCompleteUpload uploadID: String, | ||
| withResult result: Result<Media, Error>) | ||
| } | ||
|
|
||
| /// Background upload specific errors | ||
| enum BackgroundUploadError: Error { | ||
| case invalidRequestBody | ||
| case invalidResponse | ||
| case decodingError | ||
| } | ||
|
|
||
| /// Session Manager for media upload in background | ||
| /// | ||
| public final class MediaUploadSessionManager: NSObject { | ||
|
|
||
| public let backgroundSessionIdentifier: String | ||
| private lazy var backgroundSession: URLSession = { | ||
| let config = URLSessionConfiguration.background(withIdentifier: backgroundSessionIdentifier) | ||
| config.sharedContainerIdentifier = "group.com.automattic.woocommerce" | ||
| config.sessionSendsLaunchEvents = true | ||
| config.isDiscretionary = false | ||
| config.allowsCellularAccess = true | ||
| return URLSession(configuration: config, delegate: self, delegateQueue: nil) | ||
| }() | ||
|
|
||
| private var completionHandlers: [String: (Result<Media, Error>) -> Void] = [:] | ||
| private var taskResponseData: [Int: Data] = [:] | ||
| private var backgroundCompletionHandler: (() -> Void)? | ||
| weak var delegate: MediaUploadSessionManagerDelegate? | ||
|
|
||
| public init(sessionIdentifier: String = "com.automattic.woocommerce.background.upload") { | ||
| self.backgroundSessionIdentifier = sessionIdentifier | ||
| super.init() | ||
| } | ||
|
|
||
| public func uploadMedia(request: URLRequest, | ||
| mediaItem: UploadableMedia, | ||
| uploadID: String, | ||
| completion: @escaping (Result<Media, Error>) -> Void) { | ||
| completionHandlers[uploadID] = completion | ||
|
|
||
| guard let httpBody = request.httpBody else { | ||
| let error = BackgroundUploadError.invalidRequestBody | ||
| completion(.failure(error)) | ||
| return | ||
| } | ||
|
|
||
| do { | ||
| // Create temp file with proper extension from mediaItem | ||
| let tempDirectory = FileManager.default.temporaryDirectory | ||
| let tempFileURL = tempDirectory.appendingPathComponent(mediaItem.filename) | ||
| try httpBody.write(to: tempFileURL) | ||
|
|
||
| // Upload using temp file | ||
| var modifiedRequest = request | ||
| modifiedRequest.httpBody = nil | ||
|
|
||
| let task = backgroundSession.uploadTask(with: modifiedRequest, fromFile: tempFileURL) | ||
|
There was a problem hiding this comment. I see that we have already appended the data of the media item to the request body in this line. Do we still have to create a temporary file and pass it to this Is this the reason for using a temp file? If yes, can you add a comment to explain? There was a problem hiding this comment. That's exactly the reason. I will add a comment to clarify that for the future 👍 |
||
| task.taskDescription = uploadID | ||
| task.resume() | ||
|
|
||
| // Cleanup temp file | ||
| DispatchQueue.main.async { | ||
| try? FileManager.default.removeItem(at: tempFileURL) | ||
| } | ||
|
Comment on lines
+74
to
+77
There was a problem hiding this comment. I don't understand why we removed the temporary file here. Shouldn't we wait for the upload task to finish/fail? There was a problem hiding this comment. So, as you understand from reading that article, it is necessary to have the file on disk. However, when you create and start an upload task using a file URL, URLSession immediately begins accessing the file to prepare for the upload. The system reads the file data synchronously or copies the file data into its own buffer to manage the upload independently of the app's process. At that moment, it is no longer needed. Since the upload task has already been started and the system has a reference to the file, deleting it from the app's sandboxed file system doesn't interrupt the upload. There's a slight chance that if the file is deleted before URLSession has begun reading it, the upload could fail. That's why I use Thinking aloud about it, for maximum safety, I tried to defer deleting the temp file until we receive confirmation that the upload task has successfully started or even until the upload completes. However, this might not be practical because:
So I think the current solution offers a good balance between cleanup and reliability. There was a problem hiding this comment. As it is a temporary directory, we shouldn't have to worry about file remaining in disk for extended period. We can delete the file once the upload completes and let the system deal with cleaning it up in case the app is force-quit. What do you think? There was a problem hiding this comment. If we delete it right away, we somehow prevent the system from cleaning up the directory at its discretion if the device has little available disk space. Also, I believe it will be easy to test in the future, right? |
||
| } catch { | ||
| DDLogError("⛔️ MediaUploadSessionManager- Failed image upload while creating temp file: \(error)") | ||
| completion(.failure(error)) | ||
| } | ||
| } | ||
|
|
||
| public func handleBackgroundSessionCompletion(_ completionHandler: @escaping () -> Void) { | ||
| backgroundCompletionHandler = completionHandler | ||
| } | ||
| } | ||
|
|
||
| extension MediaUploadSessionManager: URLSessionDataDelegate { | ||
| public func urlSession(_ session: URLSession, | ||
| dataTask: URLSessionDataTask, | ||
| didReceive data: Data) { | ||
| if let existingData = taskResponseData[dataTask.taskIdentifier] { | ||
| taskResponseData[dataTask.taskIdentifier] = existingData + data | ||
| } else { | ||
| taskResponseData[dataTask.taskIdentifier] = data | ||
| } | ||
| } | ||
|
|
||
| public func urlSession(_ session: URLSession, | ||
| task: URLSessionTask, | ||
| didCompleteWithError error: Error?) { | ||
| guard let uploadID = task.taskDescription else { | ||
| DDLogDebug("MediaUploadSessionManager- task completed without an upload identifier. Task identifier: \(task.taskIdentifier)") | ||
| return | ||
| } | ||
| defer { | ||
| taskResponseData.removeValue(forKey: task.taskIdentifier) | ||
| } | ||
|
|
||
| if let error = error { | ||
| DDLogError("⛔️ MediaUploadSessionManager- Upload failure for task (\(uploadID)): encountered error: \(error.localizedDescription)") | ||
| notifyCompletion(.failure(error), for: uploadID) | ||
| return | ||
| } | ||
|
|
||
| guard let httpResponse = task.response as? HTTPURLResponse else { | ||
| DDLogError("⛔️ MediaUploadSessionManager- Upload failure for task (\(uploadID)): " + | ||
| "response is not a valid HTTPURLResponse. Actual response: " + | ||
| "\(String(describing: task.response))") | ||
| notifyCompletion(.failure(BackgroundUploadError.invalidResponse), for: uploadID) | ||
| return | ||
| } | ||
|
|
||
| guard let data = taskResponseData[task.taskIdentifier] else { | ||
| DDLogError("⛔️ MediaUploadSessionManager- Upload failure for task (\(uploadID)): " + | ||
| "missing response data for task with identifier \(task.taskIdentifier)") | ||
| notifyCompletion(.failure(BackgroundUploadError.invalidResponse), for: uploadID) | ||
| return | ||
| } | ||
|
|
||
| if !(200...299).contains(httpResponse.statusCode) { | ||
| DDLogError("⛔️ MediaUploadSessionManager- Upload failure for task (\(uploadID)): " + | ||
| "unexpected HTTP status code \(httpResponse.statusCode). " + | ||
| "Full response: \(httpResponse) Headers: \(httpResponse.allHeaderFields)") | ||
| notifyCompletion(.failure(BackgroundUploadError.invalidResponse), for: uploadID) | ||
| return | ||
| } | ||
|
|
||
| // Use MediaMapper to parse response | ||
| if let jsonString = String(data: data, encoding: .utf8) { | ||
|
There was a problem hiding this comment. I see that |
||
| } else { | ||
| DDLogError("⛔️ MediaUploadSessionManager- Failed to convert response data to JSON string for task (\(uploadID))") | ||
| } | ||
| let mapper = WordPressMediaMapper() | ||
| do { | ||
| let media = try mapper.map(response: data) | ||
| notifyCompletion(.success(media.toMedia()), for: uploadID) | ||
| } catch { | ||
| DDLogError("⛔️ MediaUploadSessionManager- Upload failure for task (\(uploadID)): error mapping media: \(error.localizedDescription)") | ||
| notifyCompletion(.failure(error), for: uploadID) | ||
| } | ||
| } | ||
|
|
||
| public func urlSessionDidFinishEvents(forBackgroundURLSession session: URLSession) { | ||
| DispatchQueue.main.async { [weak self] in | ||
| DDLogDebug("MediaUploadSessionManager- Background URL session did finish events. Invoking completion handler.") | ||
| self?.backgroundCompletionHandler?() | ||
| self?.backgroundCompletionHandler = nil | ||
| } | ||
| } | ||
|
|
||
| private func notifyCompletion(_ result: Result<Media, Error>, for uploadID: String) { | ||
| DispatchQueue.main.async { [weak self] in | ||
| DDLogError("⛔️ MediaUploadSessionManager- Notifying completion for task (\(uploadID)) with result: \(result)") | ||
| guard let self = self else { return } | ||
| self.completionHandlers[uploadID]?(result) | ||
| self.completionHandlers.removeValue(forKey: uploadID) | ||
| self.delegate?.mediaUploadSessionManager(self, didCompleteUpload: uploadID, withResult: result) | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use bundle based identifiers as suggested here?