Merge pull request #13338 from Automattic/vkarpov15/gh-13264

fix(query): set ObjectParameterError if calling `findOneAndX()` with filter as non-object
Automattic · Apr 27, 2023 · 2bb0fc2 · 2bb0fc2
2 parents c6398f3 + a50aa54
commit 2bb0fc2
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 18 deletions.
diff --git a/lib/error/objectParameter.js b/lib/error/objectParameter.js
@@ -18,7 +18,7 @@ class ObjectParameterError extends MongooseError {
    */
   constructor(value, paramName, fnName) {
     super('Parameter "' + paramName + '" to ' + fnName +
-      '() must be an object, got ' + value.toString());
+      '() must be an object, got "' + value.toString() + '" (type ' + typeof value + ')');
   }
 }
 

diff --git a/lib/query.js b/lib/query.js
@@ -3411,7 +3411,7 @@ function prepareDiscriminatorCriteria(query) {
  * @api public
  */
 
-Query.prototype.findOneAndUpdate = function(criteria, doc, options, callback) {
+Query.prototype.findOneAndUpdate = function(filter, doc, options, callback) {
   this.op = 'findOneAndUpdate';
   this._validateOp();
   this._validate();
@@ -3426,23 +3426,27 @@ Query.prototype.findOneAndUpdate = function(criteria, doc, options, callback) {
     case 2:
       if (typeof doc === 'function') {
         callback = doc;
-        doc = criteria;
-        criteria = undefined;
+        doc = filter;
+        filter = undefined;
       }
       options = undefined;
       break;
     case 1:
-      if (typeof criteria === 'function') {
-        callback = criteria;
-        criteria = options = doc = undefined;
+      if (typeof filter === 'function') {
+        callback = filter;
+        filter = options = doc = undefined;
       } else {
-        doc = criteria;
-        criteria = options = undefined;
+        doc = filter;
+        filter = options = undefined;
       }
   }
 
-  if (mquery.canMerge(criteria)) {
-    this.merge(criteria);
+  if (mquery.canMerge(filter)) {
+    this.merge(filter);
+  } else if (filter != null) {
+    this.error(
+      new ObjectParameterError(filter, 'filter', 'findOneAndUpdate')
+    );
   }
 
   // apply doc
@@ -3620,7 +3624,7 @@ Query.prototype.findOneAndRemove = function(conditions, options, callback) {
  *
  * @method findOneAndDelete
  * @memberOf Query
- * @param {Object} [conditions]
+ * @param {Object} [filter]
  * @param {Object} [options]
  * @param {Boolean} [options.rawResult] if true, returns the [raw result from the MongoDB driver](https://mongodb.github.io/node-mongodb-native/4.9/interfaces/ModifyResult.html)
  * @param {ClientSession} [options.session=null] The session associated with this query. See [transactions docs](/docs/transactions.html).
@@ -3631,7 +3635,7 @@ Query.prototype.findOneAndRemove = function(conditions, options, callback) {
  * @api public
  */
 
-Query.prototype.findOneAndDelete = function(conditions, options, callback) {
+Query.prototype.findOneAndDelete = function(filter, options, callback) {
   this.op = 'findOneAndDelete';
   this._validateOp();
   this._validate();
@@ -3644,16 +3648,20 @@ Query.prototype.findOneAndDelete = function(conditions, options, callback) {
       }
       break;
     case 1:
-      if (typeof conditions === 'function') {
-        callback = conditions;
-        conditions = undefined;
+      if (typeof filter === 'function') {
+        callback = filter;
+        filter = undefined;
         options = undefined;
       }
       break;
   }
 
-  if (mquery.canMerge(conditions)) {
-    this.merge(conditions);
+  if (mquery.canMerge(filter)) {
+    this.merge(filter);
+  } else if (filter != null) {
+    this.error(
+      new ObjectParameterError(filter, 'filter', 'findOneAndDelete')
+    );
   }
 
   options && this.setOptions(options);
@@ -3789,6 +3797,10 @@ Query.prototype.findOneAndReplace = function(filter, replacement, options, callb
 
   if (mquery.canMerge(filter)) {
     this.merge(filter);
+  } else if (filter != null) {
+    this.error(
+      new ObjectParameterError(filter, 'filter', 'findOneAndReplace')
+    );
   }
 
   if (replacement != null) {

diff --git a/test/model.findOneAndUpdate.test.js b/test/model.findOneAndUpdate.test.js
@@ -2403,4 +2403,14 @@ describe('model: findOneAndUpdate:', function() {
 
     assert.ifError(err);
   });
+
+  it('throws error if filter is not an object (gh-13264)', async function() {
+    const schema = new Schema({ name: String });
+    const Model = db.model('Test', schema);
+
+    const err = await Model.findOneAndUpdate('foobar', { name: 'foo' }).then(() => null, err => err);
+    assert.ok(err);
+    assert.equal(err.name, 'ObjectParameterError');
+  });
+
 });