Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't generate patch for a no-change set operation #45

Merged
merged 10 commits into from
Aug 15, 2019
Merged

Don't generate patch for a no-change set operation #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7c29a20
Don't generate patch for a no-change set operation
eriksunsol Jul 5, 2019
32ee4cf
Update jsonpatcherproxy.js
eriksunsol Jul 29, 2019
236f2cf
Added tests for no-change operations.
eriksunsol Jul 29, 2019
681d9e4
Worked through edge cases when dealing with no-change operations.
eriksunsol Jul 29, 2019
0920809
Merge branch 'master' into patch-1
eriksunsol Aug 8, 2019
d1216b8
fix typo
warpech Aug 14, 2019
0257a92
Added test for extending array with undefined or null elements.
warpech Aug 14, 2019
86753dc
DRY: single Reflect.set before conditions
warpech Aug 6, 2019
21af2ec
bugfix: generate "add" operation for every undefined/null array eleme…
warpech Aug 14, 2019
e81f6a4
explain the explicit call to trapForSet
warpech Aug 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 69 additions & 10 deletions src/jsonpatcherproxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
tomalec marked this conversation as resolved.
Show resolved Hide resolved
}
}
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);
}
Expand All @@ -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;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function (and tests) need to be extended cover scenario of adding a null beyond the end of the array:

arr = Array(5);
JSON.stringify(arr); // "[null,null,null,null,null]"
arr[7]; // undefined
arr[7] = null; // null - Even though `typeof oldValue === 'undefined'`, this is a significant change, as
JSON.stringify(arr); // [null,null,null,null,null,null,null,null]"

Copy link
Contributor Author

@eriksunsol eriksunsol Aug 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomalec Good catch! I noticed that these cases are were not dealt with properly and had intended to file a separate issue about it. The thing is that adding undefined elements is broken on master as well and therefore i figured it was a bit out of scope for this PR.

Check out failing test on 7cf32fd which is based on current master. Here the undefined element results in a replace patch when it should be an add. Based on this PR, the same test also fails, although differently: 5de895f. Here the undefined element is simply ignored. Note that both fail on elements which are added implicitly without ever being set such as arr[5] and arr[6] in the example which are not present in the patch at all.

The test is based on your example with an addition:

arr = Array(5);
JSON.stringify(arr); // "[null,null,null,null,null]"
arr[7]; // undefined
arr[7] = null;
arr[8] = undefined;
// null - Even though `typeof oldValue === 'undefined'`, these are significant changes, as
JSON.stringify(arr); // [null,null,null,null,null,null,null,null,null]"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #52.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We found another issue with this function. Is it called when you pop an array?
Then change from undefined(mapped to null) to not existing element.
I guess it should call deleteTrap but it's worth to add a test for such case as well.

/**
* A callback to be used as the proxy delete trap callback.
* It updates parenthood map if needed, calls default callbacks with the changes occurred.
Expand Down
170 changes: 152 additions & 18 deletions test/spec/proxySpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,7 @@ describe('proxy', function() {
observedObj.phoneNumbers.push({
number: '456'
});
observedObj.nothing = null;
const patches = jsonPatcherProxy.generate();

const obj2 = generateDeepObjectFixture();
Expand Down Expand Up @@ -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() {
Expand All @@ -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);
});
});

Expand All @@ -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 }]);
});
});

Expand All @@ -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);
});
});

Expand All @@ -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() {
Expand All @@ -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() {
Expand Down