Skip to content

Commit 559096f

Browse files
NotBobTheBuilderdplewis
NotBobTheBuilder
authored and
dplewis
committed
Allow disabling workaround for since-fixed MongoDB bug (#5617)
* Allow disabling workaround for fixed MongoDB bug * skipMongoDBServer13732Workaround description fix * flip test boolean * Remove CLI flag, use databaseVersion & engine * Revert "Remove CLI flag, use databaseVersion & engine" This reverts commit 042d1ba. * clean up
1 parent fcdf2d7 commit 559096f

File tree

9 files changed

+201
-96
lines changed

9 files changed

+201
-96
lines changed

‎spec/DatabaseController.spec.js

Lines changed: 102 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -3,61 +3,118 @@ const validateQuery = DatabaseController._validateQuery;
33

44
describe('DatabaseController', function() {
55
describe('validateQuery', function() {
6-
it('should restructure simple cases of SERVER-13732', done => {
7-
const query = {
8-
$or: [{ a: 1 }, { a: 2 }],
9-
_rperm: { $in: ['a', 'b'] },
10-
foo: 3,
11-
};
12-
validateQuery(query);
13-
expect(query).toEqual({
14-
$or: [
15-
{ a: 1, _rperm: { $in: ['a', 'b'] }, foo: 3 },
16-
{ a: 2, _rperm: { $in: ['a', 'b'] }, foo: 3 },
17-
],
18-
});
19-
done();
20-
});
6+
describe('with skipMongoDBServer13732Workaround disabled (the default)', function() {
7+
it('should restructure simple cases of SERVER-13732', done => {
8+
const query = {
9+
$or: [{ a: 1 }, { a: 2 }],
10+
_rperm: { $in: ['a', 'b'] },
11+
foo: 3,
12+
};
13+
validateQuery(query, false);
14+
expect(query).toEqual({
15+
$or: [
16+
{ a: 1, _rperm: { $in: ['a', 'b'] }, foo: 3 },
17+
{ a: 2, _rperm: { $in: ['a', 'b'] }, foo: 3 },
18+
],
19+
});
20+
done();
21+
});
2122

22-
it('should not restructure SERVER-13732 queries with $nears', done => {
23-
let query = { $or: [{ a: 1 }, { b: 1 }], c: { $nearSphere: {} } };
24-
validateQuery(query);
25-
expect(query).toEqual({
26-
$or: [{ a: 1 }, { b: 1 }],
27-
c: { $nearSphere: {} },
23+
it('should not restructure SERVER-13732 queries with $nears', done => {
24+
let query = { $or: [{ a: 1 }, { b: 1 }], c: { $nearSphere: {} } };
25+
validateQuery(query, false);
26+
expect(query).toEqual({
27+
$or: [{ a: 1 }, { b: 1 }],
28+
c: { $nearSphere: {} },
29+
});
30+
query = { $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } };
31+
validateQuery(query, false);
32+
expect(query).toEqual({ $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } });
33+
done();
2834
});
2935

30-
query = { $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } };
31-
validateQuery(query);
32-
expect(query).toEqual({ $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } });
36+
it('should push refactored keys down a tree for SERVER-13732', done => {
37+
const query = {
38+
a: 1,
39+
$or: [{ $or: [{ b: 1 }, { b: 2 }] }, { $or: [{ c: 1 }, { c: 2 }] }],
40+
};
41+
validateQuery(query, false);
42+
expect(query).toEqual({
43+
$or: [
44+
{ $or: [{ b: 1, a: 1 }, { b: 2, a: 1 }] },
45+
{ $or: [{ c: 1, a: 1 }, { c: 2, a: 1 }] },
46+
],
47+
});
48+
done();
49+
});
3350

34-
done();
51+
it('should reject invalid queries', done => {
52+
expect(() => validateQuery({ $or: { a: 1 } }, false)).toThrow();
53+
done();
54+
});
55+
56+
it('should accept valid queries', done => {
57+
expect(() =>
58+
validateQuery({ $or: [{ a: 1 }, { b: 2 }] }, false)
59+
).not.toThrow();
60+
done();
61+
});
3562
});
3663

37-
it('should push refactored keys down a tree for SERVER-13732', done => {
38-
const query = {
39-
a: 1,
40-
$or: [{ $or: [{ b: 1 }, { b: 2 }] }, { $or: [{ c: 1 }, { c: 2 }] }],
41-
};
42-
validateQuery(query);
43-
expect(query).toEqual({
44-
$or: [
45-
{ $or: [{ b: 1, a: 1 }, { b: 2, a: 1 }] },
46-
{ $or: [{ c: 1, a: 1 }, { c: 2, a: 1 }] },
47-
],
64+
describe('with skipMongoDBServer13732Workaround enabled', function() {
65+
it('should not restructure simple cases of SERVER-13732', done => {
66+
const query = {
67+
$or: [{ a: 1 }, { a: 2 }],
68+
_rperm: { $in: ['a', 'b'] },
69+
foo: 3,
70+
};
71+
validateQuery(query, true);
72+
expect(query).toEqual({
73+
$or: [{ a: 1 }, { a: 2 }],
74+
_rperm: { $in: ['a', 'b'] },
75+
foo: 3,
76+
});
77+
done();
4878
});
4979

50-
done();
51-
});
80+
it('should not restructure SERVER-13732 queries with $nears', done => {
81+
let query = { $or: [{ a: 1 }, { b: 1 }], c: { $nearSphere: {} } };
82+
validateQuery(query, true);
83+
expect(query).toEqual({
84+
$or: [{ a: 1 }, { b: 1 }],
85+
c: { $nearSphere: {} },
86+
});
87+
query = { $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } };
88+
validateQuery(query, true);
89+
expect(query).toEqual({ $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } });
90+
done();
91+
});
5292

53-
it('should reject invalid queries', done => {
54-
expect(() => validateQuery({ $or: { a: 1 } })).toThrow();
55-
done();
56-
});
93+
it('should not push refactored keys down a tree for SERVER-13732', done => {
94+
const query = {
95+
a: 1,
96+
$or: [{ $or: [{ b: 1 }, { b: 2 }] }, { $or: [{ c: 1 }, { c: 2 }] }],
97+
};
98+
validateQuery(query, true);
99+
expect(query).toEqual({
100+
a: 1,
101+
$or: [{ $or: [{ b: 1 }, { b: 2 }] }, { $or: [{ c: 1 }, { c: 2 }] }],
102+
});
103+
104+
done();
105+
});
57106

58-
it('should accept valid queries', done => {
59-
expect(() => validateQuery({ $or: [{ a: 1 }, { b: 2 }] })).not.toThrow();
60-
done();
107+
it('should reject invalid queries', done => {
108+
expect(() => validateQuery({ $or: { a: 1 } }, true)).toThrow();
109+
done();
110+
});
111+
112+
it('should accept valid queries', done => {
113+
expect(() =>
114+
validateQuery({ $or: [{ a: 1 }, { b: 2 }] }, true)
115+
).not.toThrow();
116+
done();
117+
});
61118
});
62119
});
63120
});

‎src/Adapters/Storage/Mongo/MongoStorageAdapter.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -818,9 +818,9 @@ export class MongoStorageAdapter implements StorageAdapter {
818818
// Pass objects down to MongoDB...this is more than likely an $exists operator.
819819
returnValue[`_p_${field}`] = pipeline[field];
820820
} else {
821-
returnValue[`_p_${field}`] = `${schema.fields[field].targetClass}$${
822-
pipeline[field]
823-
}`;
821+
returnValue[
822+
`_p_${field}`
823+
] = `${schema.fields[field].targetClass}$${pipeline[field]}`;
824824
}
825825
} else if (
826826
schema.fields[field] &&

‎src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1396,9 +1396,7 @@ export class PostgresStorageAdapter implements StorageAdapter {
13961396
if (Object.keys(query).length === 0) {
13971397
where.pattern = 'TRUE';
13981398
}
1399-
const qs = `WITH deleted AS (DELETE FROM $1:name WHERE ${
1400-
where.pattern
1401-
} RETURNING *) SELECT count(*) FROM deleted`;
1399+
const qs = `WITH deleted AS (DELETE FROM $1:name WHERE ${where.pattern} RETURNING *) SELECT count(*) FROM deleted`;
14021400
debug(qs, values);
14031401
return this._client
14041402
.one(qs, values, a => +a.count)

‎src/Config.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ export class Config {
3434
);
3535
config.database = new DatabaseController(
3636
cacheInfo.databaseController.adapter,
37-
schemaCache
37+
schemaCache,
38+
cacheInfo.skipMongoDBServer13732Workaround
3839
);
3940
} else {
4041
config[key] = cacheInfo[key];

‎src/Controllers/DatabaseController.js

Lines changed: 79 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -69,48 +69,73 @@ const isSpecialQueryKey = key => {
6969
return specialQuerykeys.indexOf(key) >= 0;
7070
};
7171

72-
const validateQuery = (query: any): void => {
72+
const validateQuery = (
73+
query: any,
74+
skipMongoDBServer13732Workaround: boolean
75+
): void => {
7376
if (query.ACL) {
7477
throw new Parse.Error(Parse.Error.INVALID_QUERY, 'Cannot query on ACL.');
7578
}
7679

7780
if (query.$or) {
7881
if (query.$or instanceof Array) {
79-
query.$or.forEach(validateQuery);
80-
81-
/* In MongoDB, $or queries which are not alone at the top level of the
82-
* query can not make efficient use of indexes due to a long standing
83-
* bug known as SERVER-13732.
84-
*
85-
* This block restructures queries in which $or is not the sole top
86-
* level element by moving all other top-level predicates inside every
87-
* subdocument of the $or predicate, allowing MongoDB's query planner
88-
* to make full use of the most relevant indexes.
89-
*
90-
* EG: {$or: [{a: 1}, {a: 2}], b: 2}
91-
* Becomes: {$or: [{a: 1, b: 2}, {a: 2, b: 2}]}
92-
*
93-
* The only exceptions are $near and $nearSphere operators, which are
94-
* constrained to only 1 operator per query. As a result, these ops
95-
* remain at the top level
96-
*
97-
* https://jira.mongodb.org/browse/SERVER-13732
98-
* https://github.com/parse-community/parse-server/issues/3767
99-
*/
100-
Object.keys(query).forEach(key => {
101-
const noCollisions = !query.$or.some(subq => subq.hasOwnProperty(key));
102-
let hasNears = false;
103-
if (query[key] != null && typeof query[key] == 'object') {
104-
hasNears = '$near' in query[key] || '$nearSphere' in query[key];
105-
}
106-
if (key != '$or' && noCollisions && !hasNears) {
107-
query.$or.forEach(subquery => {
108-
subquery[key] = query[key];
109-
});
110-
delete query[key];
111-
}
112-
});
113-
query.$or.forEach(validateQuery);
82+
query.$or.forEach(el =>
83+
validateQuery(el, skipMongoDBServer13732Workaround)
84+
);
85+
86+
if (!skipMongoDBServer13732Workaround) {
87+
/* In MongoDB 3.2 & 3.4, $or queries which are not alone at the top
88+
* level of the query can not make efficient use of indexes due to a
89+
* long standing bug known as SERVER-13732.
90+
*
91+
* This bug was fixed in MongoDB version 3.6.
92+
*
93+
* For versions pre-3.6, the below logic produces a substantial
94+
* performance improvement inside the database by avoiding the bug.
95+
*
96+
* For versions 3.6 and above, there is no performance improvement and
97+
* the logic is unnecessary. Some query patterns are even slowed by
98+
* the below logic, due to the bug having been fixed and better
99+
* query plans being chosen.
100+
*
101+
* When versions before 3.4 are no longer supported by this project,
102+
* this logic, and the accompanying `skipMongoDBServer13732Workaround`
103+
* flag, can be removed.
104+
*
105+
* This block restructures queries in which $or is not the sole top
106+
* level element by moving all other top-level predicates inside every
107+
* subdocument of the $or predicate, allowing MongoDB's query planner
108+
* to make full use of the most relevant indexes.
109+
*
110+
* EG: {$or: [{a: 1}, {a: 2}], b: 2}
111+
* Becomes: {$or: [{a: 1, b: 2}, {a: 2, b: 2}]}
112+
*
113+
* The only exceptions are $near and $nearSphere operators, which are
114+
* constrained to only 1 operator per query. As a result, these ops
115+
* remain at the top level
116+
*
117+
* https://jira.mongodb.org/browse/SERVER-13732
118+
* https://github.com/parse-community/parse-server/issues/3767
119+
*/
120+
Object.keys(query).forEach(key => {
121+
const noCollisions = !query.$or.some(subq =>
122+
subq.hasOwnProperty(key)
123+
);
124+
let hasNears = false;
125+
if (query[key] != null && typeof query[key] == 'object') {
126+
hasNears = '$near' in query[key] || '$nearSphere' in query[key];
127+
}
128+
if (key != '$or' && noCollisions && !hasNears) {
129+
query.$or.forEach(subquery => {
130+
subquery[key] = query[key];
131+
});
132+
delete query[key];
133+
}
134+
});
135+
query.$or.forEach(el =>
136+
validateQuery(el, skipMongoDBServer13732Workaround)
137+
);
138+
}
114139
} else {
115140
throw new Parse.Error(
116141
Parse.Error.INVALID_QUERY,
@@ -121,7 +146,9 @@ const validateQuery = (query: any): void => {
121146

122147
if (query.$and) {
123148
if (query.$and instanceof Array) {
124-
query.$and.forEach(validateQuery);
149+
query.$and.forEach(el =>
150+
validateQuery(el, skipMongoDBServer13732Workaround)
151+
);
125152
} else {
126153
throw new Parse.Error(
127154
Parse.Error.INVALID_QUERY,
@@ -132,7 +159,9 @@ const validateQuery = (query: any): void => {
132159

133160
if (query.$nor) {
134161
if (query.$nor instanceof Array && query.$nor.length > 0) {
135-
query.$nor.forEach(validateQuery);
162+
query.$nor.forEach(el =>
163+
validateQuery(el, skipMongoDBServer13732Workaround)
164+
);
136165
} else {
137166
throw new Parse.Error(
138167
Parse.Error.INVALID_QUERY,
@@ -381,14 +410,20 @@ class DatabaseController {
381410
adapter: StorageAdapter;
382411
schemaCache: any;
383412
schemaPromise: ?Promise<SchemaController.SchemaController>;
413+
skipMongoDBServer13732Workaround: boolean;
384414

385-
constructor(adapter: StorageAdapter, schemaCache: any) {
415+
constructor(
416+
adapter: StorageAdapter,
417+
schemaCache: any,
418+
skipMongoDBServer13732Workaround: boolean
419+
) {
386420
this.adapter = adapter;
387421
this.schemaCache = schemaCache;
388422
// We don't want a mutable this.schema, because then you could have
389423
// one request that uses different schemas for different parts of
390424
// it. Instead, use loadSchema to get a schema.
391425
this.schemaPromise = null;
426+
this.skipMongoDBServer13732Workaround = skipMongoDBServer13732Workaround;
392427
}
393428

394429
collectionExists(className: string): Promise<boolean> {
@@ -524,7 +559,7 @@ class DatabaseController {
524559
if (acl) {
525560
query = addWriteACL(query, acl);
526561
}
527-
validateQuery(query);
562+
validateQuery(query, this.skipMongoDBServer13732Workaround);
528563
return schemaController
529564
.getOneSchema(className, true)
530565
.catch(error => {
@@ -798,7 +833,7 @@ class DatabaseController {
798833
if (acl) {
799834
query = addWriteACL(query, acl);
800835
}
801-
validateQuery(query);
836+
validateQuery(query, this.skipMongoDBServer13732Workaround);
802837
return schemaController
803838
.getOneSchema(className)
804839
.catch(error => {
@@ -1197,6 +1232,7 @@ class DatabaseController {
11971232
): Promise<any> {
11981233
const isMaster = acl === undefined;
11991234
const aclGroup = acl || [];
1235+
12001236
op =
12011237
op ||
12021238
(typeof query.objectId == 'string' && Object.keys(query).length === 1
@@ -1297,7 +1333,7 @@ class DatabaseController {
12971333
query = addReadACL(query, aclGroup);
12981334
}
12991335
}
1300-
validateQuery(query);
1336+
validateQuery(query, this.skipMongoDBServer13732Workaround);
13011337
if (count) {
13021338
if (!classExists) {
13031339
return 0;
@@ -1563,7 +1599,7 @@ class DatabaseController {
15631599
]);
15641600
}
15651601

1566-
static _validateQuery: any => void;
1602+
static _validateQuery: (any, boolean) => void;
15671603
}
15681604

15691605
module.exports = DatabaseController;

0 commit comments

Comments
 (0)