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 committed Jul 16, 2018
1 parent 9df44f0 commit 3e9fef2
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 @@ -181,7 +183,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 @@ -194,7 +197,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 @@ -214,15 +218,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": {
"ot-fuzzer": "^1.0.0",
"mocha": "^1.20.1",
Expand Down
21 changes: 17 additions & 4 deletions test/json0.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,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'}]

describe '#transform()', ->
it 'splits deletes', ->
Expand Down Expand Up @@ -127,6 +131,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'}]

###
'null moves compose to nops', ->
assert.deepEqual [], type.compose [], [{p:[3],lm:3}]
Expand Down Expand Up @@ -389,6 +397,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'}]

describe 'randomizer', ->
@timeout 20000
@slow 6000
Expand Down

0 comments on commit 3e9fef2

Please sign in to comment.