From 60a0ad210384c25b43be6fa26b41e189d94bd05a Mon Sep 17 00:00:00 2001 From: angus croll Date: Sat, 25 Mar 2023 17:56:18 -0700 Subject: [PATCH] Fix #543. Simplify left vs right trim logic (#547) * poc-works * Fixed --- packages/collection-diff/index.cjs | 126 ++++++++++++++-------------- packages/collection-diff/index.mjs | 129 ++++++++++++++--------------- test/collection-diff/index.cjs | 35 ++++++-- test/object-extend/index.cjs | 1 - 4 files changed, 152 insertions(+), 139 deletions(-) diff --git a/packages/collection-diff/index.cjs b/packages/collection-diff/index.cjs index 0c2daf6a9..b74099de2 100644 --- a/packages/collection-diff/index.cjs +++ b/packages/collection-diff/index.cjs @@ -79,55 +79,43 @@ function diff(obj1, obj2, pathConverter) { return arr; }); - // we will gather all permutations and return the one with the fewest diffs - var permutations = [{remove: [], replace: [], add: []}]; - - function getDiff({obj1, obj2, basePath, basePathForRemoves, permutation}) { + function getDiff({obj1, obj2, basePath, basePathForRemoves, diffs}) { var obj1Keys = Object.keys(obj1); var obj1KeysLength = obj1Keys.length; var obj2Keys = Object.keys(obj2); var obj2KeysLength = obj2Keys.length; var path; - var newPermutation; - var lengthDelta = obj1.length - obj2.length; - // if both objects are arrays and obj1 length > obj2 length - // we create an additional permutation that trims obj1 from left - if (Array.isArray(obj1) && Array.isArray(obj2) && lengthDelta > 0) { - newPermutation = clonePermutation(permutation); - permutations.push(newPermutation); - } - // trim from right - for (var i = 0; i < obj1KeysLength; i++) { - var key = Array.isArray(obj1) ? Number(obj1Keys[i]) : obj1Keys[i]; - if (!(key in obj2)) { - path = basePathForRemoves.concat(key); - permutation.remove.push({ - op: 'remove', - path: pathConverter(path), - }); + if (trimFromRight(obj1, obj2)) { + for (var i = 0; i < obj1KeysLength; i++) { + var key = Array.isArray(obj1) ? Number(obj1Keys[i]) : obj1Keys[i]; + if (!(key in obj2)) { + path = basePathForRemoves.concat(key); + diffs.remove.push({ + op: 'remove', + path: pathConverter(path), + }); + } } - } - for (var i = 0; i < obj2KeysLength; i++) { - var key = Array.isArray(obj2) ? Number(obj2Keys[i]) : obj2Keys[i]; - pushReplaces({ - key, - obj1, - obj2, - path: basePath.concat(key), - pathForRemoves: basePath.concat(key), - permutation, - }); - } - - // if we created a new permutation above it means we should also try trimming from left - if (newPermutation) { + for (var i = 0; i < obj2KeysLength; i++) { + var key = Array.isArray(obj2) ? Number(obj2Keys[i]) : obj2Keys[i]; + pushReplaces({ + key, + obj1, + obj2, + path: basePath.concat(key), + pathForRemoves: basePath.concat(key), + diffs, + }); + } + } else { + // trim from left, objects are both arrays for (var i = 0; i < lengthDelta; i++) { path = basePathForRemoves.concat(i); - newPermutation.remove.push({ + diffs.remove.push({ op: 'remove', path: pathConverter(path), }); @@ -144,38 +132,34 @@ function diff(obj1, obj2, pathConverter) { // since list of removes are reversed before presenting result, // we need to ignore existing parent removes when doing nested removes pathForRemoves: basePath.concat(i + lengthDelta), - permutation: newPermutation, + diffs, }); } } } + var diffs = {remove: [], replace: [], add: []}; getDiff({ obj1, obj2, basePath: [], basePathForRemoves: [], - permutation: permutations[0], + diffs, }); - // find the shortest permutation - var finalDiffs = permutations.sort( - (a, b) => diffStepCount(a) > diffStepCount(b) ? 1 : -1 - )[0]; - // reverse removes since we want to maintain indexes - return finalDiffs.remove + return diffs.remove .reverse() - .concat(finalDiffs.replace) - .concat(finalDiffs.add); + .concat(diffs.replace) + .concat(diffs.add); - function pushReplaces({key, obj1, obj2, path, pathForRemoves, permutation}) { + function pushReplaces({key, obj1, obj2, path, pathForRemoves, diffs}) { var obj1AtKey = obj1[key]; var obj2AtKey = obj2[key]; if(!(key in obj1) && (key in obj2)) { var obj2Value = obj2AtKey; - permutation.add.push({ + diffs.add.push({ op: 'add', path: pathConverter(path), value: obj2Value, @@ -184,19 +168,19 @@ function diff(obj1, obj2, pathConverter) { if(Object(obj1AtKey) !== obj1AtKey || Object(obj2AtKey) !== obj2AtKey || differentTypes(obj1AtKey, obj2AtKey) ) { - pushReplace(path, permutation, obj2AtKey); + pushReplace(path, diffs, obj2AtKey); } else { if(!Object.keys(obj1AtKey).length && !Object.keys(obj2AtKey).length && String(obj1AtKey) != String(obj2AtKey)) { - pushReplace(path, permutation, obj2AtKey); + pushReplace(path, diffs, obj2AtKey); } else { getDiff({ obj1: obj1[key], obj2: obj2[key], basePath: path, basePathForRemoves: pathForRemoves, - permutation}); + diffs}); } } } @@ -211,18 +195,6 @@ function diff(obj1, obj2, pathConverter) { } } -function clonePermutation(permutation) { - return { - remove: permutation.remove.slice(0), - replace: permutation.replace.slice(0), - add: permutation.add.slice(0), - }; -} - -function diffStepCount(permutation) { - return permutation.remove.length + permutation.replace.length + permutation.add.length; -} - function jsonPatchPathConverter(arrayPath) { return [''].concat(arrayPath).join('/'); } @@ -230,3 +202,29 @@ function jsonPatchPathConverter(arrayPath) { function differentTypes(a, b) { return Object.prototype.toString.call(a) != Object.prototype.toString.call(b); } + +function trimFromRight(obj1, obj2) { + var lengthDelta = obj1.length - obj2.length; + if (Array.isArray(obj1) && Array.isArray(obj2) && lengthDelta > 0) { + var leftMatches = 0; + var rightMatches = 0; + for (var i = 0; i < obj2.length; i++) { + if (String(obj1[i]) === String(obj2[i])) { + leftMatches++; + } else { + break; + } + } + for (var j = obj2.length; j > 0; j--) { + if (String(obj1[j + lengthDelta]) === String(obj2[j])) { + rightMatches++; + } else { + break; + } + } + + // bias to trim right becase it requires less index shifting + return leftMatches >= rightMatches; + } + return true; +} diff --git a/packages/collection-diff/index.mjs b/packages/collection-diff/index.mjs index f965f7646..4a847874c 100644 --- a/packages/collection-diff/index.mjs +++ b/packages/collection-diff/index.mjs @@ -74,62 +74,51 @@ function diff(obj1, obj2, pathConverter) { return arr; }); - // we will gather all permutations and return the one with the fewest diffs - var permutations = [{remove: [], replace: [], add: []}]; - - function getDiff({obj1, obj2, basePath, basePathForRemoves, permutation}) { + function getDiff({obj1, obj2, basePath, basePathForRemoves, diffs}) { var obj1Keys = Object.keys(obj1); var obj1KeysLength = obj1Keys.length; var obj2Keys = Object.keys(obj2); var obj2KeysLength = obj2Keys.length; var path; - var newPermutation; - var lengthDelta = obj1.length - obj2.length; - // if both objects are arrays and obj1 length > obj2 length - // we create an additional permutation that trims obj1 from left - if (Array.isArray(obj1) && Array.isArray(obj2) && lengthDelta > 0) { - newPermutation = clonePermutation(permutation); - permutations.push(newPermutation); - } - // trim from right - for (var i = 0; i < obj1KeysLength; i++) { - var key = Array.isArray(obj1) ? Number(obj1Keys[i]) : obj1Keys[i]; - if (!(key in obj2)) { - path = basePathForRemoves.concat(key); - permutation.remove.push({ - op: 'remove', - path: pathConverter(path), - }); + if (trimFromRight(obj1, obj2)) { + for (var i = 0; i < obj1KeysLength; i++) { + var key = Array.isArray(obj1) ? Number(obj1Keys[i]) : obj1Keys[i]; + if (!(key in obj2)) { + path = basePathForRemoves.concat(key); + diffs.remove.push({ + op: 'remove', + path: pathConverter(path), + }); + } } - } - for (var i = 0; i < obj2KeysLength; i++) { - var key = Array.isArray(obj2) ? Number(obj2Keys[i]) : obj2Keys[i]; - pushReplaces({ - key, - obj1, - obj2, - path: basePath.concat(key), - pathForRemoves: basePath.concat(key), - permutation, - }); - } - - // if we created a new permutation above it means we should also try trimming from left - if (newPermutation) { + for (var i = 0; i < obj2KeysLength; i++) { + var key = Array.isArray(obj2) ? Number(obj2Keys[i]) : obj2Keys[i]; + pushReplaces({ + key, + obj1, + obj2, + path: basePath.concat(key), + pathForRemoves: basePath.concat(key), + diffs, + }); + } + } else { + // trim from left, objects are both arrays for (var i = 0; i < lengthDelta; i++) { path = basePathForRemoves.concat(i); - newPermutation.remove.push({ + diffs.remove.push({ op: 'remove', path: pathConverter(path), }); } // now make a copy of obj1 with excess elements left trimmed and see if there any replaces - var obj1Trimmed = obj1.slice(lengthDelta); for (var i = 0; i < obj2KeysLength; i++) { + var obj1Trimmed = obj1.slice(lengthDelta);; + for (var i = 0; i < obj2KeysLength; i++) { pushReplaces({ key: i, obj1: obj1Trimmed, @@ -138,38 +127,34 @@ function diff(obj1, obj2, pathConverter) { // since list of removes are reversed before presenting result, // we need to ignore existing parent removes when doing nested removes pathForRemoves: basePath.concat(i + lengthDelta), - permutation: newPermutation, + diffs, }); } } } + var diffs = {remove: [], replace: [], add: []}; getDiff({ obj1, obj2, basePath: [], basePathForRemoves: [], - permutation: permutations[0], + diffs, }); - // find the shortest permutation - var finalDiffs = permutations.sort( - (a, b) => diffStepCount(a) > diffStepCount(b) ? 1 : -1 - )[0]; - // reverse removes since we want to maintain indexes - return finalDiffs.remove + return diffs.remove .reverse() - .concat(finalDiffs.replace) - .concat(finalDiffs.add); + .concat(diffs.replace) + .concat(diffs.add); - function pushReplaces({key, obj1, obj2, path, pathForRemoves, permutation}) { + function pushReplaces({key, obj1, obj2, path, pathForRemoves, diffs}) { var obj1AtKey = obj1[key]; var obj2AtKey = obj2[key]; if(!(key in obj1) && (key in obj2)) { var obj2Value = obj2AtKey; - permutation.add.push({ + diffs.add.push({ op: 'add', path: pathConverter(path), value: obj2Value, @@ -178,19 +163,19 @@ function diff(obj1, obj2, pathConverter) { if(Object(obj1AtKey) !== obj1AtKey || Object(obj2AtKey) !== obj2AtKey || differentTypes(obj1AtKey, obj2AtKey) ) { - pushReplace(path, permutation, obj2AtKey); + pushReplace(path, diffs, obj2AtKey); } else { if(!Object.keys(obj1AtKey).length && !Object.keys(obj2AtKey).length && String(obj1AtKey) != String(obj2AtKey)) { - pushReplace(path, permutation, obj2AtKey); + pushReplace(path, diffs, obj2AtKey); } else { getDiff({ obj1: obj1[key], obj2: obj2[key], basePath: path, basePathForRemoves: pathForRemoves, - permutation}); + diffs}); } } } @@ -205,18 +190,6 @@ function diff(obj1, obj2, pathConverter) { } } -function clonePermutation(permutation) { - return { - remove: permutation.remove.slice(0), - replace: permutation.replace.slice(0), - add: permutation.add.slice(0), - }; -} - -function diffStepCount(permutation) { - return permutation.remove.length + permutation.replace.length + permutation.add.length; -} - function jsonPatchPathConverter(arrayPath) { return [''].concat(arrayPath).join('/'); } @@ -225,4 +198,30 @@ function differentTypes(a, b) { return Object.prototype.toString.call(a) != Object.prototype.toString.call(b); } +function trimFromRight(obj1, obj2) { + var lengthDelta = obj1.length - obj2.length; + if (Array.isArray(obj1) && Array.isArray(obj2) && lengthDelta > 0) { + var leftMatches = 0; + var rightMatches = 0; + for (var i = 0; i < obj2.length; i++) { + if (String(obj1[i]) === String(obj2[i])) { + leftMatches++; + } else { + break; + } + } + for (var j = obj2.length; j > 0; j--) { + if (String(obj1[j + lengthDelta]) === String(obj2[j])) { + rightMatches++; + } else { + break; + } + } + + // bias to trim right becase it requires less index shifting + return leftMatches >= rightMatches; + } + return true; +} + export {diff, jsonPatchPathConverter}; diff --git a/test/collection-diff/index.cjs b/test/collection-diff/index.cjs index 166879663..ddd5e0fa2 100644 --- a/test/collection-diff/index.cjs +++ b/test/collection-diff/index.cjs @@ -60,16 +60,16 @@ test('flat objects', function(t) { ); t.ok( compare(diff(obj3, obj4), [ - { op: 'remove', path: [ 'c' ] }, - { op: 'replace', path: [ 'a' ], value: 3 }, - { op: 'add', path: [ 'b' ], value: null } + {op: 'remove', path: [ 'c' ]}, + {op: 'replace', path: [ 'a' ], value: 3}, + {op: 'add', path: [ 'b' ], value: null}, ]) ); t.ok( compare(diff(obj4, obj3), [ - { op: 'remove', path: [ 'b' ] }, - { op: 'replace', path: [ 'a' ], value: 4 }, - { op: 'add', path: [ 'c' ], value: 5 } + {op: 'remove', path: [ 'b' ]}, + {op: 'replace', path: [ 'a' ], value: 4}, + {op: 'add', path: [ 'c' ], value: 5}, ]) ); }); @@ -539,7 +539,7 @@ test('objects whose properties are objects but with no properties of their own', }); test('path optimization for array', function(t) { - t.plan(10); + t.plan(11); var obj21 = [1, 2, 3, 4]; var obj22 = [2, 3, 4]; @@ -619,8 +619,10 @@ test('path optimization for array', function(t) { t.ok( compare(diff(obj29, obj30), [ - {'op': 'remove', 'path': [0, 0]}, - {'op': 'remove', 'path': [3]}, + {op: 'remove', path: [ 0 ]}, + {op: 'replace', path: [ 0 ], value: [ 'b', 'c' ]}, + {op: 'replace', path: [ 1 ], value: 3}, + {op: 'replace', path: [ 2 ], value: 4}, ]) ); @@ -632,6 +634,21 @@ test('path optimization for array', function(t) { {'op': 'add', 'path': [3], 'value': 5}, ]) ); + + var obj31 = [[0, 1, 2, 4, 5, 6], 1, {a: 4, b: 3} ]; + var obj32 = [[1, 2, 4, 5], 1, {a: 4, b: 3}, 3]; + + t.ok( + compare(diff(obj31, obj32), [ + {op: 'remove', path: [ 0, 1 ]}, + {op: 'remove', path: [ 0, 0 ]}, + {op: 'replace', path: [ 0, 0 ], value: 1}, + {op: 'replace', path: [ 0, 1 ], value: 2}, + {op: 'replace', path: [ 0, 2 ], value: 4}, + {op: 'replace', path: [ 0, 3 ], value: 5}, + {op: 'add', path: [ 3 ], value: 3}, + ]) + ); }); test('flat objects using jsPatchStandard', function(t) { diff --git a/test/object-extend/index.cjs b/test/object-extend/index.cjs index e30d8fbeb..0cfc13419 100644 --- a/test/object-extend/index.cjs +++ b/test/object-extend/index.cjs @@ -316,7 +316,6 @@ test('deep extend cannot extend native prototypes', function(t) { var attackObj2 = JSON.parse('{"__proto__": {"isAdmin": true}}'); var obj2 = extend(true, {}, attackObj2); - console.log(JSON.stringify(obj2, null, 2)); t.equal({}.isAdmin, undefined); t.equal({}.__proto__.isAdmin, undefined); t.ok(typeof {}.constructor === 'function');