Fix issue #5274 on RestQuery.each and relations #5276

percypyan · 2019-01-03T10:09:50Z

Closes: #5274

The RestQuery.prototype.each method was going through all results by adding a filter on objectId field of the concerned class when the results length exceed the limit. The problem was that if any constraint has been done on objectId field, it was overridden. That was the case for the all relation queries (like the _Roles of a user for any user authenticated queries).
To fix this, replace the assignation of an arbitrary object by the merge of the existing constraint with the new one.

The fix provided allows .each to only query on roles for this specific user instead of all roles

codecov · 2019-01-03T10:21:21Z

Codecov Report

Merging #5276 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5276      +/-   ##
==========================================
- Coverage   93.93%   93.91%   -0.03%     
==========================================
  Files         123      123              
  Lines        8975     8975              
==========================================
- Hits         8431     8429       -2     
- Misses        544      546       +2

Impacted Files	Coverage Δ
src/RestQuery.js	`96.11% <100%> (ø)`	⬆️
src/RestWrite.js	`92.88% <0%> (-0.37%)`	⬇️

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 cc0d769...a3c9330. Read the comment docs.

flovilmart

If the query has the objectId constraint then finished should be ‘true’ and we should never override the objectId part of the RESTWhere?

percypyan · 2019-01-03T19:12:34Z

For a simple equals yes, but for $in, it’s an array that can contains more items than the limit !

percypyan · 2019-01-03T19:15:41Z

And it’s the case the when you have a bunch of roles for a user ! (More than the default limit of 100 in our case)

dplewis · 2019-01-03T19:47:15Z

After a quick glance and removing your fix

but for $in, it’s an array that can contains more items than the limit

The first loop returns a result

{ objectId: { '$in': [ '...' ] } }

The second loop returns a result

{ objectId: { '$gt': '...' } }

You could stop at the first loop since your limit is 1

finished = results.length < restOptions.limit; change to <= also solves your issue.

There aren't a lot of tests for RestQuery.each so maybe more are needed were the number of results don't match the limit.

percypyan · 2019-01-04T01:39:30Z

Hum ok, I see what you mean. But in each, if we look at the current finished condition, and look at the fact that this function is used to retrieve the roles of a user in the purpose to check the read and/or write access on the result of a query, I’ve assumed that the limit here is only used to fragment the request in smaller pieces.
If it’s not, what is the purpose of a continueWhile ? Only one request is needed to retrieve all the results for that limit, isn’t it ?
But maybe I’m wrong, in what case I think that something is probably broken somewhere else since the roles of an user cannot be properly retrieved if there no other filter than the $gt

percypyan · 2019-01-04T01:41:02Z

Oh ! Ok, maybe you just mean I haven’t write enough tests ?
Sorry, I think I’ve misunderstood the purpose of your comment at the first read 😋

flovilmart · 2019-01-04T01:46:36Z

I believe what @dplewis refers to is that you mention issues with users and roles, yet no test show the issue with such users and roles.

dplewis · 2019-01-04T01:49:05Z

After playing around a bit, either something is broken or will be broken. How did you discover this? Did you have to use RestQuery.each directly or can you reproduce with the JS SDK?

flovilmart · 2019-01-04T01:55:20Z

It’s likely the bug can be found if the users appear in many many roles, perhaps there’s a bug there.

percypyan · 2019-01-04T08:15:12Z

I discovered the bug cause we have many roles by users (2000+ for the user with wich I have discovered the issue). The query through the Parse js SDK to our parse server timed out each time and I finally discovered that this issue existed. The user tried to retrieve a lot more roles from the database and of course, timed out

percypyan · 2019-01-04T09:31:15Z

The test I've written is the simplest way to reproduce the bug without creating a user and many many roles. But in fact, exactly the same thing happen for the two situation: a relation query is converted into $in query by RestQuery.execute and then overriden at the end of the first iteration (except if there is only one) of the RestQuery.each. With the limit of 1, for a relation A-B, only 2 A objects and 2 B objects are required to make one of 2 query fail each time.

For A1 <-> B1 and A2 <-> B2.
For A1.id > A2.id.

Query all A related to B1: will pass because { $gt: A1.id } will not include A2.id either.
Query all A related to B2: will not pass because { $gt: A2.id } will return A1 which is not expected.

And if A1.id < A2.id query 1 will fail and query 2 will pass.

dplewis · 2019-01-04T17:08:32Z

Here is a failing test (modified from Auth.spec.js) but passes with this fix.

fit('should load all roles for different users with config', async () => {
      const rolesNumber = 100;
      const user = new Parse.User();
      await user.signUp({
        username: 'hello',
        password: 'password',
      });
      const user2 = new Parse.User();
      await user2.signUp({
        username: 'world',
        password: '1234',
      });
      expect(user.getSessionToken()).not.toBeUndefined();
      const userAuth = await getAuthForSessionToken({
        sessionToken: user.getSessionToken(),
        config: Config.get('test'),
      });
      const user2Auth = await getAuthForSessionToken({
        sessionToken: user2.getSessionToken(),
        config: Config.get('test'),
      });
      const roles = [];
      for (let i = 0; i < rolesNumber; i += 1){
        const acl = new Parse.ACL();
        const role = new Parse.Role("roleloadtest" + i, acl);
        const role2 = new Parse.Role("role2loadtest" + i, acl);
        role.getUsers().add([user]);
        role2.getUsers().add([user2]);
        roles.push(role.save());
        roles.push(role2.save());
      }
      const savedRoles = await Promise.all(roles);
      expect(savedRoles.length).toBe(rolesNumber * 2);
      const cloudRoles = await userAuth.getRolesForUser();
      const cloudRoles2 = await user2Auth.getRolesForUser();
      expect(cloudRoles.length).toBe(rolesNumber);
      expect(cloudRoles2.length).toBe(rolesNumber);
    });

The fix provided allows .each to only query on roles for this specific user instead of all roles

percypyan · 2019-01-04T17:14:17Z

Ok @dplewis, thanks I'm adding it.

dplewis · 2019-01-04T17:15:30Z

spec/RestQuery.spec.js

+
+    const config = Config.get('test');
+
+    /* Two query needed since objectId are sorted and we can't know wich one


Typos and formatting in this comment needs to be fixed

percypyan · 2019-01-04T17:33:08Z

All ok for you? @dplewis

dplewis · 2019-01-04T17:34:24Z

spec/Auth.spec.js

      }
      const savedRoles = await Promise.all(roles);
      expect(savedRoles.length).toBe(rolesNumber);
      const cloudRoles = await userAuth.getRolesForUser();
      expect(cloudRoles.length).toBe(rolesNumber);
    });
+
+    fit('should load all roles for different users with config', async () => {


Remove fit. That makes only this test run and ignore all other.

Run it locally also

Oh shit, I've ran it locally but forget to remove the fit... Sorry

dplewis · 2019-01-04T17:45:54Z

@flovilmart How does this look?

flovilmart · 2019-01-04T19:23:14Z

Thanks for this!

…-community#5276) * Add test on RestQuery.each with relation * Fix the failing test for RestQuery.each and relations * Add test for getRolesForUser * Fix format for comment * Remove extra fit

percypyan added 2 commits January 3, 2019 10:46

Add test on RestQuery.each with relation

af1fa16

Fix the failing test for RestQuery.each and relations

d18d269

percypyan mentioned this pull request Jan 3, 2019

RestQuery.each override queries constraints on objectId field #5274

Closed

flovilmart approved these changes Jan 3, 2019

View reviewed changes

dplewis reviewed Jan 4, 2019

View reviewed changes

percypyan added 2 commits January 4, 2019 18:30

Add test for getRolesForUser

b64959e

Fix format for comment

0df70ab

dplewis suggested changes Jan 4, 2019

View reviewed changes

Remove extra fit

a3c9330

dplewis approved these changes Jan 4, 2019

View reviewed changes

flovilmart approved these changes Jan 4, 2019

View reviewed changes

flovilmart merged commit 9f2fc88 into parse-community:master Jan 4, 2019

percypyan deleted the bugfix/each#5274 branch January 5, 2019 17:43

snyk-bot mentioned this pull request Jun 9, 2023

refactor: Upgrade @graphql-tools/schema from 9.0.4 to 10.0.0 #8625

Closed

This was referenced Jun 10, 2023

refactor: Upgrade @graphql-tools/utils from 8.12.0 to 10.0.0 #8630

Closed

refactor: Upgrade @graphql-tools/merge from 8.4.1 to 9.0.0 #8635

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #5274 on RestQuery.each and relations #5276

Fix issue #5274 on RestQuery.each and relations #5276

percypyan commented Jan 3, 2019 •

edited by dplewis

Loading

codecov bot commented Jan 3, 2019 •

edited

Loading

flovilmart left a comment

percypyan commented Jan 3, 2019

percypyan commented Jan 3, 2019

dplewis commented Jan 3, 2019 •

edited

Loading

percypyan commented Jan 4, 2019

percypyan commented Jan 4, 2019 •

edited

Loading

flovilmart commented Jan 4, 2019

dplewis commented Jan 4, 2019

flovilmart commented Jan 4, 2019

percypyan commented Jan 4, 2019

percypyan commented Jan 4, 2019

dplewis commented Jan 4, 2019 •

edited

Loading

percypyan commented Jan 4, 2019

dplewis Jan 4, 2019

percypyan commented Jan 4, 2019

dplewis Jan 4, 2019 •

edited

Loading

percypyan Jan 4, 2019

dplewis Jan 4, 2019

percypyan Jan 4, 2019

dplewis commented Jan 4, 2019

flovilmart commented Jan 4, 2019


		const config = Config.get('test');

		/* Two query needed since objectId are sorted and we can't know wich one

Fix issue #5274 on RestQuery.each and relations #5276

Fix issue #5274 on RestQuery.each and relations #5276

Conversation

percypyan commented Jan 3, 2019 • edited by dplewis Loading

codecov bot commented Jan 3, 2019 • edited Loading

Codecov Report

flovilmart left a comment

Choose a reason for hiding this comment

percypyan commented Jan 3, 2019

percypyan commented Jan 3, 2019

dplewis commented Jan 3, 2019 • edited Loading

percypyan commented Jan 4, 2019

percypyan commented Jan 4, 2019 • edited Loading

flovilmart commented Jan 4, 2019

dplewis commented Jan 4, 2019

flovilmart commented Jan 4, 2019

percypyan commented Jan 4, 2019

percypyan commented Jan 4, 2019

dplewis commented Jan 4, 2019 • edited Loading

percypyan commented Jan 4, 2019

dplewis Jan 4, 2019

Choose a reason for hiding this comment

percypyan commented Jan 4, 2019

dplewis Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

percypyan Jan 4, 2019

Choose a reason for hiding this comment

dplewis Jan 4, 2019

Choose a reason for hiding this comment

percypyan Jan 4, 2019

Choose a reason for hiding this comment

dplewis commented Jan 4, 2019

flovilmart commented Jan 4, 2019

percypyan commented Jan 3, 2019 •

edited by dplewis

Loading

codecov bot commented Jan 3, 2019 •

edited

Loading

dplewis commented Jan 3, 2019 •

edited

Loading

percypyan commented Jan 4, 2019 •

edited

Loading

dplewis commented Jan 4, 2019 •

edited

Loading

dplewis Jan 4, 2019 •

edited

Loading