Fix #7340 by correclty computing function name for push event#7341

percypyan · 2021-04-12T14:14:56Z

New Pull Request Checklist

I am not disclosing a vulnerability.
I am creating this PR in reference to an issue.

Issue Description

Related issue: #7340

Approach

First create a failing test to demonstrate the issue described by #7340. Then simply replace the update of the argument message by the use of the newly created res object.

TODOs before merging

Add test cases
Add entry to changelog

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.

This prevent computing function name from a incorrect event if multiple subscriptions override one by one the message.event.

codecov · 2021-04-12T14:21:30Z

Codecov Report

Merging #7341 (f7e6d71) into master (45d00ce) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f7e6d71 differs from pull request most recent head d9cd5ff. Consider uploading reports for the commit d9cd5ff to get more accurate results

@@            Coverage Diff             @@
##           master    #7341      +/-   ##
==========================================
- Coverage   93.91%   93.90%   -0.01%     
==========================================
  Files         181      181              
  Lines       13194    13193       -1     
==========================================
- Hits        12391    12389       -2     
- Misses        803      804       +1

Impacted Files	Coverage Δ
src/LiveQuery/ParseLiveQueryServer.js	`95.32% <100.00%> (-0.02%)`	⬇️
src/batch.js	`91.37% <0.00%> (-1.73%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45d00ce...d9cd5ff. Read the comment docs.

percypyan · 2021-04-12T15:13:33Z

@mtrezza PostgreSQL 13 CI is failing, but I can't understand why...

percypyan · 2021-04-12T15:26:06Z

Also the faulty code has been merge to master on February 2021, but the last release as been made on December 2020... That is kind of weird. How can I have this code on my server if no release has been made since the change 🤔 ?

mtrezza · 2021-04-12T15:58:59Z

Thanks for this PR!

No worries, these are known faulty tests, it's one or multiple Postgres tests. I restarted the tests.

@dplewis is already working on a PR to fix the faulty Postgres tests.

mtrezza · 2021-04-12T16:35:04Z

spec/ParseLiveQueryServer.spec.js

      expect(toSend.original).toBeDefined();
      done();
-    }, jasmine.ASYNC_TEST_WAIT_TIME);
+    }, ASYNC_TEST_WAIT_TIME);


Why are the timeouts changed?

There was no timeout before: the variable was undefined. As promises are all mocked with instant resolution, we can remove it completely. I only have replaced those by a small timeout to ensure that tested process were fully executed before expecting anything.

Interesting, if it was undefined, is there maybe another issue that you have discovered? Maybe that is also why some tests are flaky.

@dplewis any ideas? You recently looked into the tests.

I've searched for other usage of jasmine.ASYNC_TEST_WAIT_TIME in the project, if found none.
This is only in this file :)

I've checked as much as I can, I only found Promise.resolve/Promise.reject, so no delay applied. But it's certainly safer to have 20-100ms of waiting time to be sure everything is completed.

I had some hard time to navigate through mocks, so I might have miss something!

Sounds good. It's probably also safer to set the timeout on a file basis rather than globally, because tests may require different timeouts and changes on a global level may cause flaky tests.

percypyan · 2021-04-13T13:02:30Z

@mtrezza Do I need to do something more or we just need an other review?

mtrezza · 2021-04-13T13:28:18Z

spec/ParseLiveQueryServer.spec.js

    setTimeout(function () {
      expect(client.pushDelete).toHaveBeenCalled();
      done();
-    }, jasmine.ASYNC_TEST_WAIT_TIME);


Do you think you could rewrite these timed blocks to use async/await instead of being nested?

I know that goes a bit beyond the scope of this PR, but it would certainly make the tests easier to read. So instead of these nests we would just do:

await new Promise(resolve => setTimeout(resolve, ASYNC_TEST_WAIT_TIME));

or maybe even shorter if you want to use a method:

await timeout(ASYNC_TEST_WAIT_TIME);

mtrezza · 2021-04-13T13:31:46Z

CHANGELOG.md

 - 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)
+- Send correct events when multiple subscriptions exists for a class, and that the events triggered are differents [#7341](https://github.com/parse-community/parse-server/pull/7341)


Can you please change to:

Fix incorrect LiveQuery events triggered for multiple subscriptions on the same class with different events

Does that make sense?

mtrezza · 2021-04-13T13:33:01Z

spec/ParseLiveQueryServer.spec.js

+    }, ASYNC_TEST_WAIT_TIME);
+  });
+
+  it('can handle object multiple commands which matches some subscriptions', async done => {


To make this more specific, how about:

sends correct events for object with multiple subscriptions

Sorry, I'm not very good in English when it comes to that kind of things 😂

This is not easy to describe for me neither 😄

Ah ah you're reassuring me!

percypyan · 2021-04-13T13:46:15Z

All done @mtrezza!

mtrezza · 2021-04-13T14:05:14Z

spec/ParseLiveQueryServer.spec.js


+const ASYNC_TEST_WAIT_TIME = 100;
+
+function resolveAfter(result, msTimeout) {


Could we shorten this to

const timeout = t => new Promise(resolve => setTimeout(resolve, t || ASYNC_TEST_WAIT_TIME));

then just use

await timeout();

We can, do you prefer timeout instead of resolveAfter?

Also, is there a util file were this method should be moved? Or should we keep it file-specific?

We can, do you prefer timeout instead of resolveAfter?

Yes because this doesn't resolve anything anymore.

Also, is there a util file were this method should be moved? Or should we keep it file-specific?

Good idea, we have a helper.js for global test declarations. I would however not move the default value, because we risk that someone changes that value in the future and suddenly tests become flaky or have a higher timeout than necessary.

So maybe move this into helper:

jasmine.timeout = t => new Promise(resolve => setTimeout(resolve, t));

then just use this on file level

const timeout200 = jasmine.timeout(200); // or whatever the number

then call

await timeout();

@mtrezza changes done

mtrezza

LGTM! Thanks for this PR, now let's wait for another reviewer to approve and it's ready for merge.

davimacedo

LGTM!

…h event (parse-community#7341) * Add a failing test for issue parse-community#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. * 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. * Update CHANGELOG.md * Replace setTimeout by async/await expressions

parseplatformorg · 2021-11-01T16:30:36Z

🎉 This change has been released in version 5.0.0-beta.1

parseplatformorg · 2022-03-14T13:02:38Z

🎉 This change has been released in version 5.0.0

percypyan added 2 commits April 12, 2021 16:01

Add a failing test for issue #7340

5518b92

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.

Remove updade of message and use res.event instead

5c23e15

This prevent computing function name from a incorrect event if multiple subscriptions override one by one the message.event.

percypyan mentioned this pull request Apr 12, 2021

Live Queries Server: wrong event returned when multiple subscription triggered #7340

Closed

6 tasks

mtrezza reviewed Apr 12, 2021

View reviewed changes

mtrezza requested changes Apr 13, 2021

View reviewed changes

Update CHANGELOG.md

1bb51c4

mtrezza reviewed Apr 13, 2021

View reviewed changes

Replace setTimeout by async/await expressions

d9cd5ff

mtrezza approved these changes Apr 13, 2021

View reviewed changes

mtrezza requested a review from dplewis April 13, 2021 15:07

davimacedo approved these changes Apr 13, 2021

View reviewed changes

davimacedo merged commit 87dcd23 into parse-community:master Apr 13, 2021

fischerscode mentioned this pull request May 12, 2021

ParseLiveListWidget doesn't work parse-community/Parse-SDK-Flutter#623

Closed

parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021

mtrezza mentioned this pull request Mar 12, 2022

build: release 5.0 #7851

Merged

parseplatformorg added the state:released Released as stable version label Mar 14, 2022


		const ASYNC_TEST_WAIT_TIME = 100;

		function resolveAfter(result, msTimeout) {

Uh oh!

Conversation

percypyan commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

Uh oh!

codecov bot commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

percypyan commented Apr 12, 2021

Uh oh!

percypyan commented Apr 12, 2021

Uh oh!

mtrezza commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrezza Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

percypyan commented Apr 13, 2021

Uh oh!

mtrezza Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

percypyan commented Apr 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrezza Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrezza left a comment

Choose a reason for hiding this comment

Uh oh!

davimacedo left a comment

Choose a reason for hiding this comment

Uh oh!

parseplatformorg commented Nov 1, 2021

Uh oh!

parseplatformorg commented Mar 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

percypyan commented Apr 12, 2021 •

edited

Loading

codecov bot commented Apr 12, 2021 •

edited

Loading

mtrezza commented Apr 12, 2021 •

edited

Loading

mtrezza Apr 13, 2021 •

edited

Loading

mtrezza Apr 13, 2021 •

edited

Loading

mtrezza Apr 13, 2021 •

edited

Loading