Skip to content

Commit

Permalink
Don't generate patch for a no-change set operation (#45)
Browse files Browse the repository at this point in the history
Don't generate patch for a no-change set operation

Co-authored-by: Marcin Warpechowski <warpech@gmail.com>
warpech authored Aug 15, 2019
2 parents e0592b6 + e81f6a4 commit 8076498
Showing 2 changed files with 221 additions and 28 deletions.
79 changes: 69 additions & 10 deletions src/jsonpatcherproxy.js
Original file line number Diff line number Diff line change
@@ -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.
170 changes: 152 additions & 18 deletions test/spec/proxySpec.js
Original file line number Diff line number Diff line change
@@ -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() {

0 comments on commit 8076498

Please sign in to comment.