Skip to content

Commit

Permalink
🥅 Stricter types in apply()
Browse files Browse the repository at this point in the history
Fixes: ottypes#37

# Background

There are some corner cases that arise in the `json0` library because -
given an object `obj`, and an array `arr` -these statements are both
`true` in JavaScript:

```js
obj['123'] === obj[123]
arr['123'] === arr[123]
```

The fact that these statements are true can lead to some unexpected
silent `transform()` failures:

```js
const op1 = [{p: ['a', '1', 0], si: 'hi'}]
const op2 = [{p: ['a', 1], lm: 0}]

json0.transform(op1, op2, 'left')
```

Actual result: `[{p: ["a", 2, 0], si: "hi"}]`
Expected result: `[{p: ["a", 0, 0], si: "hi"}]`

# Solution

In order to prevent this, it's been decided that arrays should *always*
be indexed by `number`, and objects should *always* be indexed by
`string`.

This change enforces stricter type checks when calling `apply()`, and
now throws in the following cases:

 - When a `number` is used to key an object property:
   `type.apply({'1': 'a'}, [{p:[1], od: 'a'}])`
 - When a `string` is used to key an array property:
   `type.apply(['a'], [{p:['0'], ld: 'a'}])`
 - When adding a `string` to a `number`:
   `type.apply(1, [{p:[], na: 'a}])`
 - When adding a `number` to a `string`:
   `type.apply('a', [{p:[], na: 1}])`
 - When applying a string operation to a non-string:
   `type.apply(1, [{p: [0], si: 'a'}])`
  • Loading branch information
alecgibson committed Jul 7, 2021
1 parent 95dce1a commit 504da40
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 0 deletions.
22 changes: 22 additions & 0 deletions lib/json0.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,16 @@ var clone = function(o) {
return JSON.parse(JSON.stringify(o));
};

var validateListIndex = function(key) {
if (typeof key !== 'number')
throw new Error('List index must be a number');
};

var validateObjectKey = function (key) {
if (typeof key !== 'string')
throw new Error('Object key must be a number');
};

/**
* JSON OT Type
* @type {*}
Expand Down Expand Up @@ -164,6 +174,12 @@ json.apply = function(snapshot, op) {
elem = elem[key];
key = p;

if (isArray(elem) && typeof key !== 'number')
throw new Error('List index must be a number');

if (isObject(elem) && typeof key !== 'string')
throw new Error('Object key must be a string');

if (parent == null)
throw new Error('Path invalid');
}
Expand All @@ -177,6 +193,9 @@ json.apply = function(snapshot, op) {
if (typeof elem[key] != 'number')
throw new Error('Referenced element not a number');

if (typeof c.na !== 'number')
throw new Error('Number addition is not a number');

elem[key] += c.na;
}

Expand Down Expand Up @@ -204,6 +223,9 @@ json.apply = function(snapshot, op) {

// List move
else if (c.lm !== void 0) {
if (typeof c.lm !== 'number')
throw new Error('List move target index must be a number');

json.checkList(elem);
if (c.lm != key) {
var e = elem[key];
Expand Down
4 changes: 4 additions & 0 deletions lib/text0.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ var checkValidOp = function(op) {
text.apply = function(snapshot, op) {
var deleted;

var type = typeof snapshot;
if (type !== 'string')
throw new Error('text0 operations cannot be applied to type: ' + type);

checkValidOp(op);
for (var i = 0; i < op.length; i++) {
var component = op[i];
Expand Down
38 changes: 38 additions & 0 deletions test/json0.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ genTests = (type) ->
c_s = type.apply leftHas, right_
assert.deepEqual s_c, c_s

it 'throws when adding a string to a number', ->
assert.throws -> type.apply 1, [{p: [], na: 'a'}]

it 'throws when adding a number to a string', ->
assert.throws -> type.apply 'a', [{p: [], na: 1}]

# Strings should be handled internally by the text type. We'll just do some basic sanity checks here.
describe 'string', ->
Expand All @@ -76,6 +81,12 @@ genTests = (type) ->
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'}]

it 'throws when the inserted content is not a string', ->
assert.throws -> type.apply 'a', [{p: [0], si: 1}]

describe '#transform()', ->
it 'splits deletes', ->
assert.deepEqual type.transform([{p:[0], sd:'ab'}], [{p:[1], si:'x'}], 'left'), [{p:[0], sd:'a'}, {p:[1], sd:'b'}]
Expand Down Expand Up @@ -135,6 +146,24 @@ genTests = (type) ->
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'}]

it 'throws when keying a list insertion with a string', ->
assert.throws -> type.apply ['a', 'b', 'c'], [{p: ['0'], li: 'x'}]

it 'throws when keying a list deletion with a string', ->
assert.throws -> type.apply ['a', 'b', 'c'], [{p: ['0'], ld: 'a'}]

it 'throws when keying a list move with a string', ->
assert.throws -> type.apply ['a', 'b', 'c'], [{p: ['0'], lm: 0}]

it 'throws when specifying a string as a list move target', ->
assert.throws -> type.apply ['a', 'b', 'c'], [{p: [1], lm: '0'}]

it 'throws when an array index part-way through the path is a string', ->
assert.throws -> type.apply {arr:[{x:'a'}]}, [{p:['arr', '0', 'x'], od: 'a'}]

###
'null moves compose to nops', ->
assert.deepEqual [], type.compose [], [{p:[3],lm:3}]
Expand Down Expand Up @@ -402,6 +431,15 @@ genTests = (type) ->
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'}]

it 'throws when the deletion key is a number', ->
assert.throws -> type.apply {'1':'a'}, [{p:[1], od: 'a'}]

it 'throws when an object key part-way through the path is a number', ->
assert.throws -> type.apply {'1': {x: 'a'}}, [{p:[1, 'x'], od: 'a'}]

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

0 comments on commit 504da40

Please sign in to comment.