diff --git a/src/lib/Diff.ts b/src/lib/Diff.ts index 643ec73793..e50df0193c 100644 --- a/src/lib/Diff.ts +++ b/src/lib/Diff.ts @@ -335,6 +335,19 @@ export default class Diff< action.payload, targetLocation ) + if (action.type !== ActionType.CREATE && typeof action.payload.id !== 'undefined' && typeof newAction.payload.id === 'undefined') { + Logger.log( + 'payload.location = ' + + action.payload.location + + ' | targetLocation = ' + + targetLocation + ) + const diff = new Diff() + diff.commit(action) + Logger.log('Failed to map id of action ' + diff.inspect()) + Logger.log(JSON.stringify(mappingsSnapshot, null, '\t')) + throw new MappingFailureError(String(action.payload.id)) + } } if ( @@ -397,6 +410,10 @@ export default class Diff< }) } + if (action.type !== ActionType.REORDER) { + Logger.log('Mapped action', action, newAction) + } + newDiff.commit(newAction) }) return newDiff diff --git a/src/lib/Mappings.ts b/src/lib/Mappings.ts index f87cce1498..ce3b92b0d9 100644 --- a/src/lib/Mappings.ts +++ b/src/lib/Mappings.ts @@ -48,6 +48,7 @@ export default class Mappings { } async addFolder({ localId, remoteId }: { localId?:string|number, remoteId?:string|number }):Promise { + Mappings.remove(this.folders, { localId, remoteId }) Mappings.add(this.folders, { localId, remoteId }) } @@ -56,6 +57,7 @@ export default class Mappings { } async addBookmark({ localId, remoteId }: { localId?:string|number, remoteId?:string|number }):Promise { + Mappings.remove(this.bookmarks, { localId, remoteId }) Mappings.add(this.bookmarks, { localId, remoteId }) } diff --git a/src/lib/Scanner.ts b/src/lib/Scanner.ts index ae712e5301..017d30097b 100644 --- a/src/lib/Scanner.ts +++ b/src/lib/Scanner.ts @@ -4,6 +4,7 @@ import { Bookmark, Folder, ItemLocation, ItemType, TItem, TItemLocation } from ' import Logger from './Logger' import { IHashSettings } from './interfaces/Resource' import { yieldToEventLoop } from './yieldToEventLoop' +import Mappings from './Mappings' export interface ScanResult { CREATE: Diff> @@ -14,23 +15,36 @@ export interface ScanResult } export default class Scanner { + private mappings: Mappings private oldTree: TItem private newTree: TItem private mergeable: (i1: TItem, i2: TItem) => boolean private hashSettings: IHashSettings private checkHashes: boolean private hasCache: boolean + private createMappings: boolean private iterations = 0 private result: ScanResult - constructor(oldTree:TItem, newTree:TItem, mergeable:(i1:TItem, i2:TItem)=>boolean, hashSettings: IHashSettings, checkHashes = true, hasCache = true) { + constructor( + mappings: Mappings, + oldTree:TItem, + newTree:TItem, + mergeable:(i1:TItem, i2:TItem)=>boolean, + hashSettings: IHashSettings, + checkHashes = true, + hasCache = true, + createMappings = true, + ) { + this.mappings = mappings this.oldTree = oldTree this.newTree = newTree this.mergeable = mergeable this.hashSettings = hashSettings this.checkHashes = typeof checkHashes === 'undefined' ? true : checkHashes this.hasCache = hasCache + this.createMappings = createMappings this.result = { CREATE: new Diff(), UPDATE: new Diff(), @@ -53,8 +67,14 @@ export default class Scanner async diffItem(oldItem:TItem, newItem:TItem):Promise { if (oldItem.type === 'folder' && newItem.type === 'folder') { + if (this.createMappings) { + await this.addMapping(oldItem, newItem) + } return this.diffFolder(oldItem, newItem) } else if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') { + if (this.createMappings) { + await this.addMapping(oldItem, newItem) + } return this.diffBookmark(oldItem, newItem) } else { throw new Error('Mismatched diff items: ' + oldItem.type + ', ' + newItem.type) @@ -182,7 +202,7 @@ export default class Scanner this.mergeable(removedItem, createdItem) && (removedItem.type !== 'folder' || (!this.hasCache && - removedItem.childrenSimilarity(createdItem) > 0.8)) + removedItem.childrenSimilarity(createdItem) >= 0.8)) ) { this.result.CREATE.retract(createAction) this.result.REMOVE.retract(removeAction) @@ -243,6 +263,9 @@ export default class Scanner reconciled = true if (oldItem.type === ItemType.FOLDER) { await this.diffItem(oldItem, createdItem) + } else if (this.createMappings) { + // Usually we let diffItem handle mapping creation but here we need to do it ourselves + await this.addMapping(oldItem, createdItem) } } else { const newItem = createdItem.findItemFilter( @@ -275,6 +298,9 @@ export default class Scanner reconciled = true if (removedItem.type === ItemType.FOLDER) { await this.diffItem(removedItem, newItem) + } else if (this.createMappings) { + // Usually we let diffItem handle mapping creation but here we need to do it ourselves + await this.addMapping(removedItem, newItem) } } } @@ -292,6 +318,30 @@ export default class Scanner }) } + async addMapping(oldItem: TItem, newItem: TItem): Promise { + let localId, remoteId + if ( + newItem.location === ItemLocation.SERVER && + oldItem.location === ItemLocation.LOCAL + ) { + localId = oldItem.id + remoteId = newItem.id + } else if ( + oldItem.location === ItemLocation.SERVER && + newItem.location === ItemLocation.LOCAL + ) { + localId = newItem.id + remoteId = oldItem.id + } else { + return + } + if (newItem.type === 'folder') { + await this.mappings.addFolder({ localId, remoteId }) + } else { + await this.mappings.addBookmark({ localId, remoteId }) + } + } + async addReorders(): Promise { Logger.log('Scanner: Generate reorders') const targets = {} diff --git a/src/lib/strategies/Default.ts b/src/lib/strategies/Default.ts index 6c1b9fe050..17ec9553cb 100644 --- a/src/lib/strategies/Default.ts +++ b/src/lib/strategies/Default.ts @@ -626,12 +626,11 @@ export default class SyncProcess { async getDiffs():Promise<{localScanResult:ScanResult, serverScanResult:ScanResult}> { const mappingsSnapshot = this.mappings.getSnapshot() - const newMappings = [] - const isUsingTabs = await this.localTree.isUsingBrowserTabs?.() // Since we have the cache available, Diff cache and both trees.. const localScanner = new Scanner( + this.mappings, this.cacheTreeRoot, this.localTreeRoot, (oldItem, newItem) => { @@ -656,8 +655,11 @@ export default class SyncProcess { return false }, this.hashSettings, + true, + false, ) const serverScanner = new Scanner( + this.mappings, this.cacheTreeRoot, this.serverTreeRoot, (oldItem, newItem) => { @@ -671,30 +673,28 @@ export default class SyncProcess { // The two are mappable, no-brainer... if (Mappings.mappable(mappingsSnapshot, oldItem, newItem)) { - newMappings.push([oldItem, newItem]) return true } // We also allow canMergeWith here for bookmarks, because e.g. for NextcloudFolders the id of moved bookmarks changes (because their id is ";") if (oldItem.type === 'bookmark' && newItem.type === 'bookmark' && oldItem.canMergeWith(newItem)) { - newMappings.push([oldItem, newItem]) return true } // We also allow canMergeWith here for folders, if we're dealing with tabs, because Window IDs are not stable if (isUsingTabs && oldItem.type === 'folder' && newItem.type === 'folder' && oldItem.canMergeWith(newItem)) { - newMappings.push([oldItem, newItem]) return true } return false }, this.hashSettings, + true, + true, ) Logger.log('Calculating diffs for local and server trees relative to cache tree') const localScanResult = await localScanner.run() const serverScanResult = await serverScanner.run() - await Parallel.map(newMappings, ([localItem, serverItem]) => this.addMapping(this.server, localItem, serverItem.id), 1) return {localScanResult, serverScanResult} } @@ -765,26 +765,32 @@ export default class SyncProcess { )) if (concurrentCreation) { // created on both the target and sourcely, try to reconcile - const newMappings = [] const subScanner = new Scanner( + this.mappings, concurrentCreation.payload, // target tree action.payload, // source tree (oldItem, newItem) => { - if (oldItem.type === newItem.type && oldItem.canMergeWith(newItem)) { - // if two items can be merged, we'll add mappings here directly - newMappings.push([oldItem, newItem.id]) + if ( + oldItem.type === newItem.type && + oldItem.canMergeWith(newItem) + ) { return true } return false }, this.hashSettings, - false + false, + true, + true, ) const scanResult = await subScanner.run() - newMappings.push([concurrentCreation.payload, action.payload.id]) - await Parallel.each(newMappings, async([oldItem, newId]) => { - await this.addMapping(action.payload.location === ItemLocation.LOCAL ? this.localTree : this.server, oldItem, newId) - },1) + await this.addMapping( + action.payload.location === ItemLocation.LOCAL + ? this.localTree + : this.server, + concurrentCreation.payload, + action.payload.id + ) // SubScanner may reveal residual CREATE/REMOVE actions that we add to the plan here // We do not act on REMOVEs, only on CREATEs as we merge the two sides @@ -1098,25 +1104,25 @@ export default class SyncProcess { // Try bulk import with sub folders const imported = await resource.bulkImportFolder(id, action.oldItem.copyWithLocation(false, action.payload.location)) as Folder await done() - const newMappings = [] const subScanner = new Scanner( + this.mappings, action.oldItem, imported, (oldItem, newItem) => { - if (oldItem.type === newItem.type && oldItem.canMergeWith(newItem)) { - // if two items can be merged, we'll add mappings here directly - newMappings.push([oldItem, newItem.id]) + if ( + oldItem.type === newItem.type && + oldItem.canMergeWith(newItem) + ) { return true } return false }, this.hashSettings, false, + true, + true, ) await subScanner.run() - await Parallel.each(newMappings, async([oldItem, newId]: [TItem, string|number]) => { - await this.addMapping(resource, oldItem, newId) - }, 10) if ('orderFolder' in resource) { const mappingsSnapshot = this.mappings.getSnapshot() @@ -1157,25 +1163,26 @@ export default class SyncProcess { Logger.log('Attempting chunked bulk import') tempItem.children = bookmarks.splice(0, 70) const imported = await resource.bulkImportFolder(action.payload.id, tempItem) - const newMappings = [] const subScanner = new Scanner( + this.mappings, tempItem, imported, (oldItem, newItem) => { - if (oldItem.type === newItem.type && oldItem.canMergeWith(newItem)) { + if ( + oldItem.type === newItem.type && + oldItem.canMergeWith(newItem) + ) { // if two items can be merged, we'll add mappings here directly - newMappings.push([oldItem, newItem.id]) return true } return false }, this.hashSettings, false, + true, + true, ) await subScanner.run() - await Parallel.each(newMappings, async([oldItem, newId]: [TItem, string|number]) => { - await this.addMapping(resource, oldItem, newId) - }, 10) } // create sub plan for the folders diff --git a/src/lib/strategies/Merge.ts b/src/lib/strategies/Merge.ts index 14f8e5a49a..688614022c 100644 --- a/src/lib/strategies/Merge.ts +++ b/src/lib/strategies/Merge.ts @@ -1,4 +1,4 @@ -import { Folder, ItemLocation, TItem, TItemLocation, TOppositeLocation } from '../Tree' +import { Folder, ItemLocation, TItemLocation, TOppositeLocation } from '../Tree' import Diff, { CreateAction, MoveAction, PlanStage1 } from '../Diff' import Scanner, { ScanResult } from '../Scanner' import * as Parallel from 'async-parallel' @@ -15,8 +15,8 @@ export default class MergeSyncProcess extends DefaultSyncProcess { serverScanResult: ScanResult }> { // If there's no cache, diff the two trees directly - const newMappings: TItem[][] = [] const localScanner = new Scanner( + this.mappings, this.serverTreeRoot, this.localTreeRoot, (serverItem, localItem) => { @@ -24,16 +24,17 @@ export default class MergeSyncProcess extends DefaultSyncProcess { localItem.type === serverItem.type && serverItem.canMergeWith(localItem) ) { - newMappings.push([localItem, serverItem]) return true } return false }, this.hashSettings, false, - false + false, + true ) const serverScanner = new Scanner( + this.mappings, this.localTreeRoot, this.serverTreeRoot, (localItem, serverItem) => { @@ -41,24 +42,17 @@ export default class MergeSyncProcess extends DefaultSyncProcess { serverItem.type === localItem.type && serverItem.canMergeWith(localItem) ) { - newMappings.push([localItem, serverItem]) return true } return false }, this.hashSettings, false, - false + false, + true ) const localScanResult = await localScanner.run() const serverScanResult = await serverScanner.run() - await Parallel.map( - newMappings, - ([localItem, serverItem]) => { - return this.addMapping(this.server, localItem, serverItem.id) - }, - 1 - ) return { localScanResult, serverScanResult } } @@ -119,7 +113,6 @@ export default class MergeSyncProcess extends DefaultSyncProcess { REORDER: new Diff(), } - const findChainCacheForCreations = {} await Parallel.each( sourceScanResult.CREATE.getActions(), async(action) => { @@ -134,8 +127,8 @@ export default class MergeSyncProcess extends DefaultSyncProcess { ) if (concurrentCreation) { // created on both the target and sourcely, try to reconcile - const newMappings = [] const subScanner = new Scanner( + this.mappings, concurrentCreation.payload, // target tree action.payload, // source tree (oldItem, newItem) => { @@ -144,7 +137,6 @@ export default class MergeSyncProcess extends DefaultSyncProcess { oldItem.canMergeWith(newItem) ) { // if two items can be merged, we'll add mappings here directly - newMappings.push([oldItem, newItem.id]) return true } return false @@ -153,19 +145,12 @@ export default class MergeSyncProcess extends DefaultSyncProcess { false ) const scanResult = await subScanner.run() - newMappings.push([concurrentCreation.payload, action.payload.id]) - await Parallel.each( - newMappings, - async ([oldItem, newId]) => { - await this.addMapping( - action.payload.location === ItemLocation.LOCAL - ? this.localTree - : this.server, - oldItem, - newId - ) - }, - 1 + await this.addMapping( + action.payload.location === ItemLocation.LOCAL + ? this.localTree + : this.server, + concurrentCreation.payload, + action.payload.id ) // SubScanner may reveal residual CREATE/REMOVE actions that we add to the plan here @@ -320,7 +305,7 @@ export default class MergeSyncProcess extends DefaultSyncProcess { await Parallel.each( sourceScanResult.UPDATE.getActions(), - async (action) => { + async(action) => { const concurrentUpdate = targetUpdates.find( (a) => action.payload.type === a.payload.type && @@ -338,7 +323,7 @@ export default class MergeSyncProcess extends DefaultSyncProcess { await Parallel.each( sourceScanResult.REORDER.getActions(), - async (action) => { + async(action) => { if (avoidTargetReorders[action.payload.id]) { return } diff --git a/src/lib/strategies/Unidirectional.ts b/src/lib/strategies/Unidirectional.ts index 44c906f631..b504931829 100644 --- a/src/lib/strategies/Unidirectional.ts +++ b/src/lib/strategies/Unidirectional.ts @@ -2,7 +2,7 @@ import DefaultStrategy, { ISerializedSyncProcess , ACTION_CONCURRENCY } from './ import Diff, { Action, ActionType, PlanRevert, PlanStage1, PlanStage3, ReorderAction } from '../Diff' import * as Parallel from 'async-parallel' import Mappings, { MappingSnapshot } from '../Mappings' -import { Folder, ItemLocation, TItem, TItemLocation, TOppositeLocation } from '../Tree' +import { Folder, ItemLocation, ItemType, TItem, TItemLocation, TOppositeLocation } from '../Tree' import Logger from '../Logger' import { CancelledSyncError } from '../../errors/Error' import TResource from '../interfaces/Resource' @@ -86,7 +86,6 @@ export default class UnidirectionalSyncProcess extends DefaultStrategy { async getDiff(): Promise> { const mappingsSnapshot = this.mappings.getSnapshot() - const newMappings = [] const slaveTree = this.direction === ItemLocation.SERVER ? this.serverTreeRoot @@ -96,6 +95,7 @@ export default class UnidirectionalSyncProcess extends DefaultStrategy { ? this.localTreeRoot : this.serverTreeRoot const scanner = new Scanner( + this.mappings, slaveTree, masterTree, // We can't rely on a cacheTree, thus we have to accept canMergeWith results as well @@ -116,30 +116,22 @@ export default class UnidirectionalSyncProcess extends DefaultStrategy { return false } if (serverItem.canMergeWith(localItem)) { - newMappings.push([localItem, serverItem]) return true } if (Mappings.mappable(mappingsSnapshot, serverItem, localItem)) { - newMappings.push([localItem, serverItem]) return true } return false }, this.hashSettings, false, - false + false, + true ) Logger.log( 'Unidirectional: Calculating the diff between local and server trees' ) const scanResult = await scanner.run() - await Parallel.map( - newMappings, - ([localItem, serverItem]) => { - return this.addMapping(this.server, localItem, serverItem.id) - }, - 1 - ) return scanResult } diff --git a/src/test/test.js b/src/test/test.js index 20230f488d..add8a252f6 100644 --- a/src/test/test.js +++ b/src/test/test.js @@ -2103,6 +2103,143 @@ describe('Floccus', function() { false ) }) + it('should move items without confusing folders (2)', async function() { + const localRoot = account.getData().localRoot + + const folder1 = await browser.bookmarks.create({ + title: 'a', + parentId: localRoot + }) + const folder2 = await browser.bookmarks.create({ + title: 'b', + parentId: folder1.id + }) + const folder3 = await browser.bookmarks.create({ + title: 'c', + parentId: folder2.id + }) + const folder4 = await browser.bookmarks.create({ + title: 'd', + parentId: localRoot + }) + const folder5 = await browser.bookmarks.create({ + title: 'e', + parentId: folder4.id, + }) + const folder6 = await browser.bookmarks.create({ + title: 'f', + parentId: folder5.id, + }) + const folderX = await browser.bookmarks.create({ + title: 'X', + parentId: folder3.id, + }) + await browser.bookmarks.create({ + title: 'url', + url: 'http://ur.l/', + parentId: folderX.id + }) + const folderX2 = await browser.bookmarks.create({ + title: 'X', + parentId: folder6.id, + }) + await browser.bookmarks.create({ + title: 'test', + url: 'http://urrr.l/', + parentId: folderX2.id + }) + const test2Bm = await browser.bookmarks.create({ + title: 'test2', + url: 'http://urrr2.l/', + parentId: folder1.id + }) + + await account.sync() // propagate to server + expect(account.getData().error).to.not.be.ok + + await account.sync() // make sure order is propagated + expect(account.getData().error).to.not.be.ok + + await account.init() // Remove the cache + + await browser.bookmarks.move(folderX.id, { parentId: folder2.id }) + await browser.bookmarks.move(folderX2.id, { parentId: folder5.id }) + await browser.bookmarks.move(test2Bm.id, { parentId: folderX.id }) + + await account.sync() // propagate to server + expect(account.getData().error).to.not.be.ok + + const tree = await getAllBookmarks(account) + expectTreeEqual( + tree, + new Folder({ + title: tree.title, + children: [ + new Folder({ + title: 'a', + children: [ + new Folder({ + title: 'b', + children: [ + new Folder({ + title: 'c', + children: [], + }), + new Folder({ + title: 'X', + children: [ + new Bookmark({ + title: 'url', + url: 'http://ur.l/', + }), + new Bookmark({ + title: 'test2', + url: 'http://urrr2.l/', + }), + ], + }), + ], + }), + ], + }), + new Folder({ + title: 'd', + children: [ + new Folder({ + title: 'e', + children: [ + new Folder({ + title: 'f', + children: [], + }), + new Folder({ + title: 'X', + children: [ + new Bookmark({ + title: 'test', + url: 'http://urrr.l/', + }), + ], + }), + ], + }), + ], + }), + ], + }), + false, + false + ) + + const localTree = await account.localTree.getBookmarksTree(true) + localTree.title = tree.title + expectTreeEqual( + localTree, + tree, + false, + false + ) + }) it('should integrate existing items from both sides', async function() { const localRoot = account.getData().localRoot @@ -3968,6 +4105,143 @@ describe('Floccus', function() { false ) }) + it('should move items without confusing folders', async function() { + const localRoot = account.getData().localRoot + + const folder1 = await browser.bookmarks.create({ + title: 'a', + parentId: localRoot + }) + const folder2 = await browser.bookmarks.create({ + title: 'b', + parentId: folder1.id + }) + const folder3 = await browser.bookmarks.create({ + title: 'c', + parentId: folder2.id + }) + const folder4 = await browser.bookmarks.create({ + title: 'd', + parentId: localRoot + }) + const folder5 = await browser.bookmarks.create({ + title: 'e', + parentId: folder4.id, + }) + const folder6 = await browser.bookmarks.create({ + title: 'f', + parentId: folder5.id, + }) + const folderX = await browser.bookmarks.create({ + title: 'X', + parentId: folder3.id, + }) + await browser.bookmarks.create({ + title: 'url', + url: 'http://ur.l/', + parentId: folderX.id + }) + const folderX2 = await browser.bookmarks.create({ + title: 'X', + parentId: folder6.id, + }) + await browser.bookmarks.create({ + title: 'test', + url: 'http://urrr.l/', + parentId: folderX2.id + }) + const test2Bm = await browser.bookmarks.create({ + title: 'test2', + url: 'http://urrr2.l/', + parentId: folder1.id + }) + + await account.sync() // propagate to server + expect(account.getData().error).to.not.be.ok + + await account.sync() // make sure order is propagated + expect(account.getData().error).to.not.be.ok + + await account.init() // remove cache + + await browser.bookmarks.move(folderX.id, { parentId: folder2.id }) + await browser.bookmarks.move(folderX2.id, { parentId: folder5.id }) + await browser.bookmarks.move(test2Bm.id, { parentId: folderX.id }) + + await account.sync() // propagate to server + expect(account.getData().error).to.not.be.ok + + const tree = await getAllBookmarks(account) + expectTreeEqual( + tree, + new Folder({ + title: tree.title, + children: [ + new Folder({ + title: 'a', + children: [ + new Folder({ + title: 'b', + children: [ + new Folder({ + title: 'c', + children: [], + }), + new Folder({ + title: 'X', + children: [ + new Bookmark({ + title: 'url', + url: 'http://ur.l/', + }), + new Bookmark({ + title: 'test2', + url: 'http://urrr2.l/', + }), + ], + }), + ], + }), + ], + }), + new Folder({ + title: 'd', + children: [ + new Folder({ + title: 'e', + children: [ + new Folder({ + title: 'f', + children: [], + }), + new Folder({ + title: 'X', + children: [ + new Bookmark({ + title: 'test', + url: 'http://urrr.l/', + }), + ], + }), + ], + }), + ], + }), + ], + }), + false, + false + ) + + const localTree = await account.localTree.getBookmarksTree(true) + localTree.title = tree.title + expectTreeEqual( + localTree, + tree, + false, + false + ) + }) }) }) context('with two clients', function() { diff --git a/src/ui/views/native/Home.vue b/src/ui/views/native/Home.vue index 668ae360bc..a83362a993 100644 --- a/src/ui/views/native/Home.vue +++ b/src/ui/views/native/Home.vue @@ -1,13 +1,14 @@