Skip to content

Commit

Permalink
Merge pull request #15082 from Automattic/vkarpov15/gh-15026
Browse files Browse the repository at this point in the history 
fix(query): clone PopulateOptions when setting _localModel to avoid state leaking between subpopulate instances
  • Loading branch information
@vkarpov15
vkarpov15 authored Dec 11, 2024
2 parents 2607904 + 506d125 commit 2241823
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 132 deletions.
141 changes: 43 additions & 98 deletions lib/model.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 = [];
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
);
}
Expand Down
6 changes: 4 additions & 2 deletions lib/query.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down
45 changes: 13 additions & 32 deletions lib/queryHelpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* Module dependencies
*/

const PopulateOptions = require('./options/populateOptions');
const checkEmbeddedDiscriminatorKeyProjection =
require('./helpers/discriminator/checkEmbeddedDiscriminatorKeyProjection');
const get = require('./helpers/get');
Expand All @@ -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
Expand Down Expand Up @@ -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;
};
Expand Down

0 comments on commit 2241823

Please sign in to comment.