fix: performance issue when renaming an object key, see #543 (#551)

Author: josdejong

src/lib/logic/operations.test.ts

@@ -1,10 +1,11 @@
-import { test, describe } from 'vitest'
+import { test, describe, expect } from 'vitest'
 import assert from 'assert'
 import {
   clipboardToValues,
   createNestedValueOperations,
   createNewValue,
   moveInsideParent,
+  rename,
   revertJSONPatchWithMoveOperations
 } from './operations.js'
 import { createMultiSelection } from './selection.js'
@@ -270,7 +271,6 @@ describe('operations', () => {
 
       const revertOperations = revertJSONPatchWithMoveOperations(json, operations)
       assert.deepStrictEqual(revertOperations, [
-        { op: 'move', from: '/b', path: '/b' },
         { op: 'move', from: '/a', path: '/a' },
         { op: 'move', from: '/c', path: '/c' }
       ])
@@ -280,6 +280,114 @@ describe('operations', () => {
       assert.deepStrictEqual(Object.keys(revertedJson), ['b', 'a', 'c'])
     })
 
+    test('should remove redundant move operations when renaming a key and maintaining order (1)', () => {
+      const json = {
+        a: 1,
+        b: 2,
+        c: 3
+      }
+
+      const operations: JSONPatchOperation[] = [{ op: 'move', from: '/b', path: '/b_renamed' }]
+      const updatedJson = immutableJSONPatch(json, operations)
+      assert.deepStrictEqual(Object.keys(updatedJson as Record<string, number>), [
+        'a',
+        'c',
+        'b_renamed'
+      ])
+
+      const revertOperations = revertJSONPatchWithMoveOperations(json, operations)
+      assert.deepStrictEqual(revertOperations, [
+        { op: 'move', from: '/b_renamed', path: '/b' },
+        { op: 'move', from: '/c', path: '/c' },
+        { op: 'move', from: '/c', path: '/c' } // TODO: would be neat to remove the duplicate
+      ])
+
+      const revertedJson = immutableJSONPatch(updatedJson, revertOperations)
+      assert.deepStrictEqual(revertedJson, json)
+      assert.deepStrictEqual(Object.keys(revertedJson), ['a', 'b', 'c'])
+    })
+
+    test('should remove redundant move operations when renaming a key and maintaining order (2)', () => {
+      const json = {
+        a: 1,
+        b: 2,
+        c: 3
+      }
+
+      const operations: JSONPatchOperation[] = [
+        { op: 'move', from: '/b', path: '/b_renamed' },
+        { op: 'move', from: '/c', path: '/c' }
+      ]
+      const updatedJson = immutableJSONPatch(json, operations)
+      assert.deepStrictEqual(Object.keys(updatedJson as Record<string, number>), [
+        'a',
+        'b_renamed',
+        'c'
+      ])
+
+      const revertOperations = revertJSONPatchWithMoveOperations(json, operations)
+      assert.deepStrictEqual(revertOperations, [
+        { op: 'move', from: '/b_renamed', path: '/b' },
+        { op: 'move', from: '/c', path: '/c' }
+      ])
+
+      const revertedJson = immutableJSONPatch(updatedJson, revertOperations)
+      assert.deepStrictEqual(revertedJson, json)
+      assert.deepStrictEqual(Object.keys(revertedJson), ['a', 'b', 'c'])
+    })
+
+    test('should remove redundant move operations when renaming a key and maintaining order (3)', () => {
+      const json = {
+        a: 1,
+        b: 2,
+        c: 3
+      }
+
+      const operations: JSONPatchOperation[] = [
+        { op: 'move', from: '/c', path: '/c_renamed' },
+        { op: 'move', from: '/a', path: '/a' } // we move `a` to the end (different operation)
+      ]
+      const updatedJson = immutableJSONPatch(json, operations)
+      assert.deepStrictEqual(Object.keys(updatedJson as Record<string, number>), [
+        'b',
+        'c_renamed',
+        'a'
+      ])
+
+      // Note: there are optimizations possible in the following revert operations,
+      //  but this is a made up example, not a real world example, quite an edge case.
+      const revertOperations = revertJSONPatchWithMoveOperations(json, operations)
+      assert.deepStrictEqual(revertOperations, [
+        { op: 'move', from: '/c_renamed', path: '/c' },
+        { op: 'move', from: '/b', path: '/b' },
+        { op: 'move', from: '/b', path: '/b' }, // TODO: would be neath to remove the duplicate
+        { op: 'move', from: '/c', path: '/c' }
+      ])
+
+      const revertedJson = immutableJSONPatch(updatedJson, revertOperations)
+      assert.deepStrictEqual(revertedJson, json)
+      assert.deepStrictEqual(Object.keys(revertedJson), ['a', 'b', 'c'])
+    })
+
+    test('should perform well when creating the revert patch of renaming a key in a large object', () => {
+      console.time('revert large patch')
+      const count = 100
+      const json: Record<string, number> = {}
+      for (let i = 0; i < count; i++) {
+        json['item ' + i] = i
+      }
+
+      const operations = rename([], Object.keys(json), 'item 0', 'item 0 (renamed)')
+      expect(operations.length).toEqual(count)
+      const updatedJson = immutableJSONPatch(json, operations)
+
+      const revertOperations = revertJSONPatchWithMoveOperations(json, operations)
+      expect(revertOperations.length).toEqual(count)
+      const revertedJson = immutableJSONPatch(updatedJson, revertOperations)
+      expect(revertedJson).toEqual(json)
+      console.timeEnd('revert large patch')
+    }, /* timeout */ 200)
+
     test('should restore correctly revert multiple remove operations in an array', () => {
       const json = [0, 1, 2, 3, 4]
 

src/lib/logic/operations.ts

@@ -1,4 +1,4 @@
-import { cloneDeepWith, first, initial, isEmpty, last, times } from 'lodash-es'
+import { cloneDeepWith, first, initial, isEmpty, isEqual, last, times } from 'lodash-es'
 import {
   compileJSONPointer,
   existsIn,
@@ -11,6 +11,7 @@ import {
   type JSONPatchAdd,
   type JSONPatchCopy,
   type JSONPatchDocument,
+  type JSONPatchMove,
   type JSONPatchOperation,
   type JSONPath,
   parseJSONPointer,
@@ -39,7 +40,6 @@ import {
 } from './selection.js'
 import type { ClipboardValues, DragInsideAction, JSONParser, JSONSelection } from '$lib/types'
 import { int } from '../utils/numberUtils.js'
-import { dedupeKeepLast } from '$lib/utils/arrayUtils'
 
 /**
  * Create a JSONPatch for an insert operation.
@@ -722,30 +722,146 @@ export function revertJSONPatchWithMoveOperations(
   json: unknown,
   operations: JSONPatchDocument
 ): JSONPatchDocument {
-  return dedupeKeepLast(
-    revertJSONPatch(json, operations, {
-      before: (json, operation, revertOperations) => {
-        if (isJSONPatchRemove(operation)) {
-          const path = parseJSONPointer(operation.path)
-          return {
-            revertOperations: [...revertOperations, ...createRevertMoveOperations(json, path)]
-          }
+  // first, we merge series of move operations inside an object. These move operations are used
+  // to ensure a specific ordering of the keys in the object. Reverting those move events one by
+  // one would explode the amount of move operations in the revert operations and contain mostly
+  // duplicates.
+  const filteredOperations = _filterRedundantMoveOperations(json, operations)
+
+  return revertJSONPatch(json, filteredOperations, {
+    before: (json, operation, revertOperations) => {
+      if (isJSONPatchRemove(operation)) {
+        // we must restore the key order when reverting removing of an object key
+        const path = parseJSONPointer(operation.path)
+        return {
+          revertOperations: [...revertOperations, ...createRevertMoveOperations(json, path)]
         }
+      }
 
-        if (isJSONPatchMove(operation)) {
-          const from = parseJSONPointer(operation.from)
-          return {
-            revertOperations:
-              operation.from === operation.path
-                ? [operation, ...createRevertMoveOperations(json, from)] // move in-place (just for re-ordering object keys)
-                : [...revertOperations, ...createRevertMoveOperations(json, from)]
-          }
+      if (isJSONPatchMove(operation)) {
+        const from = parseJSONPointer(operation.from)
+        return {
+          revertOperations:
+            operation.from === operation.path
+              ? [operation, ...createRevertMoveOperations(json, from)] // move in-place (just for re-ordering object keys)
+              : [...revertOperations, ...createRevertMoveOperations(json, from)]
         }
-
-        return { document: json }
       }
-    })
-  )
+
+      return { document: json }
+    }
+  })
+}
+
+/**
+ * Filter move operations which are redundant when creating the reverse patch operations.
+ *
+ * This is a preprocessing step for creating the revert patch operations. For example, having
+ * json {b:2, a:1, c:3} and the following move operations (this will reorder the keys):
+ *
+ *     [
+ *       { op: 'move', from: '/a', path: '/a' },
+ *       { op: 'move', from: '/b', path: '/b' },
+ *       { op: 'move', from: '/c', path: '/c' }
+ *     ]
+ *
+ * This function will return only moving of b, since that is the first of all the moved keys:
+ *
+ *     [
+ *       { op: 'move', from: '/b', path: '/b' }
+ *     ]
+ *
+ * So, when creating the reverse operations, this single operation is enough to create the needed
+ * move operations for all lower down keys to restore the original order of the keys.
+ */
+function _filterRedundantMoveOperations(
+  json: unknown,
+  operations: JSONPatchOperation[]
+): JSONPatchOperation[] {
+  // check whether this is a set of move operations (cheap check)
+  if (isEmpty(operations) || !operations.every(isJSONPatchMove)) {
+    return operations
+  }
+
+  // parse the JSON pointers once and split them in parent paths and keys
+  const processedOps: ProcessedOperation[] = []
+  for (const operation of operations) {
+    const from = splitParentKey(parseJSONPointer(operation.from))
+    const path = splitParentKey(parseJSONPointer(operation.path))
+    if (!from || !path) {
+      return operations
+    }
+    processedOps.push({ from, path, operation })
+  }
+
+  // check whether the first movement is inside an object (and not an array)
+  const parentPath = processedOps[0].path.parent
+  const parent = getIn(json, parentPath)
+  if (!isJSONObject(parent)) {
+    return operations
+  }
+
+  // check whether all movements are within the same parent object
+  if (!processedOps.every((op) => _equalParentPath(op, parentPath))) {
+    return operations
+  }
+
+  const firstKey = _findKeyWithLowestKeyIndex(processedOps, json)
+  const getOperation = (op: ProcessedOperation) => op.operation
+
+  // only return the operation moving the firstKey back, and all rename operations
+  const renameOps = processedOps.filter((op) => op.operation.from !== op.operation.path)
+  return renameOps.some((op) => op.path.key === firstKey)
+    ? renameOps.map(getOperation) // no need to add the firstKey move operation
+    : [moveDown(parentPath, firstKey), ...renameOps.map(getOperation)]
+}
+
+interface ParentKey {
+  parent: string[]
+  key: string
+}
+
+interface ProcessedOperation {
+  from: ParentKey
+  path: ParentKey
+  operation: JSONPatchMove
+}
+
+function splitParentKey(path: JSONPath): ParentKey | undefined {
+  return path.length > 0 ? { parent: initial(path), key: last(path) as string } : undefined
+}
+
+/**
+ * Test whether a move operation has a specific parent path for both `from` and `path`
+ */
+function _equalParentPath(
+  operation: ProcessedOperation,
+  parentPath: JSONPath | undefined
+): boolean {
+  return isEqual(operation.from.parent, parentPath) && isEqual(operation.path.parent, parentPath)
+}
+
+function _findKeyWithLowestKeyIndex(processedOps: ProcessedOperation[], json: unknown): string {
+  const keys = Object.keys(json as Record<string, unknown>)
+  const movedKeys = keys.slice()
+
+  // first execute all move operations on the list with keys
+  // here we assume all move operations have the same parent and are keys in an object
+  for (const op of processedOps) {
+    const index = movedKeys.indexOf(op.from.key)
+    if (index !== -1) {
+      movedKeys.splice(index, 1)
+      movedKeys.push(op.path.key)
+    }
+  }
+
+  // find the first changed key
+  let i = 0
+  while (i < keys.length && keys[i] === movedKeys[i]) {
+    i++
+  }
+
+  return movedKeys[i]
 }
 
 function createRevertMoveOperations(json: unknown, path: JSONPath): JSONPatchOperation[] {

src/lib/utils/arrayUtils.test.ts

@@ -4,7 +4,6 @@ import {
   arrayStartsWith,
   arrayToObject,
   compareArrays,
-  dedupeKeepLast,
   forEachSample,
   getNestedPaths,
   moveItems,
@@ -158,43 +157,4 @@ describe('arrayUtils', () => {
       expect(sample([], 4)).toEqual([])
     })
   })
-
-  describe('dedupeKeepLast', () => {
-    test('should keep the last item in case of a duplicate', () => {
-      expect(dedupeKeepLast([3, 1, 3])).toEqual([1, 3])
-      expect(dedupeKeepLast([3, 1, 3, 3])).toEqual([1, 3])
-
-      expect(
-        dedupeKeepLast([
-          { id: 1, name: 'Joe' },
-          { id: 3, name: 'Sarah' },
-          { id: 1, name: 'Joe' }
-        ])
-      ).toEqual([
-        { id: 3, name: 'Sarah' },
-        { id: 1, name: 'Joe' }
-      ])
-
-      expect(
-        dedupeKeepLast([
-          { id: 1, name: 'Joe' },
-          { id: 3, name: 'Sarah' },
-          { id: 1, name: 'Joey' }
-        ])
-      ).toEqual([
-        { id: 1, name: 'Joe' },
-        { id: 3, name: 'Sarah' },
-        { id: 1, name: 'Joey' }
-      ])
-    })
-
-    test('should pass a custom comparator', () => {
-      const comparator = (a: Record<string, unknown>, b: Record<string, unknown>) => a.id === b.id
-
-      const input = [{ id: 1, name: 'Joe' }, { id: 3 }, { id: 1, name: 'Joey' }]
-      const expected = [{ id: 3 }, { id: 1, name: 'Joey' }]
-
-      expect(dedupeKeepLast(input, comparator)).toEqual(expected)
-    })
-  })
 })

src/lib/utils/arrayUtils.ts

@@ -189,19 +189,3 @@ export function forEachSample<T>(
 export function insertItemsAt<T>(array: T[], index: number, items: T[]): T[] {
   return array.slice(0, index).concat(items).concat(array.slice(index))
 }
-
-/**
- * Remove duplicate entries from an array, keeping the last item in case of a duplicate.
- * This is similar to the `uniqWith` function of Lodash, but that function keeps the *first* item in case of a duplicate.
- */
-export function dedupeKeepLast<T>(array: T[], comparator: (a: T, b: T) => boolean = isEqual): T[] {
-  return array.filter((item, index) => {
-    for (let i = index + 1; i < array.length; i++) {
-      if (comparator(item, array[i])) {
-        return false
-      }
-    }
-
-    return true
-  })
-}