Revert "SERVER-30106 Allow syncTail replication to write to drop-pending collections"

This reverts commit d8afe01c37aa24bef255e8aea4ec6882df71b1dd.

8 files changed, 71 insertions, 56 deletions

diff --git a/src/mongo/db/cloner.cpp b/src/mongo/db/cloner.cpp
index 7c26f24ed88..42081c94b79 100644
--- a/src/mongo/db/cloner.cpp
+++ b/src/mongo/db/cloner.cpp
@@ -539,7 +539,7 @@ StatusWith<std::vector<BSONObj>> Cloner::filterCollectionsForClone(
         const NamespaceString ns(opts.fromDB, collectionName.c_str());
 
         if (ns.isSystem()) {
-            if (!ns.isLegalClientSystemNS()) {
+            if (legalClientSystemNS(ns.ns()) == 0) {
                 LOG(2) << "\t\t not cloning because system collection";
                 continue;
             }
diff --git a/src/mongo/db/concurrency/lock_state.cpp b/src/mongo/db/concurrency/lock_state.cpp
index 721067e3aad..710de4ac234 100644
--- a/src/mongo/db/concurrency/lock_state.cpp
+++ b/src/mongo/db/concurrency/lock_state.cpp
@@ -505,8 +505,6 @@ bool LockerImpl<IsForMMAPV1>::isCollectionLockedForMode(StringData ns, LockMode
     const ResourceId resIdDb(RESOURCE_DATABASE, nss.db());
 
     LockMode dbMode = getLockMode(resIdDb);
-    if (!shouldConflictWithSecondaryBatchApplication())
-        return true;
 
     switch (dbMode) {
         case MODE_NONE:
diff --git a/src/mongo/db/namespace_string.cpp b/src/mongo/db/namespace_string.cpp
index eaf1fab39ff..b9cedd47a95 100644
--- a/src/mongo/db/namespace_string.cpp
+++ b/src/mongo/db/namespace_string.cpp
@@ -79,6 +79,33 @@ constexpr auto dropPendingNSPrefix = "system.drop."_sd;
 
 }  // namespace
 
+bool legalClientSystemNS(StringData ns) {
+    if (ns == "local.system.replset")
+        return true;
+
+    if (ns.find(".system.users") != string::npos)
+        return true;
+
+    if (ns == "admin.system.roles")
+        return true;
+    if (ns == kServerConfiguration)
+        return true;
+    if (ns == kLogicalTimeKeysCollection)
+        return true;
+    if (ns == "admin.system.new_users")
+        return true;
+    if (ns == "admin.system.backup_users")
+        return true;
+
+    if (ns.find(".system.js") != string::npos)
+        return true;
+
+    if (nsToCollectionSubstring(ns) == NamespaceString::kSystemDotViewsCollectionName)
+        return true;
+
+    return false;
+}
+
 constexpr StringData NamespaceString::kAdminDb;
 constexpr StringData NamespaceString::kLocalDb;
 constexpr StringData NamespaceString::kConfigDb;
@@ -102,33 +129,6 @@ bool NamespaceString::isCollectionlessAggregateNS() const {
     return coll() == collectionlessAggregateCursorCol;
 }
 
-bool NamespaceString::isLegalClientSystemNS() const {
-    if (db() == "admin") {
-        if (ns() == "admin.system.roles")
-            return true;
-        if (ns() == kServerConfiguration)
-            return true;
-        if (ns() == kLogicalTimeKeysCollection)
-            return true;
-        if (ns() == "admin.system.new_users")
-            return true;
-        if (ns() == "admin.system.backup_users")
-            return true;
-    }
-    if (ns() == "local.system.replset")
-        return true;
-
-    if (coll() == "system.users")
-        return true;
-    if (coll() == "system.js")
-        return true;
-
-    if (coll() == kSystemDotViewsCollectionName)
-        return true;
-
-    return false;
-}
-
 NamespaceString NamespaceString::makeListCollectionsNSS(StringData dbName) {
     NamespaceString nss(dbName, listCollectionsCursorCol);
     dassert(nss.isValid());
diff --git a/src/mongo/db/namespace_string.h b/src/mongo/db/namespace_string.h
index 5572eaab886..24abea86771 100644
--- a/src/mongo/db/namespace_string.h
+++ b/src/mongo/db/namespace_string.h
@@ -43,6 +43,11 @@ namespace mongo {
 
 const size_t MaxDatabaseNameLen = 128;  // max str len for the db name, including null char
 
+/** @return true if a client can modify this namespace even though it is under ".system."
+    For example <dbname>.system.users is ok for regular clients to update.
+*/
+bool legalClientSystemNS(StringData ns);
+
 /* e.g.
    NamespaceString ns("acme.orders");
    cout << ns.coll; // "orders"
@@ -222,12 +227,6 @@ public:
     bool isListIndexesCursorNS() const;
 
     /**
-     * Returns true if a client can modify this namespace even though it is under ".system."
-     * For example <dbname>.system.users is ok for regular clients to update.
-     */
-    bool isLegalClientSystemNS() const;
-
-    /**
      * Given a NamespaceString for which isGloballyManagedNamespace() returns true, returns the
      * namespace the command targets, or boost::none for commands like 'listCollections' which
      * do not target a collection.
diff --git a/src/mongo/db/query/get_executor.cpp b/src/mongo/db/query/get_executor.cpp
index bb2d6a6b68e..5367ce9c82b 100644
--- a/src/mongo/db/query/get_executor.cpp
+++ b/src/mongo/db/query/get_executor.cpp
@@ -696,8 +696,8 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorDelete(
 
     const NamespaceString& nss(request->getNamespaceString());
     if (!request->isGod()) {
-        if (nss.isSystem() && opCtx->lockState()->shouldConflictWithSecondaryBatchApplication()) {
-            uassert(12050, "cannot delete from system namespace", nss.isLegalClientSystemNS());
+        if (nss.isSystem()) {
+            uassert(12050, "cannot delete from system namespace", legalClientSystemNS(nss.ns()));
         }
         if (nss.isVirtualized()) {
             log() << "cannot delete from a virtual collection: " << nss;
@@ -824,21 +824,33 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorDelete(
 // Update
 //
 
+namespace {
+
+// TODO: Make this a function on NamespaceString, or make it cleaner.
+inline void validateUpdate(const char* ns, const BSONObj& updateobj, const BSONObj& patternOrig) {
+    uassert(10155, "cannot update reserved $ collection", strchr(ns, '$') == 0);
+    if (strstr(ns, ".system.")) {
+        /* dm: it's very important that system.indexes is never updated as IndexDetails
+           has pointers into it */
+        uassert(10156,
+                str::stream() << "cannot update system collection: " << ns << " q: " << patternOrig
+                              << " u: "
+                              << updateobj,
+                legalClientSystemNS(ns));
+    }
+}
+
+}  // namespace
+
 StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorUpdate(
     OperationContext* opCtx, OpDebug* opDebug, Collection* collection, ParsedUpdate* parsedUpdate) {
     const UpdateRequest* request = parsedUpdate->getRequest();
     UpdateDriver* driver = parsedUpdate->getDriver();
 
-    const NamespaceString& nss = request->getNamespaceString();
+    const NamespaceString& nsString = request->getNamespaceString();
     UpdateLifecycle* lifecycle = request->getLifecycle();
 
-    if (nss.isSystem() && opCtx->lockState()->shouldConflictWithSecondaryBatchApplication()) {
-        uassert(10156, "cannot update a system namespace", nss.isLegalClientSystemNS());
-    }
-    if (nss.isVirtualized()) {
-        log() << "cannot update a virtual collection: " << nss;
-        uasserted(10155, "cannot update a virtual collection");
-    }
+    validateUpdate(nsString.ns().c_str(), request->getUpdates(), request->getQuery());
 
     // If there is no collection and this is an upsert, callers are supposed to create
     // the collection prior to calling this method. Explain, however, will never do
@@ -858,11 +870,11 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorUpdate(
     // writes on a secondary. If this is an update to a secondary from the replication system,
     // however, then we make an exception and let the write proceed.
     bool userInitiatedWritesAndNotPrimary = opCtx->writesAreReplicated() &&
-        !repl::getGlobalReplicationCoordinator()->canAcceptWritesFor(opCtx, nss);
+        !repl::getGlobalReplicationCoordinator()->canAcceptWritesFor(opCtx, nsString);
 
     if (userInitiatedWritesAndNotPrimary) {
         return Status(ErrorCodes::PrimarySteppedDown,
-                      str::stream() << "Not primary while performing update on " << nss.ns());
+                      str::stream() << "Not primary while performing update on " << nsString.ns());
     }
 
     if (lifecycle) {
@@ -884,11 +896,12 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorUpdate(
             // Treat collections that do not exist as empty collections. Note that the explain
             // reporting machinery always assumes that the root stage for an update operation is
             // an UpdateStage, so in this case we put an UpdateStage on top of an EOFStage.
-            LOG(2) << "Collection " << nss.ns() << " does not exist."
+            LOG(2) << "Collection " << nsString.ns() << " does not exist."
                    << " Using EOF stage: " << redact(unparsedQuery);
             auto updateStage = make_unique<UpdateStage>(
                 opCtx, updateStageParams, ws.get(), collection, new EOFStage(opCtx));
-            return PlanExecutor::make(opCtx, std::move(ws), std::move(updateStage), nss, policy);
+            return PlanExecutor::make(
+                opCtx, std::move(ws), std::move(updateStage), nsString, policy);
         }
 
         const IndexDescriptor* descriptor = collection->getIndexCatalog()->findIdIndex(opCtx);
@@ -944,8 +957,13 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorUpdate(
         // is invalid to use a positional projection because the query expression need not
         // match the array element after the update has been applied.
         const bool allowPositional = request->shouldReturnOldDocs();
-        StatusWith<unique_ptr<PlanStage>> projStatus = applyProjection(
-            opCtx, nss, cq.get(), request->getProj(), allowPositional, ws.get(), std::move(root));
+        StatusWith<unique_ptr<PlanStage>> projStatus = applyProjection(opCtx,
+                                                                       nsString,
+                                                                       cq.get(),
+                                                                       request->getProj(),
+                                                                       allowPositional,
+                                                                       ws.get(),
+                                                                       std::move(root));
         if (!projStatus.isOK()) {
             return projStatus.getStatus();
         }
diff --git a/src/mongo/db/repl/databases_cloner.cpp b/src/mongo/db/repl/databases_cloner.cpp
index d03f7252c8b..74c4543da21 100644
--- a/src/mongo/db/repl/databases_cloner.cpp
+++ b/src/mongo/db/repl/databases_cloner.cpp
@@ -321,7 +321,7 @@ void DatabasesCloner::_onListDatabaseFinish(const CommandCallbackArgs& cbd) {
         const auto collectionFilterPred = [dbName](const BSONObj& collInfo) {
             const auto collName = collInfo["name"].str();
             const NamespaceString ns(dbName, collName);
-            if (ns.isSystem() && !ns.isLegalClientSystemNS()) {
+            if (ns.isSystem() && !legalClientSystemNS(ns.ns())) {
                 LOG(1) << "Skipping 'system' collection: " << ns.ns();
                 return false;
             }
diff --git a/src/mongo/db/repl/storage_interface_impl.cpp b/src/mongo/db/repl/storage_interface_impl.cpp
index 672a2eb16ea..0dcb15a0110 100644
--- a/src/mongo/db/repl/storage_interface_impl.cpp
+++ b/src/mongo/db/repl/storage_interface_impl.cpp
@@ -830,7 +830,7 @@ Status StorageInterfaceImpl::deleteByFilter(OperationContext* opCtx,
     request.setMulti(true);
     request.setYieldPolicy(PlanExecutor::NO_YIELD);
 
-    // This disables the isLegalClientSystemNS() check in getExecutorDelete() which is used to
+    // This disables the legalClientSystemNS() check in getExecutorDelete() which is used to
     // disallow client deletes from unrecognized system collections.
     request.setGod();
 
diff --git a/src/mongo/db/repl/storage_interface_impl_test.cpp b/src/mongo/db/repl/storage_interface_impl_test.cpp
index 825402ef879..70e801e2fe3 100644
--- a/src/mongo/db/repl/storage_interface_impl_test.cpp
+++ b/src/mongo/db/repl/storage_interface_impl_test.cpp
@@ -1844,7 +1844,7 @@ TEST_F(StorageInterfaceImplTest,
     // Checks that we can update collections with namespaces not considered "legal client system"
     // namespaces.
     NamespaceString nss("local.system.rollback.docs");
-    ASSERT_FALSE(nss.isLegalClientSystemNS());
+    ASSERT_FALSE(legalClientSystemNS(nss.ns()));
 
     auto opCtx = getOperationContext();
     StorageInterfaceImpl storage;
@@ -2088,7 +2088,7 @@ TEST_F(StorageInterfaceImplTest, DeleteByFilterRemovesDocumentsInIllegalClientSy
     // Checks that we can remove documents from collections with namespaces not considered "legal
     // client system" namespaces.
     NamespaceString nss("local.system.rollback.docs");
-    ASSERT_FALSE(nss.isLegalClientSystemNS());
+    ASSERT_FALSE(legalClientSystemNS(nss.ns()));
 
     auto opCtx = getOperationContext();
     StorageInterfaceImpl storage;