Merge pull request #2253 from floccusaddon/fix/2230/crank-up-children-similarity

refactor(Scanner): Move mapping creation from mergeable to diffItem

Author: marcelklehr

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

src/lib/Mappings.ts

@@ -48,6 +48,7 @@ export default class Mappings {
   }
 
   async addFolder({ localId, remoteId }: { localId?:string|number, remoteId?:string|number }):Promise<void> {
+    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<void> {
+    Mappings.remove(this.bookmarks, { localId, remoteId })
     Mappings.add(this.bookmarks, { localId, remoteId })
   }
 

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<L1 extends TItemLocation, L2 extends TItemLocation> {
   CREATE: Diff<L1, L2, CreateAction<L1, L2>>
@@ -14,23 +15,36 @@ export interface ScanResult<L1 extends TItemLocation, L2 extends TItemLocation>
 }
 
 export default class Scanner<L1 extends TItemLocation, L2 extends TItemLocation> {
+  private mappings: Mappings
   private oldTree: TItem<L1>
   private newTree: TItem<L2>
   private mergeable: (i1: TItem<TItemLocation>, i2: TItem<TItemLocation>) => boolean
   private hashSettings: IHashSettings
   private checkHashes: boolean
   private hasCache: boolean
+  private createMappings: boolean
   private iterations = 0
 
   private result: ScanResult<L2, L1>
 
-  constructor(oldTree:TItem<L1>, newTree:TItem<L2>, mergeable:(i1:TItem<TItemLocation>, i2:TItem<TItemLocation>)=>boolean, hashSettings: IHashSettings, checkHashes = true, hasCache = true) {
+  constructor(
+    mappings: Mappings,
+    oldTree:TItem<L1>,
+    newTree:TItem<L2>,
+    mergeable:(i1:TItem<TItemLocation>, i2:TItem<TItemLocation>)=>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<L1 extends TItemLocation, L2 extends TItemLocation>
 
   async diffItem(oldItem:TItem<L1>, newItem:TItem<L2>):Promise<void> {
     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<L1 extends TItemLocation, L2 extends TItemLocation>
             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<L1 extends TItemLocation, L2 extends TItemLocation>
             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<L1 extends TItemLocation, L2 extends TItemLocation>
               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<L1 extends TItemLocation, L2 extends TItemLocation>
     })
   }
 
+  async addMapping(oldItem: TItem<L1>, newItem: TItem<L2>): Promise<void> {
+    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<void> {
     Logger.log('Scanner: Generate reorders')
     const targets = {}

src/lib/strategies/Default.ts

@@ -626,12 +626,11 @@ export default class SyncProcess {
   async getDiffs():Promise<{localScanResult:ScanResult<typeof ItemLocation.LOCAL, TItemLocation>, serverScanResult:ScanResult<typeof ItemLocation.SERVER, TItemLocation>}> {
     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 "<bookmarkID>;<folderId>")
         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<typeof targetLocation>
           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<TItemLocation>, 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<TItemLocation>, string|number]) => {
-              await this.addMapping(resource, oldItem, newId)
-            }, 10)
           }
 
           // create sub plan for the folders