Background image upload for products without attaching images to the products #15277
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| 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? = nil) { | ||
| // Use bundle based identifiers as suggested here | ||
| // https://www.avanderlee.com/swift/urlsession-common-pitfalls-with-background-download-upload-tasks/#bundle-based-identifiers | ||
| let appBundleName = Bundle.main.bundleURL.lastPathComponent | ||
| .lowercased() | ||
| .replacingOccurrences(of: " ", with: ".") | ||
| let defaultSessionIdentifier = "com.background.\(appBundleName)" | ||
| self.backgroundSessionIdentifier = sessionIdentifier ?? defaultSessionIdentifier | ||
| 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 | ||
|
|
||
| // Upload tasks in background works only if we use a file reference | ||
| let task = backgroundSession.uploadTask(with: modifiedRequest, fromFile: tempFileURL) | ||
| 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) | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
uploadTaskmethod?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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly the reason. I will add a comment to clarify that for the future 👍