Support query subscriptions on projections with a filter not in the projection fields

[ericyhwang]

Prior to this PR, if you issue a query subscription on a projection, with a filter field that's not in the projection fields, then an op to change that filter field will not trigger the query to update.

This is because the op projection (_sanitizeOp) was happening prior to the skipPoll checks, which are what determine if an op could affect a query. A projected op would appear like it doesn't affect the query.

The fix is to defer applying projections to query ops until after the skipPoll checks.

I couldn't write a unit test in this repo, since the MemoryDB's query implementation always returns all docs in the collection, regardless of query filters. See below for the test I'll be adding to sharedb-mongo to cover this case.

---

Test I'll be adding to sharedb-mongo, which fails prior to this change and succeeds after this change:

  describe('projection query subscribe', function() {
    var ShareDB = require('sharedb');

    var backend;
    beforeEach(function() {
      backend = new ShareDB({db: this.db});
    });

    it('query subscribe on projection will update based on fields not in projection', function(done) {
      backend.addProjection('notes_text', 'notes', {text: true});
      var connection1 = backend.connect();
      var connection2 = backend.connect();
      // Create doc and a query that doesn't match the doc.
      var doc1 = connection1.get('notes', 'doc1');
      doc1.create({color: 'blue', text: 'Hello', location: 'Work'}, null, null, function(err) {
        if (err) return done(err);
        var query = connection2.createSubscribeQuery('notes_text', {color: 'green'}, null, function(err, results) {
          if (err) return done(err);
          expect(results).to.have.length(0);

          // Submit an op on a field not in the projection, where the op should cause the doc to be
          // included in the query results.
          doc1.submitOp({p: ['color'], od: 'blue', oi: 'green'}, null, function(err) {
            if (err) return done(err);
            setTimeout(function() {
              expect(query.results).to.have.length(1);
              expect(query.results[0].id).to.equal('doc1');
              expect(query.results[0].data).to.eql({text: 'Hello'});
              done();
            }, 10);
          });
        });
      });
    });
  });
});

[coveralls]

Coverage decreased (-0.1%) to 95.857% when pulling 19ba7bc on fix-projection-queries into f01cf0e on master.

[aconrad]

Thanks for your work on this @ericyhwang!

I wonder if #199 is affected by this change and whether your PR would satisfy their concern.

[nateps]

I made some additional significant changes:

• Added the test that Eric wrote. By including it here, it will run against memory-db as well as sharedb-mongo
• Removed the shallow op cloning from pubsub and put it in agent, which now clones immediately before calling sanitizeOp, so it is more clear that it is passing in just a shallow instead of a deep clone
• Kept the idea of doing a nextTick before calling into projections or op middleware, but doing it in agent instead of query emitter. The same op objects are emitted from pubsub directly to agent for document subscriptions, so if we're going to do that, we need to make sure that all of sharedb's internal pubsub to query emitter is synchronous and then we have an async delay before any user code runs. Depending on the order that listeners are subscribed, doc streams could be getting ops pushed to them before query emitter streams.
• Removed everything related to projections from op-stream. Projections are now only done in backend.js for ops that are fetched from the db, and in agent.js for ops that come from pubsub or query streams
• Now returning ops in addition to streams from backend.subscribe and backend.subscribeBulk. Previously, these ops were pushed into the stream. By returning them separately, we can sanitize them first. We don't have to copy these ops, since they come from the db. In addition, this ends up changing the order to send the fetched ops first before any ops that may have been in the stream from pubsub. This should be better, because the pubsub ops are likely to have missed ops, and we want to send the oldest ops first so that the client is able to apply them. It can't apply the ops from pubsub if there was a gap.

[ericyhwang]

Your new changes on top LGTM!

I can't approve this since I created the PR, so you'll have to do so.