apply() accepts non-array ops that compose() and invert() reject #52

apply() walks an op with op.length and numeric indexing, so it accepts anything array-like, and treats anything without a usable length as a no-op. compose() and invert() assume a real array and throw on one. The upshot is that a single type has several different answers to "is this an op?".

On 1.1.0:

apply({a: 1}, {p: ['x'], oi: 1})      -> {"a":1}
apply({a: 1}, {0: {…}, length: 1})    -> {"a":1,"x":1}
apply({a: 1}, 5)                      -> {"a":1}

compose({p: ['x'], oi: 1}, fixup)     -> throws: Cannot read properties of undefined (reading 'p')
compose({0: {…}, length: 1}, fixup)   -> throws: dest.push is not a function
compose([null], fixup)                -> throws: Cannot read properties of null (reading 'p')
invert({p: ['x'], oi: 1})             -> throws: op.slice is not a function

normalize([null])                     -> throws: Cannot read properties of null (reading 'p')
normalize({0: {…}, length: 1})        -> [{"0":{"p":["x"],"oi":1},"length":1,"p":[]}]

(where fixup is [{p: ['fixed'], oi: true}])

So:

• {0: {…}, length: 1} is a perfectly good op to apply() — it applies — and a crash in compose().
• {p: ['x'], oi: 1} and 5 are silent no-ops to apply(), which returns the snapshot untouched and no error, and crashes in compose() / invert().
• normalize() has a third set of rules again: it throws on [null], and quietly turns {0: {…}, length: 1} into a single mangled component with an empty path.

checkValidOp() doesn't close the gap, because it walks op.length too and only validates each op[i].p:

json.checkValidOp = function(op) {
  for (var i = 0; i < op.length; i++) {
    if (!isArray(op[i].p)) throw new Error('Missing path');
  }
};

Why it matters downstream

ShareDB accepts an op submitted by a client, and lets server middleware compose a "fixup" op onto it. An op that apply() treats as a no-op therefore commits successfully, and then throws when it later reaches compose() — from inside middleware, where the throw isn't caught. We've guarded it on our side in share/sharedb#725, but the underlying inconsistency seemed worth recording here.

How this sits with #40 / #42

I realise #40 already tightened apply(), and that the discussion on #42 settled on shipping that as a breaking v2 with the checks on by default — same type URI, with ShareDB shimming historic ops, which we did in share/sharedb#494.

Worth noting that a stricter apply() wouldn't cover the case above on its own, because of where compose() sits in the flow: ShareDB triggers its apply middleware, which is where a fixup op gets composed, before it calls the type's apply(). So a badly formed op reaches compose() first, and apply() never gets a look in. We've guarded it on our side for that reason.

So this is really just a note that whenever v2 does land, it would be good for apply(), compose(), invert(), normalize() and checkValidOp() to share one definition of an op — a real array of components — rather than each having its own.