From 9d361d1bcd650700cfe52a58bc0360fe547cf8f4 Mon Sep 17 00:00:00 2001 From: Iva Horn Date: Fri, 17 Jul 2026 15:08:48 +0200 Subject: [PATCH 1/2] feat(upload): Support If-Match precondition on chunked assembly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an optional `ifMatch` parameter to `uploadChunkAsync`. When set, the ETag precondition is applied ONLY to the final assembly MOVE that materializes the destination file — never to the chunk PUTs, which target brand-new chunk resources and would spuriously fail with 412. It is also cleared before the post-assembly PROPFIND readback, whose target carries a fresh ETag after a successful MOVE. This lets clients perform optimistic-concurrency conflict detection for chunked (large-file) uploads: if the destination changed since the base version the client edited, the server rejects the assembly with 412 Precondition Failed instead of silently overwriting the newer copy. Single-request PUT uploads can already carry `If-Match` via `NKRequestOptions.customHeader`; this closes the gap for the chunked path, where the shared header bag would otherwise leak the precondition onto the chunk PUTs. Signed-off-by: Iva Horn Co-Authored-By: Claude Opus 4.8 --- Sources/NextcloudKit/NextcloudKit+Upload.swift | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Sources/NextcloudKit/NextcloudKit+Upload.swift b/Sources/NextcloudKit/NextcloudKit+Upload.swift index 6e6a072f..2609f83e 100644 --- a/Sources/NextcloudKit/NextcloudKit+Upload.swift +++ b/Sources/NextcloudKit/NextcloudKit+Upload.swift @@ -188,6 +188,7 @@ public extension NextcloudKit { /// - chunkSize: Desired chunk size in bytes. /// - account: Account identifier. /// - options: Request options (headers, timeout, etc.). + /// - ifMatch: Optional ETag precondition applied **only** to the final assembly `MOVE` (not the chunk uploads). When set, the server rejects the assembly with `412 Precondition Failed` if the destination changed since this ETag, enabling optimistic-concurrency conflict detection for chunked uploads. /// - chunkProgressHandler: Reports per-chunk preparation progress as `(totalChunks, currentIndex)`. /// - uploadStart: Called once when upload of chunks begins, with the final list of chunks. /// - uploadTaskHandler: Exposes the low-level `URLSessionTask` for each chunk upload. @@ -210,6 +211,7 @@ public extension NextcloudKit { chunkSize: Int, account: String, options: NKRequestOptions = NKRequestOptions(), + ifMatch: String? = nil, chunkProgressHandler: @escaping (_ total: Int, _ counter: Int) -> Void = { _, _ in }, uploadStart: @escaping (_ filesChunk: [(fileName: String, size: Int64)]) -> Void = { _ in }, uploadTaskHandler: @escaping (_ task: URLSessionTask) -> Void = { _ in }, @@ -410,6 +412,14 @@ public extension NextcloudKit { let assembleTimeMax: Double = 30 * 60 // 30 minutes options.timeout = max(assembleTimeMin, min(assembleTimePerGB * assembleSizeInGB, assembleTimeMax)) + // Optimistic concurrency: guard the assembly against a concurrent change to + // the destination. The precondition must ride ONLY on the MOVE that + // materializes the final file — never on the chunk PUTs above, which target + // brand-new chunk resources and would spuriously fail with 412. + if let ifMatch { + options.customHeader?["If-Match"] = ifMatch + } + assembling() let moveRes = await moveFileOrFolderAsync(serverUrlFileNameSource: serverUrlFileNameSource, @@ -418,6 +428,11 @@ public extension NextcloudKit { account: account, options: options) + // Don't let the precondition leak onto the post-assembly PROPFIND readback: + // after a successful MOVE the destination carries a fresh etag, which would + // fail an If-Match against the pre-assembly value. + options.customHeader?["If-Match"] = nil + guard moveRes.error == .success else { return (account, nil) } From 0864958092dfe3ffbe8ebc74fb783b9642dbb097 Mon Sep 17 00:00:00 2001 From: Iva Horn Date: Mon, 20 Jul 2026 13:29:18 +0200 Subject: [PATCH 2/2] fix(upload): Derive assembled file id from the chunk-assembly MOVE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Large chunked uploads intermittently failed with errorChunkMoveFile (-9997, "Move file error") and an empty ocId, even though the file had assembled correctly on the server. The name is misleading: uploadChunkAsync did not fail on the assembly MOVE. After a successful MOVE it obtained the resulting NKFile solely from a follow-up depth-0 PROPFIND read-back, which is the only source of the ocId on the chunked path. When that read-back failed or returned no files — server-side finalization lag, a proxy 5xx, or a timeout, all more likely for large files — it threw errorChunkMoveFile and the ocId came back nil. Compounding this, readFileOrFolder applied options.timeout only on the custom-body path, so the no-body read-back silently ran with the URLSession default timeout instead of the intended value. Fix: - Prefer the assembled file's identity straight from the MOVE response headers (OC-FileID, OC-ETag/ETag, Date) via a new assembledFile(fromMoveResponseHeaders:) helper, mirroring createFolder and the desktop C++ NG client, which reads and requires these headers off the same reply. This avoids the fragile second request entirely in the common case. - Fall back to the PROPFIND read-back only when OC-FileID is absent (older server, a proxy that strips it, or a 202 async assembly), now with its own 120s timeout and a bounded backoff retry; surface errorChunkMoveFile only after the retries are exhausted. - Apply options.timeout on the no-body PROPFIND path in readFileOrFolder, matching every other WebDAV method. Adds ChunkedUploadAssemblyTests covering the header derivation. Signed-off-by: Iva Horn Co-Authored-By: Claude Opus 4.8 --- .../NextcloudKit/NextcloudKit+Upload.swift | 86 ++++++++++++-- .../NextcloudKit/NextcloudKit+WebDAV.swift | 6 +- .../ChunkedUploadAssemblyTests.swift | 107 ++++++++++++++++++ 3 files changed, 190 insertions(+), 9 deletions(-) create mode 100644 Tests/NextcloudKitUnitTests/ChunkedUploadAssemblyTests.swift diff --git a/Sources/NextcloudKit/NextcloudKit+Upload.swift b/Sources/NextcloudKit/NextcloudKit+Upload.swift index 2609f83e..da5fddf5 100644 --- a/Sources/NextcloudKit/NextcloudKit+Upload.swift +++ b/Sources/NextcloudKit/NextcloudKit+Upload.swift @@ -439,16 +439,86 @@ public extension NextcloudKit { try Task.checkCancellation() - // Read back the final file to return NKFile - let readRes = await readFileOrFolderAsync(serverUrlFileName: serverUrlFileName, - depth: "0", - account: account, - options: options) + // Prefer the assembled file's identity straight from the MOVE response headers. + // On a successful chunk-assembly MOVE the server returns OC-FileID (and OC-ETag), + // exactly as it does on MKCOL/PUT — the reference desktop client reads these off the + // same reply and treats them as required. Using them here avoids a second, fragile + // PROPFIND read-back whose failure (server-side finalization lag, a proxy 5xx, or a + // short timeout) would otherwise throw errorChunkMoveFile even though the file + // assembled correctly, losing the ocId and failing an upload whose bytes already landed. + if let file = assembledFile(fromMoveResponseHeaders: moveRes.responseData?.response?.allHeaderFields, + account: account, + fileName: destinationFileName ?? fileName, + serverUrl: serverUrl, + size: totalFileSize, + fallbackDate: date) { + return (account, file) + } - guard readRes.error == .success, let file = readRes.files?.first else { - throw NKError.errorChunkMoveFile + // Fallback: the MOVE reply carried no OC-FileID (an older server, a proxy that strips + // it, or a 202 async assembly still finishing). Read the assembled file back — but with + // its OWN timeout and a bounded retry, so a transient PROPFIND failure or brief + // post-assembly visibility lag no longer fails an upload that already succeeded. Only + // after the retries are exhausted do we surface errorChunkMoveFile. + let readbackOptions = NKRequestOptions(timeout: 120, queue: options.queue) + let readbackBackoff: [UInt64] = [0, 1_000_000_000, 3_000_000_000] // attempt after 0s, 1s, 3s + + for backoff in readbackBackoff { + if backoff > 0 { + try? await Task.sleep(nanoseconds: backoff) + } + try Task.checkCancellation() + + let readRes = await readFileOrFolderAsync(serverUrlFileName: serverUrlFileName, + depth: "0", + account: account, + options: readbackOptions) + if readRes.error == .success, let file = readRes.files?.first { + return (account, file) + } } - return (account, file) + throw NKError.errorChunkMoveFile + } + + /// Builds the assembled file's `NKFile` from a chunk-assembly MOVE response's headers. + /// + /// A Nextcloud server returns `OC-FileID` (and `OC-ETag`) on a successful assembly MOVE, + /// mirroring what it returns on MKCOL/PUT — the reference desktop client reads these off the + /// same reply. Returns `nil` when no `OC-FileID` is present (an older server, a proxy that + /// strips it, or a `202` async assembly still in progress), signalling the caller to fall + /// back to a PROPFIND read-back. + /// + /// - Parameters: + /// - headers: The MOVE response's `allHeaderFields`, if any. + /// - account: The account identifier to stamp onto the returned file. + /// - fileName: The assembled file's name (the MOVE destination's leaf). + /// - serverUrl: The server URL of the assembled file's parent directory. + /// - size: The assembled file's size in bytes (the known local total). + /// - fallbackDate: Date to use when the response carries no usable `Date` header. + /// - Returns: An `NKFile` populated from the headers, or `nil` if `OC-FileID` is absent. + func assembledFile(fromMoveResponseHeaders headers: [AnyHashable: Any]?, + account: String, + fileName: String, + serverUrl: String, + size: Int64, + fallbackDate: Date?) -> NKFile? { + guard let ocId = nkCommonInstance.findHeader("oc-fileid", allHeaderFields: headers) else { + return nil + } + let etag = nkCommonInstance.normalizedETag(nkCommonInstance.findHeader("oc-etag", allHeaderFields: headers) + ?? nkCommonInstance.findHeader("etag", allHeaderFields: headers)) ?? "" + var date = fallbackDate ?? Date() + if let dateString = nkCommonInstance.findHeader("date", allHeaderFields: headers), + let headerDate = dateString.parsedDate(using: "EEE, dd MMM y HH:mm:ss zzz") { + date = headerDate + } + return NKFile(account: account, + date: date, + etag: etag, + fileName: fileName, + ocId: ocId, + size: size, + serverUrl: serverUrl) } } diff --git a/Sources/NextcloudKit/NextcloudKit+WebDAV.swift b/Sources/NextcloudKit/NextcloudKit+WebDAV.swift index e3703725..61d7c76f 100644 --- a/Sources/NextcloudKit/NextcloudKit+WebDAV.swift +++ b/Sources/NextcloudKit/NextcloudKit+WebDAV.swift @@ -392,9 +392,13 @@ public extension NextcloudKit { do { try urlRequest = URLRequest(url: url, method: method, headers: headers) + // Apply the caller's timeout on every path, not only when a custom requestBody is + // supplied. The default-body PROPFIND (requestBody == nil) previously fell back to + // the URLSession default timeout, silently ignoring options.timeout — every other + // WebDAV method here sets timeoutInterval unconditionally. + urlRequest.timeoutInterval = options.timeout if let requestBody { urlRequest.httpBody = requestBody - urlRequest.timeoutInterval = options.timeout } else { urlRequest.httpBody = NKDataFileXML(nkCommonInstance: self.nkCommonInstance).getRequestBodyFile(createProperties: options.createProperties, removeProperties: options.removeProperties).data(using: .utf8) } diff --git a/Tests/NextcloudKitUnitTests/ChunkedUploadAssemblyTests.swift b/Tests/NextcloudKitUnitTests/ChunkedUploadAssemblyTests.swift new file mode 100644 index 00000000..f409c915 --- /dev/null +++ b/Tests/NextcloudKitUnitTests/ChunkedUploadAssemblyTests.swift @@ -0,0 +1,107 @@ +// SPDX-FileCopyrightText: Nextcloud GmbH +// SPDX-License-Identifier: GPL-3.0-or-later + +import XCTest +@testable import NextcloudKit + +/// Unit tests for deriving the assembled file's `NKFile` from a chunk-assembly MOVE response's +/// headers — the primary path that lets `uploadChunkAsync` skip the fragile PROPFIND read-back +/// (whose failure otherwise surfaced as `errorChunkMoveFile`, code -9997). +final class ChunkedUploadAssemblyTests: XCTestCase { + private func makeKit() -> NextcloudKit { + #if swift(<6.0) + return NextcloudKit.shared + #else + return NextcloudKit() + #endif + } + + func test_assembledFile_withOCFileId_derivesNKFileFromHeaders() { + let kit = makeKit() + let headers: [AnyHashable: Any] = [ + "OC-FileID": "00000123oc9wxyzinstance", + "OC-ETag": "\"abc123\"", + "Date": "Wed, 07 Jun 2026 09:04:51 GMT" + ] + + let file = kit.assembledFile(fromMoveResponseHeaders: headers, + account: "user https://cloud.example.com", + fileName: "1.1Gb.mp4", + serverUrl: "https://cloud.example.com/remote.php/dav/files/user", + size: 1_181_116_006, + fallbackDate: nil) + + XCTAssertNotNil(file, "OC-FileID present should yield an NKFile, not nil") + XCTAssertEqual(file?.ocId, "00000123oc9wxyzinstance") + XCTAssertEqual(file?.etag, "abc123", "normalizedETag should strip the surrounding quotes") + XCTAssertEqual(file?.size, 1_181_116_006) + XCTAssertEqual(file?.fileName, "1.1Gb.mp4") + XCTAssertEqual(file?.account, "user https://cloud.example.com") + } + + func test_assembledFile_isCaseInsensitiveOnHeaderNames() { + let kit = makeKit() + // Lower-cased header keys must resolve the same as canonical casing. + let headers: [AnyHashable: Any] = ["oc-fileid": "fid", "oc-etag": "\"e\""] + + let file = kit.assembledFile(fromMoveResponseHeaders: headers, + account: "a", fileName: "f", serverUrl: "s", size: 1, fallbackDate: nil) + + XCTAssertEqual(file?.ocId, "fid") + XCTAssertEqual(file?.etag, "e") + } + + func test_assembledFile_prefersOCETagOverPlainETag() { + let kit = makeKit() + let headers: [AnyHashable: Any] = [ + "OC-FileID": "fid", + "OC-ETag": "\"oc-etag-value\"", + "ETag": "\"plain-etag-value\"" + ] + + let file = kit.assembledFile(fromMoveResponseHeaders: headers, + account: "a", fileName: "f", serverUrl: "s", size: 1, fallbackDate: nil) + + XCTAssertEqual(file?.etag, "oc-etag-value", "OC-ETag should win over the standard ETag") + } + + func test_assembledFile_fallsBackToPlainETagWhenNoOCETag() { + let kit = makeKit() + let headers: [AnyHashable: Any] = ["OC-FileID": "fid", "ETag": "\"plain\""] + + let file = kit.assembledFile(fromMoveResponseHeaders: headers, + account: "a", fileName: "f", serverUrl: "s", size: 1, fallbackDate: nil) + + XCTAssertEqual(file?.etag, "plain") + } + + func test_assembledFile_withoutOCFileId_returnsNilToTriggerReadbackFallback() { + let kit = makeKit() + let headers: [AnyHashable: Any] = ["ETag": "\"x\""] // no OC-FileID (e.g. 202 async assembly) + + let file = kit.assembledFile(fromMoveResponseHeaders: headers, + account: "a", fileName: "f", serverUrl: "s", size: 1, fallbackDate: nil) + + XCTAssertNil(file, "Absent OC-FileID must return nil so the caller falls back to a read-back") + } + + func test_assembledFile_withNilHeaders_returnsNil() { + let kit = makeKit() + + let file = kit.assembledFile(fromMoveResponseHeaders: nil, + account: "a", fileName: "f", serverUrl: "s", size: 1, fallbackDate: nil) + + XCTAssertNil(file) + } + + func test_assembledFile_usesFallbackDateWhenNoDateHeader() { + let kit = makeKit() + let fallback = Date(timeIntervalSince1970: 1_000_000) + let headers: [AnyHashable: Any] = ["OC-FileID": "fid"] // no Date header + + let file = kit.assembledFile(fromMoveResponseHeaders: headers, + account: "a", fileName: "f", serverUrl: "s", size: 1, fallbackDate: fallback) + + XCTAssertEqual(file?.date, fallback) + } +}