Skip to content

Commit

Permalink
fix(model+query): make findOne(null), find(null), etc. throw an e…
Browse files Browse the repository at this point in the history 
…rror instead of returning first doc

Re: #14948
  • Loading branch information
@vkarpov15
vkarpov15 committed Nov 5, 2024
1 parent 9df19b6 commit 15027c9
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 18 deletions.
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
44 changes: 29 additions & 15 deletions lib/query.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2414,7 +2414,7 @@ Query.prototype.find = function(conditions) {

this.op = 'find';

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

prepareDiscriminatorCriteria(this);
Expand All @@ -2436,9 +2436,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 @@ -2700,7 +2705,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 @@ -2874,7 +2879,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 @@ -2940,7 +2945,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 @@ -3108,7 +3113,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 @@ -3181,7 +3186,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 @@ -3354,7 +3359,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 @@ -3524,7 +3529,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 @@ -3629,7 +3634,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 @@ -4037,13 +4042,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 @@ -4108,13 +4113,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 @@ -4175,13 +4180,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 @@ -5541,6 +5546,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),
/MongoServerError: Expected field filterto be of type object/
);
await assert.rejects(
() => UserModel.findOne(null),
/MongoServerError: Expected field filterto be of type object/
);
await assert.rejects(
() => UserModel.findOneAndUpdate(null, { name: 'test2' }),
/MongoInvalidArgumentError: Argument "filter" must be an object/
);
await assert.rejects(
() => UserModel.findOneAndReplace(null, { name: 'test2' }),
/MongoInvalidArgumentError: Argument "filter" must be an object/
);
await assert.rejects(
() => UserModel.findOneAndDelete(null),
/MongoInvalidArgumentError: Argument "filter" must be an object/
);
await assert.rejects(
() => UserModel.updateOne(null, { name: 'test2' }),
/MongoInvalidArgumentError: Selector must be a valid JavaScript object/
);
await assert.rejects(
() => UserModel.updateMany(null, { name: 'test2' }),
/MongoInvalidArgumentError: Selector must be a valid JavaScript object/
);
await assert.rejects(
() => UserModel.deleteOne(null),
/MongoServerError: BSON field 'delete.deletes.q' is missing but a required field/
);
await assert.rejects(
() => UserModel.deleteMany(null),
/MongoServerError: BSON field 'delete.deletes.q' is missing but a required field/
);
});
});

0 comments on commit 15027c9

Please sign in to comment.