Merge pull request #87 from claucambra/bugfix/pagination-pt3

claucambra · web-flow · commit d7a8b48cfc6e · 2025-06-06T21:09:14.000+08:00
Further bugfixes for paginated server URL reads
diff --git a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+SyncEngine.swift b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+SyncEngine.swift
@@ -272,10 +272,17 @@ extension Enumerator {
         // target item, hence why we always strip the target item out)
         let startIndex = pageIndex > 0 ? 0 : 1
         if pageIndex == 0 {
-            guard let firstFile = files.first else {
-                return (nil, .invalidResponseError)
+            guard let firstFile = files.first else { return (nil, .invalidResponseError) }
+            // Do not ingest metadata for the root container
+            if !firstFile.fullUrlMatches(dbManager.account.davFilesUrl) {
+                var metadata = firstFile.toItemMetadata()
+                if metadata.directory,
+                   let existingMetadata = dbManager.itemMetadata(ocId: metadata.ocId)
+                {
+                    metadata.downloaded = existingMetadata.downloaded
+                }
+                dbManager.addItemMetadata(metadata)
             }
-            dbManager.addItemMetadata(firstFile.toItemMetadata())
         }
         let metadatas = files[startIndex..<files.count].map { $0.toItemMetadata() }
         metadatas.forEach { dbManager.addItemMetadata($0) }
@@ -455,7 +462,9 @@ extension Enumerator {
         // That is NOT the case for paginated results with offsets
         let isFollowUpPaginatedRequest = (pageSettings?.page != nil && pageSettings?.index ?? 0 > 0)
         if !isFollowUpPaginatedRequest {
-            guard receivedFile.directory else {
+            guard receivedFile.directory ||
+                  receivedFile.fullUrlMatches(dbManager.account.davFilesUrl)
+            else {
                 Self.logger.debug(
                     """
                     Read item is a file.
diff --git a/Sources/NextcloudFileProviderKit/Extensions/NKFile+Extensions.swift b/Sources/NextcloudFileProviderKit/Extensions/NKFile+Extensions.swift
@@ -10,6 +10,14 @@ import NextcloudKit
 import RealmSwift
 
 extension NKFile {
+    func fullUrlMatches(_ urlString: String) -> Bool {
+        var fileUrl = serverUrl + "/" + fileName
+        if fileUrl.last == "/" { // This is likely the root container, as it has no filename
+            fileUrl.removeLast()
+        }
+        return fileUrl == urlString
+    }
+
     func toItemMetadata(uploaded: Bool = true) -> SendableItemMetadata {
         let creationDate = creationDate ?? date
         let uploadDate = uploadDate ?? date
diff --git a/Tests/Interface/MockRemoteItem.swift b/Tests/Interface/MockRemoteItem.swift
@@ -60,7 +60,7 @@ public class MockRemoteItem: Equatable {
         MockRemoteItem(
             identifier: NSFileProviderItemIdentifier.rootContainer.rawValue,
             versionIdentifier: "root",
-            name: "root",
+            name: "",
             remotePath: account.davFilesUrl,
             directory: true,
             account: account.ncKitAccount,
@@ -74,7 +74,7 @@ public class MockRemoteItem: Equatable {
         return MockRemoteItem(
             identifier: NSFileProviderItemIdentifier.trashContainer.rawValue,
             versionIdentifier: "root",
-            name: "root",
+            name: "",
             remotePath: account.trashUrl,
             directory: true,
             account: account.ncKitAccount,
diff --git a/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift b/Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift
@@ -398,7 +398,6 @@ final class EnumeratorTests: XCTestCase {
         // as pages. This process continues until all directories have been enumerated.
         // We can verify that all items are discovered and stored in the database.
         let allItemIds = [
-            rootItem.identifier,
             folder1.identifier,
             folder2.identifier,
             subfolder.identifier
@@ -407,8 +406,211 @@ final class EnumeratorTests: XCTestCase {
           + subfolder.children.map(\.identifier)
 
         for itemId in allItemIds {
-            XCTAssertNotNil(Self.dbManager.itemMetadata(ocId: itemId), "Item with id \(itemId) should be in the database")
+            XCTAssertNotNil(
+                Self.dbManager.itemMetadata(ocId: itemId),
+                "Item with id \(itemId) should be in the database"
+            )
+        }
+        // Root should not be stored in database
+        XCTAssertNil(Self.dbManager.itemMetadata(ocId: rootItem.identifier))
+    }
+
+    func testReadServerUrlFollowUpPagination() async throws {
+        // 1. Arrange: Setup a folder with enough children to require multiple pages.
+        remoteFolder.children = []
+        for i in 0..<10 {
+            let childItem = MockRemoteItem(
+                identifier: "folderChild\(i)",
+                name: "folderChild\(i).txt",
+                remotePath: Self.account.davFilesUrl + "/folder/folderChild\(i).txt",
+                account: Self.account.ncKitAccount,
+                username: Self.account.username,
+                userId: Self.account.id,
+                serverUrl: Self.account.serverUrl
+            )
+            childItem.parent = remoteFolder
+            remoteFolder.children.append(childItem)
+        }
+
+        let db = Self.dbManager.ncDatabase()
+        debugPrint(db)
+        let remoteInterface = MockRemoteInterface(rootItem: rootItem, pagination: true)
+
+        // Pre-populate the folder's metadata with an old etag to verify it gets updated
+        // on the initial call.
+        let oldEtag = "OLD_ETAG"
+        var folderMetadata = remoteFolder.toItemMetadata(account: Self.account)
+        folderMetadata.etag = oldEtag
+        Self.dbManager.addItemMetadata(folderMetadata)
+
+        // --- Scenario A: Initial Paginated Request (isFollowUpPaginatedRequest == false) ---
+
+        // 2. Act: Call readServerUrl for the first page.
+        let (
+            initialMetadatas, _, _, _, initialNextPage, initialError
+        ) = await Enumerator.readServerUrl(
+            remoteFolder.remotePath,
+            pageSettings: (page: nil, index: 0, size: 5), // index is 0
+            account: Self.account,
+            remoteInterface: remoteInterface,
+            dbManager: Self.dbManager
+        )
+
+        // 3. Assert: Verify the initial request's behavior.
+        XCTAssertNil(initialError)
+        XCTAssertNotNil(initialNextPage, "Should receive a next page token for the initial request")
+        // The first request for a folder returns the folder itself plus the first page of children.
+        XCTAssertEqual(
+            initialMetadatas?.count,
+            4,
+            """
+            Should get target + first page of children,
+            but the target should not be included in the first page,
+            so count is (4).
+            """
+        )
+        XCTAssertFalse(
+            initialMetadatas?.contains(where: { $0.ocId == remoteFolder.identifier }) ?? false,
+            "The folder itself should not be in the initial results."
+        )
+
+        // The logic inside `if !isFollowUpPaginatedRequest` should have run,
+        // updating the folder's metadata.
+        let updatedFolderMetadata =
+            try XCTUnwrap(Self.dbManager.itemMetadata(ocId: remoteFolder.identifier))
+        XCTAssertNotEqual(
+            updatedFolderMetadata.etag, oldEtag, "The folder's etag should have been updated."
+        )
+        XCTAssertEqual(updatedFolderMetadata.etag, remoteFolder.versionIdentifier)
+
+        // --- Scenario B: Follow-up Paginated Request (isFollowUpPaginatedRequest == true) ---
+
+        // 4. Act: Call readServerUrl for the second page using the received page token.
+        let followUpPage = NSFileProviderPage(initialNextPage!.token.data(using: .utf8)!)
+        let (
+            followUpMetadatas, _, _, _, finalNextPage, followUpError
+        ) = await Enumerator.readServerUrl(
+            remoteFolder.remotePath,
+            pageSettings: (page: followUpPage, index: 1, size: 5), // index > 0 and page is non-nil
+            account: Self.account,
+            remoteInterface: remoteInterface,
+            dbManager: Self.dbManager
+        )
+
+        // 5. Assert: Verify the follow-up request's behavior.
+        XCTAssertNil(followUpError)
+        XCTAssertNotNil(finalNextPage, "Should receive a next page token for the second request.")
+        // A follow-up request should *only* contain the children for that page.
+        XCTAssertEqual(
+            followUpMetadatas?.count, 5, "Should get only the second page of children (5)."
+        )
+        XCTAssertFalse(
+            followUpMetadatas?.contains(where: { $0.ocId == remoteFolder.identifier }) ?? true,
+            "The folder itself should NOT be in the follow-up page results."
+        )
+
+        // This confirms the `if !isFollowUpPaginatedRequest` block was correctly skipped, as it'd
+        // have processed the first item of the result array (`folderChild5`) as the parent folder,
+        // which would lead to incorrect data or errors.
+        let child5Metadata = Self.dbManager.itemMetadata(ocId: "folderChild5")
+        XCTAssertNotNil(
+            child5Metadata, "Metadata for the items on the second page should be in the database."
+        )
+    }
+
+    func testHandlePagedReadResults() throws {
+        // 1. Arrange
+        let dbManager = Self.dbManager
+        let db = dbManager.ncDatabase()
+        debugPrint(db)
+
+        let parentNKFile = remoteFolder.toNKFile()
+        let childrenNKFiles = (0..<5).map { i in
+            MockRemoteItem(
+                identifier: "pagedChild\(i)",
+                name: "pagedChild\(i).txt",
+                remotePath: Self.account.davFilesUrl + "/folder/pagedChild\(i).txt",
+                account: Self.account.ncKitAccount,
+                username: Self.account.username,
+                userId: Self.account.id,
+                serverUrl: Self.account.serverUrl
+            ).toNKFile()
+        }
+        let followUpChildrenNKFiles = (5..<10).map { i in
+            MockRemoteItem(
+                identifier: "pagedChild\(i)",
+                name: "pagedChild\(i).txt",
+                remotePath: Self.account.davFilesUrl + "/folder/pagedChild\(i).txt",
+                account: Self.account.ncKitAccount,
+                username: Self.account.username,
+                userId: Self.account.id,
+                serverUrl: Self.account.serverUrl
+            ).toNKFile()
         }
+
+        // --- Scenario A: First Page (pageIndex == 0) ---
+        // 2. Act
+        let firstPageFiles = [parentNKFile] + childrenNKFiles
+        let (firstPageResult, firstPageError) = Enumerator.handlePagedReadResults(
+            files: firstPageFiles, pageIndex: 0, dbManager: dbManager
+        )
+
+        // 3. Assert
+        XCTAssertNil(firstPageError)
+        // Result should only contain the children, not the parent.
+        XCTAssertEqual(
+            firstPageResult?.count, 5, "First page result should only contain children."
+        )
+        // The parent's metadata should be processed and saved to the DB.
+        let parentMetadataFromDB = dbManager.itemMetadata(ocId: parentNKFile.ocId)
+        XCTAssertNotNil(
+            parentMetadataFromDB, "Parent folder metadata should be saved to DB from first page."
+        )
+        XCTAssertEqual(parentMetadataFromDB?.etag, parentNKFile.etag, "Parent etag should match.")
+        // The children's metadata should also be saved.
+        let childMetadataFromDB = dbManager.itemMetadata(ocId: "pagedChild0")
+        XCTAssertNotNil(
+            childMetadataFromDB, "Child metadata should be saved to DB from first page."
+        )
+
+        // --- Scenario B: Follow-up Page (pageIndex > 0) ---
+        // 4. Act
+        let (followUpPageResult, followUpPageError) = Enumerator.handlePagedReadResults(
+            files: followUpChildrenNKFiles, pageIndex: 1, dbManager: dbManager
+        )
+
+        // 5. Assert
+        XCTAssertNil(followUpPageError)
+        // Result should contain all items passed in, as the parent is not included.
+        XCTAssertEqual(
+            followUpPageResult?.count, 5, "Follow-up page result should contain all its items."
+        )
+        let followUpChildMetadata = dbManager.itemMetadata(ocId: "pagedChild5")
+        XCTAssertNotNil(
+            followUpChildMetadata, "Child metadata should be saved to DB from follow-up page."
+        )
+
+        // --- Scenario C: Root Folder (should not be ingested) ---
+        // 6. Act
+        var rootNKFile = MockRemoteItem.rootItem(account: Self.account).toNKFile()
+        // The check is based on URL string matching.
+        rootNKFile.path = Self.account.davFilesUrl
+
+        let (rootResult, rootError) = Enumerator.handlePagedReadResults(
+            files: [rootNKFile], pageIndex: 0, dbManager: dbManager
+        )
+
+        // 7. Assert
+        XCTAssertNil(rootError)
+        // The result should be empty as there are no children.
+        XCTAssertEqual(
+            rootResult?.count, 0, "Root folder enumeration should yield no children."
+        )
+        // No metadata should be saved for the root folder ID itself.
+        XCTAssertNil(
+            dbManager.itemMetadata(ocId: rootNKFile.ocId),
+            "Metadata for the root folder itself should not be saved."
+        )
     }
 
     func testWorkingSetChangeEnumeration() async throws {
