From ddaa32f0288700f267df4e3519138a0a8d5ad676 Mon Sep 17 00:00:00 2001 From: Pedro Diaz Date: Sat, 12 Dec 2020 05:54:54 +0100 Subject: [PATCH 1/9] Optimize redundant logic used in queries --- src/Controllers/DatabaseController.js | 38 ++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 21ba2e9477..c7c0fc1b4a 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1365,6 +1365,32 @@ class DatabaseController { }); } + objectToEntriesStrings(query: any): Array { + return Object.entries(query).map(a => a.map(s => JSON.stringify(s)).join(':')); + } + + reduceOrOperation(query: { $or: Array }): any { + const queries = query.$or.map(q => this.objectToEntriesStrings(q)); + for (let i = 0; i < queries.length - 1; i++) { + for (let j = i + 1; j < queries.length; j++) { + const [shorter, longer] = queries[i].length > queries[j].length ? [j, i] : [i, j]; + const foundEntries = queries[longer].reduce( + (acc, entry) => acc + (queries[shorter].includes(entry) ? 1 : 0), + 0 + ); + if (foundEntries === queries[shorter].length) { + // If the shorter query is completely contained on the longer one, we can skip it. + query.$or.splice(longer, 1); + queries.splice(longer, 1); + } + } + } + + if (query.$or.length === 1) return query.$or[0]; + + return query; + } + // Constraints query using CLP's pointer permissions (PP) if any. // 1. Etract the user id from caller's ACLgroup; // 2. Exctract a list of field names that are PP for target collection and operation; @@ -1448,13 +1474,23 @@ class DatabaseController { } // if we already have a constraint on the key, use the $and if (Object.prototype.hasOwnProperty.call(query, key)) { + const queryClauseEntries = this.objectToEntriesStrings(queryClause); + const queryEntries = this.objectToEntriesStrings(query); + const foundEntries = queryEntries.reduce( + (acc, entry) => acc + (queryClauseEntries.includes(entry) ? 1 : 0), + 0 + ); + if (foundEntries === queryClauseEntries.length) { + return query; + } + return { $and: [queryClause, query] }; } // otherwise just add the constaint return Object.assign({}, query, queryClause); }); - return queries.length === 1 ? queries[0] : { $or: queries }; + return queries.length === 1 ? queries[0] : this.reduceOrOperation({ $or: queries }); } else { return query; } From 0571340457ce64ea7356f19e5bfdb6a0c8a1edbc Mon Sep 17 00:00:00 2001 From: Pedro Diaz Date: Sat, 12 Dec 2020 06:39:50 +0100 Subject: [PATCH 2/9] Added CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9882230ba1..9685b4562c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ### master [Full Changelog](https://github.com/parse-community/parse-server/compare/4.4.0...master) +- IMPROVE: Optimize queries on classes with pointer permissions. ### 4.4.0 [Full Changelog](https://github.com/parse-community/parse-server/compare/4.3.0...4.4.0) From 7bcbd6f22087ebaba15a97a31a28e59771e1347b Mon Sep 17 00:00:00 2001 From: Pedro Diaz Date: Sat, 12 Dec 2020 14:15:04 +0100 Subject: [PATCH 3/9] Fixed comments and code style after recommendations. --- CHANGELOG.md | 2 +- src/Controllers/DatabaseController.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9685b4562c..2f21f44ec2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### master [Full Changelog](https://github.com/parse-community/parse-server/compare/4.4.0...master) -- IMPROVE: Optimize queries on classes with pointer permissions. +- IMPROVE: Optimize queries on classes with pointer permissions. [#7061](https://github.com/parse-community/parse-server/pull/7061). Thanks to [Pedro Diaz](https://github.com/pdiaz) ### 4.4.0 [Full Changelog](https://github.com/parse-community/parse-server/compare/4.3.0...4.4.0) diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index c7c0fc1b4a..1be01695fc 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1385,9 +1385,9 @@ class DatabaseController { } } } - - if (query.$or.length === 1) return query.$or[0]; - + if (query.$or.length === 1) { + return query.$or[0]; + } return query; } From da746f98dba6c474e819f39153cf061ee15dfbe1 Mon Sep 17 00:00:00 2001 From: Pedro Diaz Date: Sat, 12 Dec 2020 14:17:50 +0100 Subject: [PATCH 4/9] Fixed code style after recommendation. --- src/Controllers/DatabaseController.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 1be01695fc..a0cfcd98ec 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1483,7 +1483,6 @@ class DatabaseController { if (foundEntries === queryClauseEntries.length) { return query; } - return { $and: [queryClause, query] }; } // otherwise just add the constaint From dabf6924a888d5ec1bfd666af5fee5767d27626a Mon Sep 17 00:00:00 2001 From: Pedro Diaz Date: Sat, 12 Dec 2020 14:40:39 +0100 Subject: [PATCH 5/9] Improved explanation in comments --- src/Controllers/DatabaseController.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index a0cfcd98ec..cb43161502 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1365,6 +1365,9 @@ class DatabaseController { }); } + // This helps to create intermediate objects for simpler comparison of + // key value pairs used in query objects. Each key value pair will represented + // in a similar way to json objectToEntriesStrings(query: any): Array { return Object.entries(query).map(a => a.map(s => JSON.stringify(s)).join(':')); } @@ -1374,12 +1377,13 @@ class DatabaseController { for (let i = 0; i < queries.length - 1; i++) { for (let j = i + 1; j < queries.length; j++) { const [shorter, longer] = queries[i].length > queries[j].length ? [j, i] : [i, j]; - const foundEntries = queries[longer].reduce( - (acc, entry) => acc + (queries[shorter].includes(entry) ? 1 : 0), + const foundEntries = queries[shorter].reduce( + (acc, entry) => acc + (queries[longer].includes(entry) ? 1 : 0), 0 ); if (foundEntries === queries[shorter].length) { - // If the shorter query is completely contained on the longer one, we can skip it. + // If the shorter query is completely contained in the longer one, we can strike + // out the longer query. query.$or.splice(longer, 1); queries.splice(longer, 1); } From 02d6b16c7eaa04f628240f32b00cd4325def4cbe Mon Sep 17 00:00:00 2001 From: Pedro Diaz Date: Sun, 13 Dec 2020 04:45:39 +0100 Subject: [PATCH 6/9] Added tests to for logic optimizations --- spec/DatabaseController.spec.js | 43 +++++++++++++++++++++++++++ src/Controllers/DatabaseController.js | 40 ++++++++++++++++++------- 2 files changed, 73 insertions(+), 10 deletions(-) diff --git a/spec/DatabaseController.spec.js b/spec/DatabaseController.spec.js index 988248c891..ebd20f90d1 100644 --- a/spec/DatabaseController.spec.js +++ b/spec/DatabaseController.spec.js @@ -265,6 +265,49 @@ describe('DatabaseController', function () { done(); }); }); + + describe('reduceOperations', function () { + const databaseController = new DatabaseController(); + + it('objectToEntriesStrings', done => { + const output = databaseController.objectToEntriesStrings({ a: 1, b: 2, c: 3 }); + expect(output).toEqual(['"a":1', '"b":2', '"c":3']); + done(); + }); + + it('reduceOrOperation', done => { + expect(databaseController.reduceOrOperation({ $or: [{ a: 1 }, { b: 2 }] })).toEqual({ + $or: [{ a: 1 }, { b: 2 }], + }); + expect(databaseController.reduceOrOperation({ $or: [{ a: 1 }, { a: 2 }] })).toEqual({ + $or: [{ a: 1 }, { a: 2 }], + }); + expect(databaseController.reduceOrOperation({ $or: [{ a: 1 }, { a: 1 }] })).toEqual({ a: 1 }); + expect( + databaseController.reduceOrOperation({ $or: [{ a: 1, b: 2, c: 3 }, { a: 1 }] }) + ).toEqual({ a: 1 }); + expect( + databaseController.reduceOrOperation({ $or: [{ b: 2 }, { a: 1, b: 2, c: 3 }] }) + ).toEqual({ b: 2 }); + done(); + }); + + it('reduceAndOperation', done => { + expect(databaseController.reduceAndOperation({ $and: [{ a: 1 }, { b: 2 }] })).toEqual({ + $and: [{ a: 1 }, { b: 2 }], + }); + expect(databaseController.reduceAndOperation({ $and: [{ a: 1 }, { a: 2 }] })).toEqual({ + $and: [{ a: 1 }, { a: 2 }], + }); + expect(databaseController.reduceAndOperation({ $and: [{ a: 1 }, { a: 1 }] })).toEqual({ + a: 1, + }); + expect( + databaseController.reduceAndOperation({ $and: [{ a: 1, b: 2, c: 3 }, { b: 2 }] }) + ).toEqual({ a: 1, b: 2, c: 3 }); + done(); + }); + }); }); function buildCLP(pointerNames) { diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index cb43161502..556b2dfb80 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1373,6 +1373,9 @@ class DatabaseController { } reduceOrOperation(query: { $or: Array }): any { + if (!query.$or) { + return query; + } const queries = query.$or.map(q => this.objectToEntriesStrings(q)); for (let i = 0; i < queries.length - 1; i++) { for (let j = i + 1; j < queries.length; j++) { @@ -1395,6 +1398,32 @@ class DatabaseController { return query; } + reduceAndOperation(query: { $and: Array }): any { + if (!query.$and) { + return query; + } + const queries = query.$and.map(q => this.objectToEntriesStrings(q)); + for (let i = 0; i < queries.length - 1; i++) { + for (let j = i + 1; j < queries.length; j++) { + const [shorter, longer] = queries[i].length > queries[j].length ? [j, i] : [i, j]; + const foundEntries = queries[shorter].reduce( + (acc, entry) => acc + (queries[longer].includes(entry) ? 1 : 0), + 0 + ); + if (foundEntries === queries[shorter].length) { + // If the shorter query is completely contained in the longer one, we can strike + // out the shorter query. + query.$and.splice(shorter, 1); + queries.splice(shorter, 1); + } + } + } + if (query.$and.length === 1) { + return query.$and[0]; + } + return query; + } + // Constraints query using CLP's pointer permissions (PP) if any. // 1. Etract the user id from caller's ACLgroup; // 2. Exctract a list of field names that are PP for target collection and operation; @@ -1478,16 +1507,7 @@ class DatabaseController { } // if we already have a constraint on the key, use the $and if (Object.prototype.hasOwnProperty.call(query, key)) { - const queryClauseEntries = this.objectToEntriesStrings(queryClause); - const queryEntries = this.objectToEntriesStrings(query); - const foundEntries = queryEntries.reduce( - (acc, entry) => acc + (queryClauseEntries.includes(entry) ? 1 : 0), - 0 - ); - if (foundEntries === queryClauseEntries.length) { - return query; - } - return { $and: [queryClause, query] }; + return this.reduceAndOperation({ $and: [queryClause, query] }); } // otherwise just add the constaint return Object.assign({}, query, queryClause); From af9f91cf77489c8fb9dc220cf18c05a8b88a3122 Mon Sep 17 00:00:00 2001 From: Pedro Diaz Date: Sun, 13 Dec 2020 12:44:49 +0100 Subject: [PATCH 7/9] Added two test cases more and some comments --- spec/DatabaseController.spec.js | 2 ++ src/Controllers/DatabaseController.js | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/spec/DatabaseController.spec.js b/spec/DatabaseController.spec.js index ebd20f90d1..fb5b6aaac8 100644 --- a/spec/DatabaseController.spec.js +++ b/spec/DatabaseController.spec.js @@ -276,6 +276,7 @@ describe('DatabaseController', function () { }); it('reduceOrOperation', done => { + expect(databaseController.reduceOrOperation({ a: 1 })).toEqual({ a: 1 }); expect(databaseController.reduceOrOperation({ $or: [{ a: 1 }, { b: 2 }] })).toEqual({ $or: [{ a: 1 }, { b: 2 }], }); @@ -293,6 +294,7 @@ describe('DatabaseController', function () { }); it('reduceAndOperation', done => { + expect(databaseController.reduceAndOperation({ a: 1 })).toEqual({ a: 1 }); expect(databaseController.reduceAndOperation({ $and: [{ a: 1 }, { b: 2 }] })).toEqual({ $and: [{ a: 1 }, { b: 2 }], }); diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 556b2dfb80..58648d4c27 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1372,6 +1372,7 @@ class DatabaseController { return Object.entries(query).map(a => a.map(s => JSON.stringify(s)).join(':')); } + // Naive logic reducer for OR operations meant to be used only for pointer permissions. reduceOrOperation(query: { $or: Array }): any { if (!query.$or) { return query; @@ -1393,11 +1394,13 @@ class DatabaseController { } } if (query.$or.length === 1) { - return query.$or[0]; + query = { ...query, ...query.$or[0] }; + delete query.$or; } return query; } + // Naive logic reducer for AND operations meant to be used only for pointer permissions. reduceAndOperation(query: { $and: Array }): any { if (!query.$and) { return query; @@ -1419,7 +1422,8 @@ class DatabaseController { } } if (query.$and.length === 1) { - return query.$and[0]; + query = { ...query, ...query.$and[0] }; + delete query.$and; } return query; } From cc55fed8ba66ec657e4d02a985f12405d2ebaf09 Mon Sep 17 00:00:00 2001 From: Pedro Diaz Date: Mon, 14 Dec 2020 18:25:26 +0100 Subject: [PATCH 8/9] Added extra test cases and fixed issue found with them. --- spec/DatabaseController.spec.js | 59 ++++++++++++++++++++++++ src/Controllers/DatabaseController.js | 66 ++++++++++++++++----------- 2 files changed, 99 insertions(+), 26 deletions(-) diff --git a/spec/DatabaseController.spec.js b/spec/DatabaseController.spec.js index fb5b6aaac8..e087ec494f 100644 --- a/spec/DatabaseController.spec.js +++ b/spec/DatabaseController.spec.js @@ -236,6 +236,65 @@ describe('DatabaseController', function () { done(); }); + it('should not return a $or operation if the query involves one of the two fields also used as array/pointer permissions', done => { + const clp = buildCLP(['users', 'user']); + const query = { a: 'b', user: createUserPointer(USER_ID) }; + + schemaController.testPermissionsForClassName + .withArgs(CLASS_NAME, ACL_GROUP, OPERATION) + .and.returnValue(false); + schemaController.getClassLevelPermissions.withArgs(CLASS_NAME).and.returnValue(clp); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'user') + .and.returnValue({ type: 'Pointer' }); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'users') + .and.returnValue({ type: 'Array' }); + + const output = databaseController.addPointerPermissions( + schemaController, + CLASS_NAME, + OPERATION, + query, + ACL_GROUP + ); + + expect(output).toEqual({ ...query, user: createUserPointer(USER_ID) }); + + done(); + }); + + it('should not return a $or operation if the query involves one of the fields also used as array/pointer permissions', done => { + const clp = buildCLP(['user', 'users', 'userObject']); + const query = { a: 'b', user: createUserPointer(USER_ID) }; + + schemaController.testPermissionsForClassName + .withArgs(CLASS_NAME, ACL_GROUP, OPERATION) + .and.returnValue(false); + schemaController.getClassLevelPermissions.withArgs(CLASS_NAME).and.returnValue(clp); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'user') + .and.returnValue({ type: 'Pointer' }); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'users') + .and.returnValue({ type: 'Array' }); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'userObject') + .and.returnValue({ type: 'Object' }); + + const output = databaseController.addPointerPermissions( + schemaController, + CLASS_NAME, + OPERATION, + query, + ACL_GROUP + ); + + expect(output).toEqual({ ...query, user: createUserPointer(USER_ID) }); + + done(); + }); + it('should throw an error if for some unexpected reason the property specified in the CLP is neither a pointer nor an array', done => { const clp = buildCLP(['user']); const query = { a: 'b' }; diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 58648d4c27..6b132dcbe0 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1378,21 +1378,28 @@ class DatabaseController { return query; } const queries = query.$or.map(q => this.objectToEntriesStrings(q)); - for (let i = 0; i < queries.length - 1; i++) { - for (let j = i + 1; j < queries.length; j++) { - const [shorter, longer] = queries[i].length > queries[j].length ? [j, i] : [i, j]; - const foundEntries = queries[shorter].reduce( - (acc, entry) => acc + (queries[longer].includes(entry) ? 1 : 0), - 0 - ); - if (foundEntries === queries[shorter].length) { - // If the shorter query is completely contained in the longer one, we can strike - // out the longer query. - query.$or.splice(longer, 1); - queries.splice(longer, 1); + let repeat = false; + do { + repeat = false; + for (let i = 0; i < queries.length - 1; i++) { + for (let j = i + 1; j < queries.length; j++) { + const [shorter, longer] = queries[i].length > queries[j].length ? [j, i] : [i, j]; + const foundEntries = queries[shorter].reduce( + (acc, entry) => acc + (queries[longer].includes(entry) ? 1 : 0), + 0 + ); + const shorterEntries = queries[shorter].length; + if (foundEntries === shorterEntries) { + // If the shorter query is completely contained in the longer one, we can strike + // out the longer query. + query.$or.splice(longer, 1); + queries.splice(longer, 1); + repeat = true; + break; + } } } - } + } while (repeat); if (query.$or.length === 1) { query = { ...query, ...query.$or[0] }; delete query.$or; @@ -1406,21 +1413,28 @@ class DatabaseController { return query; } const queries = query.$and.map(q => this.objectToEntriesStrings(q)); - for (let i = 0; i < queries.length - 1; i++) { - for (let j = i + 1; j < queries.length; j++) { - const [shorter, longer] = queries[i].length > queries[j].length ? [j, i] : [i, j]; - const foundEntries = queries[shorter].reduce( - (acc, entry) => acc + (queries[longer].includes(entry) ? 1 : 0), - 0 - ); - if (foundEntries === queries[shorter].length) { - // If the shorter query is completely contained in the longer one, we can strike - // out the shorter query. - query.$and.splice(shorter, 1); - queries.splice(shorter, 1); + let repeat = false; + do { + repeat = false; + for (let i = 0; i < queries.length - 1; i++) { + for (let j = i + 1; j < queries.length; j++) { + const [shorter, longer] = queries[i].length > queries[j].length ? [j, i] : [i, j]; + const foundEntries = queries[shorter].reduce( + (acc, entry) => acc + (queries[longer].includes(entry) ? 1 : 0), + 0 + ); + const shorterEntries = queries[shorter].length; + if (foundEntries === shorterEntries) { + // If the shorter query is completely contained in the longer one, we can strike + // out the shorter query. + query.$and.splice(shorter, 1); + queries.splice(shorter, 1); + repeat = true; + break; + } } } - } + } while (repeat); if (query.$and.length === 1) { query = { ...query, ...query.$and[0] }; delete query.$and; From d06201d83efb5c22e796debf45eea3a2ce022695 Mon Sep 17 00:00:00 2001 From: Pedro Diaz Date: Mon, 14 Dec 2020 20:45:02 +0100 Subject: [PATCH 9/9] Removed empty lines as requested. --- spec/DatabaseController.spec.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/spec/DatabaseController.spec.js b/spec/DatabaseController.spec.js index e087ec494f..98103ce6e4 100644 --- a/spec/DatabaseController.spec.js +++ b/spec/DatabaseController.spec.js @@ -239,7 +239,6 @@ describe('DatabaseController', function () { it('should not return a $or operation if the query involves one of the two fields also used as array/pointer permissions', done => { const clp = buildCLP(['users', 'user']); const query = { a: 'b', user: createUserPointer(USER_ID) }; - schemaController.testPermissionsForClassName .withArgs(CLASS_NAME, ACL_GROUP, OPERATION) .and.returnValue(false); @@ -250,7 +249,6 @@ describe('DatabaseController', function () { schemaController.getExpectedType .withArgs(CLASS_NAME, 'users') .and.returnValue({ type: 'Array' }); - const output = databaseController.addPointerPermissions( schemaController, CLASS_NAME, @@ -258,16 +256,13 @@ describe('DatabaseController', function () { query, ACL_GROUP ); - expect(output).toEqual({ ...query, user: createUserPointer(USER_ID) }); - done(); }); it('should not return a $or operation if the query involves one of the fields also used as array/pointer permissions', done => { const clp = buildCLP(['user', 'users', 'userObject']); const query = { a: 'b', user: createUserPointer(USER_ID) }; - schemaController.testPermissionsForClassName .withArgs(CLASS_NAME, ACL_GROUP, OPERATION) .and.returnValue(false); @@ -281,7 +276,6 @@ describe('DatabaseController', function () { schemaController.getExpectedType .withArgs(CLASS_NAME, 'userObject') .and.returnValue({ type: 'Object' }); - const output = databaseController.addPointerPermissions( schemaController, CLASS_NAME, @@ -289,9 +283,7 @@ describe('DatabaseController', function () { query, ACL_GROUP ); - expect(output).toEqual({ ...query, user: createUserPointer(USER_ID) }); - done(); });