SERVER-17387 prevent invalid projections from causing findAndModify t…

…o trigger a logOp() rollback

jstests/core/find_and_modify.js

@@ -45,9 +45,71 @@ assert.throws(function() { t.findAndModify({query:{x:1}, update:{y:2}, remove:tr
 assert.throws(function() { t.findAndModify({query:{x:1}, update:{y:2}, new:true, remove:true}); });
 assert.throws(function() { t.findAndModify({query:{x:1}, upsert:true, remove:true}); });
 
-// SERVER-17372
+//
+// SERVER-17387: Find and modify should throw in the case of invalid projection.
+//
+
 t.drop();
+
+// Insert case.
 var cmdRes = db.runCommand({
+    findAndModify: t.getName(),
+    query: {_id: "miss"},
+    update: {$inc: {y: 1}},
+    fields: {foo: {$pop: ["bar"]}},
+    upsert: true,
+    new: true
+});
+assert.commandFailed(cmdRes);
+
+t.insert({_id: "found"});
+
+// Update with upsert + new.
+cmdRes = db.runCommand({
+    findAndModify: t.getName(),
+    query: {_id: "found"},
+    update: {$inc: {y: 1}},
+    fields: {foo: {$pop: ["bar"]}},
+    upsert: true,
+    new: true
+});
+assert.commandFailed(cmdRes);
+
+// Update with just new: true.
+cmdRes = db.runCommand({
+    findAndModify: t.getName(),
+    query: {_id: "found"},
+    update: {$inc: {y: 1}},
+    fields: {foo: {$pop: ["bar"]}},
+    new: true
+});
+assert.commandFailed(cmdRes);
+
+// Update with just upsert: true.
+cmdRes = db.runCommand({
+    findAndModify: t.getName(),
+    query: {_id: "found"},
+    update: {$inc: {y: 1}},
+    fields: {foo: {$pop: ["bar"]}},
+    upsert: true
+});
+assert.commandFailed(cmdRes);
+
+// Update with neither upsert nor new flags.
+cmdRes = db.runCommand({
+    findAndModify: t.getName(),
+    query: {_id: "found"},
+    update: {$inc: {y: 1}},
+    fields: {foo: {$pop: ["bar"]}},
+});
+assert.commandFailed(cmdRes);
+
+//
+// SERVER-17372
+//
+
+t.drop();
+cmdRes = db.runCommand({
     findAndModify: t.getName(),
     query: {_id: "miss"},
     update: {$inc: {y: 1}},
@@ -57,7 +119,7 @@ assert.commandWorked(cmdRes);
 assert("value" in cmdRes);
 assert.eq(null, cmdRes.value);
 
-var cmdRes = db.runCommand({
+cmdRes = db.runCommand({
     findAndModify: t.getName(),
     query: {_id: "missagain"},
     update: {$inc: {y: 1}},

src/mongo/db/commands/find_and_modify.cpp

@@ -372,6 +372,14 @@ namespace mongo {
                 if ( found ) {
                     deleteObjects(txn, ctx.db(), ns, queryModified, PlanExecutor::YIELD_MANUAL,
                                   true, true);
+
+                    // Committing the WUOW can close the current snapshot. Until this happens, the
+                    // snapshot id should not have changed.
+                    invariant(txn->recoveryUnit()->getSnapshotId() == snapshotDoc.snapshotId());
+
+                    // Must commit the write before doing anything that could throw.
+                    wuow.commit();
+
                     BSONObjBuilder le( result.subobjStart( "lastErrorObject" ) );
                     le.appendNumber( "n" , 1 );
                     le.done();
@@ -424,6 +432,9 @@ namespace mongo {
                         // for some BSON manipulation).
                         repl::logOp(txn, "i", collection->ns().ns().c_str(), newDoc);
 
+                        // Must commit the write and logOp() before doing anything that could throw.
+                        wuow.commit();
+
                         // The third argument indicates whether or not we have something for the
                         // 'value' field returned by a findAndModify command.
                         //
@@ -470,6 +481,13 @@ namespace mongo {
                     invariant(res.existing);
                     LOG(3) << "update result: "  << res;
 
+                    // Committing the WUOW can close the current snapshot. Until this happens, the
+                    // snapshot id should not have changed.
+                    invariant(txn->recoveryUnit()->getSnapshotId() == snapshotDoc.snapshotId());
+
+                    // Must commit the write before doing anything that could throw.
+                    wuow.commit();
+
                     if (returnNew) {
                         dassert(!res.newObj.isEmpty());
                         _appendHelper(result, res.newObj, true, fields, whereCallback);
@@ -485,13 +503,6 @@ namespace mongo {
                 }
             }
 
-            // Committing the WUOW can close the current snapshot. Until this happens, the
-            // snapshot id should not have changed.
-            if (found) {
-                invariant(txn->recoveryUnit()->getSnapshotId() == snapshotDoc.snapshotId());
-            }
-            wuow.commit();
-
             return true;
         }