Merge pull request #90 from claucambra/bugfix/pagination-pt6

Implement mitigation for server returning empty pages

Author: claucambra

Sources/NextcloudFileProviderKit/Enumeration/Enumerator+SyncEngine.swift

@@ -461,7 +461,9 @@ extension Enumerator {
                     not much we can do...
                 """
             )
-            return (nil, nil, nil, nil, nextPage, error)
+            // This is technically possible when doing a paginated request with the index too high.
+            // It's technically not an error reply.
+            return ([], nil, nil, nil, nextPage, nil)
         }
 
         // Generally speaking a PROPFIND will provide the target of the PROPFIND as the first result

Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift

@@ -221,9 +221,9 @@ public class Enumerator: NSObject, NSFileProviderEnumerator {
             var providedPage: NSFileProviderPage? = nil // Used for pagination token sent to server
             // Do not pass in the NSFileProviderPage default pages, these are not valid Nextcloud
             // pagination tokens
-            var scanningChildFolder = false
             var nextServerUrls: [String]? = nil
             var pageIndex = 0
+            var pageTotal: Int? = nil
             if page != NSFileProviderPage.initialPageSortedByName as NSFileProviderPage &&
                page != NSFileProviderPage.initialPageSortedByDate as NSFileProviderPage
             {
@@ -235,11 +235,13 @@ public class Enumerator: NSObject, NSFileProviderEnumerator {
                             "Setting enumerator server URL to \(actualServerUrl, privacy: .public)"
                         )
                         serverUrl = actualServerUrl
-                        scanningChildFolder = true
                     }
                     if let token = enumPageResponse.token?.data(using: .utf8) {
                         providedPage = NSFileProviderPage(token)
                     }
+                    if let total = enumPageResponse.total {
+                        pageTotal = total
+                    }
                     pageIndex = enumPageResponse.index
                     nextServerUrls = enumPageResponse.serverUrlQueue
                 } else {
@@ -290,6 +292,8 @@ public class Enumerator: NSObject, NSFileProviderEnumerator {
                 return
             }
 
+            pageTotal = nextPage?.total ?? pageTotal
+
             if enumeratedItemIdentifier == .workingSet {
                 var serverUrlQueue = nextServerUrls ?? []
                 for metadata in metadatas where metadata.directory {
@@ -304,16 +308,23 @@ public class Enumerator: NSObject, NSFileProviderEnumerator {
                         \(serverUrlQueue, privacy: .public)
                         next page is valid: \(nextPageValid, privacy: .public)
                         next page token: \(nextPageToken, privacy: .public)
+                        page total: \(pageTotal ?? -1, privacy: .public)
                     """
                 )
 
-                // If we are scanning a child folder now, add this information for the next call
-                // to an enumerator
-                if nextPage != nil, scanningChildFolder {
+                if let rPage = nextPage, let pageTotal, rPage.index * pageItemCount >= pageTotal {
+                    // Server will sometimes provide a valid next page data even though there are no
+                    // items to enumerate anymore
+                    Self.logger.debug("No more items to enumerate, stopping paged enumeration")
+                    nextPage = nil
+                }
+
+                if nextPage != nil {
                     Self.logger.debug("Applying actual server url onto server page response")
-                    nextPage?.nextServerUrl = serverUrl
-                    nextPage?.serverUrlQueue = serverUrlQueue
-                } else if nextPage == nil && !serverUrlQueue.isEmpty {
+                    nextPage!.total = pageTotal
+                    nextPage!.serverUrlQueue = serverUrlQueue
+                    nextPage!.nextServerUrl = serverUrl
+                } else if !serverUrlQueue.isEmpty {
                     // If we have finished paged enumeration of the current serverUrl, move to next
                     // child to scan
                     let nextServerUrl = serverUrlQueue.removeFirst()

Sources/NextcloudFileProviderKit/Enumeration/EnumeratorPageResponse.swift

@@ -14,7 +14,7 @@ fileprivate let logger = Logger(subsystem: Logger.subsystem, category: "enumerat
 struct EnumeratorPageResponse: Sendable, Codable {
     let token: String?   // Required by server to serve the next page of items
     let index: Int      // Needed to calculate the offset for the next paginated request
-    let total: Int?     // Total item count, provided in the first non-offset paginated response
+    var total: Int?     // Total item count, provided in the first non-offset paginated response
     var serverUrlQueue: [String]?
     var nextServerUrl: String? = nil
 

Tests/Interface/MockRemoteInterface.swift

@@ -567,6 +567,7 @@ public class MockRemoteInterface: RemoteInterface {
     public var completedChunkTransferSize: [String: Int64] = [:]
     public var pagination: Bool
     public var expectedEnumerationPaginationTokens: [String: String] = [:]
+    public var forceNextPageOnLastContentPage: Bool = false
 
     public init(
         rootItem: MockRemoteItem? = nil,
@@ -1081,6 +1082,10 @@ public class MockRemoteInterface: RemoteInterface {
             {
                 return (account.ncKitAccount, [], nil, .invalidData)
             }
+            guard !forceNextPageOnLastContentPage || firstItem < files.count else {
+                let responseData = generateResponse(itemCount: files.count, finalPage: true)
+                return (account.ncKitAccount, [], responseData, .success)
+            }
             let reachedEnd = firstItem + itemCount >= files.count
             let lastItem = min(firstItem + itemCount, files.count) - 1
             assert(firstItem <= lastItem)

Tests/NextcloudFileProviderKitTests/EnumeratorTests.swift

@@ -547,6 +547,92 @@ final class EnumeratorTests: XCTestCase {
         XCTAssertNil(Self.dbManager.itemMetadata(ocId: rootItem.identifier))
     }
 
+    func testWorkingSetEnumerationStopsOnExhaustedTotal() async throws {
+        // This test verifies that enumeration correctly terminates when a faulty server
+        // provides a `nextPage` token but all items (according to `pageTotal`) have been received.
+
+        // 1. Setup: A folder with 9 items which, when including itself in the propfind, is an exact
+        // multiple of the page size.
+        rootItem.children = []
+        let folder = MockRemoteItem(
+            identifier: "folder10",
+            name: "folder10",
+            remotePath: Self.account.davFilesUrl + "/folder10",
+            directory: true,
+            account: Self.account.ncKitAccount,
+            username: Self.account.username,
+            userId: Self.account.id,
+            serverUrl: Self.account.serverUrl
+        )
+        folder.parent = rootItem
+        rootItem.children.append(folder)
+
+        for i in 0..<9 {
+            let childItem = MockRemoteItem(
+                identifier: "item\(i)",
+                name: "item\(i).txt",
+                remotePath: folder.remotePath + "/item\(i).txt",
+                account: Self.account.ncKitAccount,
+                username: Self.account.username,
+                userId: Self.account.id,
+                serverUrl: Self.account.serverUrl
+            )
+            childItem.parent = folder
+            folder.children.append(childItem)
+        }
+
+        let db = Self.dbManager.ncDatabase()
+        debugPrint(db)
+
+        let remoteInterface = MockRemoteInterface(rootItem: rootItem, pagination: true)
+        let pageSize = 5
+
+        // This test requires a modification to `MockRemoteInterface` to simulate a faulty server
+        remoteInterface.forceNextPageOnLastContentPage = true
+
+        // 2. Manually drive the pagination loop.
+        var allEnumeratedItems: [NSFileProviderItem] = []
+        var currentPage: NSFileProviderPage? =
+            NSFileProviderPage.initialPageSortedByName as NSFileProviderPage
+        var loopCount = 0
+
+        while let pageToEnumerate = currentPage {
+            loopCount += 1
+            currentPage = nil // Reset for the next iteration
+
+            // Create a NEW enumerator and observer for this single page request
+            let enumerator = Enumerator(
+                enumeratedItemIdentifier: .workingSet,
+                account: Self.account,
+                remoteInterface: remoteInterface,
+                dbManager: Self.dbManager,
+                pageSize: pageSize
+            )
+            let observer = MockEnumerationObserver(enumerator: enumerator)
+            try await observer.enumerateItemsPage(page: pageToEnumerate)
+            XCTAssertNil(observer.error)
+            allEnumeratedItems += observer.items
+            currentPage = observer.page
+        }
+
+        // 3. Assert the results.
+        // The loop should have run 3 times:
+        // 1. For root
+        // 2. For `folder10` page 1 (items 0-4).
+        // 3. For `folder10` page 2 (items 5-9). The enumerator logic should see that all items
+        //    have been delivered and should not return the phantom page token, terminating the loop.
+        XCTAssertNil(currentPage, "Loop should have terminated with a nil next page.")
+        XCTAssertEqual(loopCount, 3, "The enumeration loop should have terminated correctly.")
+
+        // Total items expected: 1 folder + 9 children = 10.
+        let expectedTotalItems = 1 + 9
+        XCTAssertEqual(
+            allEnumeratedItems.count,
+            expectedTotalItems,
+            "The correct number of total items should be enumerated."
+        )
+    }
+
     func testReadServerUrlFollowUpPagination() async throws {
         // 1. Arrange: Setup a folder with enough children to require multiple pages.
         remoteFolder.children = []