diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index 1197695a892f7..e8997abf9e194 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -447,6 +447,52 @@ public final class FilesDatabaseManager: Sendable { } } + /// + /// Add or replace `metadata` while carrying over local-only fields the + /// server payload cannot know about: ``keepDownloaded``, ``downloaded``, + /// ``visitedDirectory``, and ``lockToken``. + /// + /// Mirrors the preservation set applied by + /// ``processItemMetadatasToUpdate`` for non-paginated reads. Use this from + /// any code path that ingests fresh PROPFIND results (e.g. paginated + /// enumeration); plain ``addItemMetadata(_:)`` would otherwise overwrite + /// these fields back to their defaults via Realm's `update: .all`, + /// silently undoing user-visible state such as "Always keep downloaded" + /// (#9923). + /// + /// Returns the merged metadata that was persisted. Callers that report + /// items back to the file-provider framework MUST forward the returned + /// value rather than the input — otherwise the framework receives the + /// pre-merge defaults and renders the item as if the local-only state + /// (e.g. pinned-via-keep-downloaded) had been cleared. + /// + /// - Parameters: + /// - metadata: The freshly-built metadata to persist. + /// - preserveVisitedDirectory: When `false`, do not carry over + /// ``visitedDirectory`` from the existing row. Callers that have just + /// visited the directory in the current request should pass `false` + /// and pre-set `metadata.visitedDirectory = true`, so the visit is + /// recorded rather than overwritten by a stale `false` from the DB. + /// + @discardableResult + public func addItemMetadataPreservingLocalState(_ metadata: SendableItemMetadata, preserveVisitedDirectory: Bool = true) -> SendableItemMetadata { + var toWrite = metadata + + if let existing = itemMetadatas.where({ $0.ocId == metadata.ocId }).first { + toWrite.downloaded = existing.downloaded + toWrite.keepDownloaded = existing.keepDownloaded + + if preserveVisitedDirectory { + toWrite.visitedDirectory = existing.visitedDirectory + } + + toWrite.lockToken = existing.lockToken + } + + addItemMetadata(toWrite) + return toWrite + } + /// /// Mark an item as deleted. /// diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+SyncEngine.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+SyncEngine.swift index a165fdd713320..283dd238a599d 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+SyncEngine.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+SyncEngine.swift @@ -15,17 +15,35 @@ extension Enumerator { if pageIndex == 0 { guard let firstFile = files.first else { return (nil, .invalidResponseError) } var metadata = firstFile.toItemMetadata() + if metadata.directory { + // Mark the directory as visited in this request, then persist + // while preserving the rest of the local-only state. Skip + // preservation of `visitedDirectory` so the just-set `true` + // is not overwritten by the existing row's value. metadata.visitedDirectory = true - if let existingMetadata = dbManager.itemMetadata(ocId: metadata.ocId) { - metadata.downloaded = existingMetadata.downloaded - metadata.keepDownloaded = existingMetadata.keepDownloaded - } + dbManager.addItemMetadataPreservingLocalState(metadata, preserveVisitedDirectory: false) + } else { + dbManager.addItemMetadataPreservingLocalState(metadata) } - dbManager.addItemMetadata(metadata) } - let metadatas = files[startIndex...].map { $0.toItemMetadata() } - metadatas.forEach { dbManager.addItemMetadata($0) } + + // Persist children — including those on follow-up pages — while + // carrying over any local-only state previously set on existing rows + // (e.g. `keepDownloaded` from a recursive "Always keep downloaded" + // enable). A plain `addItemMetadata` would clobber those flags via + // Realm's `update: .all`, which is the root cause of #9923. + // + // Use the merged metadata returned by the helper for both the DB + // write and the value reported back to the framework. Returning the + // pre-merge fresh-from-server metadata would tell the framework + // `keepDownloaded == false` for items that are pinned in the + // database, leaving the OS view (`isKeepDownloaded`, `contentPolicy`) + // out of sync with the local truth. + let metadatas = files[startIndex...].map { file -> SendableItemMetadata in + dbManager.addItemMetadataPreservingLocalState(file.toItemMetadata()) + } + return (metadatas, nil) } diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift index d24c0d18211a6..eede92c686c5b 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift @@ -472,6 +472,145 @@ final class EnumeratorTests: NextcloudFileProviderKitTestCase { ) } + /// + /// Children persisted by a page-0 paginated read must keep their existing + /// ``keepDownloaded`` flag. Before the fix for #9923 the bulk write at the + /// end of ``Enumerator.handlePagedReadResults`` used ``addItemMetadata`` + /// which replaces the entire row, silently clearing flags set by a prior + /// "Always keep downloaded" enable. + /// + /// The metadatas returned to the enumeration observer must also reflect + /// the preserved state: the framework builds `Item.contentPolicy` from + /// `metadata.keepDownloaded` on whatever metadata we hand it, so reporting + /// the pre-merge fresh-from-server metadata would leave the OS view of + /// pinned descendants stuck on `.inherited` even though the database row + /// is correct. (Symptom observed in #9923 for `/Documents/Documenter`.) + /// + func testHandlePagedReadResultsPreservesKeepDownloadedOnPageZeroChildren() { + let dbManager = Self.dbManager + debugPrint(dbManager.ncDatabase()) + + // Seed two of the upcoming page-0 children as already pinned. The + // third stays unseeded so we can also assert the flag does not leak + // onto items that never had it. + let pinnedChildren = ["pagedChild0", "pagedChild2"] + for ocId in pinnedChildren { + var seeded = SendableItemMetadata(ocId: ocId, fileName: "\(ocId).txt", account: Self.account) + seeded.keepDownloaded = true + dbManager.addItemMetadata(seeded) + } + + let parentNKFile = remoteFolder.toNKFile() + let childrenNKFiles = (0 ..< 3).map { i in + MockRemoteItem( + identifier: "pagedChild\(i)", + name: "pagedChild\(i).txt", + remotePath: Self.account.davFilesUrl + "/folder/pagedChild\(i).txt", + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ).toNKFile() + } + + let (returnedMetadatas, error) = Enumerator.handlePagedReadResults( + files: [parentNKFile] + childrenNKFiles, pageIndex: 0, dbManager: dbManager + ) + XCTAssertNil(error) + + for ocId in pinnedChildren { + let stored = dbManager.itemMetadata(ocId: ocId) + XCTAssertEqual( + stored?.keepDownloaded, true, + "Page-0 child \(ocId) must retain keepDownloaded across the paginated write (#9923)." + ) + let returned = returnedMetadatas?.first(where: { $0.ocId == ocId }) + XCTAssertEqual( + returned?.keepDownloaded, true, + "Page-0 child \(ocId) reported back to the enumeration observer must also reflect the preserved keepDownloaded — the framework builds contentPolicy from this value." + ) + } + XCTAssertEqual( + dbManager.itemMetadata(ocId: "pagedChild1")?.keepDownloaded, false, + "Children that were never pinned must not gain the flag from the paginated write." + ) + } + + /// + /// Same protection on follow-up pages: a paginated read with `pageIndex > 0` + /// must not clobber ``keepDownloaded`` on any of its items. This is the + /// path most likely to land on already-pinned descendants because the + /// recursive enable of "Always keep downloaded" runs without pagination + /// while the OS-driven enumeration that follows uses pagination. + /// + func testHandlePagedReadResultsPreservesKeepDownloadedOnFollowUpPage() { + let dbManager = Self.dbManager + debugPrint(dbManager.ncDatabase()) + + var seeded = SendableItemMetadata(ocId: "pagedChild5", fileName: "pagedChild5.txt", account: Self.account) + seeded.keepDownloaded = true + dbManager.addItemMetadata(seeded) + + let followUpChildrenNKFiles = (5 ..< 8).map { i in + MockRemoteItem( + identifier: "pagedChild\(i)", + name: "pagedChild\(i).txt", + remotePath: Self.account.davFilesUrl + "/folder/pagedChild\(i).txt", + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ).toNKFile() + } + + let (returnedMetadatas, error) = Enumerator.handlePagedReadResults( + files: followUpChildrenNKFiles, pageIndex: 1, dbManager: dbManager + ) + XCTAssertNil(error) + + XCTAssertEqual( + dbManager.itemMetadata(ocId: "pagedChild5")?.keepDownloaded, true, + "Follow-up page child must retain keepDownloaded across the paginated write (#9923)." + ) + XCTAssertEqual( + returnedMetadatas?.first(where: { $0.ocId == "pagedChild5" })?.keepDownloaded, true, + "Follow-up page child reported back to the enumeration observer must also reflect the preserved keepDownloaded." + ) + } + + /// + /// The page-0 target directory must continue to be marked as visited + /// after the preservation refactor — the original inline path applied + /// `visitedDirectory = true` unconditionally, and that behaviour must + /// survive even when a previous DB row carries `visitedDirectory = false`. + /// Previously-set ``keepDownloaded`` on the directory must also survive. + /// + func testHandlePagedReadResultsTargetDirectoryRecordsVisitAndKeepsKeepDownloaded() { + let dbManager = Self.dbManager + debugPrint(dbManager.ncDatabase()) + + var seeded = remoteFolder.toItemMetadata(account: Self.account) + seeded.keepDownloaded = true + seeded.visitedDirectory = false + dbManager.addItemMetadata(seeded) + + let parentNKFile = remoteFolder.toNKFile() + let (_, error) = Enumerator.handlePagedReadResults( + files: [parentNKFile], pageIndex: 0, dbManager: dbManager + ) + XCTAssertNil(error) + + let stored = dbManager.itemMetadata(ocId: parentNKFile.ocId) + XCTAssertEqual( + stored?.visitedDirectory, true, + "Page-0 target directory must be flagged as visited by the current request." + ) + XCTAssertEqual( + stored?.keepDownloaded, true, + "Page-0 target directory must retain its existing keepDownloaded flag." + ) + } + func testWorkingSetEnumerateChanges() async throws { // This test verifies that `enumerateChanges` for the working set correctly // queries the local database for changes since the provided sync anchor date. diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/KeepDownloadedRecursiveTests.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/KeepDownloadedRecursiveTests.swift index cf370d871bdc7..7bf419b2c295a 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/KeepDownloadedRecursiveTests.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/KeepDownloadedRecursiveTests.swift @@ -455,4 +455,90 @@ final class KeepDownloadedRecursiveTests: NextcloudFileProviderKitTestCase { "A sibling outside the pinned folder must not inherit keepDownloaded." ) } + + /// + /// End-to-end regression for #9923: a paginated read that lands on + /// already-pinned descendants must not silently clear their flag. Before + /// the fix, the bulk write at the end of + /// ``Enumerator.handlePagedReadResults`` used ``addItemMetadata`` which + /// replaces rows wholesale via Realm's `update: .all`, dropping every + /// local-only field — including ``keepDownloaded``. The user's pin walk + /// runs without pagination, but the OS-driven `enumerateItems` that + /// follows uses pagination, so a recently-pinned subtree was reliably + /// undone. + /// + func testPaginatedReadDoesNotClobberRecursivelyPinnedDescendants() throws { + // Pin the seeded tree the way `Item.set(keepDownloaded:domain:)` does. + let folderMetadata = try seedTree() + _ = try Self.dbManager.set(keepDownloaded: true, for: folderMetadata) + for child in Self.dbManager.childItems(directoryMetadata: folderMetadata) { + _ = try Self.dbManager.set(keepDownloaded: true, for: child) + } + + // Build NKFiles with ocIds matching the seeded rows so the + // preservation lookup hits. The `MockRemoteItem` parent chain + // provides the right `serverUrl` on each `NKFile`. + let parent = MockRemoteItem( + identifier: "files-parent", + name: "files", + remotePath: "https://cloud.example.com/files", + directory: true, + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + let folderMock = MockRemoteItem( + identifier: "folder-1", + name: "documents", + remotePath: "https://cloud.example.com/files/documents", + directory: true, + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + folderMock.parent = parent + let directChildMock = MockRemoteItem( + identifier: "direct-child-file", + name: "report.pdf", + remotePath: "https://cloud.example.com/files/documents/report.pdf", + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + directChildMock.parent = folderMock + let subfolderMock = MockRemoteItem( + identifier: "subfolder-1", + name: "nested", + remotePath: "https://cloud.example.com/files/documents/nested", + directory: true, + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + subfolderMock.parent = folderMock + + // Simulate the page-0 paginated PROPFIND response Finder would issue + // for the pinned folder. + let (_, error) = Enumerator.handlePagedReadResults( + files: [folderMock.toNKFile(), directChildMock.toNKFile(), subfolderMock.toNKFile()], + pageIndex: 0, + dbManager: Self.dbManager + ) + XCTAssertNil(error) + + // Both directly re-enumerated descendants and the deeper file + // (untouched by this PROPFIND but pinned in step 1) must remain + // pinned after the paginated overwrite. + for ocId in ["folder-1", "direct-child-file", "subfolder-1", "deep-file"] { + let stored = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: ocId)) + XCTAssertTrue( + stored.keepDownloaded, + "\(ocId) must remain pinned after a paginated read overwrites the row (#9923)." + ) + } + } }