From c3096e5dfe883cc4609887100eb860b890b89343 Mon Sep 17 00:00:00 2001 From: Jackson Harper Date: Wed, 25 May 2022 21:33:55 -0700 Subject: [PATCH] Better PDF upload handling on iOS Three main changes here: - If a PDF isn't a local file, dont try to download and then upload it to the backend. Just treat it as a URL. - If we are passed PDF data by an app like Files, upload it. - Wrap the whole operation in a Background task so its not killed if the user taps dismiss during the upload of larger documents. --- .../Share/ShareExtensionScene.swift | 16 ++- .../Sources/Models/PageScrapePayload.swift | 97 +++++++++++++++---- .../DataService/Mutations/SaveArticle.swift | 12 ++- .../DataService/Mutations/SavePDF.swift | 9 +- .../DataService/Mutations/SavePage.swift | 6 +- 5 files changed, 105 insertions(+), 35 deletions(-) diff --git a/apple/OmnivoreKit/Sources/App/AppExtensions/Share/ShareExtensionScene.swift b/apple/OmnivoreKit/Sources/App/AppExtensions/Share/ShareExtensionScene.swift index 39696dd35..d3fa98d5f 100644 --- a/apple/OmnivoreKit/Sources/App/AppExtensions/Share/ShareExtensionScene.swift +++ b/apple/OmnivoreKit/Sources/App/AppExtensions/Share/ShareExtensionScene.swift @@ -59,11 +59,17 @@ final class ShareExtensionViewModel: ObservableObject { return } + let backgroundTask = UIApplication.shared.beginBackgroundTask(withName: requestId) let saveLinkPublisher: AnyPublisher = { - if pageScrapePayload.contentType == .pdf { - return services.dataService.uploadPDFPublisher(pageScrapePayload: pageScrapePayload, requestId: requestId) - } else if pageScrapePayload.html != nil { - return services.dataService.savePagePublisher(pageScrapePayload: pageScrapePayload, requestId: requestId) + if case let .pdf(data) = pageScrapePayload.contentType { + return services.dataService.uploadPDFPublisher(pageScrapePayload: pageScrapePayload, + data: data, + requestId: requestId) + } else if case let .html(html, title) = pageScrapePayload.contentType { + return services.dataService.savePagePublisher(pageScrapePayload: pageScrapePayload, + html: html, + title: title, + requestId: requestId) } else { return services.dataService.saveUrlPublisher(pageScrapePayload: pageScrapePayload, requestId: requestId) } @@ -74,8 +80,10 @@ final class ShareExtensionViewModel: ObservableObject { guard case let .failure(error) = completion else { return } self?.debugText = "saveArticleError: \(error)" self?.status = .failed(error: error) + UIApplication.shared.endBackgroundTask(backgroundTask) } receiveValue: { [weak self] _ in self?.status = .success + UIApplication.shared.endBackgroundTask(backgroundTask) } .store(in: &subscriptions) diff --git a/apple/OmnivoreKit/Sources/Models/PageScrapePayload.swift b/apple/OmnivoreKit/Sources/Models/PageScrapePayload.swift index edf1baa42..27b051346 100644 --- a/apple/OmnivoreKit/Sources/Models/PageScrapePayload.swift +++ b/apple/OmnivoreKit/Sources/Models/PageScrapePayload.swift @@ -9,28 +9,34 @@ import UniformTypeIdentifiers let URLREGEX = #"[(http(s)?):\/\/(www\.)?a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z]{2,6}\b([-a-zA-Z0-9@:%_\+.~#?&//=]*)"# public struct PageScrapePayload { - public enum ContentType { - case html - case pdf + public struct HTMLPayload { + let url: String + let title: String? + let html: String + } + + public enum ContentType { + case none + case html(html: String, title: String?) + case pdf(data: Data) } - public let title: String? - public let html: String? public let url: String public let contentType: ContentType - init(url: String, title: String?, html: String?, contentType: String?) { + init(url: String) { self.url = url - self.title = title - self.html = html + self.contentType = .none + } - // If the content type was specified and we know its PDF, use that - // otherwise fallback to using file extensions. - if let contentType = contentType, contentType.contains("pdf") { - self.contentType = .pdf - } else { - self.contentType = url.hasSuffix(".pdf") ? .pdf : .html - } + init(url: String, pdfData: Data) { + self.url = url + self.contentType = .pdf(data: pdfData) + } + + init(url: String, title: String?, html: String) { + self.url = url + self.contentType = .html(html: html, title: title) } } @@ -52,10 +58,25 @@ public enum PageScraper { } var pageScrapePayload: PageScrapePayload? + let PDFKey = UTType.pdf.identifier + let publicFileKey = UTType.fileURL.identifier let propertyListKey = UTType.propertyList.identifier let group = DispatchGroup() + for attachment in attachments where attachment.hasItemConformingToTypeIdentifier(PDFKey) { + group.enter() + attachment.loadItem( + forTypeIdentifier: PDFKey, + options: nil + ) { item, _ in + if let payload = PageScrapePayload.make(item: item) { + pageScrapePayload = payload + } + group.leave() + } + } + for attachment in attachments where attachment.hasItemConformingToTypeIdentifier(propertyListKey) { group.enter() attachment.loadItem( @@ -214,17 +235,55 @@ public enum PageScraper { private extension PageScrapePayload { static func make(url: URL?) -> PageScrapePayload? { guard let url = url else { return nil } - return PageScrapePayload(url: url.absoluteString, title: nil, html: nil, contentType: nil) + return PageScrapePayload(url: url.absoluteString) } static func make(item: NSSecureCoding?) -> PageScrapePayload? { - let dictionary = item as? NSDictionary - let results = dictionary?[NSExtensionJavaScriptPreprocessingResultsKey] as? NSDictionary + if let dictionary = item as? NSDictionary { + return makeFromDictionary(dictionary) + } + if let url = item as? NSURL { + return makeFromURL(url as URL) + } + return nil + } + + static func makeFromURL(_ url: URL) -> PageScrapePayload? { + if url.isFileURL { + let type = try? url.resourceValues(forKeys: [.typeIdentifierKey]).typeIdentifier + if type == UTType.pdf.identifier, let data = try? Data(contentsOf: url) { + return PageScrapePayload(url: url.absoluteString, pdfData: data) + } + // Don't try to handle file URLs that are not PDFs. + // In the future we can add image and other file type support here + return nil + } + return PageScrapePayload(url: url.absoluteString) + } + + static func makeFromDictionary(_ dictionary: NSDictionary) -> PageScrapePayload? { + let results = dictionary[NSExtensionJavaScriptPreprocessingResultsKey] as? NSDictionary guard let url = results?["url"] as? String else { return nil } let html = results?["documentHTML"] as? String let title = results?["title"] as? String let contentType = results?["contentType"] as? String - return PageScrapePayload(url: url, title: title, html: html, contentType: contentType) + // If we were not able to capture any HTML, treat this as a URL and + // see if the backend can do better. + if html == nil || html!.isEmpty { + return PageScrapePayload(url: url) + } + + // If its a PDF that we opened through Safari we don't have access to the + // file content, so pass the URL to the backend and let it download it. + if contentType == "application/pdf" { + return PageScrapePayload(url: url) + } + + if let html = html { + return PageScrapePayload(url: url, title: title, html: html) + } + + return PageScrapePayload(url: url) } } diff --git a/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SaveArticle.swift b/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SaveArticle.swift index 3ca426ca2..cdb5a7a38 100644 --- a/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SaveArticle.swift +++ b/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SaveArticle.swift @@ -92,11 +92,13 @@ public extension DataService { } let preparedDocument: InputObjects.PreparedDocumentInput? = { - guard let html = pageScrapePayload.html, let title = pageScrapePayload.title else { return nil } - return InputObjects.PreparedDocumentInput( - document: html, - pageInfo: InputObjects.PageInfoInput(title: OptionalArgument(title)) - ) + if case let .html(html, title) = pageScrapePayload.contentType { + return InputObjects.PreparedDocumentInput( + document: html, + pageInfo: InputObjects.PageInfoInput(title: OptionalArgument(title)) + ) + } + return nil }() let input = InputObjects.CreateArticleInput( diff --git a/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SavePDF.swift b/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SavePDF.swift index 431dd2f1e..058953ba1 100644 --- a/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SavePDF.swift +++ b/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SavePDF.swift @@ -6,10 +6,11 @@ import SwiftGraphQL public extension DataService { func uploadPDFPublisher( pageScrapePayload: PageScrapePayload, + data: Data, requestId: String ) -> AnyPublisher { uploadFileRequestPublisher(pageScrapePayload: pageScrapePayload) - .flatMap { self.uploadFilePublisher(fileUploadConfig: $0, fileURLString: pageScrapePayload.url) } + .flatMap { self.uploadFilePublisher(fileUploadConfig: $0, data: data) } .flatMap { self.saveFilePublisher(pageScrapePayload: pageScrapePayload, uploadFileId: $0, requestId: requestId) } .catch { _ in self.saveUrlPublisher(pageScrapePayload: pageScrapePayload, requestId: requestId) } .receive(on: DispatchQueue.main) @@ -88,11 +89,11 @@ private extension DataService { } // swiftlint:disable:next line_length - func uploadFilePublisher(fileUploadConfig: UploadFileRequestPayload, fileURLString: String) -> AnyPublisher { - let pdfData = URL(string: fileURLString).flatMap { try? Data(contentsOf: $0) } + func uploadFilePublisher(fileUploadConfig: UploadFileRequestPayload, data: Data) -> AnyPublisher { + let pdfData = data // URL(string: "http://localhost:4000/local/debug/endpoint").flatMap { try? Data(contentsOf: $0) } let url = fileUploadConfig.urlString.flatMap { URL(string: $0) } - guard let url = url, let pdfData = pdfData else { return Future { $0(.failure(.badData)) }.eraseToAnyPublisher() } + guard let url = url else { return Future { $0(.failure(.badData)) }.eraseToAnyPublisher() } var request = URLRequest(url: url) request.httpMethod = "PUT" diff --git a/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SavePage.swift b/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SavePage.swift index cda9a6e9e..7a8ccad53 100644 --- a/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SavePage.swift +++ b/apple/OmnivoreKit/Sources/Services/DataService/Mutations/SavePage.swift @@ -5,7 +5,7 @@ import SwiftGraphQL public extension DataService { // swiftlint:disable:next line_length - func savePagePublisher(pageScrapePayload: PageScrapePayload, requestId: String) -> AnyPublisher { + func savePagePublisher(pageScrapePayload: PageScrapePayload, html: String, title: String?, requestId: String) -> AnyPublisher { enum MutationResult { case saved(requestId: String, url: String) case error(errorCode: Enums.SaveErrorCode) @@ -13,9 +13,9 @@ public extension DataService { let input = InputObjects.SavePageInput( clientRequestId: requestId, - originalContent: pageScrapePayload.html ?? "", + originalContent: html, source: "ios-page", - title: OptionalArgument(pageScrapePayload.title), + title: OptionalArgument(title), url: pageScrapePayload.url )