✨ Add multi channel query subscription. #587

dawidreedsy · 2023-01-30T11:32:22Z

At the moment we can only specify one channel the specific query, would be able to listen to, as per docs:

backend.use('query', (context, next) => {
  // Set our query to only listen for changes on our user-specific channel
  context.channel = userChannel(context)
  next()
})

However let's imagine the situation where the user wants to query all the posts, the where posted by him and all his friends. Now we would need new query every for friend separately to make sure the proper scalability is preserved and we do not receive all the changes to posts collection. This change allows to listen for multiple channels, so if we want to query all user friends posts. We can do it by this:

backend.use('query', (context, next) => {
  // Set our query to only listen for changes on our user-specific channel
  context.channels = [userChannel(context), friendChannel(context))]
  next()
})

Now this query would only listen to all the changes that were made to the user posts and his friends.

coveralls · 2023-01-30T11:34:17Z

Coverage: ?%. Remained the same when pulling f1b6a11 on feature/multi-channel-subscription into cc0e338 on master.

coveralls · 2023-01-30T11:34:20Z

Coverage: 97.484% (+0.01%) from 97.471% when pulling 628de99 on feature/multi-channel-subscription into f4845fd on master.

alecgibson · 2023-01-30T12:53:41Z

lib/backend.js

-          stream.destroy();
-          return callback(err);
+
+    var channels = [request.channel];


I think we should reorganise this a bit to log a deprecation warning for using request.channel (ie it should be unset by default, and if a consumer actively sets it, then we should log a warning).

alecgibson · 2023-01-30T13:02:04Z

lib/backend.js

-        backend.emit('timing', 'querySubscribe.initial', Date.now() - start, request);
-        callback(null, queryEmitter, snapshots, extra);
+        // Issue query on db to get our initial results
+        backend._query(agent, request, function(err, snapshots, extra) {


Would be nice if we could avoid hitting the DB with the same query multiple times

lib/backend.js

ericyhwang

Comments from last week, forgot to publish 😅

lib/backend.js

lib/query-emitter.js

ericyhwang · 2023-02-07T17:45:07Z

test/client/query-subscribe.js

+        });
+      });
+
+      it('creating a document updates a subscribed query', function(done) {


Since many of these tests are identical to above, let's either make a shared function with the shared tests invoked for each case, or use a for-each. That reduces test maintenance.

ericyhwang · 2023-02-07T17:46:56Z

test/client/query-subscribe.js

@@ -592,5 +592,769 @@ module.exports = function(options) {
        });
      });
    });
+
+    describe('custom channels', function() {


(Alec) Let's add a test around making sure things still work (no duplicate doc events) if multiple channels match, if debounce is disabled by the sharedb user.

ericyhwang

Alec notes that we don't have a "golden path" test case on the expected use of this new featurre.

Something like:

Notes query on { users: { $in: { ['user-1', 'user2'] } } }, with those channels
One op on solely channel 'user-1', another op solely on 'user-2' channel
Make sure query receives both updates

ericyhwang · 2023-02-14T17:08:38Z

lib/backend.js

+    var channels = request.channels;
+
+    if (request.channel) {
+      logger.warn('[DEPRECATED] Do no use channel for query, use channels instead.');


Suggested update to be clearer, since this only shows the log line - [DEPRECATED] "query" middleware's context.channel is deprecated, use context.channels instead.

Also update query middleware documentation - https://share.github.io/sharedb/middleware/actions#query

ericyhwang · 2023-02-14T17:16:38Z

test/client/query-subscribe.js

-        }
-      ], function(err) {
+
+  it('creating an additional document updates a subscribed query', function(done) {


We can get rid of this second one, apparently we've had this exact duplicate copy for a long time.

ericyhwang · 2023-02-14T17:22:38Z

test/client/query-subscribe.js

+          query.on('error', done);
+          query.on('insert', function() {
+            count++;
+            if (count === 3) {


Alec - let's add a > 3 condition that calls done with error, so it will catch cases where duplicate causes it to go above 3

dawidreedsy · 2023-02-15T13:49:35Z

Alec notes that we don't have a "golden path" test case on the expected use of this new featurre.

Something like:

Notes query on { users: { $in: { ['user-1', 'user2'] } } }, with those channels

One op on solely channel 'user-1', another op solely on 'user-2' channel

Make sure query receives both updates

Added new test

sharedb/test/client/query-subscribe.js

Lines 17 to 88 in 98a00be

    
           it('only informs subscribed channels', function(done) { 
        
             this.backend.use('connect', function(context, next) { 
        
               context.agent.custom = context.req; 
        
               next(); 
        
             }); 
        
             this.backend.use('commit', function(context, next) { 
        
               var user = context.agent.custom; 
        
               if (user === 'sending-user-1') { 
        
                 context.channels.push('channel-1'); 
        
               } 
        
               if (user === 'sending-user-2') { 
        
                 context.channels.push('channel-2'); 
        
               } 
        
               next(); 
        
             }); 
        
             this.backend.use('query', function(context, next) { 
        
               var user = context.agent.custom; 
        
               if (user === 'receiving-user') { 
        
                 context.channels = ['channel-1', 'channel-2']; 
        
               } else if (user === 'not-receiving-user') { 
        
                 context.channels = ['different-channel']; 
        
               } 
        
               next(); 
        
             }); 
        
             var receivingUserConnection = this.backend.connect(null, 'receiving-user'); 
        
             var notReceivingUserConnection = this.backend.connect(null, 'not-receiving-user'); 
        
             var sendingUser1Connection = this.backend.connect(null, 'sending-user-1'); 
        
             var sendingUser2Connection = this.backend.connect(null, 'sending-user-2'); 
        
             var notReceivingQuery = notReceivingUserConnection.createSubscribeQuery( 
        
               'dogs', 
        
               this.matchAllDbQuery, 
        
               null, 
        
               function(err) { 
        
                 if (err) return done(err); 
        
               } 
        
             ); 
        
             notReceivingQuery.on('error', done); 
        
             notReceivingQuery.on('insert', function() { 
        
               done('User who didn\'t subscribed to sending channels shouldn\'t get the message'); 
        
             }); 
        
             var receivingQuery = receivingUserConnection.createSubscribeQuery( 
        
               'dogs', 
        
               this.matchAllDbQuery, 
        
               null, 
        
               function(err) { 
        
                 if (err) return done(err); 
        
                 sendingUser1Connection.get('dogs', '1').on('error', done).create({}); 
        
                 sendingUser2Connection.get('dogs', '2').on('error', done).create({}); 
        
               } 
        
             ); 
        
             var receivedDogsCount = 0; 
        
             receivingQuery.on('error', done); 
        
             receivingQuery.on('insert', function() { 
        
               receivedDogsCount++; 
        
               if (receivedDogsCount === 2) { 
        
                 var allDocsIds = receivingQuery.results.map(function(doc) { 
        
                   return doc.id; 
        
                 }); 
        
                 expect(allDocsIds).to.be.deep.equal(['1', '2']); 
        
                 done(); 
        
               } else if (receivedDogsCount > 2) { 
        
                 done('It should not duplicate messages'); 
        
               } 
        
             }); 
        
           });

dawidreedsy · 2023-02-15T14:05:28Z

@ericyhwang @alecgibson I manage to get duplicated results with multiple channels. https://github.com/share/sharedb/actions/runs/4184518977/jobs/7250274650
In this test

sharedb/test/client/query-subscribe.js

Lines 120 to 147 in 98a00be

    
           it('does not duplicate messages', function(done) { 
        
             var connection = this.backend.connect(); 
        
             var count = 0; 
        
             var query = connection.createSubscribeQuery( 
        
               'dogs', 
        
               this.matchAllDbQuery, 
        
               {pollInterval: 0, pollDebounce: 0}, 
        
               function(err) { 
        
                 if (err) return done(err); 
        
                 connection.get('dogs', '1').on('error', done).create({}); 
        
                 connection.get('dogs', '2').on('error', done).create({}); 
        
                 connection.get('dogs', '3').on('error', done).create({}); 
        
               } 
        
             ); 
        
             query.on('error', done); 
        
             query.on('insert', function() { 
        
               count++; 
        
               if (count === 3) { 
        
                 var allDocsIds = query.results.map(function(doc) { 
        
                   return doc.id; 
        
                 }); 
        
                 expect(allDocsIds).to.be.deep.equal(['1', '2', '3']); 
        
                 done(); 
        
               } else if (count > 3) { 
        
                 done('It should not duplicate messages'); 
        
               } 
        
             }); 
        
           });

Although it is quite difficult to get it in test, even if the test passes we cannot be 100% sure this multi channel connection works. Maybe just test like 100 messages to ensure non of them are duplicated. 🤷

I wonder if we should force pollDebounce, but i guess it still can get duplicated if there is enough messages coming through :/

alecgibson · 2023-02-15T16:59:21Z

@dawidreedsy I'm not sure that that's a duplicate message; just looks like the inserts were reported in a different order? I'm not sure if a query's sort order is well-defined...? Or does it depend on the query? Maybe we need to fix the order?

dawidreedsy · 2023-02-15T17:23:51Z

@alecgibson Yeah sorry I missed the + - and tought there is 2 duplicated. Yeah will change to ignore order.

alecgibson · 2023-02-20T09:36:09Z

lib/error.js

@@ -68,7 +68,8 @@ ShareDBError.CODES = {
  ERR_SUBMIT_TRANSFORM_OPS_NOT_FOUND: 'ERR_SUBMIT_TRANSFORM_OPS_NOT_FOUND',
  ERR_TYPE_CANNOT_BE_PROJECTED: 'ERR_TYPE_CANNOT_BE_PROJECTED',
  ERR_TYPE_DOES_NOT_SUPPORT_PRESENCE: 'ERR_TYPE_DOES_NOT_SUPPORT_PRESENCE',
-  ERR_UNKNOWN_ERROR: 'ERR_UNKNOWN_ERROR'
+  ERR_UNKNOWN_ERROR: 'ERR_UNKNOWN_ERROR',
+  ERR_MISSING_QUERY_CHANNEL: 'ERR_MISSING_QUERY_CHANNEL'


All of our other error codes generally have the format ERR_<THING>_<DESCRIPTION>, so let's try to stick to that:

Suggested change

ERR_MISSING_QUERY_CHANNEL: 'ERR_MISSING_QUERY_CHANNEL'

ERR_QUERY_CHANNEL_MISSING: 'ERR_MISSING_QUERY_CHANNEL'

Also can you please alphabetise

ericyhwang · 2023-02-21T17:10:28Z

test/client/query-subscribe.js

-        });
-      });
+  it('returns pubSub error if fails to subscribe to channel', function(done) {
+    sinon.stub(this.backend.pubsub, 'subscribe').callsFake(function(_channel, callback) {


Let's add an afterEach with sinon.restore in test/setup.js just to be clean about the tear-down, though Alec points out we create a new backend each test case so it's fine in this case.

ericyhwang · 2023-02-21T17:21:04Z

test/client/query-subscribe.js

+  });
+
+  it('pollInterval captures additional unpublished creates', function(done) {
+    this.timeout(4000);


Let's use sinon fake timers here, to avoid artificially lengthening the test case.

Will have to advance time at least 3 times, we think?

Added fakeTimers

alecgibson · 2023-03-01T09:24:50Z

@dawidreedsy I've released the sharedb-mongo fix, but looks like Node 14 is still having issues?

curran · 2023-03-01T11:43:52Z

It may be worth considering dropping support for Node 14, as it's nearing EOL according to https://github.com/nodejs/release#release-schedule

At the moment we can only specify one channel the specific query, would be able to listen to, as per docs: ```javascript backend.use('query', (context, next) => { // Set our query to only listen for changes on our user-specific channel context.channel = userChannel(context) next() }) ``` However let's imagine the situation where the user wants to query all the posts, the where posted by him and all his friends. Now we would need new query every for friend separately to make sure the proper scalability is preserved and we do not receive all the changes to posts collection. This change allows to listen for multiple channels, so if we want to query all user friends posts. We can do it by this: ```javascript backend.use('query', (context, next) => { // Set our query to only listen for changes on our user-specific channel context.channels = [userChannel(context), friendChannel(context))] next() }) ``` Now this query would only listen to all the changes that were made to the user posts and his friends.

alecgibson

🚀

dawidreedsy requested review from alecgibson and ericyhwang January 30, 2023 11:32

dawidreedsy marked this pull request as draft January 30, 2023 11:32

alecgibson reviewed Jan 30, 2023

View reviewed changes

dawidreedsy force-pushed the feature/multi-channel-subscription branch from f1b6a11 to cec9ac9 Compare February 7, 2023 14:22

dawidreedsy requested a review from alecgibson February 7, 2023 14:22

dawidreedsy marked this pull request as ready for review February 7, 2023 14:22

dawidreedsy force-pushed the feature/multi-channel-subscription branch 2 times, most recently from a12d3af to f27470c Compare February 7, 2023 17:05

ericyhwang reviewed Feb 7, 2023

View reviewed changes

lib/backend.js Show resolved Hide resolved

lib/backend.js Show resolved Hide resolved

dawidreedsy force-pushed the feature/multi-channel-subscription branch from f27470c to bb5bcab Compare February 7, 2023 17:41

ericyhwang reviewed Feb 7, 2023

View reviewed changes

dawidreedsy force-pushed the feature/multi-channel-subscription branch 3 times, most recently from c5dd7d7 to 2ca6a2f Compare February 14, 2023 17:10

ericyhwang reviewed Feb 14, 2023

View reviewed changes

dawidreedsy force-pushed the feature/multi-channel-subscription branch 2 times, most recently from 8d2e1fe to 98a00be Compare February 15, 2023 13:47

dawidreedsy force-pushed the feature/multi-channel-subscription branch from 98a00be to 9639e94 Compare February 15, 2023 18:03

dawidreedsy requested review from ericyhwang and alecgibson and removed request for alecgibson February 15, 2023 18:04

dawidreedsy force-pushed the feature/multi-channel-subscription branch 2 times, most recently from 960ee7e to 8cf4f10 Compare February 16, 2023 09:33

dawidreedsy force-pushed the feature/multi-channel-subscription branch from 8cf4f10 to 3fde903 Compare February 16, 2023 09:45

alecgibson reviewed Feb 20, 2023

View reviewed changes

dawidreedsy force-pushed the feature/multi-channel-subscription branch 2 times, most recently from ead4de4 to 40f1f26 Compare February 21, 2023 17:10

ericyhwang reviewed Feb 21, 2023

View reviewed changes

dawidreedsy force-pushed the feature/multi-channel-subscription branch from 40f1f26 to e29155e Compare February 21, 2023 17:35

dawidreedsy requested review from ericyhwang and alecgibson February 21, 2023 17:36

dawidreedsy force-pushed the feature/multi-channel-subscription branch 14 times, most recently from b81b81e to 2373ae5 Compare February 28, 2023 17:16

dawidreedsy force-pushed the feature/multi-channel-subscription branch from 2373ae5 to d804bd2 Compare March 3, 2023 09:56

dawidreedsy force-pushed the feature/multi-channel-subscription branch from d804bd2 to 628de99 Compare March 3, 2023 10:20

alecgibson approved these changes Mar 3, 2023

View reviewed changes

dawidreedsy merged commit 20f1fa7 into master Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add multi channel query subscription. #587

✨ Add multi channel query subscription. #587

dawidreedsy commented Jan 30, 2023

coveralls commented Jan 30, 2023

coveralls commented Jan 30, 2023 •

edited

alecgibson Jan 30, 2023

alecgibson Jan 30, 2023

ericyhwang left a comment

ericyhwang Feb 7, 2023

ericyhwang Feb 7, 2023

ericyhwang left a comment

ericyhwang Feb 14, 2023

dawidreedsy Feb 15, 2023

ericyhwang Feb 14, 2023

dawidreedsy Feb 15, 2023

ericyhwang Feb 14, 2023

dawidreedsy Feb 15, 2023

dawidreedsy commented Feb 15, 2023

dawidreedsy commented Feb 15, 2023 •

edited

alecgibson commented Feb 15, 2023

dawidreedsy commented Feb 15, 2023

alecgibson Feb 20, 2023

ericyhwang Feb 21, 2023

dawidreedsy Feb 21, 2023

ericyhwang Feb 21, 2023

dawidreedsy Feb 21, 2023

alecgibson commented Mar 1, 2023

curran commented Mar 1, 2023

alecgibson left a comment

	ERR_MISSING_QUERY_CHANNEL: 'ERR_MISSING_QUERY_CHANNEL'
	ERR_QUERY_CHANNEL_MISSING: 'ERR_MISSING_QUERY_CHANNEL'

✨ Add multi channel query subscription. #587

✨ Add multi channel query subscription. #587

Conversation

dawidreedsy commented Jan 30, 2023

coveralls commented Jan 30, 2023

coveralls commented Jan 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericyhwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericyhwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawidreedsy commented Feb 15, 2023

dawidreedsy commented Feb 15, 2023 • edited

alecgibson commented Feb 15, 2023

dawidreedsy commented Feb 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecgibson commented Mar 1, 2023

curran commented Mar 1, 2023

alecgibson left a comment

Choose a reason for hiding this comment

coveralls commented Jan 30, 2023 •

edited

dawidreedsy commented Feb 15, 2023 •

edited