From c46e8a525d172bd01c84e019d76f94804fe5e710 Mon Sep 17 00:00:00 2001 From: Pedro Diaz Date: Wed, 16 Dec 2020 06:41:14 +0100 Subject: [PATCH] Optimize redundant logic used in queries (#7061) * Optimize redundant logic used in queries * Added CHANGELOG * Fixed comments and code style after recommendations. * Fixed code style after recommendation. * Improved explanation in comments * Added tests to for logic optimizations * Added two test cases more and some comments * Added extra test cases and fixed issue found with them. * Removed empty lines as requested. Co-authored-by: Pedro Diaz --- CHANGELOG.md | 1 + spec/DatabaseController.spec.js | 96 +++++++++++++++++++++++++++ src/Controllers/DatabaseController.js | 81 +++++++++++++++++++++- 3 files changed, 176 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b22cbcd8..fa847e4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ### master [Full Changelog](https://github.com/parse-community/parse-server/compare/4.5.0...master) +- 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.5.0 [Full Changelog](https://github.com/parse-community/parse-server/compare/4.4.0...4.5.0) diff --git a/spec/DatabaseController.spec.js b/spec/DatabaseController.spec.js index 988248c8..98103ce6 100644 --- a/spec/DatabaseController.spec.js +++ b/spec/DatabaseController.spec.js @@ -236,6 +236,57 @@ 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' }; @@ -265,6 +316,51 @@ 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({ a: 1 })).toEqual({ a: 1 }); + 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({ a: 1 })).toEqual({ a: 1 }); + 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 21ba2e94..6b132dcb 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1365,6 +1365,83 @@ 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(':')); + } + + // Naive logic reducer for OR operations meant to be used only for pointer permissions. + reduceOrOperation(query: { $or: Array }): any { + if (!query.$or) { + return query; + } + const queries = query.$or.map(q => this.objectToEntriesStrings(q)); + 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; + } + 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; + } + const queries = query.$and.map(q => this.objectToEntriesStrings(q)); + 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; + } + 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 +1525,13 @@ class DatabaseController { } // if we already have a constraint on the key, use the $and if (Object.prototype.hasOwnProperty.call(query, key)) { - return { $and: [queryClause, query] }; + return this.reduceAndOperation({ $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; }