From 237f6a911b6244abda063167741f44b0f333dd36 Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Wed, 7 Jul 2021 11:23:40 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=A5=85=20Stricter=20types=20in=20`apply()?= =?UTF-8?q?`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: https://github.com/ottypes/json0/issues/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'}])` --- lib/json0.js | 22 ++++++++++++++++++++++ lib/text0.js | 4 ++++ test/json0.coffee | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+) diff --git a/lib/json0.js b/lib/json0.js index dc3a405..a93ffe4 100644 --- a/lib/json0.js +++ b/lib/json0.js @@ -43,6 +43,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 {*} @@ -162,6 +172,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'); } @@ -175,6 +191,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; } @@ -200,6 +219,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]; diff --git a/lib/text0.js b/lib/text0.js index e26c6a9..238d448 100644 --- a/lib/text0.js +++ b/lib/text0.js @@ -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]; diff --git a/test/json0.coffee b/test/json0.coffee index 531f76e..e2ee6df 100644 --- a/test/json0.coffee +++ b/test/json0.coffee @@ -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', -> @@ -72,6 +77,12 @@ genTests = (type) -> 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 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'}] @@ -127,6 +138,24 @@ 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 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}] @@ -389,6 +418,15 @@ 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 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