diff --git a/src/jsonpatcherproxy.js b/src/jsonpatcherproxy.js index 8a0d8e5..1dca345 100644 --- a/src/jsonpatcherproxy.js +++ b/src/jsonpatcherproxy.js @@ -113,25 +113,44 @@ 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) { + // 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); + } + } + 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 (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(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); } @@ -142,20 +161,60 @@ 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 (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; } + /** + * 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. diff --git a/test/spec/proxySpec.js b/test/spec/proxySpec.js index 5de1ae0..3c7949f 100644 --- a/test/spec/proxySpec.js +++ b/test/spec/proxySpec.js @@ -376,6 +376,7 @@ describe('proxy', function() { observedObj.phoneNumbers.push({ number: '456' }); + observedObj.nothing = null; const patches = jsonPatcherProxy.generate(); const obj2 = generateDeepObjectFixture(); @@ -681,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() { @@ -702,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); }); }); @@ -725,20 +726,69 @@ 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() { + var objFactory = function() { + return { + foo: undefined + }; + }; + + var objChanger = function(obj) { + obj.foo = null; + }; + + var generatedPatches = getPatchesUsingGenerate( + objFactory, + objChanger + ); + var comparedPatches = getPatchesUsingCompare(objFactory, objChanger); + + expect(generatedPatches).toReallyEqual([ + { + op: 'add', + path: '/foo', + value: null + } + ]); + 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 generatedPatches = getPatchesUsingGenerate( + objFactory, + objChanger + ); + const comparedPatches = getPatchesUsingCompare(objFactory, objChanger); + 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 }]); }); }); @@ -754,19 +804,45 @@ 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(generatedPatches).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 generatedPatches = getPatchesUsingGenerate( + objFactory, + objChanger + ); + var comparedPatches = getPatchesUsingCompare(objFactory, objChanger); + + expect(generatedPatches).toReallyEqual([ { op: 'remove', path: '/foo' } ]); - expect(genereatedPatches).toReallyEqual(comparedPatches); + expect(generatedPatches).toReallyEqual(comparedPatches); }); }); @@ -782,20 +858,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() { @@ -808,25 +884,83 @@ 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); }); }); }); }); + 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 generatedPatches = getPatchesUsingGenerate( + objFactory, + objChanger + ); + var expectedPatches = []; + + expect(generatedPatches).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 generatedPatches = getPatchesUsingGenerate( + objFactory, + objChanger + ); + var expectedPatches = []; + + expect(generatedPatches).toReallyEqual(expectedPatches); + }); + }); + describe('apply', function() { // https://tools.ietf.org/html/rfc6902#appendix-A.16 it('should add an Array Value', function() {