SERVER-29802 clean up applyOps precondition checking

(cherry picked from commit c6f9f146b15b6f84613ee5f6b97572c6f9f5a1ba)

1 files changed, 51 insertions, 44 deletions

diff --git a/src/mongo/db/catalog/apply_ops.cpp b/src/mongo/db/catalog/apply_ops.cpp
index 92c2be22772..6b700785ec9 100644
--- a/src/mongo/db/catalog/apply_ops.cpp
+++ b/src/mongo/db/catalog/apply_ops.cpp
@@ -56,6 +56,8 @@
 namespace mongo {
 namespace {
 
+const auto kPreconditionFieldName = "preCondition"_sd;
+
 // If enabled, causes loop in _applyOps() to hang after applying current operation.
 MONGO_FP_DECLARE(applyOpsPauseBetweenOperations);
 
@@ -234,7 +236,7 @@ Status _applyOps(OperationContext* opCtx,
 
         for (auto elem : applyOpCmd) {
             auto name = elem.fieldNameStringData();
-            if (name == "preCondition")
+            if (name == kPreconditionFieldName)
                 continue;
             if (name == "bypassDocumentValidation")
                 continue;
@@ -275,51 +277,53 @@ Status _applyOps(OperationContext* opCtx,
     return Status::OK();
 }
 
-Status preconditionOK(OperationContext* opCtx, const BSONObj& applyOpCmd, BSONObjBuilder* result) {
-    dassert(opCtx->lockState()->isLockHeldForMode(
-        ResourceId(RESOURCE_GLOBAL, ResourceId::SINGLETON_GLOBAL), MODE_X));
+bool _hasPrecondition(const BSONObj& applyOpCmd) {
+    return applyOpCmd[kPreconditionFieldName].type() == Array;
+}
 
-    if (applyOpCmd["preCondition"].type() == Array) {
-        BSONObjIterator i(applyOpCmd["preCondition"].Obj());
-        while (i.more()) {
-            BSONObj preCondition = i.next().Obj();
-            if (preCondition["ns"].type() != BSONType::String) {
-                return {ErrorCodes::InvalidNamespace,
-                        str::stream() << "ns in preCondition must be a string, but found type: "
-                                      << typeName(preCondition["ns"].type())};
-            }
-            const NamespaceString nss(preCondition["ns"].valueStringData());
-            if (!nss.isValid()) {
-                return {ErrorCodes::InvalidNamespace, "invalid ns: " + nss.ns()};
-            }
+Status _checkPrecondition(OperationContext* opCtx,
+                          const BSONObj& applyOpCmd,
+                          BSONObjBuilder* result) {
+    invariant(opCtx->lockState()->isW());
+    invariant(_hasPrecondition(applyOpCmd));
+
+    for (auto elem : applyOpCmd[kPreconditionFieldName].Obj()) {
+        auto preCondition = elem.Obj();
+        if (preCondition["ns"].type() != BSONType::String) {
+            return {ErrorCodes::InvalidNamespace,
+                    str::stream() << "ns in preCondition must be a string, but found type: "
+                                  << typeName(preCondition["ns"].type())};
+        }
+        const NamespaceString nss(preCondition["ns"].valueStringData());
+        if (!nss.isValid()) {
+            return {ErrorCodes::InvalidNamespace, "invalid ns: " + nss.ns()};
+        }
 
-            DBDirectClient db(opCtx);
-            BSONObj realres = db.findOne(nss.ns(), preCondition["q"].Obj());
+        DBDirectClient db(opCtx);
+        BSONObj realres = db.findOne(nss.ns(), preCondition["q"].Obj());
 
-            // Get collection default collation.
-            Database* database = dbHolder().get(opCtx, nss.db());
-            if (!database) {
-                return {ErrorCodes::NamespaceNotFound,
-                        "database in ns does not exist: " + nss.ns()};
-            }
-            Collection* collection = database->getCollection(nss.ns());
-            if (!collection) {
-                return {ErrorCodes::NamespaceNotFound,
-                        "collection in ns does not exist: " + nss.ns()};
-            }
-            const CollatorInterface* collator = collection->getDefaultCollator();
-
-            // Apply-ops would never have a $where/$text matcher. Using the "DisallowExtensions"
-            // callback ensures that parsing will throw an error if $where or $text are found.
-            Matcher matcher(
-                preCondition["res"].Obj(), ExtensionsCallbackDisallowExtensions(), collator);
-            if (!matcher.matches(realres)) {
-                result->append("got", realres);
-                result->append("whatFailed", preCondition);
-                return {ErrorCodes::BadValue, "preCondition failed"};
-            }
+        // Get collection default collation.
+        Database* database = dbHolder().get(opCtx, nss.db());
+        if (!database) {
+            return {ErrorCodes::NamespaceNotFound, "database in ns does not exist: " + nss.ns()};
+        }
+        Collection* collection = database->getCollection(nss.ns());
+        if (!collection) {
+            return {ErrorCodes::NamespaceNotFound, "collection in ns does not exist: " + nss.ns()};
+        }
+        const CollatorInterface* collator = collection->getDefaultCollator();
+
+        // Apply-ops would never have a $where/$text matcher. Using the "DisallowExtensions"
+        // callback ensures that parsing will throw an error if $where or $text are found.
+        Matcher matcher(
+            preCondition["res"].Obj(), ExtensionsCallbackDisallowExtensions(), collator);
+        if (!matcher.matches(realres)) {
+            result->append("got", realres);
+            result->append("whatFailed", preCondition);
+            return {ErrorCodes::BadValue, "preCondition failed"};
         }
     }
+
     return Status::OK();
 }
 }  // namespace
@@ -333,6 +337,7 @@ Status applyOps(OperationContext* opCtx,
         bsonExtractBooleanFieldWithDefault(applyOpCmd, "allowAtomic", true, &allowAtomic));
     auto areOpsCrudOnly = _areOpsCrudOnly(applyOpCmd);
     auto isAtomic = allowAtomic && areOpsCrudOnly;
+    auto hasPrecondition = _hasPrecondition(applyOpCmd);
 
     ScopedTransaction scopedXact(opCtx, MODE_X);
     Lock::GlobalWrite globalWriteLock(opCtx->lockState());
@@ -344,9 +349,11 @@ Status applyOps(OperationContext* opCtx,
         return Status(ErrorCodes::NotMaster,
                       str::stream() << "Not primary while applying ops to database " << dbName);
 
-    Status preconditionStatus = preconditionOK(opCtx, applyOpCmd, result);
-    if (!preconditionStatus.isOK()) {
-        return preconditionStatus;
+    if (hasPrecondition) {
+        auto status = _checkPrecondition(opCtx, applyOpCmd, result);
+        if (!status.isOK()) {
+            return status;
+        }
     }
 
     int numApplied = 0;