From 0ca471a7439d8ef0470c6952a950a2e97743d188 Mon Sep 17 00:00:00 2001 From: Michal Rentka Date: Mon, 2 Oct 2023 11:19:16 +0200 Subject: [PATCH 1/2] Don't cancel sync on webdav request failure --- Zotero/Assets/en.lproj/Localizable.strings | 1 + .../AuthorizeUploadSyncAction.swift | 12 ++- Zotero/Controllers/Sync/SyncController.swift | 18 +++- .../Controllers/WebDAV/WebDavController.swift | 90 ++++++++++++------- Zotero/Extensions/Localizable.swift | 4 + Zotero/Models/Sync/SyncErrors.swift | 1 + .../General/Views/SyncToolbarController.swift | 9 ++ 7 files changed, 101 insertions(+), 34 deletions(-) diff --git a/Zotero/Assets/en.lproj/Localizable.strings b/Zotero/Assets/en.lproj/Localizable.strings index 5d2917aed..c37087830 100644 --- a/Zotero/Assets/en.lproj/Localizable.strings +++ b/Zotero/Assets/en.lproj/Localizable.strings @@ -394,6 +394,7 @@ "errors.sync_toolbar.show_items" = "Show Items"; "errors.sync_toolbar.forbidden" = "Invalid username or password"; "errors.sync_toolbar.forbidden_message" = "The Zotero sync server did not accept your username and password.\n\nPlease log out and log in with correct login information."; +"errors.sync_toolbar.webdav_request_failed" = "Your WebDAV server returned an HTTP %d error for a %@ request."; "errors.items.loading" = "Could not load items."; "errors.items.deletion" = "Could not remove item."; "errors.items.deletion_from_collection" = "Could not remove item from collection."; diff --git a/Zotero/Controllers/Sync/SyncActions/AuthorizeUploadSyncAction.swift b/Zotero/Controllers/Sync/SyncActions/AuthorizeUploadSyncAction.swift index 376d8d296..e750871d6 100644 --- a/Zotero/Controllers/Sync/SyncActions/AuthorizeUploadSyncAction.swift +++ b/Zotero/Controllers/Sync/SyncActions/AuthorizeUploadSyncAction.swift @@ -30,8 +30,16 @@ struct AuthorizeUploadSyncAction: SyncAction { var result: Single { DDLogInfo("AuthorizeUploadSyncAction: authorize upload") - let request = AuthorizeUploadRequest(libraryId: self.libraryId, userId: self.userId, key: self.key, filename: self.filename, - filesize: self.filesize, md5: self.md5, mtime: self.mtime, oldMd5: self.oldMd5) + let request = AuthorizeUploadRequest( + libraryId: self.libraryId, + userId: self.userId, + key: self.key, + filename: self.filename, + filesize: self.filesize, + md5: self.md5, + mtime: self.mtime, + oldMd5: self.oldMd5 + ) return self.apiClient.send(request: request, queue: self.queue) .mapData(httpMethod: request.httpMethod.rawValue) .observe(on: self.scheduler) diff --git a/Zotero/Controllers/Sync/SyncController.swift b/Zotero/Controllers/Sync/SyncController.swift index 2d6e400b1..7d1ffc921 100644 --- a/Zotero/Controllers/Sync/SyncController.swift +++ b/Zotero/Controllers/Sync/SyncController.swift @@ -439,7 +439,19 @@ final class SyncController: SynchronizationController { retryLibraries.append(libraryId) } - case .unknown, .schema, .parsing, .apiError, .unchanged, .quotaLimit, .attachmentMissing, .insufficientSpace, .webDavDeletion, .webDavDeletionFailed, .webDavVerification, .webDavDownload: + case .unknown, + .schema, + .parsing, + .apiError, + .unchanged, + .quotaLimit, + .attachmentMissing, + .insufficientSpace, + .webDavDeletion, + .webDavDeletionFailed, + .webDavVerification, + .webDavDownload, + .webDavUpload: reportErrors.append(error) } } @@ -1969,6 +1981,10 @@ final class SyncController: SynchronizationController { return .nonFatal(.webDavDownload(error)) } + if let error = error as? WebDavError.Upload { + return .nonFatal(.webDavUpload(error)) + } + if let error = error as? ZoteroApiError { switch error { case .unchanged: return .nonFatal(.unchanged) diff --git a/Zotero/Controllers/WebDAV/WebDavController.swift b/Zotero/Controllers/WebDAV/WebDavController.swift index 4e07b0e2b..c3e51647e 100644 --- a/Zotero/Controllers/WebDAV/WebDavController.swift +++ b/Zotero/Controllers/WebDAV/WebDavController.swift @@ -69,6 +69,7 @@ enum WebDavError { enum Upload: Swift.Error { case cantCreatePropData + case apiError(AFError) } } @@ -151,31 +152,49 @@ final class WebDavControllerImpl: WebDavController { func prepareForUpload(key: String, mtime: Int, hash: String, file: File, queue: DispatchQueue) -> Single { DDLogInfo("WebDavController: prepare for upload") return self.checkServerIfNeeded(queue: queue) - .flatMap({ url -> Single in - return self.checkMetadata(key: key, mtime: mtime, hash: hash, url: url, queue: queue) - }) - .flatMap({ result -> Single in - switch result { - case .unchanged: - return Single.just(.exists) - - case .new(let url): - return self.zip(file: file, key: key).flatMap({ return Single.just(.new(url, $0)) }) - - case .mtimeChanged(let mtime): - return self.update(mtime: mtime, key: key, queue: queue).flatMap({ Single.just(.exists) }) - - case .changed(let url): - // If metadata were available on WebDAV, but they changed, remove original .prop file. - return self.removeExistingMetadata(key: key, url: url, queue: queue) - .flatMap({ self.zip(file: file, key: key) }) - .flatMap({ Single.just(.new(url, $0)) }) - } - }) + .flatMap({ url -> Single in + return self.checkMetadata(key: key, mtime: mtime, hash: hash, url: url, queue: queue) + }) + .flatMap({ result -> Single in + switch result { + case .unchanged: + return Single.just(.exists) + + case .new(let url): + return self.zip(file: file, key: key).flatMap({ return Single.just(.new(url, $0)) }) + + case .mtimeChanged(let mtime): + return self.update(mtime: mtime, key: key, queue: queue).flatMap({ Single.just(.exists) }) + + case .changed(let url): + // If metadata were available on WebDAV, but they changed, remove original .prop file. + return self.removeExistingMetadata(key: key, url: url, queue: queue) + .flatMap({ self.zip(file: file, key: key) }) + .flatMap({ Single.just(.new(url, $0)) }) + } + }) + .catch { error in + if let responseError = error as? AFResponseError { + throw WebDavError.Upload.apiError(responseError.error) + } + if let alamoError = error as? AFError { + throw WebDavError.Upload.apiError(alamoError) + } + throw error + } } func upload(request: AttachmentUploadRequest, fromFile file: File, queue: DispatchQueue) -> Single<(Data?, HTTPURLResponse)> { return self.apiClient.upload(request: request, fromFile: file, queue: queue) + .catch { error in + if let responseError = error as? AFResponseError { + throw WebDavError.Upload.apiError(responseError.error) + } + if let alamoError = error as? AFError { + throw WebDavError.Upload.apiError(alamoError) + } + throw error + } } /// Finishes upload to WebDAV. If successful, uploads new metadata .prop file to WebDAV. In both cases removes temporary ZIP file created in `prepareForUpload`. @@ -315,7 +334,7 @@ final class WebDavControllerImpl: WebDavController { private func removeExistingMetadata(key: String, url: URL, queue: DispatchQueue) -> Single<()> { DDLogInfo("WebDavController: remove metadata for \(key)") return self.apiClient.send(request: WebDavDeleteRequest(url: url.appendingPathComponent(key + ".prop")), queue: queue) - .flatMap({ _ in return Single.just(()) }) + .flatMap({ _ in return Single.just(()) }) } private func uploadMetadata(key: String, mtime: Int, hash: String, url: URL, queue: DispatchQueue) -> Single<()> { @@ -323,21 +342,30 @@ final class WebDavControllerImpl: WebDavController { let metadataProp = "\(mtime)\(hash)" guard let data = metadataProp.data(using: .utf8) else { return Single.error(WebDavError.Upload.cantCreatePropData) } return self.apiClient.send(request: WebDavWriteRequest(url: url.appendingPathComponent(key + ".prop"), data: data), queue: queue) - .flatMap({ _ in return Single.just(()) }) + .flatMap({ _ in return Single.just(()) }) + .catch { error in + if let responseError = error as? AFResponseError { + throw WebDavError.Upload.apiError(responseError.error) + } + if let alamoError = error as? AFError { + throw WebDavError.Upload.apiError(alamoError) + } + throw error + } } private func checkMetadata(key: String, mtime: Int, hash: String, url: URL, queue: DispatchQueue) -> Single { DDLogInfo("WebDavController: check metadata for \(key)") return self.metadata(key: key, url: url, queue: queue) - .flatMap({ remoteData -> Single in - guard let (remoteMtime, remoteHash) = remoteData else { return Single.just(.new(url)) } + .flatMap({ remoteData -> Single in + guard let (remoteMtime, remoteHash) = remoteData else { return Single.just(.new(url)) } - if hash == remoteHash { - return Single.just(mtime == remoteMtime ? .unchanged : .mtimeChanged(remoteMtime)) - } else { - return Single.just(.changed(url)) - } - }) + if hash == remoteHash { + return Single.just(mtime == remoteMtime ? .unchanged : .mtimeChanged(remoteMtime)) + } else { + return Single.just(.changed(url)) + } + }) } /// Loads metadata of item from WebDAV server. diff --git a/Zotero/Extensions/Localizable.swift b/Zotero/Extensions/Localizable.swift index a247b79d0..790a7a028 100644 --- a/Zotero/Extensions/Localizable.swift +++ b/Zotero/Extensions/Localizable.swift @@ -694,6 +694,10 @@ internal enum L10n { internal static func webdavItemProp(_ p1: Any) -> String { return L10n.tr("Localizable", "errors.sync_toolbar.webdav_item_prop", String(describing: p1), fallback: "Invalid prop file: %@") } + /// Your WebDAV server returned an HTTP %d error for a %@ request. + internal static func webdavRequestFailed(_ p1: Int, _ p2: Any) -> String { + return L10n.tr("Localizable", "errors.sync_toolbar.webdav_request_failed", p1, String(describing: p2), fallback: "Your WebDAV server returned an HTTP %d error for a %@ request.") + } } internal enum Translators { /// Could not update translators from bundle. Would you like to try again? diff --git a/Zotero/Models/Sync/SyncErrors.swift b/Zotero/Models/Sync/SyncErrors.swift index aaf9f751d..62571acdc 100644 --- a/Zotero/Models/Sync/SyncErrors.swift +++ b/Zotero/Models/Sync/SyncErrors.swift @@ -77,6 +77,7 @@ enum SyncError { case webDavDeletionFailed(error: String, library: String) case webDavVerification(WebDavError.Verification) case webDavDownload(WebDavError.Download) + case webDavUpload(WebDavError.Upload) case preconditionFailed(LibraryIdentifier) var isVersionMismatch: Bool { diff --git a/Zotero/Scenes/General/Views/SyncToolbarController.swift b/Zotero/Scenes/General/Views/SyncToolbarController.swift index 073b1a248..7b17e7d5a 100644 --- a/Zotero/Scenes/General/Views/SyncToolbarController.swift +++ b/Zotero/Scenes/General/Views/SyncToolbarController.swift @@ -194,6 +194,15 @@ final class SyncToolbarController { case .notChanged: break // Should not happen } + case .webDavUpload(let error): + switch error { + case .cantCreatePropData: break // Should not happen + + case .apiError(let error): + guard let statusCode = error.unacceptableStatusCode else { break } + return (L10n.Errors.SyncToolbar.webdavRequestFailed(statusCode, error.url?.absoluteString ?? ""), nil) + } + case .annotationDidSplit(let string, let keys, let libraryId): return (string, SyncError.ErrorData(itemKeys: Array(keys), libraryId: libraryId)) From 772a7f70735912b32cca25efa2208a9b5f713975 Mon Sep 17 00:00:00 2001 From: Michal Rentka Date: Tue, 3 Oct 2023 15:15:05 +0200 Subject: [PATCH 2/2] Added http method to error message --- .../Controllers/API/Alamofire+RxSwift.swift | 26 +++++++++++++++---- .../BackgroundUploadObserver.swift | 4 +-- .../Controllers/WebDAV/WebDavController.swift | 14 +++++----- .../General/Views/SyncToolbarController.swift | 4 +-- 4 files changed, 32 insertions(+), 16 deletions(-) diff --git a/Zotero/Controllers/API/Alamofire+RxSwift.swift b/Zotero/Controllers/API/Alamofire+RxSwift.swift index b2418a60c..de85e4a15 100644 --- a/Zotero/Controllers/API/Alamofire+RxSwift.swift +++ b/Zotero/Controllers/API/Alamofire+RxSwift.swift @@ -14,6 +14,7 @@ import RxSwift struct AFResponseError: Error { let url: URL? + let httpMethod: String? let error: AFError let headers: [AnyHashable: Any]? let response: String @@ -201,16 +202,25 @@ extension AFDataResponse where Success == Data?, Failure == AFError { } // Should not happen let afError = AFError.responseValidationFailed(reason: .customValidationFailed(error: ZoteroApiError.responseMissing(logData?.id ?? ""))) - return .failure(self.createResponseError(with: afError, url: self.request?.url, data: data, response: nil, logData: logData)) + return .failure(self.createResponseError(with: afError, url: self.request?.url, httpMethod: self.request?.httpMethod, data: data, response: nil, logData: logData)) case .failure(let error): - return .failure(self.createResponseError(with: error, url: self.request?.url, data: self.data, response: self.response, logData: logData)) + return .failure( + self.createResponseError( + with: error, + url: self.request?.url, + httpMethod: self.request?.httpMethod, + data: self.data, + response: self.response, + logData: logData + ) + ) } } - private func createResponseError(with error: AFError, url: URL?, data: Data?, response: HTTPURLResponse?, logData: ApiLogger.StartData?) -> AFResponseError { + private func createResponseError(with error: AFError, url: URL?, httpMethod: String?, data: Data?, response: HTTPURLResponse?, logData: ApiLogger.StartData?) -> AFResponseError { let responseString = data.flatMap({ ResponseCreator.string(from: $0, mimeType: (response?.mimeType ?? "")) }) ?? "No Response" - let responseError = AFResponseError(url: url, error: error, headers: response?.allHeaderFields, response: responseString) + let responseError = AFResponseError(url: url, httpMethod: httpMethod, error: error, headers: response?.allHeaderFields, response: responseString) if let data = logData { ApiLogger.logFailedresponse(error: responseError, statusCode: (self.response?.statusCode ?? -1), startData: data) @@ -230,7 +240,13 @@ extension AFDownloadResponse where Success == URL?, Failure == AFError { return .success(()) case .failure(let error): - let responseError = AFResponseError(url: self.request?.url, error: error, headers: self.response?.allHeaderFields, response: "Download failed") + let responseError = AFResponseError( + url: self.request?.url, + httpMethod: self.request?.httpMethod ?? "Unknown", + error: error, + headers: self.response?.allHeaderFields, + response: "Download failed" + ) if let data = logData { ApiLogger.logFailedresponse(error: responseError, statusCode: (self.response?.statusCode ?? -1), startData: data) } diff --git a/Zotero/Controllers/Background Uploader/BackgroundUploadObserver.swift b/Zotero/Controllers/Background Uploader/BackgroundUploadObserver.swift index 9c0bd83c6..5d19b7a67 100644 --- a/Zotero/Controllers/Background Uploader/BackgroundUploadObserver.swift +++ b/Zotero/Controllers/Background Uploader/BackgroundUploadObserver.swift @@ -311,7 +311,7 @@ extension BackgroundUploadObserver: URLSessionTaskDelegate { if error != nil || task.error != nil { let someError = error ?? task.error - let responseError = AFResponseError(url: task.originalRequest?.url, error: .createURLRequestFailed(error: someError!), headers: [:], response: "Upload failed") + let responseError = AFResponseError(url: task.originalRequest?.url, httpMethod: task.originalRequest?.httpMethod, error: .createURLRequestFailed(error: someError!), headers: [:], response: "Upload failed") ApiLogger.logFailedresponse(error: responseError, statusCode: 0, startData: logStartData) return true } @@ -326,7 +326,7 @@ extension BackgroundUploadObserver: URLSessionTaskDelegate { return false } - let responseError = AFResponseError(url: task.originalRequest?.url, error: .responseValidationFailed(reason: .unacceptableStatusCode(code: response.statusCode)), headers: response.allHeaderFields, response: "Upload failed") + let responseError = AFResponseError(url: task.originalRequest?.url, httpMethod: task.originalRequest?.httpMethod, error: .responseValidationFailed(reason: .unacceptableStatusCode(code: response.statusCode)), headers: response.allHeaderFields, response: "Upload failed") ApiLogger.logFailedresponse(error: responseError, statusCode: response.statusCode, startData: logStartData) return true } diff --git a/Zotero/Controllers/WebDAV/WebDavController.swift b/Zotero/Controllers/WebDAV/WebDavController.swift index c3e51647e..038acacf4 100644 --- a/Zotero/Controllers/WebDAV/WebDavController.swift +++ b/Zotero/Controllers/WebDAV/WebDavController.swift @@ -69,7 +69,7 @@ enum WebDavError { enum Upload: Swift.Error { case cantCreatePropData - case apiError(AFError) + case apiError(error: AFError, httpMethod: String?) } } @@ -175,10 +175,10 @@ final class WebDavControllerImpl: WebDavController { }) .catch { error in if let responseError = error as? AFResponseError { - throw WebDavError.Upload.apiError(responseError.error) + throw WebDavError.Upload.apiError(error: responseError.error, httpMethod: responseError.httpMethod) } if let alamoError = error as? AFError { - throw WebDavError.Upload.apiError(alamoError) + throw WebDavError.Upload.apiError(error: alamoError, httpMethod: nil) } throw error } @@ -188,10 +188,10 @@ final class WebDavControllerImpl: WebDavController { return self.apiClient.upload(request: request, fromFile: file, queue: queue) .catch { error in if let responseError = error as? AFResponseError { - throw WebDavError.Upload.apiError(responseError.error) + throw WebDavError.Upload.apiError(error: responseError.error, httpMethod: responseError.httpMethod) } if let alamoError = error as? AFError { - throw WebDavError.Upload.apiError(alamoError) + throw WebDavError.Upload.apiError(error: alamoError, httpMethod: nil) } throw error } @@ -345,10 +345,10 @@ final class WebDavControllerImpl: WebDavController { .flatMap({ _ in return Single.just(()) }) .catch { error in if let responseError = error as? AFResponseError { - throw WebDavError.Upload.apiError(responseError.error) + throw WebDavError.Upload.apiError(error: responseError.error, httpMethod: responseError.httpMethod) } if let alamoError = error as? AFError { - throw WebDavError.Upload.apiError(alamoError) + throw WebDavError.Upload.apiError(error: alamoError, httpMethod: nil) } throw error } diff --git a/Zotero/Scenes/General/Views/SyncToolbarController.swift b/Zotero/Scenes/General/Views/SyncToolbarController.swift index 7b17e7d5a..e37e2f1bc 100644 --- a/Zotero/Scenes/General/Views/SyncToolbarController.swift +++ b/Zotero/Scenes/General/Views/SyncToolbarController.swift @@ -198,9 +198,9 @@ final class SyncToolbarController { switch error { case .cantCreatePropData: break // Should not happen - case .apiError(let error): + case .apiError(let error, let httpMethod): guard let statusCode = error.unacceptableStatusCode else { break } - return (L10n.Errors.SyncToolbar.webdavRequestFailed(statusCode, error.url?.absoluteString ?? ""), nil) + return (L10n.Errors.SyncToolbar.webdavRequestFailed(statusCode, httpMethod ?? "Unknown"), nil) } case .annotationDidSplit(let string, let keys, let libraryId):