From 574239f58f235bbe257ddb67c742c39d7bfac54a Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Thu, 12 Jan 2023 12:49:16 -0500 Subject: [PATCH] fix: more durable handling for minus paths re: #11694 --- lib/model.js | 4 +++- lib/query.js | 20 ++++++++--------- lib/queryhelpers.js | 33 +++++++++++++++++++++++----- test/model.findOneAndDelete.test.js | 2 ++ test/model.findOneAndRemove.test.js | 2 ++ test/model.findOneAndReplace.test.js | 1 + test/model.findOneAndUpdate.test.js | 2 ++ 7 files changed, 47 insertions(+), 17 deletions(-) diff --git a/lib/model.js b/lib/model.js index d3b995eff8e..953080ceb7c 100644 --- a/lib/model.js +++ b/lib/model.js @@ -4885,13 +4885,15 @@ function _execPopulateQuery(mod, match, select, assignmentOpts, callback) { } else if (queryOptions.limit != null) { queryOptions.limit = queryOptions.limit * mod.ids.length; } + const query = mod.model.find(match, select, queryOptions); // If we're doing virtual populate and projection is inclusive and foreign // field is not selected, automatically select it because mongoose needs it. // If projection is exclusive and client explicitly unselected the foreign // field, that's the client's fault. for (const foreignField of mod.foreignField) { - if (foreignField !== '_id' && query.selectedInclusively() && + if (foreignField !== '_id' && + query.selectedInclusively() && !isPathSelectedInclusive(query._fields, foreignField)) { query.select(foreignField); } diff --git a/lib/query.js b/lib/query.js index c4809f99132..2ca300bf317 100644 --- a/lib/query.js +++ b/lib/query.js @@ -1120,11 +1120,11 @@ Query.prototype.select = function select() { if (utils.isObject(arg)) { if (this.selectedInclusively()) { Object.entries(arg).forEach(([key, value]) => { - if (key[0] === '-') { - key = key.slice(1); - } if (value) { // Add the field to the projection + if (fields['-' + key] != null) { + delete fields['-' + key]; + } fields[key] = userProvidedFields[key] = sanitizeValue(value); } else { // Remove the field from the projection @@ -1138,11 +1138,11 @@ Query.prototype.select = function select() { }); } else if (this.selectedExclusively()) { Object.entries(arg).forEach(([key, value]) => { - if (key[0] === '-') { - key = key.slice(1); - } if (!value) { // Add the field to the projection + if (fields['+' + key] != null) { + delete fields['+' + key]; + } fields[key] = userProvidedFields[key] = sanitizeValue(value); } else { // Remove the field from the projection @@ -1158,14 +1158,12 @@ Query.prototype.select = function select() { const keys = Object.keys(arg); for (let i = 0; i < keys.length; ++i) { const value = arg[keys[i]]; - let key = keys[i]; - if (key[0] === '-') { - key = key.slice(1); - } + const key = keys[i]; fields[key] = sanitizeValue(value); userProvidedFields[key] = sanitizeValue(value); } } + return this; } @@ -2521,7 +2519,7 @@ Query.prototype._completeOne = function(doc, res, callback) { * @api private */ -Query.prototype._findOne = wrapThunk(function(callback) { +Query.prototype._findOne = wrapThunk(function _findOne(callback) { this._castConditions(); if (this.error()) { diff --git a/lib/queryhelpers.js b/lib/queryhelpers.js index 47d4dc50795..350344e2fa4 100644 --- a/lib/queryhelpers.js +++ b/lib/queryhelpers.js @@ -148,27 +148,30 @@ exports.applyPaths = function applyPaths(fields, schema) { // determine if query is selecting or excluding fields let exclude; let keys; + const minusPathsToSkip = new Set(); if (fields) { keys = Object.keys(fields); // Collapse minus paths + const minusPaths = []; for (let i = 0; i < keys.length; ++i) { const key = keys[i]; if (keys[i][0] !== '-') { continue; } - const type = schema.path(key); delete fields[key]; - if (!type || !type.selected) { - fields[key.slice(1)] = 0; + if (key === '-_id') { + fields['_id'] = 0; + } else { + minusPaths.push(key.slice(1)); } } keys = Object.keys(fields); for (let keyIndex = 0; keyIndex < keys.length; ++keyIndex) { - if (keys[keyIndex][0] === '+' || keys[keyIndex][0] === '-') { + if (keys[keyIndex][0] === '+') { continue; } const field = fields[keys[keyIndex]]; @@ -184,10 +187,27 @@ exports.applyPaths = function applyPaths(fields, schema) { exclude = !field; break; } + + // Potentially add back minus paths based on schema-level path config + // and whether the projection is inclusive + for (const path of minusPaths) { + const type = schema.path(path); + // If the path isn't selected by default or the projection is not + // inclusive, minus path is treated as equivalent to `key: 0`. + // But we also allow using `-name` to remove `name` from an inclusive + // projection if `name` has schema-level `select: true`. + if ((!type || !type.selected) || exclude !== false) { + fields[path] = 0; + } else if (type && type.selected && exclude === false) { + // Make a note of minus paths that are overwriting paths that are + // included by default. + minusPathsToSkip.add(path); + } + } } + // if selecting, apply default schematype select:true fields // if excluding, apply schematype select:false fields - const selected = []; const excluded = []; const stack = []; @@ -208,6 +228,9 @@ exports.applyPaths = function applyPaths(fields, schema) { } for (const fieldName of selected) { + if (minusPathsToSkip.has(fieldName)) { + continue; + } fields[fieldName] = fields[fieldName] || 1; } break; diff --git a/test/model.findOneAndDelete.test.js b/test/model.findOneAndDelete.test.js index 977f95f2fce..5dadb0d2a64 100644 --- a/test/model.findOneAndDelete.test.js +++ b/test/model.findOneAndDelete.test.js @@ -232,10 +232,12 @@ describe('model: findOneAndDelete:', function() { let query; query = M.findByIdAndDelete(_id, { select: 'author -title' }); + query._applyPaths(); assert.strictEqual(1, query._fields.author); assert.strictEqual(0, query._fields.title); query = M.findOneAndDelete({}, { select: 'author -title' }); + query._applyPaths(); assert.strictEqual(1, query._fields.author); assert.strictEqual(0, query._fields.title); }); diff --git a/test/model.findOneAndRemove.test.js b/test/model.findOneAndRemove.test.js index c966de4d669..023e4541936 100644 --- a/test/model.findOneAndRemove.test.js +++ b/test/model.findOneAndRemove.test.js @@ -239,10 +239,12 @@ describe('model: findOneAndRemove:', async function() { let query; query = M.findByIdAndRemove(_id, { select: 'author -title' }); + query._applyPaths(); assert.strictEqual(1, query._fields.author); assert.strictEqual(0, query._fields.title); query = M.findOneAndRemove({}, { select: 'author -title' }); + query._applyPaths(); assert.strictEqual(1, query._fields.author); assert.strictEqual(0, query._fields.title); done(); diff --git a/test/model.findOneAndReplace.test.js b/test/model.findOneAndReplace.test.js index c303bb598f8..cd2014c39dc 100644 --- a/test/model.findOneAndReplace.test.js +++ b/test/model.findOneAndReplace.test.js @@ -226,6 +226,7 @@ describe('model: findOneAndReplace:', function() { const M = BlogPost; const query = M.findOneAndReplace({}, {}, { select: 'author -title' }); + query._applyPaths(); assert.strictEqual(1, query._fields.author); assert.strictEqual(0, query._fields.title); }); diff --git a/test/model.findOneAndUpdate.test.js b/test/model.findOneAndUpdate.test.js index 3e7b3660bd7..b616eb9de9c 100644 --- a/test/model.findOneAndUpdate.test.js +++ b/test/model.findOneAndUpdate.test.js @@ -652,10 +652,12 @@ describe('model: findOneAndUpdate:', function() { let query; query = M.findByIdAndUpdate(_id, { $set: { date: now } }, { select: 'author -title' }); + query._applyPaths(); assert.strictEqual(1, query._fields.author); assert.strictEqual(0, query._fields.title); query = M.findOneAndUpdate({}, { $set: { date: now } }, { select: 'author -title' }); + query._applyPaths(); assert.strictEqual(1, query._fields.author); assert.strictEqual(0, query._fields.title); });