diff --git a/app/src/test/java/com/duckduckgo/app/bookmarks/model/SavedSitesRepositoryTest.kt b/app/src/test/java/com/duckduckgo/app/bookmarks/model/SavedSitesRepositoryTest.kt index c13b47b05eb3..de455a8a4385 100644 --- a/app/src/test/java/com/duckduckgo/app/bookmarks/model/SavedSitesRepositoryTest.kt +++ b/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 = "") diff --git a/app/src/test/java/com/duckduckgo/app/bookmarks/model/SyncSavedSitesRepositoryTest.kt b/app/src/test/java/com/duckduckgo/app/bookmarks/model/SyncSavedSitesRepositoryTest.kt index 4d66ef5b5aa1..ad54b8f3ad3e 100644 --- a/app/src/test/java/com/duckduckgo/app/bookmarks/model/SyncSavedSitesRepositoryTest.kt +++ b/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 diff --git a/app/src/test/java/com/duckduckgo/app/bookmarks/service/SavedSitesExporterTest.kt b/app/src/test/java/com/duckduckgo/app/bookmarks/service/SavedSitesExporterTest.kt index b3887f1c32ab..35c0d90e3d96 100644 --- a/app/src/test/java/com/duckduckgo/app/bookmarks/service/SavedSitesExporterTest.kt +++ b/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() + 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, itemList: List, diff --git a/app/src/test/java/com/duckduckgo/app/sync/SavedSitesSyncDataProviderTest.kt b/app/src/test/java/com/duckduckgo/app/sync/SavedSitesSyncDataProviderTest.kt index 992c5e316677..34bc9479db41 100644 --- a/app/src/test/java/com/duckduckgo/app/sync/SavedSitesSyncDataProviderTest.kt +++ b/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) diff --git a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/SavedSitesRepository.kt b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/SavedSitesRepository.kt index c66939e44d94..77b8ee4e7495 100644 --- a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/SavedSitesRepository.kt +++ b/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, ): List { 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,7 +179,7 @@ class RealSavedSitesRepository( override fun getFolderBranch(folder: BookmarkFolder): FolderBranch { val bookmarks = mutableListOf() 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) } @@ -177,12 +187,15 @@ class RealSavedSitesRepository( bookmarks: MutableList, folders: MutableList, folderId: String, + visitedFolderIds: MutableSet, ): Pair, List> { 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 diff --git a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/service/SavedSitesExporter.kt b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/service/SavedSitesExporter.kt index c06531b68c31..dccab0a20d2c 100644 --- a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/service/SavedSitesExporter.kt +++ b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/service/SavedSitesExporter.kt @@ -79,7 +79,7 @@ class RealSavedSitesExporter( @VisibleForTesting fun getTreeFolderStructure(): TreeNode { 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 } @@ -87,13 +87,17 @@ class RealSavedSitesExporter( parentNode: TreeNode, parentId: String, currentDepth: Int, + visitedFolderIds: MutableSet, ) { 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) diff --git a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/RealSyncSavedSitesRepository.kt b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/RealSyncSavedSitesRepository.kt index dece15e510c0..cb50c212d5c1 100644 --- a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/RealSyncSavedSitesRepository.kt +++ b/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) { + // 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) diff --git a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/SavedSitesSyncDataProvider.kt b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/SavedSitesSyncDataProvider.kt index 2ab62604e6f7..893a4ec088c6 100644 --- a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/SavedSitesSyncDataProvider.kt +++ b/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, + visitedFolderIds: MutableSet = mutableSetOf(), ): List { + // 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() 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)))