Skip to content

Commit

Permalink
Optimize redundant logic used in queries (#7061)
Browse files Browse the repository at this point in the history 
* 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 <p.diaz@wemersive.com>
  • Loading branch information
@pdiaz
pdiaz and pdiaz authored Dec 16, 2020
1 parent 4405ddd commit c46e8a5
Show file tree
Hide file tree
Showing 3 changed files with 176 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
96 changes: 96 additions & 0 deletions spec/DatabaseController.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' };
Expand Down Expand Up @@ -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) {
Expand Down
81 changes: 79 additions & 2 deletions src/Controllers/DatabaseController.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<string> {
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> }): 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> }): 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;
Expand Down Expand Up @@ -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;
}
Expand Down

0 comments on commit c46e8a5

Please sign in to comment.