Skip to content

Commit

Permalink
Merge pull request #15019 from Automattic/vkarpov15/gh-14948
Browse files Browse the repository at this point in the history 
fix(model+query): make `findOne(null)`, `find(null)`, etc. throw an error instead of returning first doc
  • Loading branch information
@vkarpov15
vkarpov15 authored Nov 8, 2024
2 parents b3d8b10 + cd1d6da commit d36ac59
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 20 deletions.
3 changes: 1 addition & 2 deletions lib/error/objectParameter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@ const MongooseError = require('./mongooseError');
*/

class ObjectParameterError extends MongooseError {

constructor(value, paramName, fnName) {
super('Parameter "' + paramName + '" to ' + fnName +
'() must be an object, got "' + value.toString() + '" (type ' + typeof value + ')');
'() must be an object, got "' + (value == null ? value : value.toString()) + '" (type ' + typeof value + ')');
}
}

Expand Down
6 changes: 3 additions & 3 deletions lib/model.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2288,8 +2288,8 @@ Model.findOneAndUpdate = function(conditions, update, options) {

if (arguments.length === 1) {
update = conditions;
conditions = null;
options = null;
conditions = undefined;
options = undefined;
}

let fields;
Expand Down Expand Up @@ -2864,7 +2864,7 @@ Model.$__insertMany = function(arr, options, callback) {
const _this = this;
if (typeof options === 'function') {
callback = options;
options = null;
options = undefined;
}

callback = callback || utils.noop;
Expand Down
48 changes: 33 additions & 15 deletions lib/query.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,10 @@ Query.prototype._validateOp = function() {
if (this.op != null && !validOpsSet.has(this.op)) {
this.error(new Error('Query has invalid `op`: "' + this.op + '"'));
}

if (this.op !== 'estimatedDocumentCount' && this._conditions === null) {
throw new ObjectParameterError(this._conditions, 'filter', this.op);
}
};

/**
Expand Down Expand Up @@ -2416,7 +2420,7 @@ Query.prototype.find = function(conditions) {

this.op = 'find';

if (mquery.canMerge(conditions)) {
if (canMerge(conditions)) {
this.merge(conditions);

prepareDiscriminatorCriteria(this);
Expand All @@ -2438,9 +2442,14 @@ Query.prototype.find = function(conditions) {

Query.prototype.merge = function(source) {
if (!source) {
if (source === null) {
this._conditions = null;
}
return this;
}

this._conditions = this._conditions ?? {};

const opts = { overwrite: true };

if (source instanceof Query) {
Expand Down Expand Up @@ -2701,7 +2710,7 @@ Query.prototype.findOne = function(conditions, projection, options) {
this.select(projection);
}

if (mquery.canMerge(conditions)) {
if (canMerge(conditions)) {
this.merge(conditions);

prepareDiscriminatorCriteria(this);
Expand Down Expand Up @@ -2875,7 +2884,7 @@ Query.prototype.countDocuments = function(conditions, options) {
this.op = 'countDocuments';
this._validateOp();

if (mquery.canMerge(conditions)) {
if (canMerge(conditions)) {
this.merge(conditions);
}

Expand Down Expand Up @@ -2941,7 +2950,7 @@ Query.prototype.distinct = function(field, conditions, options) {
this.op = 'distinct';
this._validateOp();

if (mquery.canMerge(conditions)) {
if (canMerge(conditions)) {
this.merge(conditions);

prepareDiscriminatorCriteria(this);
Expand Down Expand Up @@ -3109,7 +3118,7 @@ Query.prototype.deleteOne = function deleteOne(filter, options) {
this.op = 'deleteOne';
this.setOptions(options);

if (mquery.canMerge(filter)) {
if (canMerge(filter)) {
this.merge(filter);

prepareDiscriminatorCriteria(this);
Expand Down Expand Up @@ -3182,7 +3191,7 @@ Query.prototype.deleteMany = function(filter, options) {
this.setOptions(options);
this.op = 'deleteMany';

if (mquery.canMerge(filter)) {
if (canMerge(filter)) {
this.merge(filter);

prepareDiscriminatorCriteria(this);
Expand Down Expand Up @@ -3355,7 +3364,7 @@ Query.prototype.findOneAndUpdate = function(filter, doc, options) {
break;
}

if (mquery.canMerge(filter)) {
if (canMerge(filter)) {
this.merge(filter);
} else if (filter != null) {
this.error(
Expand Down Expand Up @@ -3525,7 +3534,7 @@ Query.prototype.findOneAndDelete = function(filter, options) {
this._validateOp();
this._validate();

if (mquery.canMerge(filter)) {
if (canMerge(filter)) {
this.merge(filter);
}

Expand Down Expand Up @@ -3630,7 +3639,7 @@ Query.prototype.findOneAndReplace = function(filter, replacement, options) {
this._validateOp();
this._validate();

if (mquery.canMerge(filter)) {
if (canMerge(filter)) {
this.merge(filter);
} else if (filter != null) {
this.error(
Expand Down Expand Up @@ -4038,13 +4047,13 @@ Query.prototype.updateMany = function(conditions, doc, options, callback) {
if (typeof options === 'function') {
// .update(conditions, doc, callback)
callback = options;
options = null;
options = undefined;
} else if (typeof doc === 'function') {
// .update(doc, callback);
callback = doc;
doc = conditions;
conditions = {};
options = null;
options = undefined;
} else if (typeof conditions === 'function') {
// .update(callback)
callback = conditions;
Expand Down Expand Up @@ -4109,13 +4118,13 @@ Query.prototype.updateOne = function(conditions, doc, options, callback) {
if (typeof options === 'function') {
// .update(conditions, doc, callback)
callback = options;
options = null;
options = undefined;
} else if (typeof doc === 'function') {
// .update(doc, callback);
callback = doc;
doc = conditions;
conditions = {};
options = null;
options = undefined;
} else if (typeof conditions === 'function') {
// .update(callback)
callback = conditions;
Expand Down Expand Up @@ -4176,13 +4185,13 @@ Query.prototype.replaceOne = function(conditions, doc, options, callback) {
if (typeof options === 'function') {
// .update(conditions, doc, callback)
callback = options;
options = null;
options = undefined;
} else if (typeof doc === 'function') {
// .update(doc, callback);
callback = doc;
doc = conditions;
conditions = {};
options = null;
options = undefined;
} else if (typeof conditions === 'function') {
// .update(callback)
callback = conditions;
Expand Down Expand Up @@ -5542,6 +5551,15 @@ Query.prototype.selectedExclusively = function selectedExclusively() {

Query.prototype.model;

/**
* Determine if we can merge the given value as a query filter. Override for mquery.canMerge() to allow null
*/

function canMerge(value) {
return value instanceof Query || utils.isObject(value) || value === null;

}

/*!
* Export
*/
Expand Down
46 changes: 46 additions & 0 deletions test/query.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4412,4 +4412,50 @@ describe('Query', function() {
assert.strictEqual(doc.passwordHash, undefined);
});
});

it('throws an error if calling find(null), findOne(null), updateOne(null, update), etc. (gh-14948)', async function() {
const userSchema = new Schema({
name: String
});
const UserModel = db.model('User', userSchema);
await UserModel.deleteMany({});
await UserModel.updateOne({ name: 'test' }, { name: 'test' }, { upsert: true });

await assert.rejects(
() => UserModel.find(null),
/ObjectParameterError: Parameter "filter" to find\(\) must be an object, got "null"/
);
await assert.rejects(
() => UserModel.findOne(null),
/ObjectParameterError: Parameter "filter" to findOne\(\) must be an object, got "null"/
);
await assert.rejects(
() => UserModel.findOneAndUpdate(null, { name: 'test2' }),
/ObjectParameterError: Parameter "filter" to findOneAndUpdate\(\) must be an object, got "null"/
);
await assert.rejects(
() => UserModel.findOneAndReplace(null, { name: 'test2' }),
/ObjectParameterError: Parameter "filter" to findOneAndReplace\(\) must be an object, got "null"/
);
await assert.rejects(
() => UserModel.findOneAndDelete(null),
/ObjectParameterError: Parameter "filter" to findOneAndDelete\(\) must be an object, got "null"/
);
await assert.rejects(
() => UserModel.updateOne(null, { name: 'test2' }),
/ObjectParameterError: Parameter "filter" to updateOne\(\) must be an object, got "null"/
);
await assert.rejects(
() => UserModel.updateMany(null, { name: 'test2' }),
/ObjectParameterError: Parameter "filter" to updateMany\(\) must be an object, got "null"/
);
await assert.rejects(
() => UserModel.deleteOne(null),
/ObjectParameterError: Parameter "filter" to deleteOne\(\) must be an object, got "null"/
);
await assert.rejects(
() => UserModel.deleteMany(null),
/ObjectParameterError: Parameter "filter" to deleteMany\(\) must be an object, got "null"/
);
});
});

0 comments on commit d36ac59

Please sign in to comment.