From 61eb934383264ba88d5e1bd75a8b6b60c9ce2dbd Mon Sep 17 00:00:00 2001 From: Alec Gibson Date: Mon, 16 Jul 2018 16:02:32 +0100 Subject: [PATCH] Throw when array and object deletion values do not match current version This change adds validation to array and object deletion. If the values provided in either `ld` or `od` do not match the current value, then `apply` will `throw`. It will also throw if `oi` overwrites an existing value without providing `od`. The primary motivation of this change is to ensure that all submitted ops remain reversible. At the moment, the following series of ops is possible: - start with `{ foo: 'bar' }` - `apply` this op: `{ p: ['foo'], oi: 'baz' }` - ...resulting in `{ foo: 'baz' }` - `invert` the previous op: `{ p: ['foo'], od: 'baz' }` - and `apply` the inverted op: `{}` When I apply, invert and apply, I should always wind up where I started, but in this case I clearly do not. By ensuring that the `od` matches the current value, we make sure that all ops remain reversible. Deep equality adds a dependency on [`fast-deep-equal`][1]. [1]: https://github.com/epoberezkin/fast-deep-equal --- lib/json0.js | 14 ++++++++++---- package.json | 4 +++- test/json0.coffee | 21 +++++++++++++++++---- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/lib/json0.js b/lib/json0.js index 9f538ee..8199416 100644 --- a/lib/json0.js +++ b/lib/json0.js @@ -1,3 +1,5 @@ +var deepEqual = require('fast-deep-equal'); + /* This is the implementation of the JSON OT type. @@ -190,7 +192,8 @@ json.apply = function(snapshot, op) { // List replace else if (c.li !== void 0 && c.ld !== void 0) { json.checkList(elem); - // Should check the list element matches c.ld + if (!deepEqual(elem[key], c.ld)) + throw new Error('List deletion value does not match current value') elem[key] = c.li; } @@ -203,7 +206,8 @@ json.apply = function(snapshot, op) { // List delete else if (c.ld !== void 0) { json.checkList(elem); - // Should check the list element matches c.ld here too. + if (!deepEqual(elem[key], c.ld)) + throw new Error('List deletion value does not match current value') elem.splice(key,1); } @@ -226,7 +230,8 @@ json.apply = function(snapshot, op) { else if (c.oi !== void 0) { json.checkObj(elem); - // Should check that elem[key] == c.od + if (!deepEqual(elem[key], c.od)) + throw new Error('Object deletion value does not match current value') elem[key] = c.oi; } @@ -234,7 +239,8 @@ json.apply = function(snapshot, op) { else if (c.od !== void 0) { json.checkObj(elem); - // Should check that elem[key] == c.od + if (!deepEqual(elem[key], c.od)) + throw new Error('Object deletion value does not match current value') delete elem[key]; } diff --git a/package.json b/package.json index 57f0c06..cc59139 100644 --- a/package.json +++ b/package.json @@ -6,7 +6,9 @@ "directories": { "test": "test" }, - "dependencies": {}, + "dependencies": { + "fast-deep-equal": "^2.0.1" + }, "devDependencies": { "coffee-script": "^1.7.1", "mocha": "^9.0.2", diff --git a/test/json0.coffee b/test/json0.coffee index e2ee6df..4af233c 100644 --- a/test/json0.coffee +++ b/test/json0.coffee @@ -72,10 +72,14 @@ genTests = (type) -> # Strings should be handled internally by the text type. We'll just do some basic sanity checks here. describe 'string', -> - describe '#apply()', -> it 'works', -> - assert.deepEqual 'abc', type.apply 'a', [{p:[1], si:'bc'}] - assert.deepEqual 'bc', type.apply 'abc', [{p:[0], sd:'a'}] - assert.deepEqual {x:'abc'}, type.apply {x:'a'}, [{p:['x', 1], si:'bc'}] + describe '#apply()', -> + it 'works', -> + assert.deepEqual 'abc', type.apply 'a', [{p:[1], si:'bc'}] + assert.deepEqual 'bc', type.apply 'abc', [{p:[0], sd:'a'}] + assert.deepEqual {x:'abc'}, type.apply {x:'a'}, [{p:['x', 1], si:'bc'}] + + it 'throws when the deletion target does not match', -> + assert.throws -> type.apply 'abc', [{p:[0], sd:'x'}] it 'throws when the target is not a string', -> assert.throws -> type.apply [1], [{p: [0], si: 'a'}] @@ -138,6 +142,10 @@ genTests = (type) -> assert.deepEqual ['a', 'b', 'c'], type.apply ['b', 'a', 'c'], [{p:[1], lm:0}] assert.deepEqual ['a', 'b', 'c'], type.apply ['b', 'a', 'c'], [{p:[0], lm:1}] + it 'throws when the deletion target does not match', -> + assert.throws -> type.apply ['a', 'b', 'c'], [{p:[0], ld: 'x'}] + assert.throws -> type.apply ['a', 'b', 'c'], [{p:[0], li: 'd', ld: 'x'}] + it 'throws when keying a list replacement with a string', -> assert.throws -> type.apply ['a', 'b', 'c'], [{p: ['0'], li: 'x', ld: 'a'}] @@ -418,6 +426,11 @@ genTests = (type) -> assert.deepEqual [], type.transform [{p:['k'], od:'x'}], [{p:['k'], od:'x'}], 'left' assert.deepEqual [], type.transform [{p:['k'], od:'x'}], [{p:['k'], od:'x'}], 'right' + it 'throws when the deletion target does not match', -> + assert.throws -> type.apply {x:'a'}, [{p:['x'], od: 'b'}] + assert.throws -> type.apply {x:'a'}, [{p:['x'], oi: 'c', od: 'b'}] + assert.throws -> type.apply {x:'a'}, [{p:['x'], oi: 'b'}] + it 'throws when the insertion key is a number', -> assert.throws -> type.apply {'1':'a'}, [{p:[2], oi: 'a'}]