SERVER-28534 Pass collection names around as NamespaceStrings more often

Before this patch 'ns' values were often passed around as std::string or
char* containing either a dbname (no '.'), a dbname with '.$cmd', or a fully qualified
collection name. Instead pass either plain 'dbName' value (as string) or a fully
qualified name using the actual NamespaceString type.

7 files changed, 52 insertions, 46 deletions

diff --git a/src/mongo/db/auth/authorization_manager.cpp b/src/mongo/db/auth/authorization_manager.cpp
index bca15e13223..66c232813b0 100644
--- a/src/mongo/db/auth/authorization_manager.cpp
+++ b/src/mongo/db/auth/authorization_manager.cpp
@@ -629,10 +629,10 @@ Status AuthorizationManager::initialize(OperationContext* opCtx) {
 }
 
 namespace {
-bool isAuthzNamespace(StringData ns) {
-    return (ns == AuthorizationManager::rolesCollectionNamespace.ns() ||
-            ns == AuthorizationManager::usersCollectionNamespace.ns() ||
-            ns == AuthorizationManager::versionCollectionNamespace.ns());
+bool isAuthzNamespace(const NamespaceString& nss) {
+    return (nss == AuthorizationManager::rolesCollectionNamespace ||
+            nss == AuthorizationManager::usersCollectionNamespace ||
+            nss == AuthorizationManager::versionCollectionNamespace);
 }
 
 bool isAuthzCollection(StringData coll) {
@@ -641,8 +641,8 @@ bool isAuthzCollection(StringData coll) {
             coll == AuthorizationManager::versionCollectionNamespace.coll());
 }
 
-bool loggedCommandOperatesOnAuthzData(const char* ns, const BSONObj& cmdObj) {
-    if (ns != AuthorizationManager::adminCommandNamespace.ns())
+bool loggedCommandOperatesOnAuthzData(const NamespaceString& nss, const BSONObj& cmdObj) {
+    if (nss != AuthorizationManager::adminCommandNamespace)
         return false;
     const StringData cmdName(cmdObj.firstElement().fieldNameStringData());
     if (cmdName == "drop") {
@@ -661,16 +661,16 @@ bool loggedCommandOperatesOnAuthzData(const char* ns, const BSONObj& cmdObj) {
     }
 }
 
-bool appliesToAuthzData(const char* op, const char* ns, const BSONObj& o) {
+bool appliesToAuthzData(const char* op, const NamespaceString& nss, const BSONObj& o) {
     switch (*op) {
         case 'i':
         case 'u':
         case 'd':
             if (op[1] != '\0')
                 return false;  // "db" op type
-            return isAuthzNamespace(ns);
+            return isAuthzNamespace(nss);
         case 'c':
-            return loggedCommandOperatesOnAuthzData(ns, o);
+            return loggedCommandOperatesOnAuthzData(nss, o);
             break;
         case 'n':
             return false;
@@ -701,11 +701,11 @@ void AuthorizationManager::_updateCacheGeneration_inlock() {
 }
 
 void AuthorizationManager::_invalidateRelevantCacheData(const char* op,
-                                                        const char* ns,
+                                                        const NamespaceString& ns,
                                                         const BSONObj& o,
                                                         const BSONObj* o2) {
-    if (ns == AuthorizationManager::rolesCollectionNamespace.ns() ||
-        ns == AuthorizationManager::versionCollectionNamespace.ns()) {
+    if (ns == AuthorizationManager::rolesCollectionNamespace ||
+        ns == AuthorizationManager::versionCollectionNamespace) {
         invalidateUserCache();
         return;
     }
@@ -713,7 +713,7 @@ void AuthorizationManager::_invalidateRelevantCacheData(const char* op,
     if (*op == 'i' || *op == 'd' || *op == 'u') {
         // If you got into this function isAuthzNamespace() must have returned true, and we've
         // already checked that it's not the roles or version collection.
-        invariant(ns == AuthorizationManager::usersCollectionNamespace.ns());
+        invariant(ns == AuthorizationManager::usersCollectionNamespace);
 
         StatusWith<UserName> userName = (*op == 'u')
             ? extractUserNameFromIdString((*o2)["_id"].str())
@@ -732,11 +732,14 @@ void AuthorizationManager::_invalidateRelevantCacheData(const char* op,
     }
 }
 
-void AuthorizationManager::logOp(
-    OperationContext* opCtx, const char* op, const char* ns, const BSONObj& o, const BSONObj* o2) {
-    if (appliesToAuthzData(op, ns, o)) {
-        _externalState->logOp(opCtx, op, ns, o, o2);
-        _invalidateRelevantCacheData(op, ns, o, o2);
+void AuthorizationManager::logOp(OperationContext* opCtx,
+                                 const char* op,
+                                 const NamespaceString& nss,
+                                 const BSONObj& o,
+                                 const BSONObj* o2) {
+    if (appliesToAuthzData(op, nss, o)) {
+        _externalState->logOp(opCtx, op, nss, o, o2);
+        _invalidateRelevantCacheData(op, nss, o, o2);
     }
 }
 
diff --git a/src/mongo/db/auth/authorization_manager.h b/src/mongo/db/auth/authorization_manager.h
index 2532bd5217e..495bcc0aa81 100644
--- a/src/mongo/db/auth/authorization_manager.h
+++ b/src/mongo/db/auth/authorization_manager.h
@@ -303,7 +303,7 @@ public:
      */
     void logOp(OperationContext* opCtx,
                const char* opstr,
-               const char* ns,
+               const NamespaceString& nss,
                const BSONObj& obj,
                const BSONObj* patt);
 
@@ -326,7 +326,7 @@ private:
      * with oplog entries that have been pre-verified to actually affect authorization data.
      */
     void _invalidateRelevantCacheData(const char* op,
-                                      const char* ns,
+                                      const NamespaceString& ns,
                                       const BSONObj& o,
                                       const BSONObj* o2);
 
diff --git a/src/mongo/db/auth/authorization_manager_test.cpp b/src/mongo/db/auth/authorization_manager_test.cpp
index 83edae4ddef..7eb3c980e3b 100644
--- a/src/mongo/db/auth/authorization_manager_test.cpp
+++ b/src/mongo/db/auth/authorization_manager_test.cpp
@@ -456,7 +456,7 @@ public:
 TEST_F(AuthorizationManagerLogOpTest, testDropDatabaseAddsRecoveryUnits) {
     authzManager->logOp(&opCtx,
                         "c",
-                        "admin.$cmd",
+                        {"admin", "$cmd"},
                         BSON("dropDatabase"
                              << "1"),
                         nullptr);
@@ -466,7 +466,7 @@ TEST_F(AuthorizationManagerLogOpTest, testDropDatabaseAddsRecoveryUnits) {
 TEST_F(AuthorizationManagerLogOpTest, testDropAuthCollectionAddsRecoveryUnits) {
     authzManager->logOp(&opCtx,
                         "c",
-                        "admin.$cmd",
+                        {"admin", "$cmd"},
                         BSON("drop"
                              << "system.users"),
                         nullptr);
@@ -474,7 +474,7 @@ TEST_F(AuthorizationManagerLogOpTest, testDropAuthCollectionAddsRecoveryUnits) {
 
     authzManager->logOp(&opCtx,
                         "c",
-                        "admin.$cmd",
+                        {"admin", "$cmd"},
                         BSON("drop"
                              << "system.roles"),
                         nullptr);
@@ -482,7 +482,7 @@ TEST_F(AuthorizationManagerLogOpTest, testDropAuthCollectionAddsRecoveryUnits) {
 
     authzManager->logOp(&opCtx,
                         "c",
-                        "admin.$cmd",
+                        {"admin", "$cmd"},
                         BSON("drop"
                              << "system.version"),
                         nullptr);
@@ -490,7 +490,7 @@ TEST_F(AuthorizationManagerLogOpTest, testDropAuthCollectionAddsRecoveryUnits) {
 
     authzManager->logOp(&opCtx,
                         "c",
-                        "admin.$cmd",
+                        {"admin", "$cmd"},
                         BSON("drop"
                              << "system.profile"),
                         nullptr);
@@ -500,21 +500,21 @@ TEST_F(AuthorizationManagerLogOpTest, testDropAuthCollectionAddsRecoveryUnits) {
 TEST_F(AuthorizationManagerLogOpTest, testCreateAnyCollectionAddsNoRecoveryUnits) {
     authzManager->logOp(&opCtx,
                         "c",
-                        "admin.$cmd",
+                        {"admin", "$cmd"},
                         BSON("create"
                              << "system.users"),
                         nullptr);
 
     authzManager->logOp(&opCtx,
                         "c",
-                        "admin.$cmd",
+                        {"admin", "$cmd"},
                         BSON("create"
                              << "system.profile"),
                         nullptr);
 
     authzManager->logOp(&opCtx,
                         "c",
-                        "admin.$cmd",
+                        {"admin", "$cmd"},
                         BSON("create"
                              << "system.other"),
                         nullptr);
@@ -525,7 +525,7 @@ TEST_F(AuthorizationManagerLogOpTest, testCreateAnyCollectionAddsNoRecoveryUnits
 TEST_F(AuthorizationManagerLogOpTest, testRawInsertToRolesCollectionAddsRecoveryUnits) {
     authzManager->logOp(&opCtx,
                         "i",
-                        "admin.system.profile",
+                        {"admin", "system.profile"},
                         BSON("_id"
                              << "admin.user"),
                         nullptr);
@@ -533,7 +533,7 @@ TEST_F(AuthorizationManagerLogOpTest, testRawInsertToRolesCollectionAddsRecovery
 
     authzManager->logOp(&opCtx,
                         "i",
-                        "admin.system.users",
+                        {"admin", "system.users"},
                         BSON("_id"
                              << "admin.user"),
                         nullptr);
@@ -541,7 +541,7 @@ TEST_F(AuthorizationManagerLogOpTest, testRawInsertToRolesCollectionAddsRecovery
 
     authzManager->logOp(&opCtx,
                         "i",
-                        "admin.system.roles",
+                        {"admin", "system.roles"},
                         BSON("_id"
                              << "admin.user"),
                         nullptr);
diff --git a/src/mongo/db/auth/authz_manager_external_state.h b/src/mongo/db/auth/authz_manager_external_state.h
index 18277c272b2..f63e05b818f 100644
--- a/src/mongo/db/auth/authz_manager_external_state.h
+++ b/src/mongo/db/auth/authz_manager_external_state.h
@@ -157,7 +157,7 @@ public:
 
     virtual void logOp(OperationContext* opCtx,
                        const char* op,
-                       const char* ns,
+                       const NamespaceString& ns,
                        const BSONObj& o,
                        const BSONObj* o2) {}
 
diff --git a/src/mongo/db/auth/authz_manager_external_state_local.cpp b/src/mongo/db/auth/authz_manager_external_state_local.cpp
index de706dcd9da..9227e1d2dd2 100644
--- a/src/mongo/db/auth/authz_manager_external_state_local.cpp
+++ b/src/mongo/db/auth/authz_manager_external_state_local.cpp
@@ -445,13 +445,13 @@ public:
     AuthzManagerLogOpHandler(OperationContext* opCtx,
                              AuthzManagerExternalStateLocal* externalState,
                              const char* op,
-                             const char* ns,
+                             const NamespaceString& nss,
                              const BSONObj& o,
                              const BSONObj* o2)
         : _opCtx(opCtx),
           _externalState(externalState),
           _op(op),
-          _ns(ns),
+          _nss(nss),
           _o(o.getOwned()),
 
           _isO2Set(o2 ? true : false),
@@ -460,13 +460,13 @@ public:
     virtual void commit() {
         stdx::lock_guard<stdx::mutex> lk(_externalState->_roleGraphMutex);
         Status status = _externalState->_roleGraph.handleLogOp(
-            _opCtx, _op.c_str(), NamespaceString(_ns.c_str()), _o, _isO2Set ? &_o2 : NULL);
+            _opCtx, _op.c_str(), _nss, _o, _isO2Set ? &_o2 : NULL);
 
         if (status == ErrorCodes::OplogOperationUnsupported) {
             _externalState->_roleGraph = RoleGraph();
             _externalState->_roleGraphState = _externalState->roleGraphStateInitial;
             BSONObjBuilder oplogEntryBuilder;
-            oplogEntryBuilder << "op" << _op << "ns" << _ns << "o" << _o;
+            oplogEntryBuilder << "op" << _op << "ns" << _nss.ns() << "o" << _o;
             if (_isO2Set)
                 oplogEntryBuilder << "o2" << _o2;
             error() << "Unsupported modification to roles collection in oplog; "
@@ -494,19 +494,22 @@ private:
     OperationContext* _opCtx;
     AuthzManagerExternalStateLocal* _externalState;
     const std::string _op;
-    const std::string _ns;
+    const NamespaceString _nss;
     const BSONObj _o;
 
     const bool _isO2Set;
     const BSONObj _o2;
 };
 
-void AuthzManagerExternalStateLocal::logOp(
-    OperationContext* opCtx, const char* op, const char* ns, const BSONObj& o, const BSONObj* o2) {
-    if (ns == AuthorizationManager::rolesCollectionNamespace.ns() ||
-        ns == AuthorizationManager::adminCommandNamespace.ns()) {
+void AuthzManagerExternalStateLocal::logOp(OperationContext* opCtx,
+                                           const char* op,
+                                           const NamespaceString& nss,
+                                           const BSONObj& o,
+                                           const BSONObj* o2) {
+    if (nss == AuthorizationManager::rolesCollectionNamespace ||
+        nss == AuthorizationManager::adminCommandNamespace) {
         opCtx->recoveryUnit()->registerChange(
-            new AuthzManagerLogOpHandler(opCtx, this, op, ns, o, o2));
+            new AuthzManagerLogOpHandler(opCtx, this, op, nss, o, o2));
     }
 }
 
diff --git a/src/mongo/db/auth/authz_manager_external_state_local.h b/src/mongo/db/auth/authz_manager_external_state_local.h
index ccaa7e2d452..d33b8059084 100644
--- a/src/mongo/db/auth/authz_manager_external_state_local.h
+++ b/src/mongo/db/auth/authz_manager_external_state_local.h
@@ -101,7 +101,7 @@ public:
 
     virtual void logOp(OperationContext* opCtx,
                        const char* op,
-                       const char* ns,
+                       const NamespaceString& ns,
                        const BSONObj& o,
                        const BSONObj* o2);
 
diff --git a/src/mongo/db/auth/authz_manager_external_state_mock.cpp b/src/mongo/db/auth/authz_manager_external_state_mock.cpp
index ff58770d230..7b8d72ad1f7 100644
--- a/src/mongo/db/auth/authz_manager_external_state_mock.cpp
+++ b/src/mongo/db/auth/authz_manager_external_state_mock.cpp
@@ -158,7 +158,7 @@ Status AuthzManagerExternalStateMock::insert(OperationContext* opCtx,
     _documents[collectionName].push_back(toInsert);
 
     if (_authzManager) {
-        _authzManager->logOp(opCtx, "i", collectionName.ns().c_str(), toInsert, NULL);
+        _authzManager->logOp(opCtx, "i", collectionName, toInsert, NULL);
     }
 
     return Status::OK();
@@ -197,7 +197,7 @@ Status AuthzManagerExternalStateMock::updateOne(OperationContext* opCtx,
         BSONObj idQuery = driver.makeOplogEntryQuery(newObj, false);
 
         if (_authzManager) {
-            _authzManager->logOp(opCtx, "u", collectionName.ns().c_str(), logObj, &idQuery);
+            _authzManager->logOp(opCtx, "u", collectionName, logObj, &idQuery);
         }
 
         return Status::OK();
@@ -244,7 +244,7 @@ Status AuthzManagerExternalStateMock::remove(OperationContext* opCtx,
         ++n;
 
         if (_authzManager) {
-            _authzManager->logOp(opCtx, "d", collectionName.ns().c_str(), idQuery, NULL);
+            _authzManager->logOp(opCtx, "d", collectionName, idQuery, NULL);
         }
     }
     *numRemoved = n;