Add ignoreMissingOps option when getting ops
#124
Conversation
Default behavior should be unchanged, returning error for missing ops.
When passing `{ignoreMissingOps: true}` via `options`, `getOps` and
`getOpsToSnapshot` will not call `checkOpsFrom` to avoid erroring when
using an incomplete set of ops
index.js
Outdated
| if (err) return callback(err); | ||
| var filtered = getLinkedOps(ops, null, snapshot._opLink); | ||
| var err = checkOpsFrom(collectionName, id, filtered, from); | ||
| var err = (options || {}).ignoreMissingOps ? null : checkOpsFrom(collectionName, id, filtered, from); |
There was a problem hiding this comment.
Might be a bit more readable and future-proof if we just make sure options is initialised in its own line:
| var err = (options || {}).ignoreMissingOps ? null : checkOpsFrom(collectionName, id, filtered, from); | |
| options = options || {}; | |
| var err = options.ignoreMissingOps ? null : checkOpsFrom(collectionName, id, filtered, from); |
There was a problem hiding this comment.
I also personally find use of the ternary a bit weird here, but this is more of a nitpick, so 🤷🏼♂️ :
| var err = (options || {}).ignoreMissingOps ? null : checkOpsFrom(collectionName, id, filtered, from); | |
| var err = null; | |
| if (!options.ignoreMissingOps) { | |
| err = checkOpsFrom(collectionName, id, filtered, from); | |
| } |
test/test_get_ops.js
Outdated
| }); | ||
| }); | ||
|
|
||
| it('ignoreMissingOps option returns ops up to the first missing op', function(done) { |
There was a problem hiding this comment.
This is worth talking about. I haven't tried this locally, but I'm pretty sure this behaviour only happens because you've set getOpsWithoutStrictLinking: true, which tries to iterate forward through ops.
However, the default behaviour (getOpsWithoutStrictLinking: false) is to start from the snapshot, and then iterate backwards through ops.
This means we get inconsistent behaviour, depending on whether getOpsWithoutStrictLinking is enabled or not. If I have an op missing in the middle of my range, then:
getOpsWithoutStrictLinking: falsewill return the end of the rangegetOpsWithoutStrictLinking: truewill return the *start of the range
One other point on getOpsWithoutStrictLinking (I think as we discussed very briefly yesterday), using getOpsWithoutStrictLinking on a collection that has been TTLed has slightly questionable validity. It's built around the assumption that if you can find an op with a unique version, then that op can be considered canonical. However, in the case of TTL, your TTL mechanism may or may not have deleted other ops with the same version, leaving a non-canonical op behind. The odds of this happening are probably pretty low, but it's still possible, and something to consider in a collection where ops are known to have been deleted.
I wonder if you set ignoreMissingOps: true if we should just ignore getOpsWithoutStrictLinking? Then we'd have consistent behaviour, at least. Also, as discussed above, this is the surest way to make sure you're returning canonical ops, since the getOpsWithoutStrictLinking assumptions may be broken.
There was a problem hiding this comment.
Might be the phrasing of the test description it issue, but it does appear to work backwards until the first missing op in both cases. So when given a set of ops [0,2,3] and fetching 0..4 it gets [2,3] stopping at 1 and doesn't return 0 in either case.
There was a problem hiding this comment.
Oh yes! I should probably read the test case code better 😅
Sorry I got confused, because I remembered getOpsWithStrictLinking iterating forwards, which it does, but it does it from the end of the requested op range, and then iterates backwards again, which is consistent with the default behaviour.
At any rate, it may be worth adding a test case with getOpsWithoutStrictLinking disabled, just to make sure we don't break this consistency in future.
alecgibson
changed the title
ignoreMissingOps option when getting ops
test/test_get_ops.js
Outdated
| }); | ||
| }; | ||
|
|
||
| require('sharedb/test/db')({create: create, getQuery: getQuery}); |
There was a problem hiding this comment.
I'm unsure if this is what's causing the CI test failure, but I did notice the number of test cases shot up a lot more than expected. I believe this is the cause, since it's re-including test cases that are already included by test_mongo.js.
Try removing this and see if it helps?
There was a problem hiding this comment.
took tests down to 702 cases (from 947)
There was a problem hiding this comment.
Push it up and let's see how the tests are on the CI runner, then!
test/test_get_ops.js
Outdated
| }; | ||
|
|
||
| // loop thru strict linking options | ||
| for (strictLinkingOption in [true, false]) { |
There was a problem hiding this comment.
Because this is ES5, strictLinkingOption is function (module) scoped and always would be false by the time the actual test cases execute.
Either an IIFE or a Array#forEach would solve this:
[true, false].forEach(function(strictLinkingOption) {
// ...
});
4 participants
Default behavior should be unchanged, returning error for missing ops. When passing
{ignoreMissingOps: true}viaoptions,getOpsandgetOpsToSnapshotwill not callcheckOpsFromto avoid erroring when using an incomplete set of ops. This allows scenarios for debugging tools to be able to fetch ops when an incomplete set of ops are available