Merge pull request #232 from aryelu/master

jsonpatch formatter array move generates incorrect patch - #230

Author: benjamine

.gitignore

@@ -18,3 +18,4 @@ dist
 build
 
 npm-debug.log
+.idea/

src/formatters/jsonpatch.js

@@ -30,14 +30,23 @@ class JSONFormatter extends BaseFormatter {
     };
 
     context.pushMoveOp = function(to) {
-      const finalTo = `/${to}`;
       const from = this.currentPath();
-      this.result.push({op: OPERATIONS.move, from: from, path: finalTo});
+      this.result.push({
+        op: OPERATIONS.move,
+        from: from,
+        path: this.toPath(to),
+      });
     };
 
     context.currentPath = function() {
       return `/${this.path.join('/')}`;
     };
+
+    context.toPath = function(toPath) {
+      const to = this.path.slice();
+      to[to.length - 1] = toPath;
+      return `/${to.join('/')}`;
+    };
   }
 
   typeFormattterErrorFormatter(context, err) {
@@ -127,31 +136,29 @@ const opsByDescendingOrder = removeOps => sortBy(removeOps, (a, b) => {
   }
 });
 
-const partition = (arr, pred) => {
-  const left = [];
-  const right = [];
-
-  arr.forEach(el => {
-    const coll = pred(el) ? left : right;
-    coll.push(el);
-  });
-  return [left, right];
-};
-
-const partitionRemovedOps = jsonFormattedDiff => {
-  const isRemoveOp = ({op}) => op === 'remove';
-  return partition(
-    jsonFormattedDiff,
-    isRemoveOp
-  );
+export const partitionOps = (arr, fns) => {
+  const initArr = Array(fns.length + 1).fill().map(() => []);
+  return arr
+    .map(item => {
+      let position = fns.map(fn => fn(item)).indexOf(true);
+      if (position < 0) {
+        position = fns.length;
+      }
+      return { item, position };
+    })
+    .reduce((acc, item) => {
+      acc[ item.position ].push(item.item);
+      return acc;
+    }, initArr);
 };
-
-const reorderOps = jsonFormattedDiff => {
-  const removeOpsOtherOps = partitionRemovedOps(jsonFormattedDiff);
-  const removeOps = removeOpsOtherOps[0];
-  const otherOps = removeOpsOtherOps[1];
-  const removeOpsReverse = opsByDescendingOrder(removeOps);
-  return removeOpsReverse.concat(otherOps);
+const isMoveOp = ({op}) => op === 'move';
+const isRemoveOp = ({op}) => op === 'remove';
+
+const reorderOps = diff => {
+  const [ moveOps, removedOps, restOps ] =
+    partitionOps(diff, [ isMoveOp, isRemoveOp ]);
+  const removeOpsReverse = opsByDescendingOrder(removedOps);
+  return [ ...removeOpsReverse, ...moveOps, ...restOps ];
 };
 
 let defaultInstance;

test/index.js

@@ -392,6 +392,12 @@ describe('DiffPatcher', () => {
         path,
       });
 
+      const moveOp = (from, path) => ({
+        op: 'move',
+        from,
+        path,
+      });
+
       const addOp = (path, value) => ({
         op: 'add',
         path,
@@ -477,10 +483,112 @@ describe('DiffPatcher', () => {
           const expectedDiff = [removeOp('/0'), removeOp('/0/items/0')];
           expectFormat(before, after, expectedDiff);
         });
+
+        it('should annotate move', () => {
+          const before = [
+            anObjectWithId('first'),
+            anObjectWithId('second'),
+          ];
+          const after = [
+            anObjectWithId('second'),
+            anObjectWithId('first'),
+          ];
+          const expectedDiff = [moveOp('/1', '/0')];
+          expectFormat(before, after, expectedDiff);
+        });
+
+        it('should sort the ops', () => {
+          expectFormat(
+            { 'hl': [ { id: 1, bla: 'bla' }, { id: 2, bla: 'ga' } ] },
+            { 'hl': [ { id: 2, bla: 'bla' }, { id: 1, bla: 'ga' } ] },
+            [
+              moveOp('/hl/1', '/hl/0'),
+              replaceOp('/hl/0/bla', 'bla'),
+              replaceOp('/hl/1/bla', 'ga'),
+            ]);
+        });
       });
 
       it('should annotate as moved op', () => {
-        expectFormat([1, 2], [2, 1], [{ op: 'move', from: '/1', path: '/0' }]);
+        expectFormat([1, 2], [2, 1], [moveOp('/1', '/0')]);
+      });
+
+      it('should add full path for moved op', () => {
+        expectFormat(
+          {'hl': [1, 2]},
+          {'hl': [2, 1]},
+          [moveOp('/hl/1', '/hl/0')]);
+      });
+
+      it('should put the full path in move op and sort by HL - #230', () => {
+        const before = {
+          'middleName': 'z',
+          'referenceNumbers': [
+            {
+              'id': 'id-3',
+              'referenceNumber': '123',
+              'index': 'index-0',
+            },
+            {
+              'id': 'id-1',
+              'referenceNumber': '456',
+              'index': 'index-1',
+            },
+            {
+              'id': 'id-2',
+              'referenceNumber': '789',
+              'index': 'index-2',
+            },
+          ],
+        };
+        const after = {
+          'middleName': 'x',
+          'referenceNumbers': [
+            {
+              'id': 'id-1',
+              'referenceNumber': '456',
+              'index': 'index-0',
+            },
+            {
+              'id': 'id-3',
+              'referenceNumber': '123',
+              'index': 'index-1',
+            },
+            {
+              'id': 'id-2',
+              'referenceNumber': '789',
+              'index': 'index-2',
+            },
+          ],
+        };
+        const diff = [
+          {
+            'op': 'move',
+            'from': '/referenceNumbers/1',
+            'path': '/referenceNumbers/0',
+          },
+          {
+            'op': 'replace',
+            'path': '/middleName',
+            'value': 'x',
+          },
+          {
+            'op': 'replace',
+            'path': '/referenceNumbers/0/index',
+            'value': 'index-0',
+          },
+          {
+            'op': 'replace',
+            'path': '/referenceNumbers/1/index',
+            'value': 'index-1',
+          },
+        ];
+        instance = new DiffPatcher({
+          objectHash(obj) {
+            return obj.id;
+          },
+        });
+        expectFormat(before, after, diff);
       });
     });