SERVER-41829 findAndModify ignores filter expressions that are not objects

(cherry picked from commit de511c6bbf22d662912f228a3ac7a8e7a8bc3c61)
(cherry picked from commit 0f41fa2d430cca4912820ed0fe929e193b005e94)

4 files changed, 159 insertions, 6 deletions

diff --git a/jstests/core/find_and_modify_invalid_query_params.js b/jstests/core/find_and_modify_invalid_query_params.js
new file mode 100644
index 00000000000..a54d9217b24
--- /dev/null
+++ b/jstests/core/find_and_modify_invalid_query_params.js
@@ -0,0 +1,93 @@
+/**
+ * Test that the 'findAndModify' command throws the expected errors for invalid query, sort and
+ * projection parameters. This test exercises the fix for SERVER-41829.
+ * @tags: [assumes_unsharded_collection]
+ */
+(function() {
+    "use strict";
+
+    const coll = db.find_and_modify_invalid_inputs;
+    coll.drop();
+    coll.insert({_id: 0});
+    coll.insert({_id: 1});
+
+    function assertFailedWithCode(cmd, errorCode) {
+        const err = assert.throws(() => coll.findAndModify(cmd));
+        assert.eq(err.code, errorCode);
+    }
+
+    function assertWorked(cmd, expectedValue) {
+        const out = assert.doesNotThrow(() => coll.findAndModify(cmd));
+        assert.eq(out.value, expectedValue);
+    }
+
+    // Verify that the findAndModify command works when we supply a valid query.
+    let out = coll.findAndModify({query: {_id: 1}, update: {$set: {value: "basic"}}, new: true});
+    assert.eq(out, {_id: 1, value: "basic"});
+
+    // Verify that invalid 'query' object fails.
+    assertFailedWithCode({query: null, update: {value: 2}}, 31160);
+    assertFailedWithCode({query: 1, update: {value: 2}}, 31160);
+    assertFailedWithCode({query: "{_id: 1}", update: {value: 2}}, 31160);
+    assertFailedWithCode({query: false, update: {value: 2}}, 31160);
+
+    // Verify that missing and empty query object is allowed.
+    assertWorked({update: {$set: {value: "missingQuery"}}, new: true}, "missingQuery");
+    assertWorked({query: {}, update: {$set: {value: "emptyQuery"}}, new: true}, "emptyQuery");
+
+    // Verify that command works when we supply a valid sort specification.
+    assertWorked({sort: {_id: -1}, update: {$set: {value: "sort"}}, new: true}, "sort");
+
+    // Verify that invaid 'sort' object fails.
+    assertFailedWithCode({sort: null, update: {value: 2}}, 31174);
+    assertFailedWithCode({sort: 1, update: {value: 2}}, 31174);
+    assertFailedWithCode({sort: "{_id: 1}", update: {value: 2}}, 31174);
+    assertFailedWithCode({sort: false, update: {value: 2}}, 31174);
+
+    // Verify that missing and empty 'sort' object is allowed.
+    assertWorked({update: {$set: {value: "missingSort"}}, new: true}, "missingSort");
+    assertWorked({sort: {}, update: {$set: {value: "emptySort"}}, new: true}, "emptySort");
+
+    // Verify that the 'fields' projection works.
+    assertWorked({fields: {_id: 0}, update: {$set: {value: "project"}}, new: true}, "project");
+
+    // Verify that invaid 'fields' object fails.
+    assertFailedWithCode({fields: null, update: {value: 2}}, 31175);
+    assertFailedWithCode({fields: 1, update: {value: 2}}, 31175);
+    assertFailedWithCode({fields: "{_id: 1}", update: {value: 2}}, 31175);
+    assertFailedWithCode({fields: false, update: {value: 2}}, 31175);
+
+    // Verify that missing and empty 'fields' object is allowed. Also verify that the command
+    // projects all the fields.
+    assertWorked({update: {$set: {value: "missingFields"}}, new: true}, "missingFields");
+    assertWorked({fields: {}, update: {$set: {value: "emptyFields"}}, new: true}, "emptyFields");
+
+    // Verify that findOneAndDelete() shell helper throws the same errors as findAndModify().
+    let err = assert.throws(() => coll.findOneAndDelete("{_id: 1}"));
+    assert.eq(err.code, 31160);
+    err = assert.throws(() => coll.findOneAndDelete(null, {sort: 1}));
+    assert.eq(err.code, 31174);
+
+    // Verify that findOneAndReplace() shell helper throws the same errors as findAndModify().
+    err = assert.throws(() => coll.findOneAndReplace("{_id: 1}", {}));
+    assert.eq(err.code, 31160);
+    err = assert.throws(() => coll.findOneAndReplace(null, {}, {sort: 1}));
+    assert.eq(err.code, 31174);
+
+    // Verify that findOneAndUpdate() shell helper throws the same errors as findAndModify().
+    err = assert.throws(() => coll.findOneAndUpdate("{_id: 1}", {$set: {value: "new"}}));
+    assert.eq(err.code, 31160);
+    err = assert.throws(() => coll.findOneAndUpdate(null, {$set: {value: "new"}}, {sort: 1}));
+    assert.eq(err.code, 31174);
+
+    // Verify that find and modify shell helpers allow null query object.
+    out =
+        coll.findOneAndUpdate(null, {$set: {value: "findOneAndUpdate"}}, {returnNewDocument: true});
+    assert.eq(out.value, "findOneAndUpdate");
+
+    out = coll.findOneAndReplace(null, {value: "findOneAndReplace"}, {returnNewDocument: true});
+    assert.eq(out.value, "findOneAndReplace");
+
+    out = coll.findOneAndDelete(null);
+    assert.eq(out.value, "findOneAndReplace");
+})();
diff --git a/src/mongo/db/query/find_and_modify_request.cpp b/src/mongo/db/query/find_and_modify_request.cpp
index e7ba7bab801..68c895baf21 100644
--- a/src/mongo/db/query/find_and_modify_request.cpp
+++ b/src/mongo/db/query/find_and_modify_request.cpp
@@ -110,10 +110,38 @@ BSONObj FindAndModifyRequest::toBSON() const {
 
 StatusWith<FindAndModifyRequest> FindAndModifyRequest::parseFromBSON(NamespaceString fullNs,
                                                                      const BSONObj& cmdObj) {
-    BSONObj query = cmdObj.getObjectField(kQueryField);
-    BSONObj fields = cmdObj.getObjectField(kFieldProjectionField);
+    BSONObj query;
+    BSONObj sort;
+    BSONObj fields;
+    if (auto queryElement = cmdObj[kQueryField]) {
+        if (queryElement.type() != Object) {
+            return {ErrorCodes::Error(31160),
+                    str::stream() << "'" << kQueryField << "' parameter must be an object, found "
+                                  << queryElement.type()};
+        }
+        query = queryElement.embeddedObject();
+    }
+
+    if (auto sortElement = cmdObj[kSortField]) {
+        if (sortElement.type() != Object) {
+            return {ErrorCodes::Error(31174),
+                    str::stream() << "'" << kSortField << "' parameter must be an object, found "
+                                  << sortElement.type()};
+        }
+        sort = sortElement.embeddedObject();
+    }
+
+    if (auto projectionElement = cmdObj[kFieldProjectionField]) {
+        if (projectionElement.type() != Object) {
+            return {ErrorCodes::Error(31175),
+                    str::stream() << "'" << kFieldProjectionField
+                                  << "' parameter must be an object, found "
+                                  << projectionElement.type()};
+        }
+        fields = projectionElement.embeddedObject();
+    }
+
     BSONObj updateObj = cmdObj.getObjectField(kUpdateField);
-    BSONObj sort = cmdObj.getObjectField(kSortField);
 
     BSONObj collation;
     {
diff --git a/src/mongo/db/query/find_and_modify_request_test.cpp b/src/mongo/db/query/find_and_modify_request_test.cpp
index 57b3f3bacc0..6cb827363c1 100644
--- a/src/mongo/db/query/find_and_modify_request_test.cpp
+++ b/src/mongo/db/query/find_and_modify_request_test.cpp
@@ -483,5 +483,37 @@ TEST(FindAndModifyRequest, ParseWithCollationTypeMismatch) {
     ASSERT_EQUALS(parseStatus.getStatus(), ErrorCodes::TypeMismatch);
 }
 
+TEST(FindAndModifyRequest, InvalidQueryParameter) {
+    BSONObj cmdObj(fromjson(R"json({
+            findAndModify: 'user',
+            query: '{ x: 1 }',
+            remove: true
+        })json"));
+
+    auto parseStatus = FindAndModifyRequest::parseFromBSON(NamespaceString("a.b"), cmdObj);
+    ASSERT_EQ(31160, parseStatus.getStatus().code());
+}
+
+TEST(FindAndModifyRequest, InvalidSortParameter) {
+    BSONObj cmdObj(fromjson(R"json({
+            findAndModify: 'user',
+            sort: 1,
+            remove: true
+        })json"));
+
+    auto parseStatus = FindAndModifyRequest::parseFromBSON(NamespaceString("a.b"), cmdObj);
+    ASSERT_EQ(31174, parseStatus.getStatus().code());
+}
+
+TEST(FindAndModifyRequest, InvalidFieldParameter) {
+    BSONObj cmdObj(fromjson(R"json({
+            findAndModify: 'user',
+            fields: null,
+            remove: true
+        })json"));
+
+    auto parseStatus = FindAndModifyRequest::parseFromBSON(NamespaceString("a.b"), cmdObj);
+    ASSERT_EQ(31175, parseStatus.getStatus().code());
+}
 }  // unnamed namespace
 }  // namespace mongo
diff --git a/src/mongo/shell/crud_api.js b/src/mongo/shell/crud_api.js
index e9678cad16a..05f444b867b 100644
--- a/src/mongo/shell/crud_api.js
+++ b/src/mongo/shell/crud_api.js
@@ -667,7 +667,7 @@ DBCollection.prototype.updateMany = function(filter, update, options) {
 DBCollection.prototype.findOneAndDelete = function(filter, options) {
     var opts = Object.extend({}, options || {});
     // Set up the command
-    var cmd = {query: filter, remove: true};
+    var cmd = {query: filter || {}, remove: true};
 
     if (opts.sort) {
         cmd.sort = opts.sort;
@@ -725,7 +725,7 @@ DBCollection.prototype.findOneAndReplace = function(filter, replacement, options
     }
 
     // Set up the command
-    var cmd = {query: filter, update: replacement};
+    var cmd = {query: filter || {}, update: replacement};
     if (opts.sort) {
         cmd.sort = opts.sort;
     }
@@ -790,7 +790,7 @@ DBCollection.prototype.findOneAndUpdate = function(filter, update, options) {
     }
 
     // Set up the command
-    var cmd = {query: filter, update: update};
+    var cmd = {query: filter || {}, update: update};
     if (opts.sort) {
         cmd.sort = opts.sort;
     }