Guard bookmark folder traversal against cyclic folder relations

app/src/test/java/com/duckduckgo/app/bookmarks/model/SavedSitesRepositoryTest.kt

@@ -769,6 +769,42 @@ class SavedSitesRepositoryTest {
         assertNull(repository.getBookmark(childBookmark.url))
     }
 
+    @Test
+    fun whenFolderRelationsContainCycleThenGetFolderBranchTerminates() = runTest {
+        val parentFolder =
+            BookmarkFolder("folder1", "Parent Folder", SavedSitesNames.BOOKMARKS_ROOT, numBookmarks = 0, numFolders = 1, lastModified = "timestamp")
+        val childFolder = BookmarkFolder("folder2", "Child Folder", "folder1", numBookmarks = 1, numFolders = 0, lastModified = "timestamp")
+        val childBookmark = Bookmark("bookmark1", "title", "www.example.com", "folder2", "timestamp")
+        repository.insertFolderBranch(FolderBranch(listOf(childBookmark), listOf(parentFolder, childFolder)))
+
+        // folder2 is already a child of folder1; making folder1 a child of folder2 closes the loop
+        savedSitesRelationsDao.insert(Relation(folderId = childFolder.id, entityId = parentFolder.id))
+
+        val branch = repository.getFolderBranch(parentFolder)
+
+        assertTrue(branch.folders.any { it.id == parentFolder.id })
+        assertTrue(branch.folders.any { it.id == childFolder.id })
+        assertEquals(branch.folders.distinctBy { it.id }.size, branch.folders.size)
+        assertEquals(listOf(childBookmark), branch.bookmarks)
+    }
+
+    @Test
+    fun whenFolderRelationsContainCycleThenDeleteFolderBranchTerminates() = runTest {
+        val parentFolder =
+            BookmarkFolder("folder1", "Parent Folder", SavedSitesNames.BOOKMARKS_ROOT, numBookmarks = 0, numFolders = 1, lastModified = "timestamp")
+        val childFolder = BookmarkFolder("folder2", "Child Folder", "folder1", numBookmarks = 1, numFolders = 0, lastModified = "timestamp")
+        val childBookmark = Bookmark("bookmark1", "title", "www.example.com", "folder2", "timestamp")
+        repository.insertFolderBranch(FolderBranch(listOf(childBookmark), listOf(parentFolder, childFolder)))
+
+        savedSitesRelationsDao.insert(Relation(folderId = childFolder.id, entityId = parentFolder.id))
+
+        repository.deleteFolderBranch(parentFolder)
+
+        assertNull(repository.getFolder(parentFolder.id))
+        assertNull(repository.getFolder(childFolder.id))
+        assertNull(repository.getBookmark(childBookmark.url))
+    }
+
     @Test
     fun whenBuildFlatStructureThenReturnFolderListWithDepth() = runTest {
         val rootFolder = BookmarkFolder(id = SavedSitesNames.BOOKMARKS_ROOT, name = "root", lastModified = "timestamp", parentId = "")
@@ -799,6 +835,36 @@ class SavedSitesRepositoryTest {
         assertEquals(items, flatStructure)
     }
 
+    @Test
+    fun whenFolderRelationsContainCycleThenGetFolderTreeTerminates() = runTest {
+        val rootFolder = BookmarkFolder(id = SavedSitesNames.BOOKMARKS_ROOT, name = "root", lastModified = "timestamp", parentId = "")
+        val parentFolder = BookmarkFolder(id = "folder1", name = "name", lastModified = "timestamp", parentId = SavedSitesNames.BOOKMARKS_ROOT)
+        val childFolder = BookmarkFolder(id = "folder2", name = "another name", lastModified = "timestamp", parentId = "folder1")
+
+        repository.insert(rootFolder)
+        repository.insert(parentFolder)
+        repository.insert(childFolder)
+
+        // close the loop: folder1 becomes a child of folder2, which is already a child of folder1
+        savedSitesRelationsDao.insert(Relation(folderId = childFolder.id, entityId = parentFolder.id))
+
+        val flatStructure = repository.getFolderTree(childFolder.id, null)
+
+        assertEquals(flatStructure.distinctBy { it.bookmarkFolder.id }.size, flatStructure.size)
+    }
+
+    @Test
+    fun whenSameFolderInsertedTwiceThenRelationIsNotDuplicated() = runTest {
+        val rootFolder = BookmarkFolder(id = SavedSitesNames.BOOKMARKS_ROOT, name = "root", lastModified = "timestamp", parentId = "")
+        val folder = BookmarkFolder(id = "folder1", name = "name", lastModified = "timestamp", parentId = SavedSitesNames.BOOKMARKS_ROOT)
+
+        repository.insert(rootFolder)
+        repository.insert(folder)
+        repository.insert(folder)
+
+        assertEquals(1, savedSitesRelationsDao.relationsByEntityId(folder.id).size)
+    }
+
     @Test
     fun whenBuildFlatStructureThenReturnFolderListWithDepthWithoutCurrentFolderBranch() = runTest {
         val rootFolder = BookmarkFolder(id = SavedSitesNames.BOOKMARKS_ROOT, name = "root", lastModified = "timestamp", parentId = "")

app/src/test/java/com/duckduckgo/app/bookmarks/model/SyncSavedSitesRepositoryTest.kt

@@ -647,6 +647,23 @@ class SyncSavedSitesRepositoryTest {
         assertTrue(savedSitesEntitiesDao.entityById(bookmarksRootFolder.id)!!.lastModified != oneHourAgo)
     }
 
+    @Test
+    fun whenFolderRelationsContainCycleThenSetLocalEntitiesForNextSyncTerminates() {
+        val twoHoursAgo = DatabaseDateFormatter.iso8601(OffsetDateTime.now(ZoneOffset.UTC).minusHours(2))
+        val folderA = BookmarkFolder(id = "folderA", name = "Folder A", parentId = bookmarksRootFolder.id, lastModified = twoHoursAgo)
+        val folderB = BookmarkFolder(id = "folderB", name = "Folder B", parentId = folderA.id, lastModified = twoHoursAgo)
+        savedSitesRepository.insert(folderA)
+        savedSitesRepository.insert(folderB)
+        // folderB now also contains folderA, forming a loop in the relations table
+        savedSitesRelationsDao.insert(Relation(folderId = folderB.id, entityId = folderA.id))
+
+        val oneHourAhead = DatabaseDateFormatter.iso8601(OffsetDateTime.now(ZoneOffset.UTC).plusHours(1))
+        repository.setLocalEntitiesForNextSync(oneHourAhead)
+
+        assertTrue(savedSitesEntitiesDao.entityById(folderA.id) != null)
+        assertTrue(savedSitesEntitiesDao.entityById(folderB.id) != null)
+    }
+
     @Test
     fun whenDeduplicatingBookmarkThenRemoteBookmarkReplacesLocal() {
         // given a local bookmark

app/src/test/java/com/duckduckgo/app/bookmarks/service/SavedSitesExporterTest.kt

@@ -38,10 +38,12 @@ import com.duckduckgo.savedsites.impl.RealFavoritesDelegate
 import com.duckduckgo.savedsites.impl.RealSavedSitesRepository
 import com.duckduckgo.savedsites.impl.service.RealSavedSitesExporter
 import com.duckduckgo.savedsites.impl.service.RealSavedSitesParser
+import com.duckduckgo.savedsites.store.Relation
 import com.duckduckgo.savedsites.store.SavedSitesEntitiesDao
 import com.duckduckgo.savedsites.store.SavedSitesRelationsDao
 import kotlinx.coroutines.test.runTest
 import org.junit.*
+import org.junit.Assert.assertEquals
 import org.junit.Assert.assertTrue
 import org.junit.runner.RunWith
 import java.io.File
@@ -189,6 +191,29 @@ class SavedSitesExporterTest {
         )
     }
 
+    @Test
+    fun whenFolderRelationsContainCycleThenGetTreeStructureTerminates() = runTest {
+        val root = BookmarkFolder(SavedSitesNames.BOOKMARKS_ROOT, "DuckDuckGo Bookmarks", "", 0, 0, "timestamp")
+        val parentFolder = BookmarkFolder("folder1", "Folder One", SavedSitesNames.BOOKMARKS_ROOT, 0, 0, "timestamp")
+        val childFolder = BookmarkFolder("folder2", "Folder Two", "folder1", 0, 0, "timestamp")
+        val childBookmark = Bookmark("bookmark1", "title", "www.example.com", "folder2", "timestamp")
+        savedSitesRepository.insertFolderBranch(FolderBranch(listOf(childBookmark), listOf(root, parentFolder, childFolder)))
+
+        // folder2 is already a child of folder1; making folder1 a child of folder2 closes the loop
+        savedSitesRelationsDao.insert(Relation(folderId = childFolder.id, entityId = parentFolder.id))
+
+        val treeStructure = exporter.getTreeFolderStructure()
+
+        val visitedFolderIds = mutableListOf<String>()
+        treeStructure.forEachVisit(
+            { node -> if (node.value.url == null) visitedFolderIds.add(node.value.id) },
+            { },
+        )
+
+        assertTrue(visitedFolderIds.containsAll(listOf(parentFolder.id, childFolder.id)))
+        assertEquals(visitedFolderIds.distinct().size, visitedFolderIds.size)
+    }
+
     private fun testNode(
         node: TreeNode<FolderTreeItem>,
         itemList: List<Any>,

app/src/test/java/com/duckduckgo/app/sync/SavedSitesSyncDataProviderTest.kt

@@ -51,6 +51,7 @@ import com.duckduckgo.savedsites.impl.sync.SyncSavedSitesRequestEntry
 import com.duckduckgo.savedsites.impl.sync.store.RealSavedSitesSyncEntitiesStore
 import com.duckduckgo.savedsites.impl.sync.store.SavedSitesSyncMetadataDao
 import com.duckduckgo.savedsites.impl.sync.store.SavedSitesSyncMetadataDatabase
+import com.duckduckgo.savedsites.store.Relation
 import com.duckduckgo.savedsites.store.SavedSitesEntitiesDao
 import com.duckduckgo.savedsites.store.SavedSitesRelationsDao
 import com.duckduckgo.sync.api.SyncCrypto
@@ -220,6 +221,25 @@ class SavedSitesSyncDataProviderTest {
         assertTrue(changes.bookmarks.updates[5].id == "bookmarks_root")
     }
 
+    @Test
+    fun whenFolderRelationsContainCycleThenGetChangesTerminatesAndListsEachFolderOnce() {
+        val folderA = aFolder("folderA", "Folder A", SavedSitesNames.BOOKMARKS_ROOT)
+        val folderB = aFolder("folderB", "Folder B", folderA.id)
+        repository.insert(folderA)
+        repository.insert(folderB)
+        repository.insert(bookmark3.copy(parentId = folderB.id))
+        // folderB now also contains folderA, forming a loop in the relations table
+        savedSitesRelationsDao.insert(Relation(folderId = folderB.id, entityId = folderA.id))
+
+        val syncChanges = parser.getChanges()
+
+        val changes = Adapters.adapter.fromJson(syncChanges.jsonString)!!
+        val updatedIds = changes.bookmarks.updates.map { it.id }
+        assertEquals(1, updatedIds.count { it == folderA.id })
+        assertEquals(1, updatedIds.count { it == folderB.id })
+        assertTrue(updatedIds.contains(bookmark3.id))
+    }
+
     @Test
     fun whenNewBookmarksSinceLastSyncThenChangesContainData() {
         repository.insert(bookmark3)

saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/SavedSitesRepository.kt

@@ -116,7 +116,15 @@ class RealSavedSitesRepository(
         return if (rootFolder != null) {
             val rootFolderItem = BookmarkFolderItem(0, rootFolder, rootFolder.id == selectedFolderId)
             val folders = mutableListOf(rootFolderItem)
-            val folderDepth = traverseFolderWithDepth(1, folders, SavedSitesNames.BOOKMARKS_ROOT, selectedFolderId = selectedFolderId, currentFolder)
+            val folderDepth =
+                traverseFolderWithDepth(
+                    1,
+                    folders,
+                    SavedSitesNames.BOOKMARKS_ROOT,
+                    selectedFolderId = selectedFolderId,
+                    currentFolder,
+                    mutableSetOf(SavedSitesNames.BOOKMARKS_ROOT),
+                )
             if (currentFolder != null) {
                 folderDepth.filterNot { it.bookmarkFolder == currentFolder }
             } else {
@@ -145,11 +153,13 @@ class RealSavedSitesRepository(
         folderId: String,
         selectedFolderId: String,
         currentFolder: BookmarkFolder?,
+        visitedFolderIds: MutableSet<String>,
     ): List<BookmarkFolderItem> {
         getFolders(folderId).map {
-            if (it.id != currentFolder?.id) {
+            // folder relations can form a loop, so a folder already on this branch is never descended into again
+            if (it.id != currentFolder?.id && visitedFolderIds.add(it.id)) {
                 folders.add(BookmarkFolderItem(depth, it, it.id == selectedFolderId))
-                traverseFolderWithDepth(depth + 1, folders, it.id, selectedFolderId = selectedFolderId, currentFolder)
+                traverseFolderWithDepth(depth + 1, folders, it.id, selectedFolderId = selectedFolderId, currentFolder, visitedFolderIds)
             }
         }
         return folders
@@ -169,20 +179,23 @@ class RealSavedSitesRepository(
     override fun getFolderBranch(folder: BookmarkFolder): FolderBranch {
         val bookmarks = mutableListOf<Bookmark>()
         val folders = mutableListOf(folder)
-        val folderContent = traverseBranch(bookmarks, folders, folder.id)
+        val folderContent = traverseBranch(bookmarks, folders, folder.id, mutableSetOf(folder.id))
         return FolderBranch(folderContent.first, folderContent.second)
     }
 
     private fun traverseBranch(
         bookmarks: MutableList<Bookmark>,
         folders: MutableList<BookmarkFolder>,
         folderId: String,
+        visitedFolderIds: MutableSet<String>,
     ): Pair<List<Bookmark>, List<BookmarkFolder>> {
         val folderContent = folderContent(folderId)
         bookmarks.addAll(folderContent.first)
-        folders.addAll(folderContent.second)
-        folderContent.second.forEach {
-            traverseBranch(bookmarks, folders, it.id)
+        // folder relations can form a loop, so a folder already on this branch is never descended into again
+        val unvisitedFolders = folderContent.second.filter { visitedFolderIds.add(it.id) }
+        folders.addAll(unvisitedFolders)
+        unvisitedFolders.forEach {
+            traverseBranch(bookmarks, folders, it.id, visitedFolderIds)
         }
         return Pair(bookmarks, folders)
     }
@@ -434,6 +447,8 @@ class RealSavedSitesRepository(
             type = FOLDER,
         )
         savedSitesEntitiesDao.insert(entity)
+        // a folder has a single parent, so re-inserting one has to replace its relation instead of adding a second
+        savedSitesRelationsDao.deleteRelationByEntity(folder.id)
         savedSitesRelationsDao.insert(Relation(folderId = folder.parentId, entityId = folder.id))
         savedSitesEntitiesDao.updateModified(folder.parentId, folder.lastModified ?: DatabaseDateFormatter.iso8601())
         return folder

saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/service/SavedSitesExporter.kt

@@ -79,21 +79,25 @@ class RealSavedSitesExporter(
     @VisibleForTesting
     fun getTreeFolderStructure(): TreeNode<FolderTreeItem> {
         val node = TreeNode(FolderTreeItem(SavedSitesNames.BOOKMARKS_ROOT, RealSavedSitesParser.BOOKMARKS_FOLDER, "", null))
-        populateNode(node, SavedSitesNames.BOOKMARKS_ROOT, 1)
+        populateNode(node, SavedSitesNames.BOOKMARKS_ROOT, 1, mutableSetOf(SavedSitesNames.BOOKMARKS_ROOT))
         return node
     }
 
     private fun populateNode(
         parentNode: TreeNode<FolderTreeItem>,
         parentId: String,
         currentDepth: Int,
+        visitedFolderIds: MutableSet<String>,
     ) {
         val combinedContent = savedSitesRepository.getFolderTreeItems(parentId)
         combinedContent.forEach { item ->
             if (item.url == null) {
-                val childNode = TreeNode(item.copy(depth = currentDepth))
-                parentNode.add(childNode)
-                populateNode(childNode, item.id, currentDepth + 1)
+                // folder relations can form a loop, so a folder already on this branch is never descended into again
+                if (visitedFolderIds.add(item.id)) {
+                    val childNode = TreeNode(item.copy(depth = currentDepth))
+                    parentNode.add(childNode)
+                    populateNode(childNode, item.id, currentDepth + 1, visitedFolderIds)
+                }
             } else {
                 val childNode = TreeNode(item.copy(depth = currentDepth))
                 parentNode.add(childNode)

saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/RealSyncSavedSitesRepository.kt

@@ -417,6 +417,8 @@ class RealSyncSavedSitesRepository(
     }
 
     private fun traverseParents(entity: String, entitiesToUpdate: MutableList<String>) {
+        // folder relations can form a loop, so an entity already collected is never walked up from again
+        if (entitiesToUpdate.contains(entity)) return
         // find parent of each entity
         entitiesToUpdate.add(entity)
         val parents = savedSitesRelationsDao.relationsByEntityId(entity)

saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/SavedSitesSyncDataProvider.kt

@@ -133,7 +133,10 @@ class SavedSitesSyncDataProvider @Inject constructor(
     private fun getRequestEntriesFor(
         folderId: String,
         requestEntries: MutableList<SyncSavedSitesRequestEntry>,
+        visitedFolderIds: MutableSet<String> = mutableSetOf(),
     ): List<SyncSavedSitesRequestEntry> {
+        // folder relations can form a loop, so a folder already on this branch is never descended into again
+        if (!visitedFolderIds.add(folderId)) return requestEntries
         val invalidItems = mutableListOf<String>()
         syncSavedSitesRepository.getAllFolderContentSync(folderId).apply {
             val folder = repository.getFolder(folderId)
@@ -156,7 +159,7 @@ class SavedSitesSyncDataProvider @Inject constructor(
                     if (eachFolder.deleted != null) {
                         requestEntries.add(deletedEntry(eachFolder.id))
                     } else {
-                        getRequestEntriesFor(eachFolder.id, requestEntries)
+                        getRequestEntriesFor(eachFolder.id, requestEntries, visitedFolderIds)
                     }
                 }
                 requestEntries.add(encryptedFolder(fixFolderIfNecessary(folder)))