From 559096f1c27caa0d7e62b80427894f3bee15624a Mon Sep 17 00:00:00 2001 From: Jack Wearden Date: Wed, 19 Jun 2019 23:30:08 +0100 Subject: [PATCH] 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 042d1ba19f636fe0da06074168c6fd5db37ea048. * clean up --- spec/DatabaseController.spec.js | 153 ++++++++++++------ .../Storage/Mongo/MongoStorageAdapter.js | 6 +- .../Postgres/PostgresStorageAdapter.js | 4 +- src/Config.js | 3 +- src/Controllers/DatabaseController.js | 120 +++++++++----- src/Controllers/index.js | 4 +- src/Options/Definitions.js | 6 + src/Options/docs.js | 1 + src/Options/index.js | 4 + 9 files changed, 203 insertions(+), 98 deletions(-) diff --git a/spec/DatabaseController.spec.js b/spec/DatabaseController.spec.js index c1d3dad2..70b554f8 100644 --- a/spec/DatabaseController.spec.js +++ b/spec/DatabaseController.spec.js @@ -3,61 +3,118 @@ const validateQuery = DatabaseController._validateQuery; describe('DatabaseController', function() { describe('validateQuery', function() { - it('should restructure simple cases of SERVER-13732', done => { - const query = { - $or: [{ a: 1 }, { a: 2 }], - _rperm: { $in: ['a', 'b'] }, - foo: 3, - }; - validateQuery(query); - expect(query).toEqual({ - $or: [ - { a: 1, _rperm: { $in: ['a', 'b'] }, foo: 3 }, - { a: 2, _rperm: { $in: ['a', 'b'] }, foo: 3 }, - ], - }); - done(); - }); - - it('should not restructure SERVER-13732 queries with $nears', done => { - let query = { $or: [{ a: 1 }, { b: 1 }], c: { $nearSphere: {} } }; - validateQuery(query); - expect(query).toEqual({ - $or: [{ a: 1 }, { b: 1 }], - c: { $nearSphere: {} }, + describe('with skipMongoDBServer13732Workaround disabled (the default)', function() { + it('should restructure simple cases of SERVER-13732', done => { + const query = { + $or: [{ a: 1 }, { a: 2 }], + _rperm: { $in: ['a', 'b'] }, + foo: 3, + }; + validateQuery(query, false); + expect(query).toEqual({ + $or: [ + { a: 1, _rperm: { $in: ['a', 'b'] }, foo: 3 }, + { a: 2, _rperm: { $in: ['a', 'b'] }, foo: 3 }, + ], + }); + done(); }); - query = { $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } }; - validateQuery(query); - expect(query).toEqual({ $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } }); - - done(); - }); - - it('should push refactored keys down a tree for SERVER-13732', done => { - const query = { - a: 1, - $or: [{ $or: [{ b: 1 }, { b: 2 }] }, { $or: [{ c: 1 }, { c: 2 }] }], - }; - validateQuery(query); - expect(query).toEqual({ - $or: [ - { $or: [{ b: 1, a: 1 }, { b: 2, a: 1 }] }, - { $or: [{ c: 1, a: 1 }, { c: 2, a: 1 }] }, - ], + it('should not restructure SERVER-13732 queries with $nears', done => { + let query = { $or: [{ a: 1 }, { b: 1 }], c: { $nearSphere: {} } }; + validateQuery(query, false); + expect(query).toEqual({ + $or: [{ a: 1 }, { b: 1 }], + c: { $nearSphere: {} }, + }); + query = { $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } }; + validateQuery(query, false); + expect(query).toEqual({ $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } }); + done(); }); - done(); + it('should push refactored keys down a tree for SERVER-13732', done => { + const query = { + a: 1, + $or: [{ $or: [{ b: 1 }, { b: 2 }] }, { $or: [{ c: 1 }, { c: 2 }] }], + }; + validateQuery(query, false); + expect(query).toEqual({ + $or: [ + { $or: [{ b: 1, a: 1 }, { b: 2, a: 1 }] }, + { $or: [{ c: 1, a: 1 }, { c: 2, a: 1 }] }, + ], + }); + done(); + }); + + it('should reject invalid queries', done => { + expect(() => validateQuery({ $or: { a: 1 } }, false)).toThrow(); + done(); + }); + + it('should accept valid queries', done => { + expect(() => + validateQuery({ $or: [{ a: 1 }, { b: 2 }] }, false) + ).not.toThrow(); + done(); + }); }); - it('should reject invalid queries', done => { - expect(() => validateQuery({ $or: { a: 1 } })).toThrow(); - done(); - }); + describe('with skipMongoDBServer13732Workaround enabled', function() { + it('should not restructure simple cases of SERVER-13732', done => { + const query = { + $or: [{ a: 1 }, { a: 2 }], + _rperm: { $in: ['a', 'b'] }, + foo: 3, + }; + validateQuery(query, true); + expect(query).toEqual({ + $or: [{ a: 1 }, { a: 2 }], + _rperm: { $in: ['a', 'b'] }, + foo: 3, + }); + done(); + }); - it('should accept valid queries', done => { - expect(() => validateQuery({ $or: [{ a: 1 }, { b: 2 }] })).not.toThrow(); - done(); + it('should not restructure SERVER-13732 queries with $nears', done => { + let query = { $or: [{ a: 1 }, { b: 1 }], c: { $nearSphere: {} } }; + validateQuery(query, true); + expect(query).toEqual({ + $or: [{ a: 1 }, { b: 1 }], + c: { $nearSphere: {} }, + }); + query = { $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } }; + validateQuery(query, true); + expect(query).toEqual({ $or: [{ a: 1 }, { b: 1 }], c: { $near: {} } }); + done(); + }); + + it('should not push refactored keys down a tree for SERVER-13732', done => { + const query = { + a: 1, + $or: [{ $or: [{ b: 1 }, { b: 2 }] }, { $or: [{ c: 1 }, { c: 2 }] }], + }; + validateQuery(query, true); + expect(query).toEqual({ + a: 1, + $or: [{ $or: [{ b: 1 }, { b: 2 }] }, { $or: [{ c: 1 }, { c: 2 }] }], + }); + + done(); + }); + + it('should reject invalid queries', done => { + expect(() => validateQuery({ $or: { a: 1 } }, true)).toThrow(); + done(); + }); + + it('should accept valid queries', done => { + expect(() => + validateQuery({ $or: [{ a: 1 }, { b: 2 }] }, true) + ).not.toThrow(); + done(); + }); }); }); }); diff --git a/src/Adapters/Storage/Mongo/MongoStorageAdapter.js b/src/Adapters/Storage/Mongo/MongoStorageAdapter.js index a289ccee..33282ce2 100644 --- a/src/Adapters/Storage/Mongo/MongoStorageAdapter.js +++ b/src/Adapters/Storage/Mongo/MongoStorageAdapter.js @@ -818,9 +818,9 @@ export class MongoStorageAdapter implements StorageAdapter { // Pass objects down to MongoDB...this is more than likely an $exists operator. returnValue[`_p_${field}`] = pipeline[field]; } else { - returnValue[`_p_${field}`] = `${schema.fields[field].targetClass}$${ - pipeline[field] - }`; + returnValue[ + `_p_${field}` + ] = `${schema.fields[field].targetClass}$${pipeline[field]}`; } } else if ( schema.fields[field] && diff --git a/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js b/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js index 5ba3a005..dda05ae6 100644 --- a/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js +++ b/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js @@ -1396,9 +1396,7 @@ export class PostgresStorageAdapter implements StorageAdapter { if (Object.keys(query).length === 0) { where.pattern = 'TRUE'; } - const qs = `WITH deleted AS (DELETE FROM $1:name WHERE ${ - where.pattern - } RETURNING *) SELECT count(*) FROM deleted`; + const qs = `WITH deleted AS (DELETE FROM $1:name WHERE ${where.pattern} RETURNING *) SELECT count(*) FROM deleted`; debug(qs, values); return this._client .one(qs, values, a => +a.count) diff --git a/src/Config.js b/src/Config.js index 7f685dd5..9eedddd3 100644 --- a/src/Config.js +++ b/src/Config.js @@ -34,7 +34,8 @@ export class Config { ); config.database = new DatabaseController( cacheInfo.databaseController.adapter, - schemaCache + schemaCache, + cacheInfo.skipMongoDBServer13732Workaround ); } else { config[key] = cacheInfo[key]; diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 473fcf02..cfe2c357 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -69,48 +69,73 @@ const isSpecialQueryKey = key => { return specialQuerykeys.indexOf(key) >= 0; }; -const validateQuery = (query: any): void => { +const validateQuery = ( + query: any, + skipMongoDBServer13732Workaround: boolean +): void => { if (query.ACL) { throw new Parse.Error(Parse.Error.INVALID_QUERY, 'Cannot query on ACL.'); } if (query.$or) { if (query.$or instanceof Array) { - query.$or.forEach(validateQuery); + query.$or.forEach(el => + validateQuery(el, skipMongoDBServer13732Workaround) + ); - /* In MongoDB, $or queries which are not alone at the top level of the - * query can not make efficient use of indexes due to a long standing - * bug known as SERVER-13732. - * - * This block restructures queries in which $or is not the sole top - * level element by moving all other top-level predicates inside every - * subdocument of the $or predicate, allowing MongoDB's query planner - * to make full use of the most relevant indexes. - * - * EG: {$or: [{a: 1}, {a: 2}], b: 2} - * Becomes: {$or: [{a: 1, b: 2}, {a: 2, b: 2}]} - * - * The only exceptions are $near and $nearSphere operators, which are - * constrained to only 1 operator per query. As a result, these ops - * remain at the top level - * - * https://jira.mongodb.org/browse/SERVER-13732 - * https://github.com/parse-community/parse-server/issues/3767 - */ - Object.keys(query).forEach(key => { - const noCollisions = !query.$or.some(subq => subq.hasOwnProperty(key)); - let hasNears = false; - if (query[key] != null && typeof query[key] == 'object') { - hasNears = '$near' in query[key] || '$nearSphere' in query[key]; - } - if (key != '$or' && noCollisions && !hasNears) { - query.$or.forEach(subquery => { - subquery[key] = query[key]; - }); - delete query[key]; - } - }); - query.$or.forEach(validateQuery); + if (!skipMongoDBServer13732Workaround) { + /* In MongoDB 3.2 & 3.4, $or queries which are not alone at the top + * level of the query can not make efficient use of indexes due to a + * long standing bug known as SERVER-13732. + * + * This bug was fixed in MongoDB version 3.6. + * + * For versions pre-3.6, the below logic produces a substantial + * performance improvement inside the database by avoiding the bug. + * + * For versions 3.6 and above, there is no performance improvement and + * the logic is unnecessary. Some query patterns are even slowed by + * the below logic, due to the bug having been fixed and better + * query plans being chosen. + * + * When versions before 3.4 are no longer supported by this project, + * this logic, and the accompanying `skipMongoDBServer13732Workaround` + * flag, can be removed. + * + * This block restructures queries in which $or is not the sole top + * level element by moving all other top-level predicates inside every + * subdocument of the $or predicate, allowing MongoDB's query planner + * to make full use of the most relevant indexes. + * + * EG: {$or: [{a: 1}, {a: 2}], b: 2} + * Becomes: {$or: [{a: 1, b: 2}, {a: 2, b: 2}]} + * + * The only exceptions are $near and $nearSphere operators, which are + * constrained to only 1 operator per query. As a result, these ops + * remain at the top level + * + * https://jira.mongodb.org/browse/SERVER-13732 + * https://github.com/parse-community/parse-server/issues/3767 + */ + Object.keys(query).forEach(key => { + const noCollisions = !query.$or.some(subq => + subq.hasOwnProperty(key) + ); + let hasNears = false; + if (query[key] != null && typeof query[key] == 'object') { + hasNears = '$near' in query[key] || '$nearSphere' in query[key]; + } + if (key != '$or' && noCollisions && !hasNears) { + query.$or.forEach(subquery => { + subquery[key] = query[key]; + }); + delete query[key]; + } + }); + query.$or.forEach(el => + validateQuery(el, skipMongoDBServer13732Workaround) + ); + } } else { throw new Parse.Error( Parse.Error.INVALID_QUERY, @@ -121,7 +146,9 @@ const validateQuery = (query: any): void => { if (query.$and) { if (query.$and instanceof Array) { - query.$and.forEach(validateQuery); + query.$and.forEach(el => + validateQuery(el, skipMongoDBServer13732Workaround) + ); } else { throw new Parse.Error( Parse.Error.INVALID_QUERY, @@ -132,7 +159,9 @@ const validateQuery = (query: any): void => { if (query.$nor) { if (query.$nor instanceof Array && query.$nor.length > 0) { - query.$nor.forEach(validateQuery); + query.$nor.forEach(el => + validateQuery(el, skipMongoDBServer13732Workaround) + ); } else { throw new Parse.Error( Parse.Error.INVALID_QUERY, @@ -381,14 +410,20 @@ class DatabaseController { adapter: StorageAdapter; schemaCache: any; schemaPromise: ?Promise; + skipMongoDBServer13732Workaround: boolean; - constructor(adapter: StorageAdapter, schemaCache: any) { + constructor( + adapter: StorageAdapter, + schemaCache: any, + skipMongoDBServer13732Workaround: boolean + ) { this.adapter = adapter; this.schemaCache = schemaCache; // We don't want a mutable this.schema, because then you could have // one request that uses different schemas for different parts of // it. Instead, use loadSchema to get a schema. this.schemaPromise = null; + this.skipMongoDBServer13732Workaround = skipMongoDBServer13732Workaround; } collectionExists(className: string): Promise { @@ -524,7 +559,7 @@ class DatabaseController { if (acl) { query = addWriteACL(query, acl); } - validateQuery(query); + validateQuery(query, this.skipMongoDBServer13732Workaround); return schemaController .getOneSchema(className, true) .catch(error => { @@ -798,7 +833,7 @@ class DatabaseController { if (acl) { query = addWriteACL(query, acl); } - validateQuery(query); + validateQuery(query, this.skipMongoDBServer13732Workaround); return schemaController .getOneSchema(className) .catch(error => { @@ -1197,6 +1232,7 @@ class DatabaseController { ): Promise { const isMaster = acl === undefined; const aclGroup = acl || []; + op = op || (typeof query.objectId == 'string' && Object.keys(query).length === 1 @@ -1297,7 +1333,7 @@ class DatabaseController { query = addReadACL(query, aclGroup); } } - validateQuery(query); + validateQuery(query, this.skipMongoDBServer13732Workaround); if (count) { if (!classExists) { return 0; @@ -1563,7 +1599,7 @@ class DatabaseController { ]); } - static _validateQuery: any => void; + static _validateQuery: (any, boolean) => void; } module.exports = DatabaseController; diff --git a/src/Controllers/index.js b/src/Controllers/index.js index d63c2042..81e7cfbe 100644 --- a/src/Controllers/index.js +++ b/src/Controllers/index.js @@ -147,6 +147,7 @@ export function getDatabaseController( const { databaseURI, databaseOptions, + skipMongoDBServer13732Workaround, collectionPrefix, schemaCacheTTL, enableSingleSchemaCache, @@ -170,7 +171,8 @@ export function getDatabaseController( } return new DatabaseController( databaseAdapter, - new SchemaCache(cacheController, schemaCacheTTL, enableSingleSchemaCache) + new SchemaCache(cacheController, schemaCacheTTL, enableSingleSchemaCache), + skipMongoDBServer13732Workaround ); } diff --git a/src/Options/Definitions.js b/src/Options/Definitions.js index dfae3935..6bde68b3 100644 --- a/src/Options/Definitions.js +++ b/src/Options/Definitions.js @@ -320,6 +320,12 @@ module.exports.ParseServerOptions = { help: 'Disables console output', action: parsers.booleanParser, }, + skipMongoDBServer13732Workaround: { + env: 'PARSE_SKIP_MONGODB_SERVER_13732_WORKAROUND', + help: 'Circumvent Parse workaround for historical MongoDB bug SERVER-13732', + action: parsers.booleanParser, + default: false, + }, startLiveQueryServer: { env: 'PARSE_SERVER_START_LIVE_QUERY_SERVER', help: 'Starts the liveQuery server', diff --git a/src/Options/docs.js b/src/Options/docs.js index adfab811..481fb911 100644 --- a/src/Options/docs.js +++ b/src/Options/docs.js @@ -57,6 +57,7 @@ * @property {String} serverURL URL to your parse server with http:// or https://. * @property {Number} sessionLength Session duration, in seconds, defaults to 1 year * @property {Boolean} silent Disables console output + * @property {Boolean} skipMongoDBServer13732Workaround Circumvent Parse workaround for historical MongoDB bug SERVER-13732 * @property {Boolean} startLiveQueryServer Starts the liveQuery server * @property {String[]} userSensitiveFields Personally identifiable information fields in the user table the should be removed for non-authorized users. Deprecated @see protectedFields * @property {Boolean} verbose Set the logging to verbose diff --git a/src/Options/index.js b/src/Options/index.js index 4734da38..fea9eef0 100644 --- a/src/Options/index.js +++ b/src/Options/index.js @@ -58,6 +58,10 @@ export interface ParseServerOptions { databaseOptions: ?any; /* Adapter module for the database */ databaseAdapter: ?Adapter; + /* Circumvent Parse workaround for historical MongoDB bug SERVER-13732 + :ENV: PARSE_SKIP_MONGODB_SERVER_13732_WORKAROUND + :DEFAULT: false */ + skipMongoDBServer13732Workaround: ?boolean; /* Full path to your cloud code main.js */ cloud: ?string; /* A collection prefix for the classes