From 7c29a20347b6bc70458782c8e5f54e57ce9fb596 Mon Sep 17 00:00:00 2001 From: eriksunsol Date: Fri, 5 Jul 2019 17:04:02 +0200 Subject: [PATCH 1/9] Don't generate patch for a no-change set operation --- src/jsonpatcherproxy.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/jsonpatcherproxy.js b/src/jsonpatcherproxy.js index a3d9b5c..aaa23cc 100644 --- a/src/jsonpatcherproxy.js +++ b/src/jsonpatcherproxy.js @@ -136,7 +136,11 @@ const JSONPatcherProxy = (function() { } operation.op = 'add'; if (tree.hasOwnProperty(key)) { - if (typeof tree[key] !== 'undefined' || isTreeAnArray) { + if (typeof tree[key] !== 'undefined') { + if (tree[key] == newValue) + return true; // Value wasn't actually changed, pretend set was successful but don't generate a patch + operation.op = 'replace'; + } else if (isTreeAnArray) { operation.op = 'replace'; // setting `undefined` array elements is a `replace` op } } From 32ee4cfcc6bf29835e796d1381de59fca6ce88ef Mon Sep 17 00:00:00 2001 From: eriksunsol Date: Mon, 29 Jul 2019 15:07:17 +0200 Subject: [PATCH 2/9] Update jsonpatcherproxy.js Added braces for readability. --- src/jsonpatcherproxy.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/jsonpatcherproxy.js b/src/jsonpatcherproxy.js index aaa23cc..f787eff 100644 --- a/src/jsonpatcherproxy.js +++ b/src/jsonpatcherproxy.js @@ -137,8 +137,9 @@ const JSONPatcherProxy = (function() { operation.op = 'add'; if (tree.hasOwnProperty(key)) { if (typeof tree[key] !== 'undefined') { - if (tree[key] == newValue) + if (tree[key] == newValue) { return true; // Value wasn't actually changed, pretend set was successful but don't generate a patch + } operation.op = 'replace'; } else if (isTreeAnArray) { operation.op = 'replace'; // setting `undefined` array elements is a `replace` op From 236f2cfdc6d2f89e5bb7c8dd4f6d5943408c00e2 Mon Sep 17 00:00:00 2001 From: eriksunsol Date: Tue, 30 Jul 2019 00:35:24 +0200 Subject: [PATCH 3/9] Added tests for no-change operations. These operations should result in empty patches. Also updated some tests to also check proper handling of `null` values when replaced with or replacing `undefined` values. --- test/spec/proxySpec.js | 112 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/test/spec/proxySpec.js b/test/spec/proxySpec.js index e199d44..234f92f 100644 --- a/test/spec/proxySpec.js +++ b/test/spec/proxySpec.js @@ -378,6 +378,7 @@ describe('proxy', function() { observedObj.phoneNumbers.push({ number: '456' }); + observedObj.nothing = null; var patches = jsonPatcherProxy.generate(); var obj2 = generateDeepObjectFixture(); @@ -727,6 +728,33 @@ describe('proxy', function() { ]); expect(genereatedPatches).toReallyEqual(comparedPatches); }); + + it('`undefined` property is set to `null`', function() { + var objFactory = function() { + return { + foo: undefined + }; + }; + + var objChanger = function(obj) { + obj.foo = null; + }; + + var genereatedPatches = getPatchesUsingGenerate( + objFactory, + objChanger + ); + var comparedPatches = getPatchesUsingCompare(objFactory, objChanger); + + expect(genereatedPatches).toReallyEqual([ + { + op: 'add', + path: '/foo', + value: null + } + ]); + expect(genereatedPatches).toReallyEqual(comparedPatches); + }); }); describe('should generate remove, when', function() { @@ -755,6 +783,32 @@ describe('proxy', function() { ]); expect(genereatedPatches).toReallyEqual(comparedPatches); }); + + it('property with `null` value is set to `undefined`', function() { + var objFactory = function() { + return { + foo: null + }; + }; + + var objChanger = function(obj) { + obj.foo = undefined; + }; + + var genereatedPatches = getPatchesUsingGenerate( + objFactory, + objChanger + ); + var comparedPatches = getPatchesUsingCompare(objFactory, objChanger); + + expect(genereatedPatches).toReallyEqual([ + { + op: 'remove', + path: '/foo' + } + ]); + expect(genereatedPatches).toReallyEqual(comparedPatches); + }); }); describe('should generate replace, when', function() { @@ -814,6 +868,64 @@ describe('proxy', function() { }); }); + describe('no-change operations should generate empty patch', function () { + it('in arrays', function() { + var objFactory = function() { + return { + foo: [0, undefined, null, undefined, null, 2, 'bar'] + }; + }; + + var objChanger = function(obj) { + obj.foo[0] = 0; + obj.foo[1] = undefined; + obj.foo[2] = null; + obj.foo[3] = null; + obj.foo[4] = undefined; + obj.foo[5] = 2; + obj.foo[6] = 'bar'; + }; + + var genereatedPatches = getPatchesUsingGenerate( + objFactory, + objChanger + ); + var expectedPatches = []; + + expect(genereatedPatches).toReallyEqual(expectedPatches); + }); + + it('in objects', function() { + var objFactory = function() { + return { + foo:{ + a: 0, + b: undefined, + c: null, + f: 2, + g: 'bar'} + }; + }; + + var objChanger = function(obj) { + obj.foo.a = 0; + obj.foo.b = undefined; + obj.foo.c = null; + obj.foo.f = 2; + obj.foo.g = 'bar'; + obj.foo.h = undefined; + }; + + var genereatedPatches = getPatchesUsingGenerate( + objFactory, + objChanger + ); + var expectedPatches = []; + + expect(genereatedPatches).toReallyEqual(expectedPatches); + }); + }); + describe('apply', function() { // https://tools.ietf.org/html/rfc6902#appendix-A.16 it('should add an Array Value', function() { From 681d9e4d1ddb484dc7817844eed2ee0bd39d4f38 Mon Sep 17 00:00:00 2001 From: eriksunsol Date: Tue, 30 Jul 2019 00:38:50 +0200 Subject: [PATCH 4/9] Worked through edge cases when dealing with no-change operations. --- src/jsonpatcherproxy.js | 49 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/src/jsonpatcherproxy.js b/src/jsonpatcherproxy.js index f787eff..f72b3a6 100644 --- a/src/jsonpatcherproxy.js +++ b/src/jsonpatcherproxy.js @@ -113,6 +113,9 @@ const JSONPatcherProxy = (function() { // `undefined` is being set to an already undefined value, keep silent return Reflect.set(tree, key, newValue); } else { + if (!isSignificantChange(tree[key], newValue, isTreeAnArray)) { + return Reflect.set(tree, key, newValue);; // Value wasn't actually changed with respect to its JSON projection + } // when array element is set to `undefined`, should generate replace to `null` if (isTreeAnArray) { // undefined array elements are JSON.stringified to `null` @@ -136,12 +139,10 @@ const JSONPatcherProxy = (function() { } operation.op = 'add'; if (tree.hasOwnProperty(key)) { - if (typeof tree[key] !== 'undefined') { - if (tree[key] == newValue) { - return true; // Value wasn't actually changed, pretend set was successful but don't generate a patch + if (typeof tree[key] !== 'undefined' || isTreeAnArray) { + if (!isSignificantChange(tree[key], newValue, isTreeAnArray)) { + return Reflect.set(tree, key, newValue); // Value wasn't actually changed with respect to its JSON projection } - operation.op = 'replace'; - } else if (isTreeAnArray) { operation.op = 'replace'; // setting `undefined` array elements is a `replace` op } } @@ -151,6 +152,44 @@ const JSONPatcherProxy = (function() { instance._defaultCallback(operation); return reflectionResult; } + /** + * Test if replacing old value with new value is a significant change, i.e. whether or not + * it soiuld result in a patch being generated. + * @param {*} oldValue old value + * @param {*} newValue new value + * @param {boolean} isTreeAnArray value resides in an array + */ + function isSignificantChange(oldValue, newValue, isTreeAnArray) { + if (isTreeAnArray) { + return isSignificantChangeInArray(oldValue, newValue); + } else { + return isSignificantChangeInObject(oldValue, newValue); + } + } + /** + * Test if replacing old value with new value is a significant change in an object, i.e. + * whether or not it should result in a patch being generated. + * @param {*} oldValue old value + * @param {*} newValue new value + */ + function isSignificantChangeInObject(oldValue, newValue) { + return oldValue !== newValue; + } + /** + * Test if replacing old value with new value is a significant change in an array, i.e. + * whether or not it should result in a patch being generated. + * @param {*} oldValue old value + * @param {*} newValue new value + */ + function isSignificantChangeInArray(oldValue, newValue) { + if (typeof oldValue === 'undefined') { + oldValue = null; + } + if (typeof newValue === 'undefined') { + newValue = null; + } + return oldValue !== newValue; + } /** * A callback to be used as the proxy delete trap callback. * It updates parenthood map if needed, calls default callbacks with the changes occurred. From d1216b8486d382e32a16053ceb044940f49ec309 Mon Sep 17 00:00:00 2001 From: Marcin Warpechowski Date: Wed, 14 Aug 2019 17:53:31 +0200 Subject: [PATCH 5/9] fix typo --- test/spec/proxySpec.js | 56 +++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/test/spec/proxySpec.js b/test/spec/proxySpec.js index 3f17145..e4dc8b0 100644 --- a/test/spec/proxySpec.js +++ b/test/spec/proxySpec.js @@ -682,14 +682,14 @@ describe('proxy', function() { obj.baz = undefined; }; - const genereatedPatches = getPatchesUsingGenerate( + const generatedPatches = getPatchesUsingGenerate( objFactory, objChanger ); const comparedPatches = getPatchesUsingCompare(objFactory, objChanger); - expect(genereatedPatches).toReallyEqual([]); - expect(genereatedPatches).toReallyEqual(comparedPatches); + expect(generatedPatches).toReallyEqual([]); + expect(generatedPatches).toReallyEqual(comparedPatches); }); it('when an `undefined` property is deleted', function() { @@ -703,14 +703,14 @@ describe('proxy', function() { delete obj.foo; }; - const genereatedPatches = getPatchesUsingGenerate( + const generatedPatches = getPatchesUsingGenerate( objFactory, objChanger ); const comparedPatches = getPatchesUsingCompare(objFactory, objChanger); - expect(genereatedPatches).toReallyEqual([]); - expect(genereatedPatches).toReallyEqual(comparedPatches); + expect(generatedPatches).toReallyEqual([]); + expect(generatedPatches).toReallyEqual(comparedPatches); }); }); @@ -726,20 +726,20 @@ describe('proxy', function() { obj.foo = 'something'; }; - const genereatedPatches = getPatchesUsingGenerate( + const generatedPatches = getPatchesUsingGenerate( objFactory, objChanger ); const comparedPatches = getPatchesUsingCompare(objFactory, objChanger); - expect(genereatedPatches).toReallyEqual([ + expect(generatedPatches).toReallyEqual([ { op: 'add', path: '/foo', value: 'something' } ]); - expect(genereatedPatches).toReallyEqual(comparedPatches); + expect(generatedPatches).toReallyEqual(comparedPatches); }); it('`undefined` property is set to `null`', function() { @@ -753,20 +753,20 @@ describe('proxy', function() { obj.foo = null; }; - var genereatedPatches = getPatchesUsingGenerate( + var generatedPatches = getPatchesUsingGenerate( objFactory, objChanger ); var comparedPatches = getPatchesUsingCompare(objFactory, objChanger); - expect(genereatedPatches).toReallyEqual([ + expect(generatedPatches).toReallyEqual([ { op: 'add', path: '/foo', value: null } ]); - expect(genereatedPatches).toReallyEqual(comparedPatches); + expect(generatedPatches).toReallyEqual(comparedPatches); }); }); @@ -782,19 +782,19 @@ describe('proxy', function() { obj.foo = undefined; }; - const genereatedPatches = getPatchesUsingGenerate( + const generatedPatches = getPatchesUsingGenerate( objFactory, objChanger ); const comparedPatches = getPatchesUsingCompare(objFactory, objChanger); - expect(genereatedPatches).toReallyEqual([ + expect(generatedPatches).toReallyEqual([ { op: 'remove', path: '/foo' } ]); - expect(genereatedPatches).toReallyEqual(comparedPatches); + expect(generatedPatches).toReallyEqual(comparedPatches); }); it('property with `null` value is set to `undefined`', function() { @@ -808,19 +808,19 @@ describe('proxy', function() { obj.foo = undefined; }; - var genereatedPatches = getPatchesUsingGenerate( + var generatedPatches = getPatchesUsingGenerate( objFactory, objChanger ); var comparedPatches = getPatchesUsingCompare(objFactory, objChanger); - expect(genereatedPatches).toReallyEqual([ + expect(generatedPatches).toReallyEqual([ { op: 'remove', path: '/foo' } ]); - expect(genereatedPatches).toReallyEqual(comparedPatches); + expect(generatedPatches).toReallyEqual(comparedPatches); }); }); @@ -836,20 +836,20 @@ describe('proxy', function() { obj.foo[1] = undefined; }; - const genereatedPatches = getPatchesUsingGenerate( + const generatedPatches = getPatchesUsingGenerate( objFactory, objChanger ); const comparedPatches = getPatchesUsingCompare(objFactory, objChanger); - expect(genereatedPatches).toReallyEqual([ + expect(generatedPatches).toReallyEqual([ { op: 'replace', path: '/foo/1', value: null } ]); - expect(genereatedPatches).toReallyEqual(comparedPatches); + expect(generatedPatches).toReallyEqual(comparedPatches); }); it('`undefined` array element is set to something', function() { const objFactory = function() { @@ -862,20 +862,20 @@ describe('proxy', function() { obj.foo[1] = 1; }; - const genereatedPatches = getPatchesUsingGenerate( + const generatedPatches = getPatchesUsingGenerate( objFactory, objChanger ); const comparedPatches = getPatchesUsingCompare(objFactory, objChanger); - expect(genereatedPatches).toReallyEqual([ + expect(generatedPatches).toReallyEqual([ { op: 'replace', path: '/foo/1', value: 1 } ]); - expect(genereatedPatches).toReallyEqual(comparedPatches); + expect(generatedPatches).toReallyEqual(comparedPatches); }); }); }); @@ -899,13 +899,13 @@ describe('proxy', function() { obj.foo[6] = 'bar'; }; - var genereatedPatches = getPatchesUsingGenerate( + var generatedPatches = getPatchesUsingGenerate( objFactory, objChanger ); var expectedPatches = []; - expect(genereatedPatches).toReallyEqual(expectedPatches); + expect(generatedPatches).toReallyEqual(expectedPatches); }); it('in objects', function() { @@ -929,13 +929,13 @@ describe('proxy', function() { obj.foo.h = undefined; }; - var genereatedPatches = getPatchesUsingGenerate( + var generatedPatches = getPatchesUsingGenerate( objFactory, objChanger ); var expectedPatches = []; - expect(genereatedPatches).toReallyEqual(expectedPatches); + expect(generatedPatches).toReallyEqual(expectedPatches); }); }); From 0257a923ad761dc0ec9507c869039f57d7142170 Mon Sep 17 00:00:00 2001 From: Marcin Warpechowski Date: Wed, 14 Aug 2019 17:57:34 +0200 Subject: [PATCH 6/9] Added test for extending array with undefined or null elements. --- test/spec/proxySpec.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/spec/proxySpec.js b/test/spec/proxySpec.js index e4dc8b0..e29c291 100644 --- a/test/spec/proxySpec.js +++ b/test/spec/proxySpec.js @@ -768,6 +768,27 @@ describe('proxy', function() { ]); expect(generatedPatches).toReallyEqual(comparedPatches); }); + + it('Array extended with `null` or `undefined` element', function () { + const objFactory = function() { + return { + arr: Array(5) + }; + }; + + const objChanger = function(obj) { + obj.arr[7]; + obj.arr[7] = null; + obj.arr[8] = undefined; + }; + + const genereatedPatches = getPatchesUsingGenerate( + objFactory, + objChanger + ); + const comparedPatches = getPatchesUsingCompare(objFactory, objChanger); + expect(genereatedPatches).toReallyEqual(comparedPatches); + }); }); describe('should generate remove, when', function() { From 86753dcd45cb455eb9e3c9f204e7da98fa81464f Mon Sep 17 00:00:00 2001 From: Marcin Warpechowski Date: Tue, 6 Aug 2019 16:43:01 +0200 Subject: [PATCH 7/9] DRY: single Reflect.set before conditions --- src/jsonpatcherproxy.js | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/jsonpatcherproxy.js b/src/jsonpatcherproxy.js index b1647e0..b17d137 100644 --- a/src/jsonpatcherproxy.js +++ b/src/jsonpatcherproxy.js @@ -113,28 +113,31 @@ const JSONPatcherProxy = (function() { } } // let's start with this operation, and may or may not update it later + const valueBeforeReflection = tree[key]; + const wasKeyInTreeBeforeReflection = tree.hasOwnProperty(key); + const reflectionResult = Reflect.set(tree, key, newValue); const operation = { op: 'remove', path: pathToKey }; if (typeof newValue == 'undefined') { // applying De Morgan's laws would be a tad faster, but less readable - if (!isTreeAnArray && !tree.hasOwnProperty(key)) { + if (!isTreeAnArray && !wasKeyInTreeBeforeReflection) { // `undefined` is being set to an already undefined value, keep silent - return Reflect.set(tree, key, newValue); + return reflectionResult; } else { - if (!isSignificantChange(tree[key], newValue, isTreeAnArray)) { - return Reflect.set(tree, key, newValue);; // Value wasn't actually changed with respect to its JSON projection + if (!isSignificantChange(valueBeforeReflection, newValue, isTreeAnArray)) { + return reflectionResult; // Value wasn't actually changed with respect to its JSON projection } // when array element is set to `undefined`, should generate replace to `null` if (isTreeAnArray) { // undefined array elements are JSON.stringified to `null` (operation.op = 'replace'), (operation.value = null); } - const oldSubtreeMetadata = instance._treeMetadataMap.get(tree[key]); + const oldSubtreeMetadata = instance._treeMetadataMap.get(valueBeforeReflection); if (oldSubtreeMetadata) { //TODO there is no test for this! - instance._parenthoodMap.delete(tree[key]); + instance._parenthoodMap.delete(valueBeforeReflection); instance._disableTrapsForTreeMetadata(oldSubtreeMetadata); instance._treeMetadataMap.delete(oldSubtreeMetadata); } @@ -145,20 +148,19 @@ const JSONPatcherProxy = (function() { if(key != 'length' && !warnedAboutNonIntegrerArrayProp) { console.warn(`JSONPatcherProxy noticed a non-integer property ('${key}') was set for an array. This interception will not emit a patch`); } - return Reflect.set(tree, key, newValue); + return reflectionResult; } operation.op = 'add'; - if (tree.hasOwnProperty(key)) { - if (typeof tree[key] !== 'undefined' || isTreeAnArray) { - if (!isSignificantChange(tree[key], newValue, isTreeAnArray)) { - return Reflect.set(tree, key, newValue); // Value wasn't actually changed with respect to its JSON projection + if (wasKeyInTreeBeforeReflection) { + if (typeof valueBeforeReflection !== 'undefined' || isTreeAnArray) { + if (!isSignificantChange(valueBeforeReflection, newValue, isTreeAnArray)) { + return reflectionResult; // Value wasn't actually changed with respect to its JSON projection } operation.op = 'replace'; // setting `undefined` array elements is a `replace` op } } operation.value = newValue; } - const reflectionResult = Reflect.set(tree, key, newValue); instance._defaultCallback(operation); return reflectionResult; } From 21af2ec724b4f1d51d878f80188056717b8d18fb Mon Sep 17 00:00:00 2001 From: Marcin Warpechowski Date: Wed, 14 Aug 2019 14:37:52 +0200 Subject: [PATCH 8/9] bugfix: generate "add" operation for every undefined/null array element until array.length is reached --- src/jsonpatcherproxy.js | 17 ++++++++++++++--- test/spec/proxySpec.js | 5 +++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/jsonpatcherproxy.js b/src/jsonpatcherproxy.js index b17d137..be362f5 100644 --- a/src/jsonpatcherproxy.js +++ b/src/jsonpatcherproxy.js @@ -115,6 +115,12 @@ const JSONPatcherProxy = (function() { // let's start with this operation, and may or may not update it later const valueBeforeReflection = tree[key]; const wasKeyInTreeBeforeReflection = tree.hasOwnProperty(key); + if (isTreeAnArray && !isNonSerializableArrayProperty) { + const index = parseInt(key, 10); + if (index > tree.length) { + trapForSet(instance, tree, (index - 1) + '', undefined); + } + } const reflectionResult = Reflect.set(tree, key, newValue); const operation = { op: 'remove', @@ -126,13 +132,18 @@ const JSONPatcherProxy = (function() { // `undefined` is being set to an already undefined value, keep silent return reflectionResult; } else { - if (!isSignificantChange(valueBeforeReflection, newValue, isTreeAnArray)) { + if (wasKeyInTreeBeforeReflection && !isSignificantChange(valueBeforeReflection, newValue, isTreeAnArray)) { return reflectionResult; // Value wasn't actually changed with respect to its JSON projection } // when array element is set to `undefined`, should generate replace to `null` if (isTreeAnArray) { - // undefined array elements are JSON.stringified to `null` - (operation.op = 'replace'), (operation.value = null); + operation.value = null; + if (wasKeyInTreeBeforeReflection) { + operation.op = 'replace'; + } + else { + operation.op = 'add'; + } } const oldSubtreeMetadata = instance._treeMetadataMap.get(valueBeforeReflection); if (oldSubtreeMetadata) { diff --git a/test/spec/proxySpec.js b/test/spec/proxySpec.js index e29c291..3c7949f 100644 --- a/test/spec/proxySpec.js +++ b/test/spec/proxySpec.js @@ -782,12 +782,13 @@ describe('proxy', function() { obj.arr[8] = undefined; }; - const genereatedPatches = getPatchesUsingGenerate( + const generatedPatches = getPatchesUsingGenerate( objFactory, objChanger ); const comparedPatches = getPatchesUsingCompare(objFactory, objChanger); - expect(genereatedPatches).toReallyEqual(comparedPatches); + expect(generatedPatches).toReallyEqual(comparedPatches); + expect(generatedPatches).toReallyEqual([{ op: 'add', path: '/arr/5', value: null }, { op: 'add', path: '/arr/6', value: null }, { op: 'add', path: '/arr/7', value: null }, { op: 'add', path: '/arr/8', value: null }]); }); }); From e81f6a432774becfb3679ae99834b5d89ffd561e Mon Sep 17 00:00:00 2001 From: Marcin Warpechowski Date: Thu, 15 Aug 2019 11:03:46 +0200 Subject: [PATCH 9/9] explain the explicit call to trapForSet --- src/jsonpatcherproxy.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/jsonpatcherproxy.js b/src/jsonpatcherproxy.js index be362f5..1dca345 100644 --- a/src/jsonpatcherproxy.js +++ b/src/jsonpatcherproxy.js @@ -118,6 +118,8 @@ const JSONPatcherProxy = (function() { if (isTreeAnArray && !isNonSerializableArrayProperty) { const index = parseInt(key, 10); if (index > tree.length) { + // force call trapForSet for implicit undefined elements of the array added by the JS engine + // because JSON-Patch spec prohibits adding an index that is higher than array.length trapForSet(instance, tree, (index - 1) + '', undefined); } }