Live query CLP #4387

flovilmart · 2017-11-26T01:57:17Z

Just a proof of concept for now, we'll see how down the rabbit hole it will lead us.

The premise is passing the perms alongside the object when notifying the LiveQuery server so it can do the matches agains the clients without having to access the DB.

I wanna check how badly it is in terms of coverage now that I refactored it all.

codecov · 2017-11-26T03:54:26Z

Codecov Report

Merging #4387 into master will increase coverage by 0.05%.
The diff coverage is 97.95%.

@@            Coverage Diff            @@
##           master   #4387      +/-   ##
=========================================
+ Coverage   93.75%   93.8%   +0.05%     
=========================================
  Files         123     123              
  Lines        8884    8915      +31     
=========================================
+ Hits         8329    8363      +34     
+ Misses        555     552       -3

Impacted Files	Coverage Δ
src/Routers/UsersRouter.js	`93.46% <ø> (ø)`	⬆️
src/triggers.js	`94.31% <100%> (ø)`	⬆️
src/rest.js	`97.7% <100%> (+0.05%)`	⬆️
src/Controllers/DatabaseController.js	`94.73% <100%> (ø)`	⬆️
src/Controllers/LiveQueryController.js	`95.65% <100%> (+38.5%)`	⬆️
src/RestWrite.js	`92.88% <100%> (-0.27%)`	⬇️
src/Controllers/SchemaController.js	`96.32% <88.88%> (ø)`	⬆️
src/LiveQuery/ParseLiveQueryServer.js	`87.78% <98.64%> (+1.18%)`	⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`91.52% <0%> (-0.73%)`	⬇️
... and 2 more

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 8dff708...247cc23. Read the comment docs.

flovilmart · 2017-11-27T23:24:03Z

src/LiveQuery/ParseLiveQueryServer.js

+          logger.verbose(`Failed matching CLP for ${object.id} ${foundUserId} ${e}`);
+          return Parse.Promise.as(false);
+        }
+        // TODO: handle roles permissions


we can probably let those out for now, because

nothing was supported before

The default behaviour is the safe one.

roles require more i/o / query, and could be cached properly

Hmmm, I don't know about this one. Having the live query responses behave differently in terms of roles could cause some unexpected headaches :/.

An alternative could be a cache of roles?

yep, we'd need a roles cache somehow, perhaps refactoring the Auth module.

Refactoring is finally done in #4940 :)

acinader · 2017-11-27T23:32:40Z

spec/helper.js

      const newConfiguration = Object.assign({}, defaultConfiguration, changedConfiguration, {
-        __indexBuildCompletionCallbackForTests: indexBuildPromise => indexBuildPromise.then(resolve, reject),
+        __indexBuildCompletionCallbackForTests: indexBuildPromise => indexBuildPromise.then(() => {


I'm confused here, what is going on? I don't see who wants this parse server back? And I don't see how line 140 is or needs to be changed?? just trying to follow.

We resolve with the parseServer instance now, that has access to everything, config. HttpServer etc...

acinader · 2017-11-27T23:34:26Z

src/LiveQuery/ParseLiveQueryServer.js

+        }
+        return Parse.Promise.as(['*']);
+      }).then((aclGroup) => {
+        console.log(aclGroup); // eslint-disable-line


prolly didn't mean to leave this in?

Nope my mistake :)

acinader

lgtm. I had a nit and question.

acinader · 2017-11-27T23:34:57Z

src/LiveQuery/ParseLiveQueryServer.js

+  }
+
+  _getCLPOperation(query: any) {
+    return typeof query == 'object'


flovilmart · 2017-11-27T23:37:05Z

Just FWI, i didn’t manage to do E2E test on it...

acinader · 2017-11-27T23:47:04Z

sorry to hear that ;)

montymxb · 2017-11-28T08:43:13Z

.travis.yml

@@ -52,7 +52,6 @@ jobs:
      env: 
      before_script: skip
      after_script: skip
-      script: npm install -g nsp && nsp check


Liking that we have nsp showing up as a separate check now 👍

flovilmart · 2018-08-09T02:40:52Z

@flavionegrao Can you have a look at this one? I believe it shoudl be pefromant enough for your needs. Let me know if you need anything more ;)

@acinader Can you have a look? the last feature missing is still checking for role permissions in CLPs if needed. I'll finish it up tomorrow most likely.

This will fit well in the v3.0!

acinader

looks like you have more to do.

unfortunately, I am missing some base level knowledge to be of much help reviewing this. interesting for me to read, but I can't really validate the logic.

I do see how this would be a big performance win, so we should get it out there.

acinader · 2018-08-09T11:45:24Z

src/Auth.js

@@ -5,8 +5,9 @@ const Parse = require('parse/node');
 // An Auth object tells you who is requesting something and whether
 // the master key was used.
 // userObject is a Parse.User and can be null if there's no user.
-function Auth({ config, isMaster = false, isReadOnly = false, user, installationId } = {}) {
+function Auth({ config, cacheController = undefined, isMaster = false, isReadOnly = false, user, installationId }) {


may as well match the false pattern of the rest of the signature instead of using underfined?

undefined is more accurate as it's an object, and it may not be passed :)

acinader · 2018-08-09T11:48:40Z

src/Auth.js

+  if (results.length !== 1 || !results[0]['user']) {
+    throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token');
+  }
+  var now = new Date(),


make const

acinader · 2018-08-09T11:49:04Z

src/Auth.js

+    throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN,
+      'Session token is expired.');
+  }
+  var obj = results[0]['user'];


make const

acinader · 2018-08-09T11:49:48Z

src/Auth.js

+    cacheController.user.put(sessionToken, obj);
+  }
+  const userObject = Parse.Object.fromJSON(obj);
+  return new Auth({config, cacheController, isMaster: false, installationId, user: userObject });


missing whitespace { config

acinader · 2018-08-09T11:50:12Z

src/Auth.js

 };

-var getAuthForLegacySessionToken = function({config, sessionToken, installationId } = {}) {
+var getAuthForLegacySessionToken = function({config, sessionToken, installationId }) {


missing whitespace{ config

acinader · 2018-08-09T12:07:05Z

src/Auth.js

+
+  return new RestQuery(this.config, master(this.config), '_Role', restWhere, {})
+    .execute()
+    .then(({ results }) => results);


like that ;)

acinader · 2018-08-09T12:10:19Z

src/Controllers/SchemaController.js

+    return SchemaController.testBaseCLP(this.perms[className], aclGroup, operation);
+  }
+
+  static testBaseCLP(classPermissions: ?any, aclGroup: string[], operation: string) {


hm. object member testBaseCLP and static member testBaseCLP...fishy?

acinader · 2018-08-09T12:12:17Z

src/LiveQuery/ParseLiveQueryServer.js

+    Parse.initialize(config.appId,  Parse.javaScriptKey, config.masterKey);
+
+    // The cache controller is a proper cache controller
+    // With access to User and Roles


lower case w in with: with access to....

acinader · 2018-08-09T12:12:44Z

src/LiveQuery/ParseLiveQueryServer.js

+    this.cacheController = getCacheController(config)
+
+    // This auth cache stores the promises for each auth resolution
+    // The main benefit is to be able to reuse the same user / session token resolution


leading caps.....

acinader · 2018-08-09T12:14:41Z

src/LiveQuery/ParseLiveQueryServer.js

+    const authPromise = getAuthForSessionToken({ cacheController: this.cacheController, sessionToken: sessionToken })
+      .then((auth) => {
+        return { auth, userId: auth && auth.user && auth.user.id };
+      }, () => {


maybe make the error handler a catch instead?

stale · 2018-09-23T13:13:28Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

flovilmart · 2018-10-09T00:22:37Z

@acinader I would still wish to bring that one in as it contains very important roles performance improvements for live queries :)

src/rest.js

spec/ParseLiveQueryServer.spec.js

acinader · 2018-10-10T15:49:08Z

spec/ParseLiveQueryServer.spec.js

+    const promise = parseLiveQueryServer.getAuthForSessionToken('pleaseThrow');
+    expect(parseLiveQueryServer.authCache.get('pleaseThrow')).toBe(promise);
+    // after the promise finishes, it should have removed it from the cache
+    expect(await promise).toEqual({});


shouldn't the promise be rejected? is this testing that? i'm confused.

No it should not, the error should be swallowed and the cache should be cleaned if it throws. this the behaviour that I expected.

spec/ParseLiveQueryServer.spec.js

acinader

lgtm

Moumouls · 2018-10-17T20:51:41Z

The new system will fix, an important problem when a Parse.User is referenced in more than 100 Roles. Because, the Parse Live Server get the first 100 randoms Roles where the user is referenced so sometimes ( and more and more often), the Role associated to the current Object Event doesn't exist in results and then the Live Query fail...

flovilmart · 2018-10-18T01:01:54Z

src/LiveQuery/ParseLiveQueryServer.js

+        // If you can't continue, let's just wrap it up and delete it.
+        // Next time, one will try again
+        this.authCache.del(sessionToken);
+        return {};


@westhom This is why the CPU is spinning like a madman on invalid tokens. here we delete the entry in the cache which forces the session to be retried. If the session token is invalid, it's unlikely it will become valid any time soon.

@acinader @dplewis what do you think? should we

continue with a default auth and no user ID and expire after a shorter period like 5 minutes?

cut the connection

reject connections where the auth token is invalid?

What would be best?

* Auth module refactoring in order to be reusable * Ensure cache controller is properly forwarded from helpers * Nits * Adds support for static validation * Adds support for CLP in Live query (no support for roles yet) * Adds e2e test to validate liveQuery hooks is properly called * Adds tests over LiveQueryController to ensure data is correctly transmitted * nits * Fixes for flow types * Removes usage of Parse.Promise * Use the Auth module for authentication and caches * Cleaner implementation of getting auth * Adds authCache that stores auth promises * Proper testing of the caching * nits

flovilmart added the work in progress label Nov 26, 2017

flovilmart force-pushed the liveQuery-CLP branch 2 times, most recently from 7e05e2f to 3e31d7f Compare November 26, 2017 03:17

parse-community deleted a comment from codecov bot Nov 26, 2017

flovilmart requested a review from acinader November 27, 2017 23:22

flovilmart commented Nov 27, 2017

View reviewed changes

flovilmart removed the work in progress label Nov 27, 2017

flovilmart requested a review from montymxb November 27, 2017 23:24

acinader reviewed Nov 27, 2017

View reviewed changes

acinader previously approved these changes Nov 27, 2017

View reviewed changes

src/LiveQuery/ParseLiveQueryServer.js Outdated

}

_getCLPOperation(query: any) {

return typeof query == 'object'

Copy link

Contributor

acinader Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer ===

flovilmart dismissed acinader’s stale review via fa956c7 November 28, 2017 03:18

montymxb reviewed Nov 28, 2017

View reviewed changes

flovilmart force-pushed the liveQuery-CLP branch from b968d2f to 9b623a7 Compare February 19, 2018 14:05

Auth module refactoring in order to be reusable

82f8d5b

flovilmart mentioned this pull request Aug 8, 2018

SessionToken cache for invalid sessions tokens #4926

Closed

flovilmart force-pushed the liveQuery-CLP branch from 289c37e to 3f31039 Compare August 8, 2018 21:22

Ensure cache controller is properly forwarded from helpers

455ab16

flovilmart force-pushed the liveQuery-CLP branch 2 times, most recently from 3a41ce9 to c4f90b8 Compare August 9, 2018 02:00

acinader previously approved these changes Aug 9, 2018

View reviewed changes

acinader mentioned this pull request Aug 9, 2018

Auth module refactoring in order to be reusable #4940

Merged

flovilmart and others added 4 commits August 9, 2018 08:43

Nits

cf5c32c

Adds support for static validation

d3fea5c

Adds support for CLP in Live query (no support for roles yet)

0f7b523

Adds e2e test to validate liveQuery hooks is properly called

8c81112

flovilmart added 5 commits August 9, 2018 08:44

Removes usage of Parse.Promise

45b63a2

Use the Auth module for authentication and caches

055c768

Cleaner implementation of getting auth

ca8bcd9

Adds authCache that stores auth promises

e357059

Proper testing of the caching

08bba4f

flovilmart dismissed acinader’s stale review via 08bba4f August 9, 2018 12:44

flovilmart force-pushed the liveQuery-CLP branch from ecc212b to 08bba4f Compare August 9, 2018 12:44

nits

bd1cd5b

stale bot added the wontfix label Sep 23, 2018

stale bot closed this Sep 30, 2018

flovilmart reopened this Oct 8, 2018

stale bot removed the wontfix label Oct 8, 2018

flovilmart mentioned this pull request Oct 9, 2018

Parse LiveQuery Excessive CPU #5106

Closed

Merge branch 'master' into liveQuery-CLP

bf467b2

flovilmart force-pushed the liveQuery-CLP branch from eab73bb to bf467b2 Compare October 9, 2018 01:15

Merge branch 'master' into liveQuery-CLP

095e38d

acinader reviewed Oct 9, 2018

View reviewed changes

src/rest.js Show resolved Hide resolved

acinader reviewed Oct 10, 2018

View reviewed changes

spec/ParseLiveQueryServer.spec.js Show resolved Hide resolved

acinader reviewed Oct 10, 2018

View reviewed changes

spec/ParseLiveQueryServer.spec.js Show resolved Hide resolved

acinader approved these changes Oct 11, 2018

View reviewed changes

Merge branch 'master' into liveQuery-CLP

247cc23

flovilmart merged commit 7c81290 into master Oct 17, 2018

flovilmart deleted the liveQuery-CLP branch October 17, 2018 21:53

flovilmart commented Oct 18, 2018

View reviewed changes

flovilmart mentioned this pull request Nov 10, 2018

Compared to 2.5.0, 2.8.0 uses 60%+ memory in a resting state #5168

Closed

Live query CLP #4387

Live query CLP #4387

Conversation

flovilmart commented Nov 26, 2017 • edited

codecov bot commented Nov 26, 2017 • edited

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acinader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flovilmart commented Nov 27, 2017

acinader commented Nov 27, 2017

Choose a reason for hiding this comment

flovilmart commented Aug 9, 2018

acinader 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

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

stale bot commented Sep 23, 2018

flovilmart commented Oct 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acinader left a comment

Choose a reason for hiding this comment

Moumouls commented Oct 17, 2018

Choose a reason for hiding this comment

flovilmart commented Nov 26, 2017 •

edited

codecov bot commented Nov 26, 2017 •

edited

flovilmart commented Oct 9, 2018 •

edited