Optimizing pointer CLP query decoration done by DatabaseController#addPointerPermissions #6747

mess-lelouch · 2020-06-24T00:05:47Z

This change implements what was suggested in the following issue: #6708 (comment)

Basically, the code added looks for the type of the CLP pointers and decorates the query accordingly if the field is of type Pointer or Array. Also, it only introduces an $or query if there are multiple CLP pointer entries.

Below is the explain output of a simple find query when only a pointer CLP entry user is present

    "parsedQuery": {
      "$and": [
        {
          "_p_user": {
            "$eq": "_User$Qve7aWMSZo"
          }
        },
        {
          "_rperm": {
            "$in": [
              null,
              "*",
              "Qve7aWMSZo"
            ]
          }
        }
      ]
    },

Below is the explain output of a simple find query when only an array CLP entry users is present

    "parsedQuery": {
      "$and": [
        {
          "users": {
            "$eq": {
              "__type": "Pointer",
              "className": "_User",
              "objectId": "Qve7aWMSZo"
            }
          }
        },
        {
          "_rperm": {
            "$in": [
              null,
              "*",
              "Qve7aWMSZo"
            ]
          }
        }
      ]
    },

Below is the explain output of a simple find query when pointer/array CLP entries user/users are present

    "parsedQuery": {
      "$and": [
        {
          "$or": [
            {
              "_p_user": {
                "$eq": "_User$Qve7aWMSZo"
              }
            },
            {
              "users": {
                "$eq": {
                  "__type": "Pointer",
                  "className": "_User",
                  "objectId": "Qve7aWMSZo"
                }
              }
            }
          ]
        },
        {
          "_rperm": {
            "$in": [
              null,
              "*",
              "Qve7aWMSZo"
            ]
          }
        }
      ]
    },

Please feel free to share any questions, concerns or observations that would help me improve this PR.

mess-lelouch · 2020-06-24T00:26:14Z

src/Controllers/DatabaseController.js

-        };
+      const ors = permFields.map((key) => {
+        const fieldDescriptor = schema.getExpectedType(className, key);
+        const fieldType = fieldDescriptor.type;


The CI tests failed because I can get null/undefined/string in fieldDescriptor. Basically I assumed that I could only get a descriptor with a type of Array or Pointer, but it seems this is not the case. I will investigate this issue further and revisit this code. If anyone can point me in the right direction as to why I can get null/undefined or a string as the pointer CLP I will appreciate it.

I think I realized that the CI build failed because of the flow type checker and I know why. I am still getting used to flow since I haven't used it before.

mess-lelouch · 2020-06-24T20:02:25Z

I just added some tests...

codecov · 2020-06-24T20:12:27Z

Codecov Report

Merging #6747 into master will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6747      +/-   ##
==========================================
- Coverage   93.90%   93.83%   -0.08%     
==========================================
  Files         169      169              
  Lines       12197    12267      +70     
==========================================
+ Hits        11454    11511      +57     
- Misses        743      756      +13

Impacted Files	Coverage Δ
src/Controllers/DatabaseController.js	`95.33% <100.00%> (+0.05%)`	⬆️
src/GraphQL/loaders/usersMutations.js	`91.73% <0.00%> (-1.51%)`	⬇️
src/GraphQL/loaders/usersQueries.js	`96.42% <0.00%> (-1.08%)`	⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`92.87% <0.00%> (-0.67%)`	⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`96.02% <0.00%> (-0.32%)`	⬇️

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 ea1ec9b...01d1812. Read the comment docs.

mess-lelouch · 2020-06-24T21:26:10Z

I am going to ignore the code coverage errors for now. It was not failing until my last push to the branch, which added tests to cover areas that weren't previously covered. It did not failed when I originally created the PR.

mess-lelouch · 2020-07-07T16:49:26Z

@mtrezza I apologize in advance for tagging you. I do have one question to ask and I didn't know where to ask.

What actions do I need to take to get this work reviewed? Asking since this PR has been up for a while and I do not know if I need to take some additional actions so that my work starts to get reviewed.

If I just need to wait for a maintainer to review my work, then I do not mind the wait :)

Thanks in advance!

mtrezza · 2020-07-07T17:06:24Z

@mess-lelouch Thanks for your contribution and apologies for the wait. I remember the analysis of this issue this PR intends to fix. Let me look into this again in some more detail. In the mean time, can you please make sure (if you haven't already) that we have a test case for every possible scenario:

pointer: _User$...
literal pointer object: {"__type": "Pointer", ... }
single / multiple pointers references (?)
...?

Also, can you please make sure there are no (lint) changes in the PR of unrelated code, so we can focus on the actual change.

mess-lelouch · 2020-07-07T17:19:23Z

@mess-lelouch Thanks for your contribution and apologies for the wait. I remember the analysis of this issue this PR intends to fix. Let me look into this again in some more detail. In the mean time, can you please make sure (if you haven't already) that we have a test case for every possible scenario:

pointer: _User$...

literal pointer object: {"__type": "Pointer", ... }

single / multiple pointers references (?)

...?

Also, can you please make sure there are no (lint) changes in the PR of unrelated code, so we can focus on the actual change.

No need to apologize. On the contrary, thank you for dedicating your time to maintaining this piece of software that I use.

I added test cases for everything you mentioned except for the literal object case. And the reason I didn't is because I assumed it was not supported since the dashboard does not let me set a field of type Object in the CLP. It only lets me set user pointers and (as of recently) arrays.

That said, I could easily add it if you think it is necessary (i.e. due to backwards compatibility reasons).

mtrezza · 2020-07-07T17:35:33Z

That said, I could easily add it if you think it is necessary (i.e. due to backwards compatibility reasons).

Exactly because of that - IIRC I made an assumption as to why literal pointer objects are supported, but there was no definitive conclusion, so to be safe I suggest we add this for backwards compatibility and add a code comment for future contributors, so we may remove that code at some point in the future. Unless you want to do some more research regarding this to remove this on more solid ground.

mess-lelouch · 2020-07-07T17:42:39Z

That said, I could easily add it if you think it is necessary (i.e. due to backwards compatibility reasons).

Exactly because of that - IIRC I made an assumption as to why literal pointer objects are supported, but there was no definitive conclusion, so to be safe I suggest we add this for backwards compatibility and add a code comment for future contributors, so we may remove that code at some point in the future. Unless you want to do some more research regarding this to remove this on more solid ground.

Makes sense... I think I will save myself some time and add it.

mess-lelouch · 2020-07-08T03:24:52Z

@mtrezza ok, I think that with my latest changes I now handle the Object case properly. I updated the tests as well.

dplewis

LGTM! @BufferUnderflower Can you look this over?

davimacedo

It looks good to me.

dplewis · 2020-07-17T17:22:36Z

Travis didn't rerun, I'll close and reopen.

Musa Yassin-Fort added 3 commits June 23, 2020 15:25

Optimize CLP pointer query

f3e8bb2

remove console log

649c37b

Update changelog

bfad56a

mess-lelouch commented Jun 24, 2020

View reviewed changes

Musa Yassin-Fort and others added 2 commits June 23, 2020 20:49

Fix flow type checker issues

42c2dd8

Remove unused properties

b86d7eb

Fix typo, add one more test case for coverage

241256a

Add support for CLP entry of type Object

ea1a478

dplewis approved these changes Jul 13, 2020

View reviewed changes

Merge branch 'master' into feature/optimize-pointer-queries

01d1812

dplewis requested review from davimacedo and mtrezza July 17, 2020 14:33

davimacedo approved these changes Jul 17, 2020

View reviewed changes

dplewis closed this Jul 17, 2020

dplewis reopened this Jul 17, 2020

dplewis merged commit d698333 into parse-community:master Jul 17, 2020

mtrezza mentioned this pull request Jul 27, 2020

Simple Parse query translated to a mongodb "or" query #6708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimizing pointer CLP query decoration done by DatabaseController#addPointerPermissions #6747

Optimizing pointer CLP query decoration done by DatabaseController#addPointerPermissions #6747

mess-lelouch commented Jun 24, 2020 •

edited

Loading

mess-lelouch Jun 24, 2020

mess-lelouch Jun 24, 2020

mess-lelouch Jun 24, 2020

mess-lelouch commented Jun 24, 2020

codecov bot commented Jun 24, 2020 •

edited

Loading

mess-lelouch commented Jun 24, 2020

mess-lelouch commented Jul 7, 2020

mtrezza commented Jul 7, 2020 •

edited

Loading

mess-lelouch commented Jul 7, 2020 •

edited

Loading

mtrezza commented Jul 7, 2020 •

edited

Loading

mess-lelouch commented Jul 7, 2020

mess-lelouch commented Jul 8, 2020

dplewis left a comment

davimacedo left a comment

dplewis commented Jul 17, 2020

Optimizing pointer CLP query decoration done by DatabaseController#addPointerPermissions #6747

Optimizing pointer CLP query decoration done by DatabaseController#addPointerPermissions #6747

Conversation

mess-lelouch commented Jun 24, 2020 • edited Loading

mess-lelouch Jun 24, 2020

Choose a reason for hiding this comment

mess-lelouch Jun 24, 2020

Choose a reason for hiding this comment

mess-lelouch Jun 24, 2020

Choose a reason for hiding this comment

mess-lelouch commented Jun 24, 2020

codecov bot commented Jun 24, 2020 • edited Loading

Codecov Report

mess-lelouch commented Jun 24, 2020

mess-lelouch commented Jul 7, 2020

mtrezza commented Jul 7, 2020 • edited Loading

mess-lelouch commented Jul 7, 2020 • edited Loading

mtrezza commented Jul 7, 2020 • edited Loading

mess-lelouch commented Jul 7, 2020

mess-lelouch commented Jul 8, 2020

dplewis left a comment

Choose a reason for hiding this comment

davimacedo left a comment

Choose a reason for hiding this comment

dplewis commented Jul 17, 2020

mess-lelouch commented Jun 24, 2020 •

edited

Loading

codecov bot commented Jun 24, 2020 •

edited

Loading

mtrezza commented Jul 7, 2020 •

edited

Loading

mess-lelouch commented Jul 7, 2020 •

edited

Loading

mtrezza commented Jul 7, 2020 •

edited

Loading