From ce40c937013e947f7ac87652f1bde2179903ba1e Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 17 Jun 2025 18:49:15 +0800 Subject: [PATCH 01/42] Add syncTime and deleted properties to item metadata Signed-off-by: Claudio Cambra --- .../NextcloudFileProviderKit/Metadata/ItemMetadata.swift | 2 ++ .../Metadata/RealmItemMetadata.swift | 4 ++++ .../Metadata/SendableItemMetadata.swift | 8 ++++++++ 3 files changed, 14 insertions(+) diff --git a/Sources/NextcloudFileProviderKit/Metadata/ItemMetadata.swift b/Sources/NextcloudFileProviderKit/Metadata/ItemMetadata.swift index d7e3d58d..2a1f59fb 100644 --- a/Sources/NextcloudFileProviderKit/Metadata/ItemMetadata.swift +++ b/Sources/NextcloudFileProviderKit/Metadata/ItemMetadata.swift @@ -48,6 +48,8 @@ public protocol ItemMetadata: Equatable { var creationDate: Date { get set } var dataFingerprint: String { get set } var date: Date { get set } + var syncTime: Date { get set } + var deleted: Bool { get set } var directory: Bool { get set } var downloadURL: String { get set } var e2eEncrypted: Bool { get set } diff --git a/Sources/NextcloudFileProviderKit/Metadata/RealmItemMetadata.swift b/Sources/NextcloudFileProviderKit/Metadata/RealmItemMetadata.swift index 748d0334..3ffd833e 100644 --- a/Sources/NextcloudFileProviderKit/Metadata/RealmItemMetadata.swift +++ b/Sources/NextcloudFileProviderKit/Metadata/RealmItemMetadata.swift @@ -19,6 +19,8 @@ internal class RealmItemMetadata: Object, ItemMetadata { @Persisted public var creationDate = Date() @Persisted public var dataFingerprint = "" @Persisted public var date = Date() + @Persisted public var syncTime = Date() + @Persisted public var deleted = false @Persisted public var directory: Bool = false @Persisted public var downloadURL = "" @Persisted public var e2eEncrypted: Bool = false @@ -116,6 +118,8 @@ internal class RealmItemMetadata: Object, ItemMetadata { self.creationDate = value.creationDate self.dataFingerprint = value.dataFingerprint self.date = value.date + self.syncTime = value.syncTime + self.deleted = value.deleted self.directory = value.directory self.downloadURL = value.downloadURL self.e2eEncrypted = value.e2eEncrypted diff --git a/Sources/NextcloudFileProviderKit/Metadata/SendableItemMetadata.swift b/Sources/NextcloudFileProviderKit/Metadata/SendableItemMetadata.swift index adf27f11..36a21e3b 100644 --- a/Sources/NextcloudFileProviderKit/Metadata/SendableItemMetadata.swift +++ b/Sources/NextcloudFileProviderKit/Metadata/SendableItemMetadata.swift @@ -20,6 +20,8 @@ public struct SendableItemMetadata: ItemMetadata, Sendable { public var creationDate: Date public var dataFingerprint: String public var date: Date + public var syncTime: Date + public var deleted: Bool public var directory: Bool public var downloadURL: String public var e2eEncrypted: Bool @@ -84,6 +86,8 @@ public struct SendableItemMetadata: ItemMetadata, Sendable { creationDate: Date, dataFingerprint: String = "", date: Date = Date(), + syncTime: Date = Date(), + deleted: Bool = false, directory: Bool, downloadURL: String = "", e2eEncrypted: Bool, @@ -147,6 +151,8 @@ public struct SendableItemMetadata: ItemMetadata, Sendable { self.creationDate = creationDate self.dataFingerprint = dataFingerprint self.date = date + self.syncTime = syncTime + self.deleted = deleted self.directory = directory self.downloadURL = downloadURL self.e2eEncrypted = e2eEncrypted @@ -212,6 +218,8 @@ public struct SendableItemMetadata: ItemMetadata, Sendable { self.creationDate = value.creationDate self.dataFingerprint = value.dataFingerprint self.date = value.date + self.syncTime = value.syncTime + self.deleted = value.deleted self.directory = value.directory self.downloadURL = value.downloadURL self.e2eEncrypted = value.e2eEncrypted From f3d2e69e35d3cdff1f644132b4a2b1bb2eb5723c Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 17 Jun 2025 18:50:26 +0800 Subject: [PATCH 02/42] Add method to get items modified since a given date in files database manager Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager.swift | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index a5932357..a575f1d9 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -652,12 +652,25 @@ public final class FilesDatabaseManager: Sendable { return NSFileProviderItemIdentifier(parentMetadata.ocId) } - public func materialisedItemMetadatas(account: String) -> [SendableItemMetadata] { + private func managedMaterialisedItemMetadatas(account: String) -> Results { itemMetadatas .where { $0.account == account && (($0.directory && $0.visitedDirectory) || (!$0.directory && $0.downloaded)) } - .toUnmanagedResults() + } + + public func materialisedItemMetadatas(account: String) -> [SendableItemMetadata] { + managedMaterialisedItemMetadatas(account: account).toUnmanagedResults() + } + + public func pendingWorkingSetChanges( + account: Account, since date: Date + ) -> (updated: [SendableItemMetadata], deleted: [SendableItemMetadata]) { + let accId = account.ncKitAccount + let pending = managedMaterialisedItemMetadatas(account: accId).where { $0.syncTime > date } + let updated = pending.where { !$0.deleted }.toUnmanagedResults() + let deleted = pending.where { $0.deleted }.toUnmanagedResults() + return (updated, deleted) } } From 69f6fa79f06f99b2a940f87aec7983440b38c410 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 17 Jun 2025 18:55:19 +0800 Subject: [PATCH 03/42] Add test for database pending working set changes database method Signed-off-by: Claudio Cambra --- .../FilesDatabaseManagerTests.swift | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift b/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift index 012ef09b..c1094aff 100644 --- a/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift +++ b/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift @@ -1197,4 +1197,84 @@ final class FilesDatabaseManagerTests: XCTestCase { XCTAssertTrue(materialisedOcIds.contains(sItemC.ocId)) XCTAssertTrue(materialisedOcIds.contains(sDirD.ocId)) } + + func testPendingWorkingSetChanges() { + // 1. Arrange + let fiveMinutesAgo = Date().addingTimeInterval(-300) + let tenMinutesAgo = Date().addingTimeInterval(-600) + let now = Date() + + // --- Items to populate the database --- + + // Item 1: A materialised item synced too long ago. Should NOT be returned. + var itemTooOld = + SendableItemMetadata(ocId: "tooOld", fileName: "old.txt", account: Self.account) + itemTooOld.downloaded = true + itemTooOld.syncTime = tenMinutesAgo + Self.dbManager.addItemMetadata(itemTooOld) + + // Item 2: A materialised item synced recently. Should be returned in the 'updated' list. + var itemRecentlyUpdated = + SendableItemMetadata(ocId: "updated", fileName: "updated.txt", account: Self.account) + itemRecentlyUpdated.downloaded = true + itemRecentlyUpdated.deleted = false + itemRecentlyUpdated.syncTime = now + Self.dbManager.addItemMetadata(itemRecentlyUpdated) + + // Item 3: A materialised item synced recently but marked as deleted. + // Should be returned in the 'deleted' list. + var itemRecentlyDeleted = + SendableItemMetadata(ocId: "deleted", fileName: "deleted.txt", account: Self.account) + itemRecentlyDeleted.downloaded = true + itemRecentlyDeleted.deleted = true + itemRecentlyDeleted.syncTime = now + Self.dbManager.addItemMetadata(itemRecentlyDeleted) + + // Item 4: A non-materialised item synced recently. Should NOT be returned. + var itemNotMaterialised = SendableItemMetadata( + ocId: "notMaterialised", fileName: "notMaterialised.txt", account: Self.account + ) + itemNotMaterialised.downloaded = false // Not part of the working set + itemNotMaterialised.syncTime = now + Self.dbManager.addItemMetadata(itemNotMaterialised) + + // Item 5: A materialised directory synced recently. Should be returned in 'updated'. + var dirRecentlyUpdated = + SendableItemMetadata(ocId: "updatedDir", fileName: "updatedDir", account: Self.account) + dirRecentlyUpdated.directory = true + dirRecentlyUpdated.visitedDirectory = true // Materialised as a directory + dirRecentlyUpdated.deleted = false + dirRecentlyUpdated.syncTime = now + Self.dbManager.addItemMetadata(dirRecentlyUpdated) + + // 2. Act + // Fetch changes that have occurred since five minutes ago. + let result = Self.dbManager.pendingWorkingSetChanges( + account: Self.account, since: fiveMinutesAgo + ) + + // 3. Assert + // Verify the 'updated' results + XCTAssertEqual( + result.updated.count, 2, "There should be two updated items (one file, one directory)." + ) + let updatedIds = Set(result.updated.map { $0.ocId }) + print(updatedIds) + XCTAssertTrue( + updatedIds.contains("updated"), + "The recently updated file should be in the updated list." + ) + XCTAssertTrue( + updatedIds.contains("updatedDir"), + "The recently updated directory should be in the updated list." + ) + + // Verify the 'deleted' results + XCTAssertEqual(result.deleted.count, 1, "There should be one deleted item.") + XCTAssertEqual( + result.deleted.first?.ocId, + "deleted", + "The recently deleted file should be in the deleted list." + ) + } } From 9e1d7bfc5cd57e26de00ada1039636b24aeabac3 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 1 Jul 2025 10:24:31 +0800 Subject: [PATCH 04/42] Return all modified items since sync anchor date for changes enumeration Signed-off-by: Claudio Cambra --- .../Enumeration/Enumerator.swift | 137 +++--------------- 1 file changed, 21 insertions(+), 116 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift index 13ebafe5..91670b35 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift @@ -25,8 +25,8 @@ public class Enumerator: NSObject, NSFileProviderEnumerator { let domain: NSFileProviderDomain? let dbManager: FilesDatabaseManager - // TODO: actually use this in NCKit and server requests - private let anchor = NSFileProviderSyncAnchor(Date().description.data(using: .utf8)!) + private let anchor = + NSFileProviderSyncAnchor(ISO8601DateFormatter().string(from: Date()).data(using: .utf8)!) private let pageItemCount: Int static let logger = Logger(subsystem: Logger.subsystem, category: "enumerator") let account: Account @@ -336,121 +336,26 @@ public class Enumerator: NSObject, NSFileProviderEnumerator { "Enumerating working set changes for \(self.account.ncKitAccount, privacy: .public)" ) - // Unlike when enumerating items we can't progressively enumerate items as we need to - // wait to see which items are truly deleted and which have just been moved elsewhere. - Task { - let ncKitAccount = account.ncKitAccount - // Visited folders and downloaded files. Sort in terms of their remote URLs. - // This way we ensure we visit parent folders before their children. - let materialisedItems = dbManager - .materialisedItemMetadatas(account: ncKitAccount) - .sorted { - ($0.serverUrl + "/" + $0.fileName).count < - ($1.serverUrl + "/" + $1.fileName).count - } - - - var allNewMetadatas = [SendableItemMetadata]() - var allUpdatedMetadatas = [SendableItemMetadata]() - var allDeletedMetadatas = [SendableItemMetadata]() - var examinedItemIds = Set() - for item in materialisedItems where !examinedItemIds.contains(item.ocId) { - let itemRemoteUrl = item.serverUrl + "/" + item.fileName - let ( - metadatas, newMetadatas, updatedMetadatas, deletedMetadatas, _, readError - ) = await Self.readServerUrl( - itemRemoteUrl, - account: account, - remoteInterface: remoteInterface, - dbManager: dbManager, - depth: item.directory ? .targetAndDirectChildren : .target - ) - if readError?.errorCode == 404 { - allDeletedMetadatas.append(item) - examinedItemIds.insert(item.ocId) - } else if let readError, readError != .success { - Self.logger.info( - """ - Finished change enumeration of working set for user: - \(self.account.ncKitAccount, privacy: .public) - with error: \(readError.errorDescription, privacy: .public) - """ - ) - let fpError = readError.fileProviderError( - handlingNoSuchItemErrorUsingItemIdentifier: self.enumeratedItemIdentifier - ) ?? NSFileProviderError(.cannotSynchronize) - observer.finishEnumeratingWithError(fpError) - } else { - allDeletedMetadatas += deletedMetadatas ?? [] - allUpdatedMetadatas += updatedMetadatas ?? [] - allNewMetadatas += newMetadatas ?? [] - - // Just because we have read child directories metadata doesn't mean we need - // to in turn scan their children. This is not the case for files - var examinedChildFilesAndDeletedItems = Set() - if let metadatas, let target = metadatas.first { - examinedItemIds.insert(target.ocId) - - if metadatas.count > 1 { - examinedChildFilesAndDeletedItems.formUnion( - metadatas[1...].filter { !$0.directory }.map(\.ocId) - ) - } - - // If the target is not in the updated metadatas then neither it, nor - // any of its kids have changed. So skip examining all of them - if !allUpdatedMetadatas.contains(where: { $0.ocId == target.ocId }) { - let materialisedChildren = materialisedItems.filter { - $0.serverUrl.hasPrefix(itemRemoteUrl) - }.map(\.ocId) - examinedChildFilesAndDeletedItems.formUnion(materialisedChildren) - } - - if let deletedMetadataOcIds = deletedMetadatas?.map(\.ocId) { - examinedChildFilesAndDeletedItems.formUnion(deletedMetadataOcIds) - } - } - - examinedItemIds.formUnion(examinedChildFilesAndDeletedItems) - } - } - - // Run a check to ensure files deleted in one location are not updated in another - // (e.g. when moved) - // The recursive scan provides us with updated/deleted metadatas only on a folder by - // folder basis; so we need to check we are not simultaneously marking a moved file as - // deleted and updated - var checkedDeletedMetadatas = allDeletedMetadatas - - for updatedMetadata in allUpdatedMetadatas { - guard let matchingDeletedMetadataIdx = checkedDeletedMetadatas.firstIndex( - where: { $0.ocId == updatedMetadata.ocId } - ) else { continue } - checkedDeletedMetadatas.remove(at: matchingDeletedMetadataIdx) - } - - allDeletedMetadatas = checkedDeletedMetadatas - - Self.logger.info( - """ - Finished change enumeration of working set for user: - \(self.account.ncKitAccount, privacy: .public). - Enumerating items. - """ - ) - - Self.completeChangesObserver( - observer, - anchor: anchor, - enumeratedItemIdentifier: self.enumeratedItemIdentifier, - account: account, - remoteInterface: remoteInterface, - dbManager: dbManager, - newMetadatas: allNewMetadatas, - updatedMetadatas: allUpdatedMetadatas, - deletedMetadatas: allDeletedMetadatas - ) + let formatter = ISO8601DateFormatter() + guard let anchorDateString = String(data: anchor.rawValue, encoding: .utf8), + let date = formatter.date(from: anchorDateString) + else { + Self.logger.error("Couldn't parse sync anchor \(anchor.rawValue, privacy: .public)") + observer.finishEnumeratingWithError(NSFileProviderError(.syncAnchorExpired)) + return } + let pendingChanges = dbManager.pendingWorkingSetChanges(account: account, since: date) + Self.completeChangesObserver( + observer, + anchor: anchor, + enumeratedItemIdentifier: enumeratedItemIdentifier, + account: account, + remoteInterface: remoteInterface, + dbManager: dbManager, + newMetadatas: [], + updatedMetadatas: pendingChanges.updated, + deletedMetadatas: pendingChanges.deleted + ) return } else if enumeratedItemIdentifier == .trashContainer { Self.logger.debug( From b630c034b66168c96d24deb4161797dba3e68daf Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 1 Jul 2025 10:30:09 +0800 Subject: [PATCH 05/42] Do not actually delete deleted metadatas We will run clean up later after enumerating working set changes Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index a575f1d9..ba4a35f9 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -416,7 +416,7 @@ public final class FilesDatabaseManager: Sendable { } try database.write { - database.delete(metadatasToDelete) + // Do not delete the metadatas that have been deleted database.add(metadatasToUpdate.map { RealmItemMetadata(value: $0) }, update: .modified) database.add(metadatasToCreate.map { RealmItemMetadata(value: $0) }, update: .all) } @@ -513,6 +513,7 @@ public final class FilesDatabaseManager: Sendable { downloaded: \(metadata.downloaded, privacy: .public) uploaded: \(metadata.uploaded, privacy: .public) visitedDirectory: \(metadata.visitedDirectory, privacy: .public) + deleted: \(metadata.deleted, privacy: .public) """ ) } From 7112baac95f1ab00b968a72b6b465fec1d5872f8 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 1 Jul 2025 10:45:30 +0800 Subject: [PATCH 06/42] Use simpler syntax for going to last array index Signed-off-by: Claudio Cambra --- .../Enumeration/Enumerator+SyncEngine.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+SyncEngine.swift b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+SyncEngine.swift index 56c6921d..c10caf09 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+SyncEngine.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+SyncEngine.swift @@ -35,7 +35,7 @@ extension Enumerator { } dbManager.addItemMetadata(metadata) } - let metadatas = files[startIndex.. Date: Tue, 1 Jul 2025 12:50:36 +0800 Subject: [PATCH 07/42] During depth 1 read handling, mark deleted files as deleted Signed-off-by: Claudio Cambra --- .../NextcloudFileProviderKit/Database/FilesDatabaseManager.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index ba4a35f9..26fa1a37 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -417,6 +417,7 @@ public final class FilesDatabaseManager: Sendable { try database.write { // Do not delete the metadatas that have been deleted + metadatasToDelete.forEach { $0.deleted = true } database.add(metadatasToUpdate.map { RealmItemMetadata(value: $0) }, update: .modified) database.add(metadatasToCreate.map { RealmItemMetadata(value: $0) }, update: .all) } From 1d2ae0239d6cf48287a8a4a5606125b933f6dc2e Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 1 Jul 2025 12:51:11 +0800 Subject: [PATCH 08/42] Fix changed folder enumerator test Signed-off-by: Claudio Cambra --- Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift b/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift index 1dc378fd..fe1234eb 100644 --- a/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift +++ b/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift @@ -794,10 +794,13 @@ final class EnumeratorTests: XCTestCase { let dbItemAMetadata = try XCTUnwrap( Self.dbManager.itemMetadata(ocId: remoteItemA.identifier) ) + let dbItemBMetadata = try XCTUnwrap( + Self.dbManager.itemMetadata(ocId: remoteItemB.identifier) + ) let dbItemCMetadata = try XCTUnwrap( Self.dbManager.itemMetadata(ocId: remoteItemC.identifier) ) - XCTAssertNil(Self.dbManager.itemMetadata(ocId: remoteItemB.identifier)) + XCTAssertTrue(dbItemBMetadata.deleted) XCTAssertEqual(dbFolderMetadata.etag, remoteFolder.versionIdentifier) XCTAssertTrue(dbFolderMetadata.downloaded) XCTAssertEqual(dbItemAMetadata.etag, remoteItemA.versionIdentifier) From 406deb24647a087c57f51cfc6254c96b7abb435d Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 1 Jul 2025 13:11:23 +0800 Subject: [PATCH 09/42] Adapt FIlesDatabaseManagerTests to deleted item changes Signed-off-by: Claudio Cambra --- .../FilesDatabaseManagerTests.swift | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift b/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift index c1094aff..85f6003a 100644 --- a/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift +++ b/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift @@ -344,9 +344,8 @@ final class FilesDatabaseManagerTests: XCTestCase { let remainingMetadatas = Self.dbManager.itemMetadatas( account: "TestAccount", underServerUrl: "https://example.com" ) - XCTAssertEqual( - remainingMetadatas.count, 2, "Should have two remaining metadata after update" - ) + XCTAssertEqual(remainingMetadatas.filter { $0.deleted }.count, 1) + XCTAssertEqual(remainingMetadatas.filter { !$0.deleted }.count, 2) XCTAssertNotNil(remainingMetadatas.first { $0.ocId == "id-1" }) XCTAssertNotNil(remainingMetadatas.first { $0.ocId == "id-3" }) @@ -472,9 +471,8 @@ final class FilesDatabaseManagerTests: XCTestCase { // Check the actual database state after the write transaction - XCTAssertEqual( // The root item and the remaining item - remainingMetadatas.count, 2, "Only two items should remain in the database." - ) + XCTAssertEqual(remainingMetadatas.filter { $0.deleted }.count, 1) + XCTAssertEqual(remainingMetadatas.filter { !$0.deleted }.count, 2) let survivingItem = remainingMetadatas.last XCTAssertNotNil(survivingItem, "An item should survive.") From 1c6097ffa41aeb51983974739367deac528e0036 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 1 Jul 2025 16:23:42 +0800 Subject: [PATCH 10/42] Add passable anchor to enumeate changes in mock change observer Signed-off-by: Claudio Cambra --- Tests/Interface/MockChangeObserver.swift | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Tests/Interface/MockChangeObserver.swift b/Tests/Interface/MockChangeObserver.swift index 253e55b5..cac1941d 100644 --- a/Tests/Interface/MockChangeObserver.swift +++ b/Tests/Interface/MockChangeObserver.swift @@ -36,8 +36,13 @@ public class MockChangeObserver: NSObject, NSFileProviderChangeObserver { isComplete = true } - public func enumerateChanges() async throws { - let anchor = NSFileProviderSyncAnchor(.init()) + public func enumerateChanges(from anchor: NSFileProviderSyncAnchor = + .init( + ISO8601DateFormatter() + .string(from: Date(timeIntervalSince1970: 1)) + .data(using: .utf8)! + ) + ) async throws { enumerator.enumerateChanges?(for: self, from: anchor) while !isComplete { try await Task.sleep(nanoseconds: 1_000_000) From 3b7e65af29df22a1ccfb238b156a123719ebebf8 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 1 Jul 2025 16:16:47 +0800 Subject: [PATCH 11/42] Amend testWorkingSetEnumerateChanges to test current behaviour of working set Signed-off-by: Claudio Cambra --- .../EnumeratorTests.swift | 169 ++++++------------ 1 file changed, 52 insertions(+), 117 deletions(-) diff --git a/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift b/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift index fe1234eb..66785510 100644 --- a/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift +++ b/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift @@ -476,149 +476,84 @@ final class EnumeratorTests: XCTestCase { } func testWorkingSetEnumerateChanges() async throws { - // This test verifies that `enumerateChanges` for the working set correctly identifies - // new, updated, and deleted items by iterating through materialised items and fetching - // their state. + // This test verifies that `enumerateChanges` for the working set correctly + // queries the local database for changes since the provided sync anchor date. - // 1. Arrange let db = Self.dbManager.ncDatabase() debugPrint(db) - // --- Database State (The "local truth" before enumeration) --- - var root = rootItem.toItemMetadata(account: Self.account) - root.visitedDirectory = true - root.etag = "ETAG_OLD" - Self.dbManager.addItemMetadata(root) - - // A materialised folder that we will check for changes. - var materialisedFolder = remoteFolder.toItemMetadata(account: Self.account) - materialisedFolder.visitedDirectory = true - materialisedFolder.etag = "ETAG_OLD" - Self.dbManager.addItemMetadata(materialisedFolder) - - let remoteUnmaterialisedFolder = MockRemoteItem( - identifier: "unmaterialised-folder", - versionIdentifier: "NEW", - name: "unmaterialised folder", - remotePath: Self.account.davFilesUrl + "/" + "unmaterialised folder", - directory: true, - account: Self.account.ncKitAccount, - username: Self.account.username, - userId: Self.account.id, - serverUrl: Self.account.serverUrl - ) - remoteUnmaterialisedFolder.parent = rootItem - rootItem.children.append(remoteUnmaterialisedFolder) - - var unmaterialisedFolder = remoteUnmaterialisedFolder.toItemMetadata(account: Self.account) - unmaterialisedFolder.visitedDirectory = false - unmaterialisedFolder.etag = "ETAG_OLD" - Self.dbManager.addItemMetadata(unmaterialisedFolder) - - let remoteUnmaterialisedFolderChild = MockRemoteItem( - identifier: "unmaterialised-folder-child", - versionIdentifier: "NEW", - name: "unmaterialised folder child", - remotePath: remoteUnmaterialisedFolder.remotePath + "/" + "unmaterialised folder child", - account: Self.account.ncKitAccount, - username: Self.account.username, - userId: Self.account.id, - serverUrl: Self.account.serverUrl - ) - remoteUnmaterialisedFolderChild.parent = remoteUnmaterialisedFolder - remoteUnmaterialisedFolder.children.append(remoteUnmaterialisedFolderChild) - - // A materialised file inside the folder that will be updated on the server. - var fileToUpdate = remoteItemA.toItemMetadata(account: Self.account) - fileToUpdate.downloaded = true - fileToUpdate.etag = "ETAG_OLD" - Self.dbManager.addItemMetadata(fileToUpdate) + let anchorDate = Date().addingTimeInterval(-300) // 5 minutes ago + let tenMinutesAgo = Date().addingTimeInterval(-600) + let now = Date() - // A materialised file inside the folder that will be deleted from the server. - var fileToBeDeleted = remoteItemB.toItemMetadata(account: Self.account) - fileToBeDeleted.downloaded = true - Self.dbManager.addItemMetadata(fileToBeDeleted) + // Create a sync anchor from our date. + let formatter = ISO8601DateFormatter() + let anchor = NSFileProviderSyncAnchor(formatter.string(from: anchorDate).data(using: .utf8)!) - // A top-level materialised file that does not exist remotely (i.e. deleted), will cause 404 - var topLevelFileToBeDeleted = SendableItemMetadata( - ocId: "toplevel-deleted", fileName: "toplevel-deleted.txt", account: Self.account - ) - topLevelFileToBeDeleted.downloaded = true - topLevelFileToBeDeleted.uploaded = true - Self.dbManager.addItemMetadata(topLevelFileToBeDeleted) + // --- Database State --- + var rootMetadata = rootItem.toItemMetadata(account: Self.account) + rootMetadata.syncTime = now + Self.dbManager.addItemMetadata(rootMetadata) - // --- Server State (The "remote truth" for the test) --- - let remoteInterface = MockRemoteInterface(rootItem: rootItem) - - // Update server state to reflect the changes - remoteItemA.versionIdentifier = "ETAG_NEW" + var folderMetadata = remoteFolder.toItemMetadata(account: Self.account) + folderMetadata.syncTime = now + Self.dbManager.addItemMetadata(folderMetadata) - // Add a new file on the server inside the materialised folder - let newChildFile = MockRemoteItem( - identifier: "new-child", - name: "new-child.txt", - remotePath: remoteFolder.remotePath + "/new-child.txt", + // Item synced BEFORE the anchor date (should not be reported). + var oldItem = remoteItemA.toItemMetadata(account: Self.account) + oldItem.downloaded = true // Materialised + oldItem.syncTime = tenMinutesAgo + Self.dbManager.addItemMetadata(oldItem) + + // Item synced AFTER the anchor date (should be reported as updated). + var updatedItem = remoteItemB.toItemMetadata(account: Self.account) + updatedItem.downloaded = true // Materialised + updatedItem.deleted = false + updatedItem.syncTime = now + Self.dbManager.addItemMetadata(updatedItem) + + // Item marked as deleted AFTER the anchor date (should be reported as deleted). + var deletedItem = remoteItemC.toItemMetadata(account: Self.account) + deletedItem.downloaded = true // Materialised + deletedItem.deleted = true + deletedItem.syncTime = now + Self.dbManager.addItemMetadata(deletedItem) + + // Non-materialised item synced after anchor date (should be ignored). + var nonMaterialisedItem = MockRemoteItem( + identifier: "non-mat", + name: "non-mat.txt", + remotePath: Self.account.davFilesUrl + "/non-mat.txt", account: Self.account.ncKitAccount, username: Self.account.username, userId: Self.account.id, serverUrl: Self.account.serverUrl - ) - newChildFile.parent = remoteFolder - remoteFolder.children.append(newChildFile) - - // Remove the "deleted" file from the server - remoteFolder.children.removeAll(where: { $0.identifier == fileToBeDeleted.ocId }) + ).toItemMetadata(account: Self.account) + nonMaterialisedItem.downloaded = false + nonMaterialisedItem.syncTime = now + Self.dbManager.addItemMetadata(nonMaterialisedItem) let enumerator = Enumerator( enumeratedItemIdentifier: .workingSet, account: Self.account, - remoteInterface: remoteInterface, + remoteInterface: MockRemoteInterface(), // Not needed and no remote calls should be made dbManager: Self.dbManager ) let observer = MockChangeObserver(enumerator: enumerator) // 2. Act - try await observer.enumerateChanges() + try await observer.enumerateChanges(from: anchor) // 3. Assert XCTAssertNil(observer.error, "Enumeration should complete without error.") - // Verify Deletions - let deletedIds = observer.deletedItemIdentifiers.map(\.rawValue) - XCTAssertEqual(deletedIds.count, 2, "There should be two deleted items.") - XCTAssertTrue( - deletedIds.contains(fileToBeDeleted.ocId), - "The file deleted from the server folder should be reported as deleted." - ) - XCTAssertTrue( - deletedIds.contains(topLevelFileToBeDeleted.ocId), - "The top-level file that resulted in a 404 should be reported as deleted." - ) - - // Verify Updates and Additions - let changedIds = observer.changedItems.map(\.itemIdentifier.rawValue) - print(changedIds) - XCTAssertEqual( - changedIds.count, - 5, - "There should be one updated file, three updated folders, and one new file." - ) - XCTAssertTrue(changedIds.contains(NSFileProviderItemIdentifier.rootContainer.rawValue)) - XCTAssertTrue(changedIds.contains(materialisedFolder.ocId)) - XCTAssertTrue(changedIds.contains(unmaterialisedFolder.ocId)) - XCTAssertTrue(changedIds.contains(fileToUpdate.ocId)) - XCTAssertTrue(changedIds.contains(newChildFile.identifier)) - - // Verify the database state was updated - let finalUpdatedFile = Self.dbManager.itemMetadata(ocId: fileToUpdate.ocId) - XCTAssertEqual( - finalUpdatedFile?.etag, - "ETAG_NEW", - "The ETag for the updated file should be changed in the database." - ) + // Check for updated items + XCTAssertEqual(observer.changedItems.count, 1, "There should be one updated item.") + XCTAssertEqual(observer.changedItems.first?.itemIdentifier.rawValue, updatedItem.ocId, "The correct item should be reported as updated.") - let finalNewFile = Self.dbManager.itemMetadata(ocId: newChildFile.identifier) - XCTAssertNotNil(finalNewFile, "The new file should now exist in the database.") + // Check for deleted items + XCTAssertEqual(observer.deletedItemIdentifiers.count, 1, "There should be one deleted item.") + XCTAssertEqual(observer.deletedItemIdentifiers.first?.rawValue, deletedItem.ocId, "The correct item should be reported as deleted.") } func testFolderEnumeration() async throws { From 24cc81ce765b46b890568fd381ddb7dffad4d6a3 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 1 Jul 2025 17:12:40 +0800 Subject: [PATCH 12/42] Perform a check of the working set's items via the remote change observer Signed-off-by: Claudio Cambra --- .../Enumeration/RemoteChangeObserver.swift | 139 +++++++++++++++++- 1 file changed, 135 insertions(+), 4 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift index 0a271305..256d1e2c 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift @@ -18,6 +18,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb public let remoteInterface: RemoteInterface public let changeNotificationInterface: ChangeNotificationInterface public let domain: NSFileProviderDomain? + public let dbManager: FilesDatabaseManager public var account: Account public var accountId: String { account.ncKitAccount } @@ -29,6 +30,8 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb private let logger = Logger(subsystem: Logger.subsystem, category: "changeobserver") + private var workingSetCheckOngoing = false + private var webSocketUrlSession: URLSession? private var webSocketTask: URLSessionWebSocketTask? private var webSocketOperationQueue = OperationQueue() @@ -56,7 +59,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb } else if oldValue == .notReachable { logger.info("Network reachable, trying to reconnect to websocket") reconnectWebSocket() - changeNotificationInterface.notifyChange() + startWorkingSetCheck() } } } @@ -65,12 +68,14 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb account: Account, remoteInterface: RemoteInterface, changeNotificationInterface: ChangeNotificationInterface, - domain: NSFileProviderDomain? + domain: NSFileProviderDomain?, + dbManager: FilesDatabaseManager ) { self.account = account self.remoteInterface = remoteInterface self.changeNotificationInterface = changeNotificationInterface self.domain = domain + self.dbManager = dbManager super.init() connect() } @@ -81,7 +86,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb withTimeInterval: pollInterval, repeats: true ) { [weak self] timer in self?.logger.info("Polling timer timeout, notifying change") - self?.changeNotificationInterface.notifyChange() + self?.startWorkingSetCheck() } logger.info("Starting polling timer") } @@ -245,6 +250,13 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb didCloseWith closeCode: URLSessionWebSocketTask.CloseCode, reason: Data? ) { + // If the task that closed is not the current active task, it means we have + // already initiated a reset and this is a stale callback. Ignore it. + guard webSocketTask === self.webSocketTask else { + logger.debug("An old websocket task closed, ignoring.") + return + } + logger.debug("Socket connection closed for \(self.accountId, privacy: .public).") if let reason = reason { logger.debug("Reason: \(String(data: reason, encoding: .utf8) ?? "", privacy: .public)") @@ -350,7 +362,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb logger.debug("Received websocket string: \(string, privacy: .public)") if string == "notify_file" { logger.debug("Received file notification for \(self.accountId, privacy: .public)") - changeNotificationInterface.notifyChange() + startWorkingSetCheck() } else if string == "notify_activity" { logger.debug("Ignoring activity notification: \(self.accountId, privacy: .public)") } else if string == "notify_notification" { @@ -439,4 +451,123 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb public func request( _ request: Alamofire.DataRequest, didParseResponse response: Alamofire.AFDataResponse ) { } + + private func startWorkingSetCheck() { + guard !workingSetCheckOngoing else { return } + Task { await checkWorkingSet() } + } + + private func checkWorkingSet() async { + workingSetCheckOngoing = true + defer { workingSetCheckOngoing = false } + + // Unlike when enumerating items we can't progressively enumerate items as we need to + // wait to see which items are truly deleted and which have just been moved elsewhere. + // Visited folders and downloaded files. Sort in terms of their remote URLs. + // This way we ensure we visit parent folders before their children. + let materialisedItems = dbManager + .materialisedItemMetadatas(account: accountId) + .sorted { + ($0.serverUrl + "/" + $0.fileName).count < + ($1.serverUrl + "/" + $1.fileName).count + } + + + var allNewMetadatas = [SendableItemMetadata]() + var allUpdatedMetadatas = [SendableItemMetadata]() + var allDeletedMetadatas = [SendableItemMetadata]() + var examinedItemIds = Set() + for item in materialisedItems where !examinedItemIds.contains(item.ocId) { + let itemRemoteUrl = item.serverUrl + "/" + item.fileName + let ( + metadatas, newMetadatas, updatedMetadatas, deletedMetadatas, _, readError + ) = await Enumerator.readServerUrl( + itemRemoteUrl, + account: account, + remoteInterface: remoteInterface, + dbManager: dbManager, + depth: item.directory ? .targetAndDirectChildren : .target + ) + if readError?.errorCode == 404 { + allDeletedMetadatas.append(item) + examinedItemIds.insert(item.ocId) + materialisedItems + .filter { $0.serverUrl == itemRemoteUrl } + .forEach { + allDeletedMetadatas.append($0) + examinedItemIds.insert(item.ocId) + } + } else if let readError, readError != .success { + logger.info( + """ + Finished change enumeration of working set for user: + \(self.accountId, privacy: .public) + with error: \(readError.errorDescription, privacy: .public) + """ + ) + return + } else { + allDeletedMetadatas += deletedMetadatas ?? [] + allUpdatedMetadatas += updatedMetadatas ?? [] + allNewMetadatas += newMetadatas ?? [] + + // Just because we have read child directories metadata doesn't mean we need + // to in turn scan their children. This is not the case for files + var examinedChildFilesAndDeletedItems = Set() + if let metadatas, let target = metadatas.first { + examinedItemIds.insert(target.ocId) + + if metadatas.count > 1 { + examinedChildFilesAndDeletedItems.formUnion( + metadatas[1...].filter { !$0.directory }.map(\.ocId) + ) + } + + // If the target is not in the updated metadatas then neither it, nor + // any of its kids have changed. So skip examining all of them + if !allUpdatedMetadatas.contains(where: { $0.ocId == target.ocId }) { + let materialisedChildren = materialisedItems.filter { + $0.serverUrl.hasPrefix(itemRemoteUrl) + }.map(\.ocId) + examinedChildFilesAndDeletedItems.formUnion(materialisedChildren) + } + + if let deletedMetadataOcIds = deletedMetadatas?.map(\.ocId) { + examinedChildFilesAndDeletedItems.formUnion(deletedMetadataOcIds) + } + } + + examinedItemIds.formUnion(examinedChildFilesAndDeletedItems) + } + } + + // Run a check to ensure files deleted in one location are not updated in another + // (e.g. when moved) + // The recursive scan provides us with updated/deleted metadatas only on a folder by + // folder basis; so we need to check we are not simultaneously marking a moved file as + // deleted and updated + var checkedDeletedMetadatas = allDeletedMetadatas + + for updatedMetadata in allUpdatedMetadatas { + guard let matchingDeletedMetadataIdx = checkedDeletedMetadatas.firstIndex( + where: { $0.ocId == updatedMetadata.ocId } + ) else { continue } + checkedDeletedMetadatas.remove(at: matchingDeletedMetadataIdx) + } + + allDeletedMetadatas = checkedDeletedMetadatas + for var metadata in allDeletedMetadatas { + metadata.deleted = true + } + + logger.info( + "Finished change checking of working set for user: \(self.accountId, privacy: .public)" + ) + + if allUpdatedMetadatas.isEmpty, allDeletedMetadatas.isEmpty { + logger.info("No changes found.") + } else { + changeNotificationInterface.notifyChange() + } + } } From fe2b69e9fc63b558e7f4cc44264c1c5cb050afc3 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 1 Jul 2025 17:12:58 +0800 Subject: [PATCH 13/42] Implement tests for new behaviour in remote change observer Signed-off-by: Claudio Cambra --- .../RemoteChangeObserverTests.swift | 266 ++++++++++-------- 1 file changed, 141 insertions(+), 125 deletions(-) diff --git a/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift index 31bf5096..34afd47d 100644 --- a/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift +++ b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift @@ -7,6 +7,7 @@ import Foundation import NextcloudCapabilitiesKit +import RealmSwift import TestInterface import XCTest @testable import NextcloudFileProviderKit @@ -24,6 +25,9 @@ final class RemoteChangeObserverTests: XCTestCase { static let account = Account( user: username, id: userId, serverUrl: serverUrl, password: password ) + static let dbManager = FilesDatabaseManager( + realmConfig: .defaultConfiguration, account: account + ) static let notifyPushServer = MockNotifyPushServer( host: serverUrl, port: 8888, @@ -32,8 +36,11 @@ final class RemoteChangeObserverTests: XCTestCase { eventLoopGroup: .singleton ) var remoteChangeObserver: RemoteChangeObserver? + var dbManager: FilesDatabaseManager! override func setUp() { + Realm.Configuration.defaultConfiguration.inMemoryIdentifier = name + dbManager = FilesDatabaseManager(realmConfig: .defaultConfiguration, account: Self.account) Task { try await Self.notifyPushServer.run() } } @@ -43,6 +50,14 @@ final class RemoteChangeObserverTests: XCTestCase { Self.notifyPushServer.reset() } + /// Helper to wait for an expectation with a standard timeout. + private func wait(for expectation: XCTestExpectation, description: String) async { + let result = await XCTWaiter.fulfillment(of: [expectation], timeout: 5.0) + if result != .completed { + XCTFail("Timeout waiting for \(description)") + } + } + func testAuthentication() async throws { let remoteInterface = MockRemoteInterface() remoteInterface.capabilities = mockCapabilities @@ -59,7 +74,8 @@ final class RemoteChangeObserverTests: XCTestCase { account: Self.account, remoteInterface: remoteInterface, changeNotificationInterface: MockChangeNotificationInterface(), - domain: nil + domain: nil, + dbManager: Self.dbManager ) for _ in 0...Self.timeout { @@ -90,7 +106,8 @@ final class RemoteChangeObserverTests: XCTestCase { account: incorrectAccount, remoteInterface: remoteInterface, changeNotificationInterface: MockChangeNotificationInterface(), - domain: nil + domain: nil, + dbManager: Self.dbManager ) let remoteChangeObserver = remoteChangeObserver! @@ -122,7 +139,8 @@ final class RemoteChangeObserverTests: XCTestCase { account: incorrectAccount, remoteInterface: remoteInterface, changeNotificationInterface: MockChangeNotificationInterface(), - domain: nil + domain: nil, + dbManager: Self.dbManager ) for _ in 0...Self.timeout { @@ -141,43 +159,44 @@ final class RemoteChangeObserverTests: XCTestCase { } func testChangeRecognised() async throws { - let remoteInterface = MockRemoteInterface() + // 1. Arrange + let db = dbManager.ncDatabase() + debugPrint(db) + + let remoteInterface = MockRemoteInterface(rootItem: MockRemoteItem.rootItem(account: Self.account)) remoteInterface.capabilities = mockCapabilities - var authenticated = false - var notified = false + // DB State: A materialised file with an old ETag. + var fileToUpdate = SendableItemMetadata(ocId: "item1", fileName: "file.txt", account: Self.account) + fileToUpdate.downloaded = true + fileToUpdate.etag = "ETAG_OLD" + dbManager.addItemMetadata(fileToUpdate) - NotificationCenter.default.addObserver( - forName: NotifyPushAuthenticatedNotificationName, object: nil, queue: nil - ) { _ in - authenticated = true + // Server State: The same file now has a new ETag. + let serverItem = MockRemoteItem(identifier: "item1", versionIdentifier: "ETAG_NEW", name: "file.txt", remotePath: Self.account.davFilesUrl + "/file.txt", account: Self.account.ncKitAccount, username: Self.account.username, userId: Self.account.id, serverUrl: Self.account.serverUrl) + remoteInterface.rootItem?.children = [serverItem] + + let authExpectation = XCTestExpectation(description: "Websocket Authenticated") + NotificationCenter.default.addObserver(forName: NotifyPushAuthenticatedNotificationName, object: nil, queue: nil) { _ in + authExpectation.fulfill() } + let changeNotifiedExpectation = XCTestExpectation(description: "Change Notified") let notificationInterface = MockChangeNotificationInterface() - notificationInterface.changeHandler = { notified = true } + notificationInterface.changeHandler = { changeNotifiedExpectation.fulfill() } + remoteChangeObserver = RemoteChangeObserver( account: Self.account, remoteInterface: remoteInterface, changeNotificationInterface: notificationInterface, - domain: nil + domain: nil, + dbManager: dbManager ) - for _ in 0...Self.timeout { - try await Task.sleep(nanoseconds: 1_000_000) - if authenticated { - break - } - } - XCTAssertTrue(authenticated) - + // 2. Act & Assert + await wait(for: authExpectation, description: "authentication") Self.notifyPushServer.send(message: "notify_file") - for _ in 0...Self.timeout { - try await Task.sleep(nanoseconds: 1_000_000) - if notified { - break - } - } - XCTAssertTrue(notified) + await wait(for: changeNotifiedExpectation, description: "change notification") } func testIgnoreNonFileNotifications() async throws { @@ -199,7 +218,8 @@ final class RemoteChangeObserverTests: XCTestCase { account: Self.account, remoteInterface: remoteInterface, changeNotificationInterface: notificationInterface, - domain: nil + domain: nil, + dbManager: Self.dbManager ) for _ in 0...Self.timeout { @@ -223,43 +243,43 @@ final class RemoteChangeObserverTests: XCTestCase { } func testPolling() async throws { - var notified = false - let remoteInterface = MockRemoteInterface() + // 1. Arrange + let db = dbManager.ncDatabase() + debugPrint(db) + + let remoteInterface = MockRemoteInterface(rootItem: MockRemoteItem.rootItem(account: Self.account)) + // No capabilities -> will force polling. remoteInterface.capabilities = "" + + // DB State: A materialised file with an old ETag. + var fileToUpdate = SendableItemMetadata(ocId: "item1", fileName: "file.txt", account: Self.account) + fileToUpdate.downloaded = true + fileToUpdate.etag = "ETAG_OLD" + dbManager.addItemMetadata(fileToUpdate) + + // Server State: The same file now has a new ETag. + let serverItem = MockRemoteItem(identifier: "item1", versionIdentifier: "ETAG_NEW", name: "file.txt", remotePath: Self.account.davFilesUrl + "/file.txt", account: Self.account.ncKitAccount, username: Self.account.username, userId: Self.account.id, serverUrl: Self.account.serverUrl) + remoteInterface.rootItem?.children = [serverItem] + + let changeNotifiedExpectation = XCTestExpectation(description: "Change Notified via Polling") let notificationInterface = MockChangeNotificationInterface() - notificationInterface.changeHandler = { notified = true } + notificationInterface.changeHandler = { changeNotifiedExpectation.fulfill() } + remoteChangeObserver = RemoteChangeObserver( account: Self.account, remoteInterface: remoteInterface, changeNotificationInterface: notificationInterface, - domain: nil + domain: nil, + dbManager: dbManager ) - remoteChangeObserver?.webSocketAuthenticationFailLimit = 1 - remoteChangeObserver?.webSocketPingFailLimit = 1 - remoteChangeObserver?.webSocketPingIntervalNanoseconds = 1 - remoteChangeObserver?.webSocketReconfigureIntervalNanoseconds = 1 - remoteChangeObserver?.pollInterval = 2_000_000 - - for _ in 0...Self.timeout { - try await Task.sleep(nanoseconds: 1_000_000) - if remoteChangeObserver?.webSocketTaskActive == false { - break - } - } - XCTAssertFalse(remoteChangeObserver?.webSocketTaskActive ?? true) - - for _ in 0...Self.timeout { - try await Task.sleep(nanoseconds: 1_000_000) - if remoteChangeObserver?.pollingActive == true { - break - } - } - XCTAssertTrue(remoteChangeObserver?.pollingActive ?? false) - remoteChangeObserver?.pollInterval = 1 - remoteChangeObserver?.pollingTimer?.fire() // TODO: Fix firing not automatically working - - try await Task.sleep(nanoseconds: 1_000) - XCTAssertTrue(notified) + // Set a very short poll interval for the test. + remoteChangeObserver?.pollInterval = 0.5 + + // 2. Act & Assert + // The observer will fail to connect to websocket and start polling. + // We just need to wait for the poll to fire and detect the change. + await wait(for: changeNotifiedExpectation, description: "polling to trigger change") + XCTAssertTrue(remoteChangeObserver?.pollingActive ?? false, "Polling should be active.") } func testRetryOnRemoteClose() async throws { @@ -278,7 +298,8 @@ final class RemoteChangeObserverTests: XCTestCase { account: Self.account, remoteInterface: remoteInterface, changeNotificationInterface: MockChangeNotificationInterface(), - domain: nil + domain: nil, + dbManager: Self.dbManager ) for _ in 0...Self.timeout { @@ -318,7 +339,8 @@ final class RemoteChangeObserverTests: XCTestCase { account: Self.account, remoteInterface: remoteInterface, changeNotificationInterface: MockChangeNotificationInterface(), - domain: nil + domain: nil, + dbManager: Self.dbManager ) let pingIntervalNsecs = 500_000_000 @@ -346,86 +368,80 @@ final class RemoteChangeObserverTests: XCTestCase { } func testRetryOnConnectionLoss() async throws { - let remoteInterface = MockRemoteInterface() - remoteInterface.capabilities = mockCapabilities + // 1. Arrange + let db = dbManager.ncDatabase() + debugPrint(db) - var authenticated = false - var notified = false + let remoteInterface = + MockRemoteInterface(rootItem: MockRemoteItem.rootItem(account: Self.account)) + remoteInterface.capabilities = mockCapabilities - NotificationCenter.default.addObserver( - forName: NotifyPushAuthenticatedNotificationName, object: nil, queue: nil - ) { _ in - authenticated = true - } + // Setup a change scenario + var fileToUpdate = + SendableItemMetadata(ocId: "item1", fileName: "file.txt", account: Self.account) + fileToUpdate.downloaded = true + fileToUpdate.etag = "ETAG_OLD" + dbManager.addItemMetadata(fileToUpdate) + let serverItem = MockRemoteItem( + identifier: "item1", + versionIdentifier: "ETAG_NEW", + name: "file.txt", + remotePath: Self.account.davFilesUrl + "/file.txt", + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + remoteInterface.rootItem?.children = [serverItem] let notificationInterface = MockChangeNotificationInterface() - notificationInterface.changeHandler = { notified = true } - remoteChangeObserver = RemoteChangeObserver( + let remoteChangeObserver = RemoteChangeObserver( account: Self.account, remoteInterface: remoteInterface, changeNotificationInterface: notificationInterface, - domain: nil + domain: nil, + dbManager: dbManager ) - remoteChangeObserver?.networkReachabilityObserver(.reachableEthernetOrWiFi) + self.remoteChangeObserver = remoteChangeObserver - for _ in 0...Self.timeout { - try await Task.sleep(nanoseconds: 1_000_000) - if authenticated { - break - } - } - XCTAssertTrue(authenticated) + // --- Phase 1: Test connection and change notification --- + let authExpectation = + XCTNSNotificationExpectation(name: NotifyPushAuthenticatedNotificationName) + remoteChangeObserver.networkReachabilityObserver(.reachableEthernetOrWiFi) + await wait(for: authExpectation, description: "initial authentication") + let changeExpectation1 = XCTestExpectation(description: "First change notification") + notificationInterface.changeHandler = { changeExpectation1.fulfill() } Self.notifyPushServer.send(message: "notify_file") - for _ in 0...Self.timeout { - try await Task.sleep(nanoseconds: 1_000_000) - if notified { - break - } - } - XCTAssertTrue(notified) // Check notification handling is working properly + await wait(for: changeExpectation1, description: "first change") + + // --- Phase 2: Test connection loss --- + remoteChangeObserver.networkReachabilityObserver(.notReachable) + // Give it a moment to process the disconnection + try await Task.sleep(nanoseconds: 200_000_000) + XCTAssertFalse( + remoteChangeObserver.webSocketTaskActive, + "Websocket should be inactive after connection loss." + ) + Self.notifyPushServer.reset() - remoteChangeObserver?.networkReachabilityObserver(.notReachable) - Self.notifyPushServer.resetCredentialsState() - authenticated = false - for _ in 0...Self.timeout { - try await Task.sleep(nanoseconds: 1_000_000) - if authenticated { - break - } - } - // Should still be false. The mock notify push server is still online so if we the - // remote change observer attempts to connect it _will_ be correctly authentiated, - // but once we have set the network reachability to unreachable it shouldn't be - // trying to connect at all. - XCTAssertFalse(authenticated) + // --- Phase 3: Test reconnection and change notification --- + let reauthExpectation = + XCTNSNotificationExpectation(name: NotifyPushAuthenticatedNotificationName) - notified = false - Self.notifyPushServer.send(message: "notify_file") - for _ in 0...Self.timeout { - try await Task.sleep(nanoseconds: 1_000_000) - if notified { - break - } - } - XCTAssertFalse(notified) // Check we disconnected and are not listening to the server + // Trigger the reconnection logic. + remoteChangeObserver.networkReachabilityObserver(.reachableEthernetOrWiFi) - remoteChangeObserver?.networkReachabilityObserver(.reachableEthernetOrWiFi) - for _ in 0...Self.timeout { - try await Task.sleep(nanoseconds: 1_000_000) - if authenticated { - break - } - } - XCTAssertTrue(authenticated) + // Now, wait for the expectation to be fulfilled. + await wait(for: reauthExpectation, description: "re-authentication") + XCTAssertTrue( + remoteChangeObserver.webSocketTaskActive, + "Websocket should be active again after reconnection." + ) + let changeExpectation2 = XCTestExpectation(description: "Second change notification") + notificationInterface.changeHandler = { changeExpectation2.fulfill() } Self.notifyPushServer.send(message: "notify_file") - for _ in 0...Self.timeout { - try await Task.sleep(nanoseconds: 1_000_000) - if notified { - break - } - } - XCTAssertTrue(notified) // Check notification handling is working properly again + await wait(for: changeExpectation2, description: "second change") } } From f1a8f675c0bb599903fe359d48af73ac84ba76f1 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 1 Jul 2025 17:14:08 +0800 Subject: [PATCH 14/42] Store deletion state in db Signed-off-by: Claudio Cambra --- .../Enumeration/RemoteChangeObserver.swift | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift index 256d1e2c..2482fb5e 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift @@ -556,9 +556,14 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb } allDeletedMetadatas = checkedDeletedMetadatas - for var metadata in allDeletedMetadatas { - metadata.deleted = true + let task = Task { @MainActor in + allDeletedMetadatas.forEach { + var deleteMarked = $0 + deleteMarked.deleted = true + dbManager.addItemMetadata(deleteMarked) + } } + _ = await task.result logger.info( "Finished change checking of working set for user: \(self.accountId, privacy: .public)" From 69afa7a52dcc7bc8579f693783d5847ea2f4b381 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 12:30:49 +0800 Subject: [PATCH 15/42] Assert database state in remote change observer test Signed-off-by: Claudio Cambra --- .../RemoteChangeObserverTests.swift | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift index 34afd47d..a48cdfac 100644 --- a/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift +++ b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift @@ -163,6 +163,7 @@ final class RemoteChangeObserverTests: XCTestCase { let db = dbManager.ncDatabase() debugPrint(db) + let testStartDate = Date() let remoteInterface = MockRemoteInterface(rootItem: MockRemoteItem.rootItem(account: Self.account)) remoteInterface.capabilities = mockCapabilities @@ -197,6 +198,13 @@ final class RemoteChangeObserverTests: XCTestCase { await wait(for: authExpectation, description: "authentication") Self.notifyPushServer.send(message: "notify_file") await wait(for: changeNotifiedExpectation, description: "change notification") + + // 3. Assert Database State + let finalItemState = try XCTUnwrap(dbManager.itemMetadata(ocId: "item1"), "The item should still exist in the database.") + + XCTAssertEqual(finalItemState.etag, "ETAG_NEW", "The ETag of the item should be updated to the new value from the server.") + XCTAssertNotNil(finalItemState.syncTime, "The syncTime should be set.") + XCTAssertTrue(finalItemState.syncTime >= testStartDate, "The syncTime should be updated to a recent date.") } func testIgnoreNonFileNotifications() async throws { From a95f0d6bcc298a9328dfbd22d00fb17c4768a479 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 12:37:47 +0800 Subject: [PATCH 16/42] Expand remote change observer detected change test Signed-off-by: Claudio Cambra --- .../RemoteChangeObserverTests.swift | 117 +++++++++++++++--- 1 file changed, 99 insertions(+), 18 deletions(-) diff --git a/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift index a48cdfac..935a8796 100644 --- a/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift +++ b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift @@ -164,24 +164,84 @@ final class RemoteChangeObserverTests: XCTestCase { debugPrint(db) let testStartDate = Date() - let remoteInterface = MockRemoteInterface(rootItem: MockRemoteItem.rootItem(account: Self.account)) + let remoteInterface = + MockRemoteInterface(rootItem: MockRemoteItem.rootItem(account: Self.account)) remoteInterface.capabilities = mockCapabilities - // DB State: A materialised file with an old ETag. - var fileToUpdate = SendableItemMetadata(ocId: "item1", fileName: "file.txt", account: Self.account) - fileToUpdate.downloaded = true - fileToUpdate.etag = "ETAG_OLD" - dbManager.addItemMetadata(fileToUpdate) - - // Server State: The same file now has a new ETag. - let serverItem = MockRemoteItem(identifier: "item1", versionIdentifier: "ETAG_NEW", name: "file.txt", remotePath: Self.account.davFilesUrl + "/file.txt", account: Self.account.ncKitAccount, username: Self.account.username, userId: Self.account.id, serverUrl: Self.account.serverUrl) - remoteInterface.rootItem?.children = [serverItem] + // --- DB State (What the app thinks is true) --- + // A materialised file in the root that will be updated. + var rootFileToUpdate = + SendableItemMetadata(ocId: "rootFile", fileName: "root-file.txt", account: Self.account) + rootFileToUpdate.downloaded = true + rootFileToUpdate.etag = "ETAG_OLD_ROOTFILE" + dbManager.addItemMetadata(rootFileToUpdate) + + // A materialised folder that will have its contents changed. + var folderA = + SendableItemMetadata(ocId: "folderA", fileName: "FolderA", account: Self.account) + folderA.directory = true + folderA.visitedDirectory = true + folderA.etag = "ETAG_OLD_FOLDERA" + dbManager.addItemMetadata(folderA) + + // A materialised file inside FolderA that will be deleted. + var fileInAToDelete = + SendableItemMetadata(ocId: "fileInA", fileName: "file-in-a.txt", account: Self.account) + fileInAToDelete.downloaded = true + fileInAToDelete.serverUrl = Self.account.davFilesUrl + "/FolderA" + dbManager.addItemMetadata(fileInAToDelete) + + // A materialised folder that will be deleted entirely. + var folderBToDelete = + SendableItemMetadata(ocId: "folderB", fileName: "FolderB", account: Self.account) + folderBToDelete.directory = true + folderBToDelete.visitedDirectory = true + dbManager.addItemMetadata(folderBToDelete) + + // --- Server State (The new "remote truth") --- + let rootItem = remoteInterface.rootItem! + + // Update the root file on the server. + let serverRootFile = MockRemoteItem( + identifier: "rootFile", + versionIdentifier: "ETAG_NEW_ROOTFILE", + name: "root-file.txt", + remotePath: Self.account.davFilesUrl + "/root-file.txt", + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + rootItem.children.append(serverRootFile) + + // Update FolderA on the server and modify its contents. + let serverFolderA = MockRemoteItem( + identifier: "folderA", + versionIdentifier: "ETAG_NEW_FOLDERA", + name: "FolderA", + remotePath: Self.account.davFilesUrl + "/FolderA", + directory: true, + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + rootItem.children.append(serverFolderA) - let authExpectation = XCTestExpectation(description: "Websocket Authenticated") - NotificationCenter.default.addObserver(forName: NotifyPushAuthenticatedNotificationName, object: nil, queue: nil) { _ in - authExpectation.fulfill() - } + // Add a new file inside FolderA on the server. + let newFileInA = MockRemoteItem( + identifier: "newFileInA", + name: "new-file.txt", + remotePath: serverFolderA.remotePath + "/new-file.txt", + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + serverFolderA.children.append(newFileInA) + let authExpectation = + XCTNSNotificationExpectation(name: NotifyPushAuthenticatedNotificationName) let changeNotifiedExpectation = XCTestExpectation(description: "Change Notified") let notificationInterface = MockChangeNotificationInterface() notificationInterface.changeHandler = { changeNotifiedExpectation.fulfill() } @@ -196,15 +256,36 @@ final class RemoteChangeObserverTests: XCTestCase { // 2. Act & Assert await wait(for: authExpectation, description: "authentication") + Self.notifyPushServer.send(message: "notify_file") + await wait(for: changeNotifiedExpectation, description: "change notification") // 3. Assert Database State - let finalItemState = try XCTUnwrap(dbManager.itemMetadata(ocId: "item1"), "The item should still exist in the database.") + // Check updated items + let finalRootFile = try XCTUnwrap(dbManager.itemMetadata(ocId: "rootFile")) + XCTAssertEqual(finalRootFile.etag, "ETAG_NEW_ROOTFILE") + XCTAssertTrue(finalRootFile.syncTime >= testStartDate) + + let finalFolderA = try XCTUnwrap(dbManager.itemMetadata(ocId: "folderA")) + XCTAssertEqual(finalFolderA.etag, "ETAG_NEW_FOLDERA") + XCTAssertTrue(finalFolderA.syncTime >= testStartDate) + + // Check new item + let finalNewFile = try XCTUnwrap(dbManager.itemMetadata(ocId: "newFileInA")) + XCTAssertEqual(finalNewFile.fileName, "new-file.txt") + XCTAssertNotNil(finalNewFile.syncTime) + + // Check deleted items + let deletedFileInA = try XCTUnwrap(dbManager.itemMetadata(ocId: "fileInA")) + XCTAssertTrue( + deletedFileInA.deleted, "File inside updated folder should be marked as deleted." + ) + XCTAssertTrue(deletedFileInA.syncTime >= testStartDate) - XCTAssertEqual(finalItemState.etag, "ETAG_NEW", "The ETag of the item should be updated to the new value from the server.") - XCTAssertNotNil(finalItemState.syncTime, "The syncTime should be set.") - XCTAssertTrue(finalItemState.syncTime >= testStartDate, "The syncTime should be updated to a recent date.") + let deletedFolderB = try XCTUnwrap(dbManager.itemMetadata(ocId: "folderB")) + XCTAssertTrue(deletedFolderB.deleted, "The entire folder should be marked as deleted.") + XCTAssertTrue(deletedFolderB.syncTime >= testStartDate) } func testIgnoreNonFileNotifications() async throws { From 7c24e92b7c075cdb88f0b45de4faa2eb95506516 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 12:38:10 +0800 Subject: [PATCH 17/42] Add more logging regarding parent missing in enumerator Signed-off-by: Claudio Cambra --- Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift index 91670b35..ee08a4dd 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift @@ -679,6 +679,7 @@ public class Enumerator: NSObject, NSFileProviderEnumerator { observer.finishEnumeratingWithError(error) return } + Self.logger.info("Attempting handling invalid parent in change enumeration") do { let metadata = try await Self.attemptInvalidParentRecovery( error: error, From e550fb3a5b671f88f218c557780be6e052da63ea Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 14:24:27 +0800 Subject: [PATCH 18/42] Add sync anchor logging Signed-off-by: Claudio Cambra --- Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift index ee08a4dd..e85d563f 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift @@ -319,6 +319,7 @@ public class Enumerator: NSObject, NSFileProviderEnumerator { Received enumerate changes request for enumerator for user: \(self.account.ncKitAccount, privacy: .public) with serverUrl: \(self.serverUrl, privacy: .public) + with sync anchor: \(String(data: anchor.rawValue, encoding: .utf8) ?? "", privacy: .public) """ ) /* From cc269c400540403abdfa3456221f7abf47caefd8 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 14:53:52 +0800 Subject: [PATCH 19/42] Complete change enumeration with correct sync anchor Signed-off-by: Claudio Cambra --- .../NextcloudFileProviderKit/Enumeration/Enumerator.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift index e85d563f..d5a7984a 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift @@ -25,7 +25,7 @@ public class Enumerator: NSObject, NSFileProviderEnumerator { let domain: NSFileProviderDomain? let dbManager: FilesDatabaseManager - private let anchor = + private let currentAnchor = NSFileProviderSyncAnchor(ISO8601DateFormatter().string(from: Date()).data(using: .utf8)!) private let pageItemCount: Int static let logger = Logger(subsystem: Logger.subsystem, category: "enumerator") @@ -348,7 +348,7 @@ public class Enumerator: NSObject, NSFileProviderEnumerator { let pendingChanges = dbManager.pendingWorkingSetChanges(account: account, since: date) Self.completeChangesObserver( observer, - anchor: anchor, + anchor: currentAnchor, enumeratedItemIdentifier: enumeratedItemIdentifier, account: account, remoteInterface: remoteInterface, @@ -544,7 +544,7 @@ public class Enumerator: NSObject, NSFileProviderEnumerator { } public func currentSyncAnchor(completionHandler: @escaping (NSFileProviderSyncAnchor?) -> Void) { - completionHandler(anchor) + completionHandler(currentAnchor) } // MARK: - Helper methods From ceeb71783608b7ced5a206a22898011719dd53a7 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 15:40:09 +0800 Subject: [PATCH 20/42] Add more logging to remote change observer change calculation Signed-off-by: Claudio Cambra --- .../Enumeration/RemoteChangeObserver.swift | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift index 2482fb5e..02aab416 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift @@ -526,6 +526,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb // If the target is not in the updated metadatas then neither it, nor // any of its kids have changed. So skip examining all of them if !allUpdatedMetadatas.contains(where: { $0.ocId == target.ocId }) { + logger.debug("Target \(itemRemoteUrl, privacy: .public) has not changed. Skipping children") let materialisedChildren = materialisedItems.filter { $0.serverUrl.hasPrefix(itemRemoteUrl) }.map(\.ocId) @@ -568,6 +569,8 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb logger.info( "Finished change checking of working set for user: \(self.accountId, privacy: .public)" ) + logger.debug("Examined item ids: \(examinedItemIds, privacy: .public)") + logger.debug("Materialised item ids: \(materialisedItems.map(\.ocId), privacy: .public)") if allUpdatedMetadatas.isEmpty, allDeletedMetadatas.isEmpty { logger.info("No changes found.") From ae7b6df37059b65baba44bf18a2441fe6ec57145 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 16:12:02 +0800 Subject: [PATCH 21/42] Log sync time on items Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager.swift | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index 26fa1a37..7435d8b7 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -245,6 +245,7 @@ public final class FilesDatabaseManager: Sendable { ocID: \(existingMetadata.ocId, privacy: .public) etag: \(existingMetadata.etag, privacy: .public) fileName: \(existingMetadata.fileName, privacy: .public)" + syncTime: \(existingMetadata.syncTime, privacy: .public) """ ) } @@ -291,6 +292,7 @@ public final class FilesDatabaseManager: Sendable { ocID: \(updatedMetadata.ocId, privacy: .public) etag: \(updatedMetadata.etag, privacy: .public) fileName: \(updatedMetadata.fileName, privacy: .public) + syncTime: \(updatedMetadata.syncTime, privacy: .public) """ ) } else { @@ -300,6 +302,7 @@ public final class FilesDatabaseManager: Sendable { ocID: \(updatedMetadata.ocId, privacy: .public) etag: \(updatedMetadata.etag, privacy: .public) fileName: \(updatedMetadata.fileName, privacy: .public) + syncTime: \(updatedMetadata.syncTime, privacy: .public) """ ) } @@ -313,6 +316,7 @@ public final class FilesDatabaseManager: Sendable { ocID: \(updatedMetadata.ocId, privacy: .public) etag: \(updatedMetadata.etag, privacy: .public) fileName: \(updatedMetadata.fileName, privacy: .public) + syncTime: \(updatedMetadata.syncTime, privacy: .public) """ ) } @@ -468,6 +472,7 @@ public final class FilesDatabaseManager: Sendable { ocID: \(metadata.ocId, privacy: .public) etag: \(metadata.etag, privacy: .public) fileName: \(metadata.fileName, privacy: .public) + syncTime: \(metadata.syncTime, privacy: .public) """ ) } @@ -515,6 +520,7 @@ public final class FilesDatabaseManager: Sendable { uploaded: \(metadata.uploaded, privacy: .public) visitedDirectory: \(metadata.visitedDirectory, privacy: .public) deleted: \(metadata.deleted, privacy: .public) + syncTime: \(metadata.syncTime, privacy: .public) """ ) } @@ -525,6 +531,7 @@ public final class FilesDatabaseManager: Sendable { ocID: \(metadata.ocId, privacy: .public) etag: \(metadata.etag, privacy: .public) fileName: \(metadata.fileName, privacy: .public) + syncTime: \(metadata.syncTime, privacy: .public) received error: \(error.localizedDescription, privacy: .public) """ ) @@ -616,6 +623,7 @@ public final class FilesDatabaseManager: Sendable { fileName: \(metadata.fileName, privacy: .public), serverUrl: \(metadata.serverUrl, privacy: .public), account: \(metadata.account, privacy: .public), + syncTime: \(metadata.syncTime, privacy: .public) """ ) return nil From cf723b08343e4e777d4f69bba85893aef760ab9b Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 16:12:32 +0800 Subject: [PATCH 22/42] Mark items as deleted, do not delete actual db items Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager+Directories.swift | 4 ++-- .../Database/FilesDatabaseManager.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager+Directories.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager+Directories.swift index 76b5ac8e..f5206a90 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager+Directories.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager+Directories.swift @@ -90,7 +90,7 @@ extension FilesDatabaseManager { let database = ncDatabase() do { - try database.write { database.delete(directoryMetadata) } + try database.write { directoryMetadata.deleted = true } } catch let error { Self.logger.error( """ @@ -113,7 +113,7 @@ extension FilesDatabaseManager { for result in results { let inactiveItemMetadata = SendableItemMetadata(value: result) do { - try database.write { database.delete(result) } + try database.write { result.deleted = true } deletedMetadatas.append(inactiveItemMetadata) } catch let error { Self.logger.error( diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index 7435d8b7..8d6ccf0a 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -544,7 +544,7 @@ public final class FilesDatabaseManager: Sendable { let database = ncDatabase() try database.write { Self.logger.debug("Deleting item metadata. \(ocId, privacy: .public)") - database.delete(results) + results.forEach { $0.deleted = true } } return true } catch { From 059ee9fc11facc9ea38f2e488a6dd2fc2c7399f3 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 19:26:53 +0800 Subject: [PATCH 23/42] In pending working set changes also return children to updated/deleted folders Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager.swift | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index 8d6ccf0a..be528f17 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -677,10 +677,30 @@ public final class FilesDatabaseManager: Sendable { public func pendingWorkingSetChanges( account: Account, since date: Date ) -> (updated: [SendableItemMetadata], deleted: [SendableItemMetadata]) { + func handleMaterialisedFolderChildren(items: inout [SendableItemMetadata]) { + var handledOcIds = Set(items.map(\.ocId)) + + items + .map { $0.serverUrl + "/" + $0.fileName } + .forEach { serverUrl in + itemMetadatas + .where { $0.serverUrl == serverUrl } + .forEach { metadata in + guard !handledOcIds.contains(metadata.ocId) else { return } + handledOcIds.insert(metadata.ocId) + items.append(SendableItemMetadata(value: metadata)) + } + } + } + let accId = account.ncKitAccount let pending = managedMaterialisedItemMetadatas(account: accId).where { $0.syncTime > date } - let updated = pending.where { !$0.deleted }.toUnmanagedResults() - let deleted = pending.where { $0.deleted }.toUnmanagedResults() + var updated = pending.where { !$0.deleted }.toUnmanagedResults() + var deleted = pending.where { $0.deleted }.toUnmanagedResults() + + handleMaterialisedFolderChildren(items: &updated) + handleMaterialisedFolderChildren(items: &deleted) + return (updated, deleted) } } From f008cd3011f4a0d7ff09f4b2c281c2ea713e8f39 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 19:27:25 +0800 Subject: [PATCH 24/42] Improve testPendingWorkingSetChanges Signed-off-by: Claudio Cambra --- .../FilesDatabaseManagerTests.swift | 121 +++++++++--------- 1 file changed, 58 insertions(+), 63 deletions(-) diff --git a/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift b/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift index 85f6003a..4305643a 100644 --- a/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift +++ b/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift @@ -1198,81 +1198,76 @@ final class FilesDatabaseManagerTests: XCTestCase { func testPendingWorkingSetChanges() { // 1. Arrange - let fiveMinutesAgo = Date().addingTimeInterval(-300) - let tenMinutesAgo = Date().addingTimeInterval(-600) - let now = Date() + let anchorDate = Date().addingTimeInterval(-300) // 5 minutes ago + let oldSyncDate = Date().addingTimeInterval(-600) + let recentSyncDate = Date() // --- Items to populate the database --- - // Item 1: A materialised item synced too long ago. Should NOT be returned. - var itemTooOld = - SendableItemMetadata(ocId: "tooOld", fileName: "old.txt", account: Self.account) - itemTooOld.downloaded = true - itemTooOld.syncTime = tenMinutesAgo - Self.dbManager.addItemMetadata(itemTooOld) - - // Item 2: A materialised item synced recently. Should be returned in the 'updated' list. - var itemRecentlyUpdated = - SendableItemMetadata(ocId: "updated", fileName: "updated.txt", account: Self.account) - itemRecentlyUpdated.downloaded = true - itemRecentlyUpdated.deleted = false - itemRecentlyUpdated.syncTime = now - Self.dbManager.addItemMetadata(itemRecentlyUpdated) - - // Item 3: A materialised item synced recently but marked as deleted. - // Should be returned in the 'deleted' list. - var itemRecentlyDeleted = - SendableItemMetadata(ocId: "deleted", fileName: "deleted.txt", account: Self.account) - itemRecentlyDeleted.downloaded = true - itemRecentlyDeleted.deleted = true - itemRecentlyDeleted.syncTime = now - Self.dbManager.addItemMetadata(itemRecentlyDeleted) - - // Item 4: A non-materialised item synced recently. Should NOT be returned. - var itemNotMaterialised = SendableItemMetadata( - ocId: "notMaterialised", fileName: "notMaterialised.txt", account: Self.account - ) - itemNotMaterialised.downloaded = false // Not part of the working set - itemNotMaterialised.syncTime = now - Self.dbManager.addItemMetadata(itemNotMaterialised) - - // Item 5: A materialised directory synced recently. Should be returned in 'updated'. - var dirRecentlyUpdated = - SendableItemMetadata(ocId: "updatedDir", fileName: "updatedDir", account: Self.account) - dirRecentlyUpdated.directory = true - dirRecentlyUpdated.visitedDirectory = true // Materialised as a directory - dirRecentlyUpdated.deleted = false - dirRecentlyUpdated.syncTime = now - Self.dbManager.addItemMetadata(dirRecentlyUpdated) + // A materialised folder updated recently. It should be returned, along with its child. + var updatedDir = SendableItemMetadata(ocId: "updatedDir", fileName: "UpdatedDir", account: Self.account) + updatedDir.directory = true + updatedDir.visitedDirectory = true // Materialised + updatedDir.syncTime = recentSyncDate + Self.dbManager.addItemMetadata(updatedDir) + + // A child of the updated folder. Its syncTime is OLD, but it should be included + // because its parent was updated. + var childOfUpdatedDir = SendableItemMetadata(ocId: "childOfUpdated", fileName: "child.txt", account: Self.account) + childOfUpdatedDir.serverUrl = Self.account.davFilesUrl + "/UpdatedDir" + childOfUpdatedDir.syncTime = oldSyncDate // Old sync time + Self.dbManager.addItemMetadata(childOfUpdatedDir) + + // A materialised folder deleted recently. It should be returned, along with its child. + var deletedDir = SendableItemMetadata(ocId: "deletedDir", fileName: "DeletedDir", account: Self.account) + deletedDir.directory = true + deletedDir.visitedDirectory = true // Materialised + deletedDir.deleted = true + deletedDir.syncTime = recentSyncDate + Self.dbManager.addItemMetadata(deletedDir) + + // A child of the deleted folder. Its syncTime is OLD, but it should be included. + var childOfDeletedDir = SendableItemMetadata(ocId: "childOfDeleted", fileName: "child2.txt", account: Self.account) + childOfDeletedDir.serverUrl = Self.account.davFilesUrl + "/DeletedDir" + childOfDeletedDir.syncTime = oldSyncDate // Old sync time + Self.dbManager.addItemMetadata(childOfDeletedDir) + + // A standalone materialised file synced recently. Should be returned. + var standaloneUpdatedFile = SendableItemMetadata(ocId: "standaloneUpdated", fileName: "standalone.txt", account: Self.account) + standaloneUpdatedFile.downloaded = true // Materialised + standaloneUpdatedFile.syncTime = recentSyncDate + Self.dbManager.addItemMetadata(standaloneUpdatedFile) + + // A materialised file synced too long ago. Should NOT be returned. + var standaloneOldFile = SendableItemMetadata(ocId: "standaloneOld", fileName: "old.txt", account: Self.account) + standaloneOldFile.downloaded = true // Materialised + standaloneOldFile.syncTime = oldSyncDate + Self.dbManager.addItemMetadata(standaloneOldFile) + + // A non-materialised item synced recently. Should NOT be returned. + var nonMaterialisedFile = SendableItemMetadata(ocId: "nonMaterialised", fileName: "non-mat.txt", account: Self.account) + nonMaterialisedFile.downloaded = false + nonMaterialisedFile.syncTime = recentSyncDate + Self.dbManager.addItemMetadata(nonMaterialisedFile) // 2. Act - // Fetch changes that have occurred since five minutes ago. + // Fetch changes that have occurred since the anchor date. let result = Self.dbManager.pendingWorkingSetChanges( - account: Self.account, since: fiveMinutesAgo + account: Self.account, since: anchorDate ) // 3. Assert // Verify the 'updated' results - XCTAssertEqual( - result.updated.count, 2, "There should be two updated items (one file, one directory)." - ) let updatedIds = Set(result.updated.map { $0.ocId }) - print(updatedIds) - XCTAssertTrue( - updatedIds.contains("updated"), - "The recently updated file should be in the updated list." - ) - XCTAssertTrue( - updatedIds.contains("updatedDir"), - "The recently updated directory should be in the updated list." - ) + XCTAssertEqual(updatedIds.count, 3, "There should be three updated items: the folder, its child, and the standalone file.") + XCTAssertTrue(updatedIds.contains("updatedDir")) + XCTAssertTrue(updatedIds.contains("childOfUpdated"), "The child of the updated directory should be included.") + XCTAssertTrue(updatedIds.contains("standaloneUpdated")) // Verify the 'deleted' results - XCTAssertEqual(result.deleted.count, 1, "There should be one deleted item.") - XCTAssertEqual( - result.deleted.first?.ocId, - "deleted", - "The recently deleted file should be in the deleted list." - ) + let deletedIds = Set(result.deleted.map { $0.ocId }) + XCTAssertEqual(deletedIds.count, 2, "There should be two deleted items: the folder and its child.") + XCTAssertTrue(deletedIds.contains("deletedDir")) + XCTAssertTrue(deletedIds.contains("childOfDeleted"), "The child of the deleted directory should be included.") } } From 0453b5ec7fc41e76abc43186aeceab2df5f0f783 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 21:09:48 +0800 Subject: [PATCH 25/42] Improve new item metadata creation during update logging Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager.swift | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index be528f17..170bb0e7 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -316,6 +316,22 @@ public final class FilesDatabaseManager: Sendable { ocID: \(updatedMetadata.ocId, privacy: .public) etag: \(updatedMetadata.etag, privacy: .public) fileName: \(updatedMetadata.fileName, privacy: .public) + parentDirectoryUrl: \(updatedMetadata.serverUrl, privacy: .public) + account: \(updatedMetadata.account, privacy: .public) + content type: \(updatedMetadata.contentType, privacy: .public) + is directory: \(updatedMetadata.directory, privacy: .public) + creation date: \(updatedMetadata.creationDate, privacy: .public) + date: \(updatedMetadata.date, privacy: .public) + lock: \(updatedMetadata.lock, privacy: .public) + lockTimeOut: \(updatedMetadata.lockTimeOut?.description ?? "", privacy: .public) + lockOwner: \(updatedMetadata.lockOwner ?? "", privacy: .public) + permissions: \(updatedMetadata.permissions, privacy: .public) + size: \(updatedMetadata.size, privacy: .public) + trashbinFileName: \(updatedMetadata.trashbinFileName, privacy: .public) + downloaded: \(updatedMetadata.downloaded, privacy: .public) + uploaded: \(updatedMetadata.uploaded, privacy: .public) + visitedDirectory: \(updatedMetadata.visitedDirectory, privacy: .public) + deleted: \(updatedMetadata.deleted, privacy: .public) syncTime: \(updatedMetadata.syncTime, privacy: .public) """ ) From 054a335844a5b816c0e8cfa17591a6702286565a Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 21:10:54 +0800 Subject: [PATCH 26/42] Ensure sync time is updated during deletion Signed-off-by: Claudio Cambra --- .../Enumeration/RemoteChangeObserver.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift index 02aab416..e2c6026c 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift @@ -561,6 +561,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb allDeletedMetadatas.forEach { var deleteMarked = $0 deleteMarked.deleted = true + deleteMarked.syncTime = Date() dbManager.addItemMetadata(deleteMarked) } } From cc285696e2c062bfba9aa6cc0c211e2b6241eb2f Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 21:11:57 +0800 Subject: [PATCH 27/42] TEMP: Debug logging Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index 170bb0e7..4a1fd3cc 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -699,12 +699,15 @@ public final class FilesDatabaseManager: Sendable { items .map { $0.serverUrl + "/" + $0.fileName } .forEach { serverUrl in + Self.logger.debug("Checking \(serverUrl, privacy: .public)") itemMetadatas - .where { $0.serverUrl == serverUrl } + .where { $0.serverUrl == serverUrl && $0.syncTime > date } .forEach { metadata in + Self.logger.debug("Checking item: \(metadata.fileName, privacy: .public)") guard !handledOcIds.contains(metadata.ocId) else { return } handledOcIds.insert(metadata.ocId) items.append(SendableItemMetadata(value: metadata)) + Self.logger.debug("Appended item: \(metadata.fileName, privacy: .public)") } } } From 398ee7261b0f437bcb75203202136a2113040742 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 22:00:26 +0800 Subject: [PATCH 28/42] Properly handle root item server url Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index 4a1fd3cc..d75c5be4 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -697,7 +697,11 @@ public final class FilesDatabaseManager: Sendable { var handledOcIds = Set(items.map(\.ocId)) items - .map { $0.serverUrl + "/" + $0.fileName } + .map { + var serverUrl = $0.serverUrl + "/" + $0.fileName + if serverUrl.last == "/" { serverUrl.removeLast() } + return serverUrl + } .forEach { serverUrl in Self.logger.debug("Checking \(serverUrl, privacy: .public)") itemMetadatas From d9a91e76e94922413c523fc7f5401e7c343c2d35 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Wed, 2 Jul 2025 22:58:06 +0800 Subject: [PATCH 29/42] Fix handling of visitedDirectory state Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager.swift | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index d75c5be4..59b9d408 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -283,6 +283,7 @@ public final class FilesDatabaseManager: Sendable { if keepExistingDownloadState { updatedMetadata.downloaded = existingMetadata.downloaded } + updatedMetadata.visitedDirectory = existingMetadata.visitedDirectory returningUpdatedMetadatas.append(updatedMetadata) @@ -416,21 +417,22 @@ public final class FilesDatabaseManager: Sendable { } if var readTargetMetadata { + if readTargetMetadata.directory { + readTargetMetadata.visitedDirectory = true + } + if let existing = itemMetadata(ocId: readTargetMetadata.ocId) { if existing.status == Status.normal.rawValue, !existing.isInSameDatabaseStoreableRemoteState(readTargetMetadata) { - Self.logger.info("Depth 1 read target changed: \(readTargetMetadata.ocId)") + Self.logger.info("Depth 1 read target changed: \(readTargetMetadata.ocId, privacy: .public)") if keepExistingDownloadState { readTargetMetadata.downloaded = existing.downloaded } - if readTargetMetadata.directory { - readTargetMetadata.visitedDirectory = true - } metadatasToUpdate.insert(readTargetMetadata, at: 0) } } else { - Self.logger.info("Depth 1 read target is new: \(readTargetMetadata.ocId)") + Self.logger.info("Depth 1 read target is new: \(readTargetMetadata.ocId, privacy: .public)") metadatasToCreate.insert(readTargetMetadata, at: 0) } } From 7195d1ab0fb99cc888b781f570c0ae07e312392b Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Thu, 3 Jul 2025 00:14:27 +0800 Subject: [PATCH 30/42] Handle deleted files properly in pending working set retrieval Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager.swift | 66 ++++++++++++------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index 59b9d408..874e46e3 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -695,36 +695,52 @@ public final class FilesDatabaseManager: Sendable { public func pendingWorkingSetChanges( account: Account, since date: Date ) -> (updated: [SendableItemMetadata], deleted: [SendableItemMetadata]) { - func handleMaterialisedFolderChildren(items: inout [SendableItemMetadata]) { - var handledOcIds = Set(items.map(\.ocId)) - - items - .map { - var serverUrl = $0.serverUrl + "/" + $0.fileName - if serverUrl.last == "/" { serverUrl.removeLast() } - return serverUrl - } - .forEach { serverUrl in - Self.logger.debug("Checking \(serverUrl, privacy: .public)") - itemMetadatas - .where { $0.serverUrl == serverUrl && $0.syncTime > date } - .forEach { metadata in - Self.logger.debug("Checking item: \(metadata.fileName, privacy: .public)") - guard !handledOcIds.contains(metadata.ocId) else { return } - handledOcIds.insert(metadata.ocId) - items.append(SendableItemMetadata(value: metadata)) - Self.logger.debug("Appended item: \(metadata.fileName, privacy: .public)") - } - } - } - let accId = account.ncKitAccount let pending = managedMaterialisedItemMetadatas(account: accId).where { $0.syncTime > date } var updated = pending.where { !$0.deleted }.toUnmanagedResults() var deleted = pending.where { $0.deleted }.toUnmanagedResults() - handleMaterialisedFolderChildren(items: &updated) - handleMaterialisedFolderChildren(items: &deleted) + var handledUpdateOcIds = Set(updated.map(\.ocId)) + updated + .map { + var serverUrl = $0.serverUrl + "/" + $0.fileName + if serverUrl.last == "/" { serverUrl.removeLast() } + return serverUrl + } + .forEach { serverUrl in + Self.logger.debug("Checking (updated) \(serverUrl, privacy: .public)") + itemMetadatas + .where { $0.serverUrl == serverUrl && $0.syncTime > date } + .forEach { metadata in + Self.logger.debug("Checking item: \(metadata.fileName, privacy: .public)") + guard !handledUpdateOcIds.contains(metadata.ocId) else { return } + handledUpdateOcIds.insert(metadata.ocId) + let sendableMetadata = SendableItemMetadata(value: metadata) + if metadata.deleted { + deleted.append(sendableMetadata) + } else { + updated.append(sendableMetadata) + } + Self.logger.debug("Appended item: \(metadata.fileName, privacy: .public)") + } + } + + var handledDeleteOcIds = Set(deleted.map(\.ocId)) + deleted + .map { + var serverUrl = $0.serverUrl + "/" + $0.fileName + if serverUrl.last == "/" { serverUrl.removeLast() } + return serverUrl + } + .forEach { serverUrl in + Self.logger.debug("Checking (deletion) \(serverUrl, privacy: .public)") + itemMetadatas + .where { $0.serverUrl.starts(with: serverUrl) && $0.syncTime > date } + .forEach { metadata in + guard !handledDeleteOcIds.contains(metadata.ocId) else { return } + deleted.append(SendableItemMetadata(value: metadata)) + } + } return (updated, deleted) } From 9bf90bd23cb1a89607ff89b8ff7cf7b697948a3e Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Thu, 3 Jul 2025 00:14:44 +0800 Subject: [PATCH 31/42] Ensure marking of deleted files in depth1 handling Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager.swift | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index 874e46e3..2cc0ac35 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -234,9 +234,10 @@ public final class FilesDatabaseManager: Sendable { for existingMetadata in existingMetadatas { guard !updatedMetadatas.contains(where: { $0.ocId == existingMetadata.ocId }), - let metadataToDelete = itemMetadatas.where({ $0.ocId == existingMetadata.ocId }).first + var metadataToDelete = itemMetadatas.where({ $0.ocId == existingMetadata.ocId }).first else { continue } + metadataToDelete.deleted = true deletedMetadatas.append(metadataToDelete) Self.logger.debug( @@ -439,7 +440,7 @@ public final class FilesDatabaseManager: Sendable { try database.write { // Do not delete the metadatas that have been deleted - metadatasToDelete.forEach { $0.deleted = true } + database.add(metadatasToDelete) database.add(metadatasToUpdate.map { RealmItemMetadata(value: $0) }, update: .modified) database.add(metadatasToCreate.map { RealmItemMetadata(value: $0) }, update: .all) } From b841e598ddd2fe2c210418713b0a8a909a9c4e0f Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Thu, 3 Jul 2025 00:35:10 +0800 Subject: [PATCH 32/42] Fix crash from improper metadata modification Signed-off-by: Claudio Cambra --- .../Database/FilesDatabaseManager.swift | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index 2cc0ac35..ffc781b4 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -237,7 +237,6 @@ public final class FilesDatabaseManager: Sendable { var metadataToDelete = itemMetadatas.where({ $0.ocId == existingMetadata.ocId }).first else { continue } - metadataToDelete.deleted = true deletedMetadatas.append(metadataToDelete) Self.logger.debug( @@ -390,12 +389,14 @@ public final class FilesDatabaseManager: Sendable { readTargetMetadata = nil } - // NOTE: These metadatas are managed -- be careful! let metadatasToDelete = processItemMetadatasToDelete( existingMetadatas: existingMetadatas, updatedMetadatas: updatedChildMetadatas - ) - let metadatasToDeleteCopy = metadatasToDelete.map { SendableItemMetadata(value: $0) } + ).map { + var metadata = SendableItemMetadata(value: $0) + metadata.deleted = true + return metadata + } let metadatasToChange = processItemMetadatasToUpdate( existingMetadatas: existingMetadatas, @@ -440,12 +441,12 @@ public final class FilesDatabaseManager: Sendable { try database.write { // Do not delete the metadatas that have been deleted - database.add(metadatasToDelete) + database.add(metadatasToDelete.map { RealmItemMetadata(value: $0) }, update: .modified) database.add(metadatasToUpdate.map { RealmItemMetadata(value: $0) }, update: .modified) database.add(metadatasToCreate.map { RealmItemMetadata(value: $0) }, update: .all) } - return (metadatasToCreate, metadatasToUpdate, metadatasToDeleteCopy) + return (metadatasToCreate, metadatasToUpdate, metadatasToDelete) } catch { Self.logger.error( """ From ee4eeff5cc5dbedde5841c6696043654e3ad6cef Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Fri, 4 Jul 2025 18:49:46 +0800 Subject: [PATCH 33/42] Fix tests to conform to new item metadata deletion approach Signed-off-by: Claudio Cambra --- Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift | 2 +- .../FilesDatabaseManagerTests.swift | 5 +++-- Tests/NextcloudFileProviderKitTests/ItemDeleteTests.swift | 6 +++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift b/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift index 66785510..519b343d 100644 --- a/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift +++ b/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift @@ -1097,7 +1097,7 @@ final class EnumeratorTests: XCTestCase { try await observer.enumerateChanges() XCTAssertEqual(observer.changedItems.count, 1) XCTAssertEqual(observer.deletedItemIdentifiers.count, 1) - XCTAssertNil(Self.dbManager.itemMetadata(ocId: remoteTrashItemA.identifier)) + XCTAssertEqual(Self.dbManager.itemMetadata(ocId: remoteTrashItemA.identifier)?.deleted, true) XCTAssertNotNil(Self.dbManager.itemMetadata(ocId: remoteTrashItemB.identifier)) XCTAssertNotNil(Self.dbManager.itemMetadata(ocId: remoteTrashItemC.identifier)) } diff --git a/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift b/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift index 4305643a..d39bc31c 100644 --- a/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift +++ b/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift @@ -269,8 +269,9 @@ final class FilesDatabaseManagerTests: XCTestCase { let result = Self.dbManager.deleteItemMetadata(ocId: ocId) XCTAssertTrue(result, "deleteItemMetadata should return true on successful deletion") - XCTAssertNil( - Self.dbManager.itemMetadata(ocId: ocId), + XCTAssertEqual( + Self.dbManager.itemMetadata(ocId: ocId)?.deleted, + true, "Metadata should be deleted from the database" ) } diff --git a/Tests/NextcloudFileProviderKitTests/ItemDeleteTests.swift b/Tests/NextcloudFileProviderKitTests/ItemDeleteTests.swift index 06efc25e..607c54d6 100644 --- a/Tests/NextcloudFileProviderKitTests/ItemDeleteTests.swift +++ b/Tests/NextcloudFileProviderKitTests/ItemDeleteTests.swift @@ -65,7 +65,7 @@ final class ItemDeleteTests: XCTestCase { XCTAssertNil(error) XCTAssertTrue(rootItem.children.isEmpty) - XCTAssertNil(Self.dbManager.itemMetadata(ocId: itemIdentifier)) + XCTAssertEqual(Self.dbManager.itemMetadata(ocId: itemIdentifier)?.deleted, true) } func testDeleteFolderAndContents() async { @@ -189,7 +189,7 @@ final class ItemDeleteTests: XCTestCase { dbManager: Self.dbManager ) XCTAssertNil(error) - XCTAssertNil(Self.dbManager.itemMetadata(ocId: metadata.ocId)) + XCTAssertEqual(Self.dbManager.itemMetadata(ocId: metadata.ocId)?.deleted, true) } func testDeleteLockFileUnlocksTargetFile() async throws { @@ -257,7 +257,7 @@ final class ItemDeleteTests: XCTestCase { // Delete the lock file let error = await lockItem.delete(dbManager: Self.dbManager) - XCTAssertNil(Self.dbManager.itemMetadata(ocId: lockFileMetadata.ocId)) + XCTAssertEqual(Self.dbManager.itemMetadata(ocId: lockFileMetadata.ocId)?.deleted, true) // Assert: no error returned XCTAssertNil(error) From f2c2a57c762c0885d5e61f1f8e845cc1b00434a2 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Fri, 4 Jul 2025 18:50:12 +0800 Subject: [PATCH 34/42] Improve testPendingWorkingSetChanges Signed-off-by: Claudio Cambra --- .../FilesDatabaseManagerTests.swift | 151 ++++++++++++++---- 1 file changed, 118 insertions(+), 33 deletions(-) diff --git a/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift b/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift index d39bc31c..2799215f 100644 --- a/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift +++ b/Tests/NextcloudFileProviderKitTests/FilesDatabaseManagerTests.swift @@ -1200,26 +1200,48 @@ final class FilesDatabaseManagerTests: XCTestCase { func testPendingWorkingSetChanges() { // 1. Arrange let anchorDate = Date().addingTimeInterval(-300) // 5 minutes ago - let oldSyncDate = Date().addingTimeInterval(-600) - let recentSyncDate = Date() + let oldSyncDate = Date().addingTimeInterval(-600) // 10 minutes ago (before anchor) + let recentSyncDate = Date() // Now (after anchor) - // --- Items to populate the database --- + // --- Multi-level file structure to test the full scope --- - // A materialised folder updated recently. It should be returned, along with its child. + // LEVEL 1: Root level materialised folder updated recently var updatedDir = SendableItemMetadata(ocId: "updatedDir", fileName: "UpdatedDir", account: Self.account) updatedDir.directory = true updatedDir.visitedDirectory = true // Materialised updatedDir.syncTime = recentSyncDate Self.dbManager.addItemMetadata(updatedDir) - // A child of the updated folder. Its syncTime is OLD, but it should be included - // because its parent was updated. - var childOfUpdatedDir = SendableItemMetadata(ocId: "childOfUpdated", fileName: "child.txt", account: Self.account) - childOfUpdatedDir.serverUrl = Self.account.davFilesUrl + "/UpdatedDir" - childOfUpdatedDir.syncTime = oldSyncDate // Old sync time - Self.dbManager.addItemMetadata(childOfUpdatedDir) - - // A materialised folder deleted recently. It should be returned, along with its child. + // LEVEL 2: Child of updated folder with OLD sync time - should NOT be included + var childOfUpdatedDirOld = SendableItemMetadata(ocId: "childOfUpdatedOld", fileName: "childOld.txt", account: Self.account) + childOfUpdatedDirOld.serverUrl = Self.account.davFilesUrl + "/UpdatedDir" + childOfUpdatedDirOld.syncTime = oldSyncDate // Old sync time + childOfUpdatedDirOld.downloaded = true // Materialised + Self.dbManager.addItemMetadata(childOfUpdatedDirOld) + + // LEVEL 2: Child of updated folder with RECENT sync time - should be included (regardless of materialisation) + var childOfUpdatedDirRecent = SendableItemMetadata(ocId: "childOfUpdatedRecent", fileName: "childRecent.txt", account: Self.account) + childOfUpdatedDirRecent.serverUrl = Self.account.davFilesUrl + "/UpdatedDir" + childOfUpdatedDirRecent.syncTime = recentSyncDate // Recent sync time + childOfUpdatedDirRecent.downloaded = false // NOT materialised - but should still be included + Self.dbManager.addItemMetadata(childOfUpdatedDirRecent) + + // LEVEL 2: Child folder of updated folder with recent sync time + var childFolderOfUpdated = SendableItemMetadata(ocId: "childFolderOfUpdated", fileName: "ChildFolder", account: Self.account) + childFolderOfUpdated.directory = true + childFolderOfUpdated.visitedDirectory = false // Not materialised + childFolderOfUpdated.serverUrl = Self.account.davFilesUrl + "/UpdatedDir" + childFolderOfUpdated.syncTime = recentSyncDate + Self.dbManager.addItemMetadata(childFolderOfUpdated) + + // LEVEL 3: Grandchild with recent sync time - should not be included + var grandchildOfUpdated = SendableItemMetadata(ocId: "grandchildOfUpdated", fileName: "grandchild.txt", account: Self.account) + grandchildOfUpdated.serverUrl = Self.account.davFilesUrl + "/UpdatedDir/ChildFolder" + grandchildOfUpdated.syncTime = recentSyncDate + grandchildOfUpdated.downloaded = false // Not materialised + Self.dbManager.addItemMetadata(grandchildOfUpdated) + + // DELETED STRUCTURE: Root level materialised folder deleted recently var deletedDir = SendableItemMetadata(ocId: "deletedDir", fileName: "DeletedDir", account: Self.account) deletedDir.directory = true deletedDir.visitedDirectory = true // Materialised @@ -1227,48 +1249,111 @@ final class FilesDatabaseManagerTests: XCTestCase { deletedDir.syncTime = recentSyncDate Self.dbManager.addItemMetadata(deletedDir) - // A child of the deleted folder. Its syncTime is OLD, but it should be included. - var childOfDeletedDir = SendableItemMetadata(ocId: "childOfDeleted", fileName: "child2.txt", account: Self.account) - childOfDeletedDir.serverUrl = Self.account.davFilesUrl + "/DeletedDir" - childOfDeletedDir.syncTime = oldSyncDate // Old sync time - Self.dbManager.addItemMetadata(childOfDeletedDir) - - // A standalone materialised file synced recently. Should be returned. + // Child of deleted folder with OLD sync time - should NOT be included + var childOfDeletedDirOld = SendableItemMetadata(ocId: "childOfDeletedOld", fileName: "childDelOld.txt", account: Self.account) + childOfDeletedDirOld.serverUrl = Self.account.davFilesUrl + "/DeletedDir" + childOfDeletedDirOld.syncTime = oldSyncDate // Old sync time + childOfDeletedDirOld.downloaded = true + Self.dbManager.addItemMetadata(childOfDeletedDirOld) + + // Child of deleted folder with RECENT sync time - should be included in deleted + var childOfDeletedDirRecent = SendableItemMetadata(ocId: "childOfDeletedRecent", fileName: "childDelRecent.txt", account: Self.account) + childOfDeletedDirRecent.serverUrl = Self.account.davFilesUrl + "/DeletedDir" + childOfDeletedDirRecent.syncTime = recentSyncDate + childOfDeletedDirRecent.downloaded = false // Not materialised + Self.dbManager.addItemMetadata(childOfDeletedDirRecent) + + // Nested structure under deleted folder + var nestedFolderInDeleted = SendableItemMetadata(ocId: "nestedFolderInDeleted", fileName: "NestedFolder", account: Self.account) + nestedFolderInDeleted.directory = true + nestedFolderInDeleted.visitedDirectory = false + nestedFolderInDeleted.serverUrl = Self.account.davFilesUrl + "/DeletedDir" + nestedFolderInDeleted.syncTime = recentSyncDate + Self.dbManager.addItemMetadata(nestedFolderInDeleted) + + // Deep nested item under deleted structure + var deepNestedInDeleted = SendableItemMetadata(ocId: "deepNestedInDeleted", fileName: "deepNested.txt", account: Self.account) + deepNestedInDeleted.serverUrl = Self.account.davFilesUrl + "/DeletedDir/NestedFolder" + deepNestedInDeleted.syncTime = recentSyncDate + deepNestedInDeleted.downloaded = false + Self.dbManager.addItemMetadata(deepNestedInDeleted) + + // STANDALONE ITEMS: Materialised file synced recently - should be returned var standaloneUpdatedFile = SendableItemMetadata(ocId: "standaloneUpdated", fileName: "standalone.txt", account: Self.account) standaloneUpdatedFile.downloaded = true // Materialised standaloneUpdatedFile.syncTime = recentSyncDate Self.dbManager.addItemMetadata(standaloneUpdatedFile) - // A materialised file synced too long ago. Should NOT be returned. + // Materialised file synced too long ago - should NOT be returned var standaloneOldFile = SendableItemMetadata(ocId: "standaloneOld", fileName: "old.txt", account: Self.account) standaloneOldFile.downloaded = true // Materialised standaloneOldFile.syncTime = oldSyncDate Self.dbManager.addItemMetadata(standaloneOldFile) - // A non-materialised item synced recently. Should NOT be returned. + // Non-materialised item synced recently - should NOT be returned (not in initial materialised set) var nonMaterialisedFile = SendableItemMetadata(ocId: "nonMaterialised", fileName: "non-mat.txt", account: Self.account) nonMaterialisedFile.downloaded = false nonMaterialisedFile.syncTime = recentSyncDate Self.dbManager.addItemMetadata(nonMaterialisedFile) + // MIXED MATERIALISATION: Another materialised folder to test child inclusion + var anotherMaterialisedDir = SendableItemMetadata(ocId: "anotherMatDir", fileName: "AnotherMatDir", account: Self.account) + anotherMaterialisedDir.directory = true + anotherMaterialisedDir.visitedDirectory = true + anotherMaterialisedDir.syncTime = recentSyncDate + Self.dbManager.addItemMetadata(anotherMaterialisedDir) + + // Child with recent sync but NOT materialised - should still be included due to recent sync + var nonMatChildRecent = SendableItemMetadata(ocId: "nonMatChildRecent", fileName: "nonMatChild.txt", account: Self.account) + nonMatChildRecent.serverUrl = Self.account.davFilesUrl + "/AnotherMatDir" + nonMatChildRecent.syncTime = recentSyncDate + nonMatChildRecent.downloaded = false // Not materialised + Self.dbManager.addItemMetadata(nonMatChildRecent) + // 2. Act - // Fetch changes that have occurred since the anchor date. let result = Self.dbManager.pendingWorkingSetChanges( account: Self.account, since: anchorDate ) - // 3. Assert - // Verify the 'updated' results + // 3. Assert - Updated items let updatedIds = Set(result.updated.map { $0.ocId }) - XCTAssertEqual(updatedIds.count, 3, "There should be three updated items: the folder, its child, and the standalone file.") - XCTAssertTrue(updatedIds.contains("updatedDir")) - XCTAssertTrue(updatedIds.contains("childOfUpdated"), "The child of the updated directory should be included.") - XCTAssertTrue(updatedIds.contains("standaloneUpdated")) - - // Verify the 'deleted' results + + // Should include materialised items with recent sync + XCTAssertTrue(updatedIds.contains("updatedDir"), "Updated materialised directory should be included") + XCTAssertTrue(updatedIds.contains("standaloneUpdated"), "Updated materialised file should be included") + XCTAssertTrue(updatedIds.contains("anotherMatDir"), "Another materialised directory should be included") + + // Should include children with recent sync regardless of materialisation + XCTAssertTrue(updatedIds.contains("childOfUpdatedRecent"), "Child with recent sync should be included regardless of materialisation") + XCTAssertTrue(updatedIds.contains("childFolderOfUpdated"), "Child folder with recent sync should be included") + XCTAssertTrue(updatedIds.contains("nonMatChildRecent"), "Non-materialised child with recent sync should be included") + + // Should NOT include items with old sync times + XCTAssertFalse(updatedIds.contains("childOfUpdatedOld"), "Child with old sync time should NOT be included") + XCTAssertFalse(updatedIds.contains("standaloneOld"), "Materialised file with old sync should NOT be included") + + // Should NOT include non-materialised items not under a recently updated path + XCTAssertFalse(updatedIds.contains("nonMaterialised"), "Standalone non-materialised item should NOT be included") + + // 4. Assert - Deleted items let deletedIds = Set(result.deleted.map { $0.ocId }) - XCTAssertEqual(deletedIds.count, 2, "There should be two deleted items: the folder and its child.") - XCTAssertTrue(deletedIds.contains("deletedDir")) - XCTAssertTrue(deletedIds.contains("childOfDeleted"), "The child of the deleted directory should be included.") + + // Should include the deleted materialised directory + XCTAssertTrue(deletedIds.contains("deletedDir"), "Deleted materialised directory should be included") + + // Should include children/descendants with recent sync under deleted paths + XCTAssertTrue(deletedIds.contains("childOfDeletedRecent"), "Child of deleted dir with recent sync should be included") + XCTAssertTrue(deletedIds.contains("nestedFolderInDeleted"), "Nested folder under deleted dir should be included") + XCTAssertTrue(deletedIds.contains("deepNestedInDeleted"), "Deep nested item under deleted structure should be included") + + // Should NOT include children with old sync times + XCTAssertFalse(deletedIds.contains("childOfDeletedOld"), "Child of deleted dir with old sync should NOT be included") + + // 5. Verify expected counts + let expectedUpdatedCount = 6 // updatedDir, standaloneUpdated, anotherMatDir, childOfUpdatedRecent, childFolderOfUpdated, nonMatChildRecent + let expectedDeletedCount = 4 // deletedDir, childOfDeletedRecent, nestedFolderInDeleted, deepNestedInDeleted + + XCTAssertEqual(updatedIds.count, expectedUpdatedCount, "Should have \(expectedUpdatedCount) updated items, got \(updatedIds.count): \(updatedIds)") + XCTAssertEqual(deletedIds.count, expectedDeletedCount, "Should have \(expectedDeletedCount) deleted items, got \(deletedIds.count): \(deletedIds)") } } From 11b7c94edc7c1bf1f111a60357830fa810b3b356 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Fri, 4 Jul 2025 18:56:28 +0800 Subject: [PATCH 35/42] Test for updated sync time on deleted item during remote change observation change check Signed-off-by: Claudio Cambra --- .../RemoteChangeObserverTests.swift | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift index 935a8796..8cafd18e 100644 --- a/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift +++ b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift @@ -189,6 +189,8 @@ final class RemoteChangeObserverTests: XCTestCase { SendableItemMetadata(ocId: "fileInA", fileName: "file-in-a.txt", account: Self.account) fileInAToDelete.downloaded = true fileInAToDelete.serverUrl = Self.account.davFilesUrl + "/FolderA" + // Set an explicit old sync time to verify it gets updated during deletion + fileInAToDelete.syncTime = Date(timeIntervalSince1970: 1000) dbManager.addItemMetadata(fileInAToDelete) // A materialised folder that will be deleted entirely. @@ -196,8 +198,14 @@ final class RemoteChangeObserverTests: XCTestCase { SendableItemMetadata(ocId: "folderB", fileName: "FolderB", account: Self.account) folderBToDelete.directory = true folderBToDelete.visitedDirectory = true + // Set an explicit old sync time to verify it gets updated during deletion + folderBToDelete.syncTime = Date(timeIntervalSince1970: 2000) dbManager.addItemMetadata(folderBToDelete) + // Record original sync times to verify they are updated during deletion + let originalFileInASyncTime = try XCTUnwrap(dbManager.itemMetadata(ocId: "fileInA")).syncTime + let originalFolderBSyncTime = try XCTUnwrap(dbManager.itemMetadata(ocId: "folderB")).syncTime + // --- Server State (The new "remote truth") --- let rootItem = remoteInterface.rootItem! @@ -281,11 +289,17 @@ final class RemoteChangeObserverTests: XCTestCase { XCTAssertTrue( deletedFileInA.deleted, "File inside updated folder should be marked as deleted." ) - XCTAssertTrue(deletedFileInA.syncTime >= testStartDate) + XCTAssertTrue(deletedFileInA.syncTime >= testStartDate, + "Deleted file's sync time should be updated to current time") + XCTAssertGreaterThan(deletedFileInA.syncTime, originalFileInASyncTime, + "Deleted file's sync time should be newer than original sync time") let deletedFolderB = try XCTUnwrap(dbManager.itemMetadata(ocId: "folderB")) XCTAssertTrue(deletedFolderB.deleted, "The entire folder should be marked as deleted.") - XCTAssertTrue(deletedFolderB.syncTime >= testStartDate) + XCTAssertTrue(deletedFolderB.syncTime >= testStartDate, + "Deleted folder's sync time should be updated to current time") + XCTAssertGreaterThan(deletedFolderB.syncTime, originalFolderBSyncTime, + "Deleted folder's sync time should be newer than original sync time") } func testIgnoreNonFileNotifications() async throws { From b6ead3ef50167a943aefdd96f44ef15fc52586d5 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Fri, 4 Jul 2025 19:09:47 +0800 Subject: [PATCH 36/42] Test for visitedDirectory state in enumerator tests Signed-off-by: Claudio Cambra --- .../EnumeratorTests.swift | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) diff --git a/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift b/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift index 519b343d..a37c23a4 100644 --- a/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift +++ b/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift @@ -1413,4 +1413,179 @@ final class EnumeratorTests: XCTestCase { ) } } + + func testVisitedDirectoryStatePreservedDuringUpdate() async throws { + // This test verifies that visitedDirectory state is preserved when updating + // existing folder metadata during enumeration, addressing the fix in FilesDatabaseManager + let db = Self.dbManager.ncDatabase() + debugPrint(db) + let remoteInterface = MockRemoteInterface(rootItem: rootItem) + + // Setup root container metadata in database (required for enumeration) + Self.dbManager.addItemMetadata(rootItem.toItemMetadata(account: Self.account)) + + // 1. Setup: Create a folder that has already been visited (visitedDirectory = true) + var existingFolderMetadata = remoteFolder.toItemMetadata(account: Self.account) + existingFolderMetadata.visitedDirectory = true + existingFolderMetadata.etag = "OLD_ETAG" + Self.dbManager.addItemMetadata(existingFolderMetadata) + + // Verify initial state + let initialMetadata = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: remoteFolder.identifier)) + XCTAssertTrue(initialMetadata.visitedDirectory, "Folder should initially be marked as visited") + XCTAssertEqual(initialMetadata.etag, "OLD_ETAG") + + // 2. Act: Enumerate the folder, which will trigger an update + let enumerator = Enumerator( + enumeratedItemIdentifier: .init(remoteFolder.identifier), + account: Self.account, + remoteInterface: remoteInterface, + dbManager: Self.dbManager + ) + let observer = MockEnumerationObserver(enumerator: enumerator) + try await observer.enumerateItems() + + // 3. Assert: Verify that visitedDirectory state is preserved after update + let updatedMetadata = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: remoteFolder.identifier)) + XCTAssertTrue(updatedMetadata.visitedDirectory, + "visitedDirectory state should be preserved during update") + XCTAssertEqual(updatedMetadata.etag, remoteFolder.versionIdentifier, + "ETag should be updated to new value") + XCTAssertNotEqual(updatedMetadata.etag, "OLD_ETAG", + "ETag should have changed from old value") + } + + func testVisitedDirectorySetDuringDirectoryRead() async throws { + // This test verifies that visitedDirectory is correctly set to true + // when a directory is the target of a depth-1 read operation + let db = Self.dbManager.ncDatabase() + debugPrint(db) + let remoteInterface = MockRemoteInterface(rootItem: rootItem) + + // Setup root container metadata in database (required for enumeration) + Self.dbManager.addItemMetadata(rootItem.toItemMetadata(account: Self.account)) + + // 1. Setup: Create a new folder that hasn't been visited yet + var newFolderMetadata = remoteFolder.toItemMetadata(account: Self.account) + newFolderMetadata.visitedDirectory = false + newFolderMetadata.etag = "INITIAL_ETAG" + Self.dbManager.addItemMetadata(newFolderMetadata) + + // Verify initial state + let initialMetadata = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: remoteFolder.identifier)) + XCTAssertFalse(initialMetadata.visitedDirectory, "Folder should initially not be marked as visited") + + // 2. Act: Enumerate the folder (depth-1 read) + let enumerator = Enumerator( + enumeratedItemIdentifier: .init(remoteFolder.identifier), + account: Self.account, + remoteInterface: remoteInterface, + dbManager: Self.dbManager + ) + let observer = MockEnumerationObserver(enumerator: enumerator) + try await observer.enumerateItems() + + // 3. Assert: Verify that visitedDirectory is now set to true + let updatedMetadata = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: remoteFolder.identifier)) + XCTAssertTrue(updatedMetadata.visitedDirectory, + "visitedDirectory should be set to true after directory enumeration") + XCTAssertEqual(updatedMetadata.etag, remoteFolder.versionIdentifier, + "ETag should be updated") + } + + func testVisitedDirectoryStateInWorkingSet() async throws { + // This test verifies that folders marked as visitedDirectory appear in working set + // and that the state is preserved correctly across operations + let db = Self.dbManager.ncDatabase() + debugPrint(db) + let remoteInterface = MockRemoteInterface(rootItem: rootItem) + + // Setup root container metadata in database (required for enumeration) + Self.dbManager.addItemMetadata(rootItem.toItemMetadata(account: Self.account)) + + // 1. Setup: Create folders with different visited states + var visitedFolder = remoteFolder.toItemMetadata(account: Self.account) + visitedFolder.visitedDirectory = true + visitedFolder.downloaded = false // Not downloaded, but visited + Self.dbManager.addItemMetadata(visitedFolder) + + var notVisitedFolder = MockRemoteItem( + identifier: "notVisitedFolder", + versionIdentifier: "V1", + name: "NotVisitedFolder", + remotePath: Self.account.davFilesUrl + "/NotVisitedFolder", + directory: true, + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ).toItemMetadata(account: Self.account) + notVisitedFolder.visitedDirectory = false + notVisitedFolder.downloaded = false + Self.dbManager.addItemMetadata(notVisitedFolder) + + // 2. Act: Enumerate working set + let workingSetEnumerator = Enumerator( + enumeratedItemIdentifier: .workingSet, + account: Self.account, + remoteInterface: remoteInterface, + dbManager: Self.dbManager + ) + let workingSetObserver = MockEnumerationObserver(enumerator: workingSetEnumerator) + try await workingSetObserver.enumerateItems() + + // 3. Assert: Only visited folder should be in working set + let workingSetIds = Set(workingSetObserver.items.map(\.itemIdentifier.rawValue)) + XCTAssertTrue(workingSetIds.contains(visitedFolder.ocId), + "Visited folder should be in working set") + XCTAssertFalse(workingSetIds.contains(notVisitedFolder.ocId), + "Non-visited folder should not be in working set") + + // 4. Act: Now enumerate the not-visited folder to make it visited + // Add the not-visited folder to the remote structure for enumeration + let notVisitedRemoteFolder = MockRemoteItem( + identifier: notVisitedFolder.ocId, + versionIdentifier: "V2", + name: notVisitedFolder.fileName, + remotePath: notVisitedFolder.serverUrl + "/" + notVisitedFolder.fileName, + directory: true, + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + rootItem.children.append(notVisitedRemoteFolder) + notVisitedRemoteFolder.parent = rootItem + + let folderEnumerator = Enumerator( + enumeratedItemIdentifier: .init(notVisitedFolder.ocId), + account: Self.account, + remoteInterface: remoteInterface, + dbManager: Self.dbManager + ) + let folderObserver = MockEnumerationObserver(enumerator: folderEnumerator) + try await folderObserver.enumerateItems() + + // 5. Assert: Verify the folder is now marked as visited + let updatedNotVisitedMetadata = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: notVisitedFolder.ocId)) + XCTAssertTrue(updatedNotVisitedMetadata.visitedDirectory, + "Folder should now be marked as visited after enumeration") + + // 6. Act: Enumerate working set again + let workingSetEnumerator2 = Enumerator( + enumeratedItemIdentifier: .workingSet, + account: Self.account, + remoteInterface: remoteInterface, + dbManager: Self.dbManager + ) + let workingSetObserver2 = MockEnumerationObserver(enumerator: workingSetEnumerator2) + try await workingSetObserver2.enumerateItems() + + // 7. Assert: Both folders should now be in working set + let workingSetIds2 = Set(workingSetObserver2.items.map(\.itemIdentifier.rawValue)) + XCTAssertTrue(workingSetIds2.contains(visitedFolder.ocId), + "Original visited folder should still be in working set") + XCTAssertTrue(workingSetIds2.contains(notVisitedFolder.ocId), + "Newly visited folder should now be in working set") + } } From b25a3a15bc4b0bbe6629085b0fdb819bb655b29b Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Mon, 7 Jul 2025 18:24:58 +0800 Subject: [PATCH 37/42] Add method to invalidate the RCO Signed-off-by: Claudio Cambra --- .../Enumeration/RemoteChangeObserver.swift | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift index e2c6026c..873d9095 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift @@ -31,6 +31,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb private let logger = Logger(subsystem: Logger.subsystem, category: "changeobserver") private var workingSetCheckOngoing = false + private var invalidated = false private var webSocketUrlSession: URLSession? private var webSocketTask: URLSessionWebSocketTask? @@ -81,6 +82,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb } private func startPollingTimer() { + guard !invalidated else { return } Task { @MainActor in pollingTimer = Timer.scheduledTimer( withTimeInterval: pollInterval, repeats: true @@ -100,6 +102,11 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb } } + public func invalidate() { + invalidated = true + resetWebSocket() + } + public func connect() { // Authentication fixes require some type of user or external change. // We don't want to reset the auth tries within reconnect web socket as this is called @@ -144,6 +151,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb } private func configureNotifyPush() async { + guard !invalidated else { return } let (_, capabilities, _, error) = await remoteInterface.currentCapabilities( account: account, options: .init(), @@ -210,6 +218,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb didReceive challenge: URLAuthenticationChallenge, completionHandler: @escaping (URLSession.AuthChallengeDisposition, URLCredential?) -> Void ) { + guard !invalidated else { return } let authMethod = challenge.protectionSpace.authenticationMethod logger.debug("Received auth challenge with method: \(authMethod, privacy: .public)") if authMethod == NSURLAuthenticationMethodHTTPBasic { @@ -240,6 +249,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb webSocketTask: URLSessionWebSocketTask, didOpenWithProtocol protocol: String? ) { + guard !invalidated else { return } logger.debug("Websocket connected \(self.accountId, privacy: .public) sending auth details") Task { await authenticateWebSocket() } } @@ -250,6 +260,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb didCloseWith closeCode: URLSessionWebSocketTask.CloseCode, reason: Data? ) { + guard !invalidated else { return } // If the task that closed is not the current active task, it means we have // already initiated a reset and this is a stale callback. Ignore it. guard webSocketTask === self.webSocketTask else { @@ -266,6 +277,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb } private func authenticateWebSocket() async { + guard !invalidated else { return } do { try await webSocketTask?.send(.string(account.username)) try await webSocketTask?.send(.string(account.password)) @@ -281,7 +293,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb } private func startNewWebSocketPingTask() { - guard !Task.isCancelled else { return } + guard !Task.isCancelled, !invalidated else { return } if let webSocketPingTask, !webSocketPingTask.isCancelled { webSocketPingTask.cancel() @@ -304,13 +316,14 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb } private func pingWebSocket() { // Keep the socket connection alive + guard !invalidated else { return } guard networkReachability != .notReachable else { logger.error("Not pinging \(self.accountId, privacy: .public), network is unreachable") return } webSocketTask?.sendPing { [weak self] error in - guard let self else { return } + guard let self, !self.invalidated else { return } guard error == nil else { self.logger.warning( """ @@ -331,6 +344,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb } private func readWebSocket() { + guard !invalidated else { return } webSocketTask?.receive { result in switch result { case .failure: @@ -351,6 +365,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb } private func processWebsocket(data: Data) { + guard !invalidated else { return } guard let string = String(data: data, encoding: .utf8) else { logger.error("Could parse websocket data for id: \(self.accountId, privacy: .public)") return @@ -453,7 +468,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb ) { } private func startWorkingSetCheck() { - guard !workingSetCheckOngoing else { return } + guard !workingSetCheckOngoing, !invalidated else { return } Task { await checkWorkingSet() } } @@ -478,6 +493,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb var allDeletedMetadatas = [SendableItemMetadata]() var examinedItemIds = Set() for item in materialisedItems where !examinedItemIds.contains(item.ocId) { + guard !invalidated else { return } let itemRemoteUrl = item.serverUrl + "/" + item.fileName let ( metadatas, newMetadatas, updatedMetadatas, deletedMetadatas, _, readError @@ -541,6 +557,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb examinedItemIds.formUnion(examinedChildFilesAndDeletedItems) } } + guard !invalidated else { return } // Run a check to ensure files deleted in one location are not updated in another // (e.g. when moved) From 43fd059de914208b17f970149ecbe7da0b608ae3 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Mon, 7 Jul 2025 18:25:21 +0800 Subject: [PATCH 38/42] Use weak self in websocket reconnect task Signed-off-by: Claudio Cambra --- .../Enumeration/RemoteChangeObserver.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift index 873d9095..da93471d 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift @@ -133,9 +133,9 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb startPollingTimer() return } - Task { - try await Task.sleep(nanoseconds: webSocketReconfigureIntervalNanoseconds) - await self.configureNotifyPush() + Task { [weak self] in + try await Task.sleep(nanoseconds: self?.webSocketReconfigureIntervalNanoseconds ?? 0) + await self?.configureNotifyPush() } } From aca79689e67a00f98096d5488d997ba3ecf411c6 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 8 Jul 2025 17:11:46 +0800 Subject: [PATCH 39/42] Also handle invalidation after server read Signed-off-by: Claudio Cambra --- .../Enumeration/RemoteChangeObserver.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift index da93471d..02d2379a 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift @@ -504,6 +504,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb dbManager: dbManager, depth: item.directory ? .targetAndDirectChildren : .target ) + guard !invalidated else { return } if readError?.errorCode == 404 { allDeletedMetadatas.append(item) examinedItemIds.insert(item.ocId) From e65461a8252f7f2edccd2441d8096f309cb87579 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 8 Jul 2025 17:12:19 +0800 Subject: [PATCH 40/42] Ensure unchanged directories don't get examined even if they are materialised Signed-off-by: Claudio Cambra --- .../Enumeration/RemoteChangeObserver.swift | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift index 02d2379a..d6ab9119 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift @@ -550,6 +550,28 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb examinedChildFilesAndDeletedItems.formUnion(materialisedChildren) } + // OPTIMIZATION: For any child directories returned in this enumeration, + // if they haven't changed (etag matches database), mark them as examined + // so we don't enumerate them separately later + if metadatas.count > 1 { + let childDirectories = metadatas[1...].filter { $0.directory } + for childDir in childDirectories { + // Check if this directory is in our materialized items list + if let localItem = materialisedItems.first(where: { $0.ocId == childDir.ocId }), + localItem.etag == childDir.etag { + // Directory hasn't changed, mark as examined to skip separate enumeration + logger.debug("Child directory \(childDir.fileName, privacy: .public) etag unchanged (\(childDir.etag, privacy: .public)), marking as examined") + examinedChildFilesAndDeletedItems.insert(childDir.ocId) + + // Also mark any materialized children of this directory as examined + let grandChildren = materialisedItems.filter { + $0.serverUrl.hasPrefix(localItem.serverUrl + "/" + localItem.fileName) + } + examinedChildFilesAndDeletedItems.formUnion(grandChildren.map(\.ocId)) + } + } + } + if let deletedMetadataOcIds = deletedMetadatas?.map(\.ocId) { examinedChildFilesAndDeletedItems.formUnion(deletedMetadataOcIds) } From 1350e18388807db2f89b3ff9ace00df80f21997f Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 8 Jul 2025 17:13:03 +0800 Subject: [PATCH 41/42] Add RCO optimisation test Signed-off-by: Claudio Cambra --- .../Enumeration/RemoteChangeObserver.swift | 2 +- Tests/Interface/MockRemoteInterface.swift | 7 + ...eChangeObserverEtagOptimizationTests.swift | 164 ++++++++++++++++++ 3 files changed, 172 insertions(+), 1 deletion(-) create mode 100644 Tests/NextcloudFileProviderKitTests/RemoteChangeObserverEtagOptimizationTests.swift diff --git a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift index d6ab9119..df817985 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/RemoteChangeObserver.swift @@ -467,7 +467,7 @@ public class RemoteChangeObserver: NSObject, NextcloudKitDelegate, URLSessionWeb _ request: Alamofire.DataRequest, didParseResponse response: Alamofire.AFDataResponse ) { } - private func startWorkingSetCheck() { + func startWorkingSetCheck() { guard !workingSetCheckOngoing, !invalidated else { return } Task { await checkWorkingSet() } } diff --git a/Tests/Interface/MockRemoteInterface.swift b/Tests/Interface/MockRemoteInterface.swift index 15894b71..6d23222f 100644 --- a/Tests/Interface/MockRemoteInterface.swift +++ b/Tests/Interface/MockRemoteInterface.swift @@ -568,6 +568,9 @@ public class MockRemoteInterface: RemoteInterface { public var pagination: Bool public var expectedEnumerationPaginationTokens: [String: String] = [:] public var forceNextPageOnLastContentPage: Bool = false + + // Handler to track enumerate calls + public var enumerateCallHandler: ((String, EnumerateDepth, Bool, [String], Data?, Account, NKRequestOptions, @escaping (URLSessionTask) -> Void) -> Void)? public init( rootItem: MockRemoteItem? = nil, @@ -1041,6 +1044,10 @@ public class MockRemoteInterface: RemoteInterface { remotePath.removeLast() } print("Enumerating \(remotePath)") + + // Call the enumerate call handler if it exists + enumerateCallHandler?(remotePath, depth, showHiddenFiles, includeHiddenFiles, requestBody, account, options, taskHandler) + guard let item = item(remotePath: remotePath, account: account) else { print("Item at \(remotePath) not found.") return ( diff --git a/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverEtagOptimizationTests.swift b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverEtagOptimizationTests.swift new file mode 100644 index 00000000..e68b74ae --- /dev/null +++ b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverEtagOptimizationTests.swift @@ -0,0 +1,164 @@ +// +// RemoteChangeObserverEtagOptimizationTests.swift +// +// +// Created by Claudio Cambra on 07/07/25. +// + +import Foundation +import NextcloudCapabilitiesKit +import RealmSwift +import TestInterface +import XCTest +@testable import NextcloudFileProviderKit + +@available(macOS 14.0, iOS 17.0, *) +final class RemoteChangeObserverEtagOptimizationTests: XCTestCase { + static let account = Account( + user: "testUser", id: "testUserId", serverUrl: "localhost", password: "abcd" + ) + + var dbManager: FilesDatabaseManager! + var mockRemoteInterface: MockRemoteInterface! + + override func setUp() { + Realm.Configuration.defaultConfiguration.inMemoryIdentifier = name + dbManager = FilesDatabaseManager(realmConfig: .defaultConfiguration, account: Self.account) + mockRemoteInterface = MockRemoteInterface(rootItem: MockRemoteItem.rootItem(account: Self.account)) + } + + func testUnchangedDirectoryShouldNotBeEnumerated() async throws { + // This test demonstrates the original issue where multiple working set checks + // would repeatedly enumerate the same unchanged folders + + // 1. Setup: Create a materialized root directory and subdirectory + var rootFolder = SendableItemMetadata( + ocId: "root", fileName: "", account: Self.account + ) + rootFolder.directory = true + rootFolder.visitedDirectory = true + rootFolder.etag = "rootetag123" + rootFolder.serverUrl = Self.account.davFilesUrl + dbManager.addItemMetadata(rootFolder) + + var customersFolder = SendableItemMetadata( + ocId: "customers", fileName: "Customers", account: Self.account + ) + customersFolder.directory = true + customersFolder.visitedDirectory = true + customersFolder.etag = "68662da77122d" // The etag from the logs + dbManager.addItemMetadata(customersFolder) + + // Add some child files + var childFile1 = SendableItemMetadata( + ocId: "child1", fileName: "child1.txt", account: Self.account + ) + childFile1.downloaded = true + childFile1.serverUrl = Self.account.davFilesUrl + "/Customers" + dbManager.addItemMetadata(childFile1) + + var childFile2 = SendableItemMetadata( + ocId: "child2", fileName: "child2.txt", account: Self.account + ) + childFile2.downloaded = true + childFile2.serverUrl = Self.account.davFilesUrl + "/Customers" + dbManager.addItemMetadata(childFile2) + + // 2. Setup server to return same etag (no changes) + let serverCustomersFolder = MockRemoteItem( + identifier: "customers", + versionIdentifier: "68662da77122d", // Same etag - no changes + name: "Customers", + remotePath: Self.account.davFilesUrl + "/Customers", + directory: true, + account: Self.account.ncKitAccount, + username: Self.account.username, + userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + + // Add the same children to server + let serverChild1 = MockRemoteItem( + identifier: "child1", name: "child1.txt", + remotePath: serverCustomersFolder.remotePath + "/child1.txt", + account: Self.account.ncKitAccount, + username: Self.account.username, userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + let serverChild2 = MockRemoteItem( + identifier: "child2", name: "child2.txt", + remotePath: serverCustomersFolder.remotePath + "/child2.txt", + account: Self.account.ncKitAccount, + username: Self.account.username, userId: Self.account.id, + serverUrl: Self.account.serverUrl + ) + serverCustomersFolder.children = [serverChild1, serverChild2] + mockRemoteInterface.rootItem?.children = [serverCustomersFolder] + + // 3. Track how many times enumerate is called and for which paths + var enumerateCallCount = 0 + var enumeratedPaths: [String] = [] + mockRemoteInterface.enumerateCallHandler = { remotePath, depth, showHiddenFiles, includeHiddenFiles, requestBody, account, options, taskHandler in + enumerateCallCount += 1 + enumeratedPaths.append(remotePath) + print("ENUMERATE CALLED #\(enumerateCallCount) for: \(remotePath) (depth: \(depth))") + } + + // 4. Create observer and manually trigger working set check + let changeNotificationInterface = MockChangeNotificationInterface() + let remoteChangeObserver = RemoteChangeObserver( + account: Self.account, + remoteInterface: mockRemoteInterface, + changeNotificationInterface: changeNotificationInterface, + domain: nil, + dbManager: dbManager + ) + + // 5. Debug: Check what materialized items we have + let materializedItems = dbManager.materialisedItemMetadatas(account: Self.account.ncKitAccount) + print("Materialized items found: \(materializedItems.count)") + for item in materializedItems { + print(" - \(item.fileName) (ocId: \(item.ocId), directory: \(item.directory), etag: \(item.etag))") + } + + // 6. Simulate the original issue: multiple working set checks in quick succession + // This would happen when multiple notify_file messages arrive or polling occurs frequently + print("\n=== Running multiple working set checks ===") + + // First working set check + remoteChangeObserver.startWorkingSetCheck() + try await Task.sleep(nanoseconds: 100_000_000) // 0.1 seconds + + // Second working set check (simulating rapid notify_file messages) + remoteChangeObserver.startWorkingSetCheck() + try await Task.sleep(nanoseconds: 100_000_000) // 0.1 seconds + + // Third working set check + remoteChangeObserver.startWorkingSetCheck() + try await Task.sleep(nanoseconds: 100_000_000) // 0.1 seconds + + // Wait for all operations to complete + try await Task.sleep(nanoseconds: 500_000_000) // 0.5 seconds + + print("\n=== Results ===") + print("Total enumerate calls: \(enumerateCallCount)") + print("Enumerated paths: \(enumeratedPaths)") + + // 7. Assert: With the optimization, we should not make excessive enumerate calls + // Each unique path should only be enumerated once or very few times + XCTAssertGreaterThan(enumerateCallCount, 0, "At least one enumerate call should be made") + + // Count how many times the Customers folder was enumerated + let customersEnumerateCount = enumeratedPaths.filter { + $0.contains("Customers") + }.count + + print("Customers folder enumerated \(customersEnumerateCount) times") + + // The key optimization we want: the same folder with unchanged etag shouldn't be + // enumerated repeatedly. Ideally, it should be enumerated only once. + // However, without optimization, it might be enumerated 3 times (once per working set check) + XCTAssertLessThanOrEqual(customersEnumerateCount, 1, + "Customers folder with unchanged etag should not be enumerated repeatedly") + } +} From 67de21f67f8c04c012fb339cccd2c5a678814e57 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 8 Jul 2025 17:27:52 +0800 Subject: [PATCH 42/42] Remove duplicate dbManager from RCO tests, just use one Signed-off-by: Claudio Cambra --- .../RemoteChangeObserverTests.swift | 40 +++++++++---------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift index 8cafd18e..c814eef7 100644 --- a/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift +++ b/Tests/NextcloudFileProviderKitTests/RemoteChangeObserverTests.swift @@ -36,11 +36,9 @@ final class RemoteChangeObserverTests: XCTestCase { eventLoopGroup: .singleton ) var remoteChangeObserver: RemoteChangeObserver? - var dbManager: FilesDatabaseManager! override func setUp() { Realm.Configuration.defaultConfiguration.inMemoryIdentifier = name - dbManager = FilesDatabaseManager(realmConfig: .defaultConfiguration, account: Self.account) Task { try await Self.notifyPushServer.run() } } @@ -160,7 +158,7 @@ final class RemoteChangeObserverTests: XCTestCase { func testChangeRecognised() async throws { // 1. Arrange - let db = dbManager.ncDatabase() + let db = Self.dbManager.ncDatabase() debugPrint(db) let testStartDate = Date() @@ -174,7 +172,7 @@ final class RemoteChangeObserverTests: XCTestCase { SendableItemMetadata(ocId: "rootFile", fileName: "root-file.txt", account: Self.account) rootFileToUpdate.downloaded = true rootFileToUpdate.etag = "ETAG_OLD_ROOTFILE" - dbManager.addItemMetadata(rootFileToUpdate) + Self.dbManager.addItemMetadata(rootFileToUpdate) // A materialised folder that will have its contents changed. var folderA = @@ -182,7 +180,7 @@ final class RemoteChangeObserverTests: XCTestCase { folderA.directory = true folderA.visitedDirectory = true folderA.etag = "ETAG_OLD_FOLDERA" - dbManager.addItemMetadata(folderA) + Self.dbManager.addItemMetadata(folderA) // A materialised file inside FolderA that will be deleted. var fileInAToDelete = @@ -191,7 +189,7 @@ final class RemoteChangeObserverTests: XCTestCase { fileInAToDelete.serverUrl = Self.account.davFilesUrl + "/FolderA" // Set an explicit old sync time to verify it gets updated during deletion fileInAToDelete.syncTime = Date(timeIntervalSince1970: 1000) - dbManager.addItemMetadata(fileInAToDelete) + Self.dbManager.addItemMetadata(fileInAToDelete) // A materialised folder that will be deleted entirely. var folderBToDelete = @@ -200,11 +198,11 @@ final class RemoteChangeObserverTests: XCTestCase { folderBToDelete.visitedDirectory = true // Set an explicit old sync time to verify it gets updated during deletion folderBToDelete.syncTime = Date(timeIntervalSince1970: 2000) - dbManager.addItemMetadata(folderBToDelete) + Self.dbManager.addItemMetadata(folderBToDelete) // Record original sync times to verify they are updated during deletion - let originalFileInASyncTime = try XCTUnwrap(dbManager.itemMetadata(ocId: "fileInA")).syncTime - let originalFolderBSyncTime = try XCTUnwrap(dbManager.itemMetadata(ocId: "folderB")).syncTime + let originalFileInASyncTime = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: "fileInA")).syncTime + let originalFolderBSyncTime = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: "folderB")).syncTime // --- Server State (The new "remote truth") --- let rootItem = remoteInterface.rootItem! @@ -259,7 +257,7 @@ final class RemoteChangeObserverTests: XCTestCase { remoteInterface: remoteInterface, changeNotificationInterface: notificationInterface, domain: nil, - dbManager: dbManager + dbManager: Self.dbManager ) // 2. Act & Assert @@ -271,21 +269,21 @@ final class RemoteChangeObserverTests: XCTestCase { // 3. Assert Database State // Check updated items - let finalRootFile = try XCTUnwrap(dbManager.itemMetadata(ocId: "rootFile")) + let finalRootFile = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: "rootFile")) XCTAssertEqual(finalRootFile.etag, "ETAG_NEW_ROOTFILE") XCTAssertTrue(finalRootFile.syncTime >= testStartDate) - let finalFolderA = try XCTUnwrap(dbManager.itemMetadata(ocId: "folderA")) + let finalFolderA = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: "folderA")) XCTAssertEqual(finalFolderA.etag, "ETAG_NEW_FOLDERA") XCTAssertTrue(finalFolderA.syncTime >= testStartDate) // Check new item - let finalNewFile = try XCTUnwrap(dbManager.itemMetadata(ocId: "newFileInA")) + let finalNewFile = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: "newFileInA")) XCTAssertEqual(finalNewFile.fileName, "new-file.txt") XCTAssertNotNil(finalNewFile.syncTime) // Check deleted items - let deletedFileInA = try XCTUnwrap(dbManager.itemMetadata(ocId: "fileInA")) + let deletedFileInA = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: "fileInA")) XCTAssertTrue( deletedFileInA.deleted, "File inside updated folder should be marked as deleted." ) @@ -294,7 +292,7 @@ final class RemoteChangeObserverTests: XCTestCase { XCTAssertGreaterThan(deletedFileInA.syncTime, originalFileInASyncTime, "Deleted file's sync time should be newer than original sync time") - let deletedFolderB = try XCTUnwrap(dbManager.itemMetadata(ocId: "folderB")) + let deletedFolderB = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: "folderB")) XCTAssertTrue(deletedFolderB.deleted, "The entire folder should be marked as deleted.") XCTAssertTrue(deletedFolderB.syncTime >= testStartDate, "Deleted folder's sync time should be updated to current time") @@ -347,7 +345,7 @@ final class RemoteChangeObserverTests: XCTestCase { func testPolling() async throws { // 1. Arrange - let db = dbManager.ncDatabase() + let db = Self.dbManager.ncDatabase() debugPrint(db) let remoteInterface = MockRemoteInterface(rootItem: MockRemoteItem.rootItem(account: Self.account)) @@ -358,7 +356,7 @@ final class RemoteChangeObserverTests: XCTestCase { var fileToUpdate = SendableItemMetadata(ocId: "item1", fileName: "file.txt", account: Self.account) fileToUpdate.downloaded = true fileToUpdate.etag = "ETAG_OLD" - dbManager.addItemMetadata(fileToUpdate) + Self.dbManager.addItemMetadata(fileToUpdate) // Server State: The same file now has a new ETag. let serverItem = MockRemoteItem(identifier: "item1", versionIdentifier: "ETAG_NEW", name: "file.txt", remotePath: Self.account.davFilesUrl + "/file.txt", account: Self.account.ncKitAccount, username: Self.account.username, userId: Self.account.id, serverUrl: Self.account.serverUrl) @@ -373,7 +371,7 @@ final class RemoteChangeObserverTests: XCTestCase { remoteInterface: remoteInterface, changeNotificationInterface: notificationInterface, domain: nil, - dbManager: dbManager + dbManager: Self.dbManager ) // Set a very short poll interval for the test. remoteChangeObserver?.pollInterval = 0.5 @@ -472,7 +470,7 @@ final class RemoteChangeObserverTests: XCTestCase { func testRetryOnConnectionLoss() async throws { // 1. Arrange - let db = dbManager.ncDatabase() + let db = Self.dbManager.ncDatabase() debugPrint(db) let remoteInterface = @@ -484,7 +482,7 @@ final class RemoteChangeObserverTests: XCTestCase { SendableItemMetadata(ocId: "item1", fileName: "file.txt", account: Self.account) fileToUpdate.downloaded = true fileToUpdate.etag = "ETAG_OLD" - dbManager.addItemMetadata(fileToUpdate) + Self.dbManager.addItemMetadata(fileToUpdate) let serverItem = MockRemoteItem( identifier: "item1", versionIdentifier: "ETAG_NEW", @@ -503,7 +501,7 @@ final class RemoteChangeObserverTests: XCTestCase { remoteInterface: remoteInterface, changeNotificationInterface: notificationInterface, domain: nil, - dbManager: dbManager + dbManager: Self.dbManager ) self.remoteChangeObserver = remoteChangeObserver