Skip to content

Commit

Permalink
Throw when array and object deletion values do not match current version
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Alec Gibson authored and alecgibson committed Jul 14, 2021
1 parent 73db17e commit 61eb934
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 9 deletions.
14 changes: 10 additions & 4 deletions lib/json0.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
var deepEqual = require('fast-deep-equal');

/*
This is the implementation of the JSON OT type.
Expand Down Expand Up @@ -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;
}

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

Expand All @@ -226,15 +230,17 @@ 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;
}

// Object delete
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];
}

Expand Down
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
21 changes: 17 additions & 4 deletions test/json0.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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'}]
Expand Down Expand Up @@ -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'}]

Expand Down Expand Up @@ -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'}]

Expand Down

0 comments on commit 61eb934

Please sign in to comment.