Merge pull request Automattic#15082 from Automattic/vkarpov15/Automatticgh-15026

fix(query): clone PopulateOptions when setting _localModel to avoid state leaking between subpopulate instances

Author: vkarpov15

lib/model.js

@@ -4245,67 +4245,34 @@ Model.populate = async function populate(docs, paths) {
   if (typeof paths === 'function' || typeof arguments[2] === 'function') {
     throw new MongooseError('Model.populate() no longer accepts a callback');
   }
-  const _this = this;
   // normalized paths
   paths = utils.populate(paths);
-  // data that should persist across subPopulate calls
-  const cache = {};
-
-  return new Promise((resolve, reject) => {
-    _populate(_this, docs, paths, cache, (err, res) => {
-      if (err) {
-        return reject(err);
-      }
-      resolve(res);
-    });
-  });
-};
-
-/**
- * Populate helper
- *
- * @param {Model} model the model to use
- * @param {Document|Array} docs Either a single document or array of documents to populate.
- * @param {Object} paths
- * @param {never} cache Unused
- * @param {Function} [callback] Optional callback, executed upon completion. Receives `err` and the `doc(s)`.
- * @return {Function}
- * @api private
- */
 
-function _populate(model, docs, paths, cache, callback) {
-  let pending = paths.length;
   if (paths.length === 0) {
-    return callback(null, docs);
+    return docs;
   }
+
   // each path has its own query options and must be executed separately
+  const promises = [];
   for (const path of paths) {
-    populate(model, docs, path, next);
+    promises.push(_populatePath(this, docs, path));
   }
+  await Promise.all(promises);
 
-  function next(err) {
-    if (err) {
-      return callback(err, null);
-    }
-    if (--pending) {
-      return;
-    }
-    callback(null, docs);
-  }
-}
+  return docs;
+};
 
 /*!
- * Populates `docs`
+ * Populates `docs` for a single `populateOptions` instance.
  */
 const excludeIdReg = /\s?-_id\s?/;
 const excludeIdRegGlobal = /\s?-_id\s?/g;
 
-function populate(model, docs, options, callback) {
-  const populateOptions = options;
-  if (options.strictPopulate == null) {
-    if (options._localModel != null && options._localModel.schema._userProvidedOptions.strictPopulate != null) {
-      populateOptions.strictPopulate = options._localModel.schema._userProvidedOptions.strictPopulate;
-    } else if (options._localModel != null && model.base.options.strictPopulate != null) {
+async function _populatePath(model, docs, populateOptions) {
+  if (populateOptions.strictPopulate == null) {
+    if (populateOptions._localModel != null && populateOptions._localModel.schema._userProvidedOptions.strictPopulate != null) {
+      populateOptions.strictPopulate = populateOptions._localModel.schema._userProvidedOptions.strictPopulate;
+    } else if (populateOptions._localModel != null && model.base.options.strictPopulate != null) {
       populateOptions.strictPopulate = model.base.options.strictPopulate;
     } else if (model.base.options.strictPopulate != null) {
       populateOptions.strictPopulate = model.base.options.strictPopulate;
@@ -4317,15 +4284,12 @@ function populate(model, docs, options, callback) {
     docs = [docs];
   }
   if (docs.length === 0 || docs.every(utils.isNullOrUndefined)) {
-    return callback();
+    return;
   }
 
   const modelsMap = getModelsMapForPopulate(model, docs, populateOptions);
-
   if (modelsMap instanceof MongooseError) {
-    return immediate(function() {
-      callback(modelsMap);
-    });
+    throw modelsMap;
   }
   const len = modelsMap.length;
   let vals = [];
@@ -4335,7 +4299,6 @@ function populate(model, docs, options, callback) {
     return undefined !== item;
   }
 
-  let _remaining = len;
   let hasOne = false;
   const params = [];
   for (let i = 0; i < len; ++i) {
@@ -4366,7 +4329,6 @@ function populate(model, docs, options, callback) {
       // Ensure that we set to 0 or empty array even
       // if we don't actually execute a query to make sure there's a value
       // and we know this path was populated for future sets. See gh-7731, gh-8230
-      --_remaining;
       _assign(model, [], mod, assignmentOpts);
       continue;
     }
@@ -4397,72 +4359,58 @@ function populate(model, docs, options, callback) {
     } else if (mod.options.limit != null) {
       assignmentOpts.originalLimit = mod.options.limit;
     }
-    params.push([mod, match, select, assignmentOpts, _next]);
+    params.push([mod, match, select, assignmentOpts]);
   }
   if (!hasOne) {
     // If models but no docs, skip further deep populate.
     if (modelsMap.length !== 0) {
-      return callback();
+      return;
     }
-    // If no models to populate but we have a nested populate,
-    // keep trying, re: gh-8946
+    // If no models and no docs to populate but we have a nested populate,
+    // probably a case of unnecessarily populating a non-ref path re: gh-8946
     if (populateOptions.populate != null) {
       const opts = utils.populate(populateOptions.populate).map(pop => Object.assign({}, pop, {
         path: populateOptions.path + '.' + pop.path
       }));
-      model.populate(docs, opts).then(res => { callback(null, res); }, err => { callback(err); });
-      return;
+      return model.populate(docs, opts);
     }
-    return callback();
+    return;
   }
 
+  const promises = [];
   for (const arr of params) {
-    _execPopulateQuery.apply(null, arr);
-  }
-  function _next(err, valsFromDb) {
-    if (err != null) {
-      return callback(err, null);
-    }
-    vals = vals.concat(valsFromDb);
-    if (--_remaining === 0) {
-      _done();
-    }
+    promises.push(_execPopulateQuery.apply(null, arr).then(valsFromDb => { vals = vals.concat(valsFromDb); }));
   }
 
-  function _done() {
-    for (const arr of params) {
-      const mod = arr[0];
-      const assignmentOpts = arr[3];
-      for (const val of vals) {
-        mod.options._childDocs.push(val);
-      }
-      try {
-        _assign(model, vals, mod, assignmentOpts);
-      } catch (err) {
-        return callback(err);
-      }
-    }
+  await Promise.all(promises);
 
-    for (const arr of params) {
-      removeDeselectedForeignField(arr[0].foreignField, arr[0].options, vals);
+  for (const arr of params) {
+    const mod = arr[0];
+    const assignmentOpts = arr[3];
+    for (const val of vals) {
+      mod.options._childDocs.push(val);
     }
-    for (const arr of params) {
-      const mod = arr[0];
-      if (mod.options && mod.options.options && mod.options.options._leanTransform) {
-        for (const doc of vals) {
-          mod.options.options._leanTransform(doc);
-        }
+    _assign(model, vals, mod, assignmentOpts);
+  }
+
+  for (const arr of params) {
+    removeDeselectedForeignField(arr[0].foreignField, arr[0].options, vals);
+  }
+  for (const arr of params) {
+    const mod = arr[0];
+    if (mod.options && mod.options.options && mod.options.options._leanTransform) {
+      for (const doc of vals) {
+        mod.options.options._leanTransform(doc);
       }
     }
-    callback();
   }
 }
 
 /*!
  * ignore
  */
 
-function _execPopulateQuery(mod, match, select, assignmentOpts, callback) {
+function _execPopulateQuery(mod, match, select) {
   let subPopulate = clone(mod.options.populate);
   const queryOptions = Object.assign({
     skip: mod.options.skip,
@@ -4528,15 +4476,12 @@ function _execPopulateQuery(mod, match, select, assignmentOpts, callback) {
     query.populate(subPopulate);
   }
 
-  query.exec().then(
+  return query.exec().then(
     docs => {
       for (const val of docs) {
         leanPopulateMap.set(val, mod.model);
       }
-      callback(null, docs);
-    },
-    err => {
-      callback(err);
+      return docs;
     }
   );
 }

lib/query.js

@@ -2381,6 +2381,7 @@ Query.prototype._find = async function _find() {
       _completeManyLean(_this.model.schema, docs, null, completeManyOptions) :
       _this._completeMany(docs, fields, userProvidedFields, completeManyOptions);
   }
+
   const pop = helpers.preparePopulationOptionsMQ(_this, mongooseOptions);
 
   if (mongooseOptions.lean) {
@@ -4762,12 +4763,13 @@ Query.prototype._castUpdate = function _castUpdate(obj) {
  */
 
 Query.prototype.populate = function() {
+  const args = Array.from(arguments);
   // Bail when given no truthy arguments
-  if (!Array.from(arguments).some(Boolean)) {
+  if (!args.some(Boolean)) {
     return this;
   }
 
-  const res = utils.populate.apply(null, arguments);
+  const res = utils.populate.apply(null, args);
 
   // Propagate readConcern and readPreference and lean from parent query,
   // unless one already specified

lib/queryHelpers.js

@@ -4,6 +4,7 @@
  * Module dependencies
  */
 
+const PopulateOptions = require('./options/populateOptions');
 const checkEmbeddedDiscriminatorKeyProjection =
   require('./helpers/discriminator/checkEmbeddedDiscriminatorKeyProjection');
 const get = require('./helpers/get');
@@ -13,32 +14,6 @@ const isDefiningProjection = require('./helpers/projection/isDefiningProjection'
 const clone = require('./helpers/clone');
 const isPathSelectedInclusive = require('./helpers/projection/isPathSelectedInclusive');
 
-/**
- * Prepare a set of path options for query population.
- *
- * @param {Query} query
- * @param {Object} options
- * @return {Array}
- */
-
-exports.preparePopulationOptions = function preparePopulationOptions(query, options) {
-  const _populate = query.options.populate;
-  const pop = Object.keys(_populate).reduce((vals, key) => vals.concat([_populate[key]]), []);
-
-  // lean options should trickle through all queries
-  if (options.lean != null) {
-    pop
-      .filter(p => (p && p.options && p.options.lean) == null)
-      .forEach(makeLean(options.lean));
-  }
-
-  pop.forEach(opts => {
-    opts._localModel = query.model;
-  });
-
-  return pop;
-};
-
 /**
  * Prepare a set of path options for query population. This is the MongooseQuery
  * version
@@ -73,12 +48,18 @@ exports.preparePopulationOptionsMQ = function preparePopulationOptionsMQ(query,
   }
 
   const projection = query._fieldsForExec();
-  pop.forEach(p => {
-    p._queryProjection = projection;
-  });
-  pop.forEach(opts => {
-    opts._localModel = query.model;
-  });
+  for (let i = 0; i < pop.length; ++i) {
+    if (pop[i] instanceof PopulateOptions) {
+      pop[i] = new PopulateOptions({
+        ...pop[i],
+        _queryProjection: projection,
+        _localModel: query.model
+      });
+    } else {
+      pop[i]._queryProjection = projection;
+      pop[i]._localModel = query.model;
+    }
+  }
 
   return pop;
 };