From 5518b9213df571012cd21e1e71a4ff375393352c Mon Sep 17 00:00:00 2001 From: percypyan Date: Mon, 12 Apr 2021 16:01:03 +0200 Subject: [PATCH 1/4] Add a failing test for issue #7340 If any delay occurs after "message.event" assignation in LiveQueryServer._onAfterSave, the next subscription or request with a different event might overwrite it, and by that using the wrong "push" function name. --- spec/ParseLiveQueryServer.spec.js | 100 ++++++++++++++++++++++++++---- 1 file changed, 89 insertions(+), 11 deletions(-) diff --git a/spec/ParseLiveQueryServer.spec.js b/spec/ParseLiveQueryServer.spec.js index 5e9c71d731..7d7bd4a5f1 100644 --- a/spec/ParseLiveQueryServer.spec.js +++ b/spec/ParseLiveQueryServer.spec.js @@ -9,6 +9,16 @@ const queryHashValue = 'hash'; const testUserId = 'userId'; const testClassName = 'TestObject'; +const ASYNC_TEST_WAIT_TIME = 100; + +function resolveAfter(result, msTimeout) { + return new Promise(res => { + setTimeout(() => { + res(result); + }, msTimeout); + }); +} + describe('ParseLiveQueryServer', function () { beforeEach(function (done) { // Mock ParseWebSocketServer @@ -753,7 +763,7 @@ describe('ParseLiveQueryServer', function () { setTimeout(function () { expect(client.pushDelete).toHaveBeenCalled(); done(); - }, jasmine.ASYNC_TEST_WAIT_TIME); + }, ASYNC_TEST_WAIT_TIME); }); it('has no subscription and can handle object save command', async () => { @@ -792,7 +802,7 @@ describe('ParseLiveQueryServer', function () { expect(client.pushDelete).not.toHaveBeenCalled(); expect(client.pushLeave).not.toHaveBeenCalled(); done(); - }, jasmine.ASYNC_TEST_WAIT_TIME); + }, ASYNC_TEST_WAIT_TIME); }); it('can handle object enter command which matches some subscriptions', async done => { @@ -829,7 +839,7 @@ describe('ParseLiveQueryServer', function () { expect(client.pushDelete).not.toHaveBeenCalled(); expect(client.pushLeave).not.toHaveBeenCalled(); done(); - }, jasmine.ASYNC_TEST_WAIT_TIME); + }, ASYNC_TEST_WAIT_TIME); }); it('can handle object update command which matches some subscriptions', async done => { @@ -862,7 +872,7 @@ describe('ParseLiveQueryServer', function () { expect(client.pushDelete).not.toHaveBeenCalled(); expect(client.pushLeave).not.toHaveBeenCalled(); done(); - }, jasmine.ASYNC_TEST_WAIT_TIME); + }, ASYNC_TEST_WAIT_TIME); }); it('can handle object leave command which matches some subscriptions', async done => { @@ -899,7 +909,74 @@ describe('ParseLiveQueryServer', function () { expect(client.pushDelete).not.toHaveBeenCalled(); expect(client.pushLeave).toHaveBeenCalled(); done(); - }, jasmine.ASYNC_TEST_WAIT_TIME); + }, ASYNC_TEST_WAIT_TIME); + }); + + it('can handle object multiple commands which matches some subscriptions', async done => { + const parseLiveQueryServer = new ParseLiveQueryServer({}); + + Parse.Cloud.afterLiveQueryEvent('TestObject', () => { + // Simulate delay due to trigger, auth, etc. + return resolveAfter(null, 10); + }); + + // Make mock request message + const message = generateMockMessage(true); + // Add mock client + const clientId = 1; + const client = addMockClient(parseLiveQueryServer, clientId); + client.sessionToken = 'sessionToken'; + + // Mock queryHash for this special test + const mockQueryHash = jasmine.createSpy('matchesQuery').and.returnValue('hash1'); + jasmine.mockLibrary('../lib/LiveQuery/QueryTools', 'queryHash', mockQueryHash); + // Add mock subscription 1 + const requestId2 = 2; + await addMockSubscription(parseLiveQueryServer, clientId, requestId2, null, null, 'hash1'); + + // Mock queryHash for this special test + const mockQueryHash2 = jasmine.createSpy('matchesQuery').and.returnValue('hash2'); + jasmine.mockLibrary('../lib/LiveQuery/QueryTools', 'queryHash', mockQueryHash2); + // Add mock subscription 2 + const requestId3 = 3; + await addMockSubscription(parseLiveQueryServer, clientId, requestId3, null, null, 'hash2'); + // Mock _matchesSubscription to return matching + // In order to mimic a leave, then enter, we need original match return true + // and the current match return false, then the other way around + let counter = 0; + parseLiveQueryServer._matchesSubscription = function (parseObject) { + if (!parseObject) { + return false; + } + counter += 1; + // true, false, false, true + return counter < 2 || counter > 3; + }; + parseLiveQueryServer._matchesACL = function () { + // Simulate call + return resolveAfter(true, 10); + }; + parseLiveQueryServer._onAfterSave(message); + + // Make sure we send leave and enter command to client + setTimeout(function () { + expect(client.pushCreate).not.toHaveBeenCalled(); + expect(client.pushEnter).toHaveBeenCalledTimes(1); + expect(client.pushEnter).toHaveBeenCalledWith( + requestId3, + { key: 'value', className: 'TestObject' }, + { key: 'originalValue', className: 'TestObject' } + ); + expect(client.pushUpdate).not.toHaveBeenCalled(); + expect(client.pushDelete).not.toHaveBeenCalled(); + expect(client.pushLeave).toHaveBeenCalledTimes(1); + expect(client.pushLeave).toHaveBeenCalledWith( + requestId2, + { key: 'value', className: 'TestObject' }, + { key: 'originalValue', className: 'TestObject' } + ); + done(); + }, ASYNC_TEST_WAIT_TIME); }); it('can handle update command with original object', async done => { @@ -944,7 +1021,7 @@ describe('ParseLiveQueryServer', function () { expect(toSend.object).toBeDefined(); expect(toSend.original).toBeDefined(); done(); - }, jasmine.ASYNC_TEST_WAIT_TIME); + }, ASYNC_TEST_WAIT_TIME); }); it('can handle object create command which matches some subscriptions', async done => { @@ -977,7 +1054,7 @@ describe('ParseLiveQueryServer', function () { expect(client.pushDelete).not.toHaveBeenCalled(); expect(client.pushLeave).not.toHaveBeenCalled(); done(); - }, jasmine.ASYNC_TEST_WAIT_TIME); + }, ASYNC_TEST_WAIT_TIME); }); it('can handle create command with fields', async done => { @@ -1027,7 +1104,7 @@ describe('ParseLiveQueryServer', function () { expect(toSend.object).toBeDefined(); expect(toSend.original).toBeUndefined(); done(); - }, jasmine.ASYNC_TEST_WAIT_TIME); + }, ASYNC_TEST_WAIT_TIME); }); it('can match subscription for null or undefined parse object', function () { @@ -1737,7 +1814,8 @@ describe('ParseLiveQueryServer', function () { clientId, requestId, parseWebSocket, - query + query, + customQueryHashValue ) { // If parseWebSocket is null, we use the default one if (!parseWebSocket) { @@ -1765,12 +1843,12 @@ describe('ParseLiveQueryServer', function () { // Make mock subscription const subscription = parseLiveQueryServer.subscriptions .get(query.className) - .get(queryHashValue); + .get(customQueryHashValue || queryHashValue); subscription.hasSubscribingClient = function () { return false; }; subscription.className = query.className; - subscription.hash = queryHashValue; + subscription.hash = customQueryHashValue || queryHashValue; if (subscription.clientRequestIds && subscription.clientRequestIds.has(clientId)) { subscription.clientRequestIds.get(clientId).push(requestId); } else { From 5c23e154082a220d8ee8af0034d3372bc62ebb44 Mon Sep 17 00:00:00 2001 From: percypyan Date: Mon, 12 Apr 2021 16:04:16 +0200 Subject: [PATCH 2/4] Remove updade of message and use res.event instead This prevent computing function name from a incorrect event if multiple subscriptions override one by one the message.event. --- src/LiveQuery/ParseLiveQueryServer.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/LiveQuery/ParseLiveQueryServer.js b/src/LiveQuery/ParseLiveQueryServer.js index a4a6e6e777..d60615d5b5 100644 --- a/src/LiveQuery/ParseLiveQueryServer.js +++ b/src/LiveQuery/ParseLiveQueryServer.js @@ -298,7 +298,6 @@ class ParseLiveQueryServer { } else { return null; } - message.event = type; res = { event: type, sessionToken: client.sessionToken, @@ -334,8 +333,7 @@ class ParseLiveQueryServer { originalParseObject = res.original.toJSON(); originalParseObject.className = res.original.className || className; } - const functionName = - 'push' + message.event.charAt(0).toUpperCase() + message.event.slice(1); + const functionName = 'push' + res.event.charAt(0).toUpperCase() + res.event.slice(1); if (client[functionName]) { client[functionName](requestId, currentParseObject, originalParseObject); } From 1bb51c411dc62bb2c908bb193b401b09851c1dbb Mon Sep 17 00:00:00 2001 From: percypyan Date: Mon, 12 Apr 2021 16:21:38 +0200 Subject: [PATCH 3/4] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4027ef4535..1b6e132ee3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -130,6 +130,7 @@ ___ - Fix file upload issue for S3 compatible storage (Linode, DigitalOcean) by avoiding empty tags property when creating a file (Ali Oguzhan Yildiz) [#7300](https://github.com/parse-community/parse-server/pull/7300) - Add building Docker image as CI check (Manuel Trezza) [#7332](https://github.com/parse-community/parse-server/pull/7332) - Add NPM package-lock version check to CI (Manuel Trezza) [#7333](https://github.com/parse-community/parse-server/pull/7333) +- Fix incorrect LiveQuery events triggered for multiple subscriptions on the same class with different events [#7341](https://github.com/parse-community/parse-server/pull/7341) ___ ## 4.5.0 [Full Changelog](https://github.com/parse-community/parse-server/compare/4.4.0...4.5.0) From d9cd5ffe8b4e1080ee1f61527df228536073fca1 Mon Sep 17 00:00:00 2001 From: percypyan Date: Tue, 13 Apr 2021 15:42:30 +0200 Subject: [PATCH 4/4] Replace setTimeout by async/await expressions --- spec/ParseLiveQueryServer.spec.js | 172 ++++++++++++++---------------- spec/helper.js | 2 + 2 files changed, 84 insertions(+), 90 deletions(-) diff --git a/spec/ParseLiveQueryServer.spec.js b/spec/ParseLiveQueryServer.spec.js index 7d7bd4a5f1..0d1a1e6387 100644 --- a/spec/ParseLiveQueryServer.spec.js +++ b/spec/ParseLiveQueryServer.spec.js @@ -9,15 +9,7 @@ const queryHashValue = 'hash'; const testUserId = 'userId'; const testClassName = 'TestObject'; -const ASYNC_TEST_WAIT_TIME = 100; - -function resolveAfter(result, msTimeout) { - return new Promise(res => { - setTimeout(() => { - res(result); - }, msTimeout); - }); -} +const timeout = () => jasmine.timeout(100); describe('ParseLiveQueryServer', function () { beforeEach(function (done) { @@ -760,10 +752,10 @@ describe('ParseLiveQueryServer', function () { // Make sure we send command to client, since _matchesACL is async, we have to // wait and check - setTimeout(function () { - expect(client.pushDelete).toHaveBeenCalled(); - done(); - }, ASYNC_TEST_WAIT_TIME); + await timeout(); + + expect(client.pushDelete).toHaveBeenCalled(); + done(); }); it('has no subscription and can handle object save command', async () => { @@ -795,14 +787,14 @@ describe('ParseLiveQueryServer', function () { parseLiveQueryServer._onAfterSave(message); // Make sure we do not send command to client - setTimeout(function () { - expect(client.pushCreate).not.toHaveBeenCalled(); - expect(client.pushEnter).not.toHaveBeenCalled(); - expect(client.pushUpdate).not.toHaveBeenCalled(); - expect(client.pushDelete).not.toHaveBeenCalled(); - expect(client.pushLeave).not.toHaveBeenCalled(); - done(); - }, ASYNC_TEST_WAIT_TIME); + await timeout(); + + expect(client.pushCreate).not.toHaveBeenCalled(); + expect(client.pushEnter).not.toHaveBeenCalled(); + expect(client.pushUpdate).not.toHaveBeenCalled(); + expect(client.pushDelete).not.toHaveBeenCalled(); + expect(client.pushLeave).not.toHaveBeenCalled(); + done(); }); it('can handle object enter command which matches some subscriptions', async done => { @@ -832,14 +824,14 @@ describe('ParseLiveQueryServer', function () { parseLiveQueryServer._onAfterSave(message); // Make sure we send enter command to client - setTimeout(function () { - expect(client.pushCreate).not.toHaveBeenCalled(); - expect(client.pushEnter).toHaveBeenCalled(); - expect(client.pushUpdate).not.toHaveBeenCalled(); - expect(client.pushDelete).not.toHaveBeenCalled(); - expect(client.pushLeave).not.toHaveBeenCalled(); - done(); - }, ASYNC_TEST_WAIT_TIME); + await timeout(); + + expect(client.pushCreate).not.toHaveBeenCalled(); + expect(client.pushEnter).toHaveBeenCalled(); + expect(client.pushUpdate).not.toHaveBeenCalled(); + expect(client.pushDelete).not.toHaveBeenCalled(); + expect(client.pushLeave).not.toHaveBeenCalled(); + done(); }); it('can handle object update command which matches some subscriptions', async done => { @@ -865,14 +857,14 @@ describe('ParseLiveQueryServer', function () { parseLiveQueryServer._onAfterSave(message); // Make sure we send update command to client - setTimeout(function () { - expect(client.pushCreate).not.toHaveBeenCalled(); - expect(client.pushEnter).not.toHaveBeenCalled(); - expect(client.pushUpdate).toHaveBeenCalled(); - expect(client.pushDelete).not.toHaveBeenCalled(); - expect(client.pushLeave).not.toHaveBeenCalled(); - done(); - }, ASYNC_TEST_WAIT_TIME); + await timeout(); + + expect(client.pushCreate).not.toHaveBeenCalled(); + expect(client.pushEnter).not.toHaveBeenCalled(); + expect(client.pushUpdate).toHaveBeenCalled(); + expect(client.pushDelete).not.toHaveBeenCalled(); + expect(client.pushLeave).not.toHaveBeenCalled(); + done(); }); it('can handle object leave command which matches some subscriptions', async done => { @@ -902,22 +894,22 @@ describe('ParseLiveQueryServer', function () { parseLiveQueryServer._onAfterSave(message); // Make sure we send leave command to client - setTimeout(function () { - expect(client.pushCreate).not.toHaveBeenCalled(); - expect(client.pushEnter).not.toHaveBeenCalled(); - expect(client.pushUpdate).not.toHaveBeenCalled(); - expect(client.pushDelete).not.toHaveBeenCalled(); - expect(client.pushLeave).toHaveBeenCalled(); - done(); - }, ASYNC_TEST_WAIT_TIME); + await timeout(); + + expect(client.pushCreate).not.toHaveBeenCalled(); + expect(client.pushEnter).not.toHaveBeenCalled(); + expect(client.pushUpdate).not.toHaveBeenCalled(); + expect(client.pushDelete).not.toHaveBeenCalled(); + expect(client.pushLeave).toHaveBeenCalled(); + done(); }); - it('can handle object multiple commands which matches some subscriptions', async done => { + it('sends correct events for object with multiple subscriptions', async done => { const parseLiveQueryServer = new ParseLiveQueryServer({}); Parse.Cloud.afterLiveQueryEvent('TestObject', () => { // Simulate delay due to trigger, auth, etc. - return resolveAfter(null, 10); + return jasmine.timeout(10); }); // Make mock request message @@ -954,29 +946,29 @@ describe('ParseLiveQueryServer', function () { }; parseLiveQueryServer._matchesACL = function () { // Simulate call - return resolveAfter(true, 10); + return jasmine.timeout(10).then(() => true); }; parseLiveQueryServer._onAfterSave(message); // Make sure we send leave and enter command to client - setTimeout(function () { - expect(client.pushCreate).not.toHaveBeenCalled(); - expect(client.pushEnter).toHaveBeenCalledTimes(1); - expect(client.pushEnter).toHaveBeenCalledWith( - requestId3, - { key: 'value', className: 'TestObject' }, - { key: 'originalValue', className: 'TestObject' } - ); - expect(client.pushUpdate).not.toHaveBeenCalled(); - expect(client.pushDelete).not.toHaveBeenCalled(); - expect(client.pushLeave).toHaveBeenCalledTimes(1); - expect(client.pushLeave).toHaveBeenCalledWith( - requestId2, - { key: 'value', className: 'TestObject' }, - { key: 'originalValue', className: 'TestObject' } - ); - done(); - }, ASYNC_TEST_WAIT_TIME); + await timeout(); + + expect(client.pushCreate).not.toHaveBeenCalled(); + expect(client.pushEnter).toHaveBeenCalledTimes(1); + expect(client.pushEnter).toHaveBeenCalledWith( + requestId3, + { key: 'value', className: 'TestObject' }, + { key: 'originalValue', className: 'TestObject' } + ); + expect(client.pushUpdate).not.toHaveBeenCalled(); + expect(client.pushDelete).not.toHaveBeenCalled(); + expect(client.pushLeave).toHaveBeenCalledTimes(1); + expect(client.pushLeave).toHaveBeenCalledWith( + requestId2, + { key: 'value', className: 'TestObject' }, + { key: 'originalValue', className: 'TestObject' } + ); + done(); }); it('can handle update command with original object', async done => { @@ -1013,15 +1005,15 @@ describe('ParseLiveQueryServer', function () { parseLiveQueryServer._onAfterSave(message); // Make sure we send update command to client - setTimeout(function () { - expect(client.pushUpdate).toHaveBeenCalled(); - const args = parseWebSocket.send.calls.mostRecent().args; - const toSend = JSON.parse(args[0]); + await timeout(); - expect(toSend.object).toBeDefined(); - expect(toSend.original).toBeDefined(); - done(); - }, ASYNC_TEST_WAIT_TIME); + expect(client.pushUpdate).toHaveBeenCalled(); + const args = parseWebSocket.send.calls.mostRecent().args; + const toSend = JSON.parse(args[0]); + + expect(toSend.object).toBeDefined(); + expect(toSend.original).toBeDefined(); + done(); }); it('can handle object create command which matches some subscriptions', async done => { @@ -1047,14 +1039,14 @@ describe('ParseLiveQueryServer', function () { parseLiveQueryServer._onAfterSave(message); // Make sure we send create command to client - setTimeout(function () { - expect(client.pushCreate).toHaveBeenCalled(); - expect(client.pushEnter).not.toHaveBeenCalled(); - expect(client.pushUpdate).not.toHaveBeenCalled(); - expect(client.pushDelete).not.toHaveBeenCalled(); - expect(client.pushLeave).not.toHaveBeenCalled(); - done(); - }, ASYNC_TEST_WAIT_TIME); + await timeout(); + + expect(client.pushCreate).toHaveBeenCalled(); + expect(client.pushEnter).not.toHaveBeenCalled(); + expect(client.pushUpdate).not.toHaveBeenCalled(); + expect(client.pushDelete).not.toHaveBeenCalled(); + expect(client.pushLeave).not.toHaveBeenCalled(); + done(); }); it('can handle create command with fields', async done => { @@ -1097,14 +1089,14 @@ describe('ParseLiveQueryServer', function () { parseLiveQueryServer._onAfterSave(message); // Make sure we send create command to client - setTimeout(function () { - expect(client.pushCreate).toHaveBeenCalled(); - const args = parseWebSocket.send.calls.mostRecent().args; - const toSend = JSON.parse(args[0]); - expect(toSend.object).toBeDefined(); - expect(toSend.original).toBeUndefined(); - done(); - }, ASYNC_TEST_WAIT_TIME); + await timeout(); + + expect(client.pushCreate).toHaveBeenCalled(); + const args = parseWebSocket.send.calls.mostRecent().args; + const toSend = JSON.parse(args[0]); + expect(toSend.object).toBeDefined(); + expect(toSend.original).toBeUndefined(); + done(); }); it('can match subscription for null or undefined parse object', function () { diff --git a/spec/helper.js b/spec/helper.js index 44f87c430b..49de5c453f 100644 --- a/spec/helper.js +++ b/spec/helper.js @@ -515,3 +515,5 @@ jasmine.restoreLibrary = function (library, name) { } require(library)[name] = libraryCache[library][name]; }; + +jasmine.timeout = t => new Promise(resolve => setTimeout(resolve, t));