ottypes/json1 support

[Vinky]

Submitting operations using the json1 syntax (directly or through helpers such as insertOp) fails the opContainsAnyField check, which checks the existence of a p field (which contains the path in json0).

Is it intended for sharedb-mongo to have built-in checks on the json0 syntax?
The call fails, but as mentioned in the comments, it is caught in sharedb and I don't see any side-effect apart from log pollution (for my usecase anyway).

[alecgibson]

Unfortunately I can't say I'm particularly familiar with ShareDB's polling. It certainly does look like this code makes the assumption we're dealing only with json0, which isn't a good assumption (or maybe it was at the time — looks like this code is 8 years old).

It looks like this is just an optimisation path for json0, so should still work if we early return for other types.

Will try to discuss with @ericyhwang

[alecgibson]

Good catch by the way!

[ericyhwang]

Yeah, skipPoll is an optimization to avoid a DB round-trip from polling, if the code determines an op coming from pub/sub couldn't possibly affect the subscribed query.

Log pollution is annoying, so we can look into handling it gracefully instead of relying on a try/catch in sharedb core.

• Add a test for a json1 op
• Have the code return false if the op isn't json0

Shouldn't take too long too do, I'll take a look tomorrow.

It would be nice to also have skipPoll work for json1 ops, but thoroughly testing that would be a bigger undertaking since I don't normally work with any apps using json1 myself.

[Vinky]

Great to hear, thanks for the prompt response!

[ericyhwang]

Found a potential bug while looking into it.

sharedb's catch clause falls back to not polling, so because of the error, queries with json1 ops will never poll:
https://github.com/share/sharedb/blob/e29936ca2dffd187b3fcfa3ca1b7e83becb6a2dc/lib/query-emitter.js#L110-L120

Not polling means a subscribed query's results won't automatically pick up additions, removals, or reorders in the result list.

Fixing the error here in sharedb-mongo would mean such queries with json1 would start correctly updating, though it would mean more DB queries.

Separately, for sharedb, I think the default should be to poll. If the code can't definitely determine that it's ok to skip polling, then it should poll the query for updated results, just in case. @alecgibson - your thoughts?

[alecgibson]

I can't say I'm super familiar with the query/polling code and data flows. The sharedb-mongo fix sounds uncontroversial enough to me.

As far as defaulting to poll in sharedb error cases, I'm not sure: this would mean if a DB adapter has a bug (as in this case), then ShareDB would poll the database for every non-skipped op? In this case of this bug, where the error is thrown in the skipPoll() check itself, then this would mean every op on the whole collection channel. This feels like a bad failure mode in my opinion.