SERVER-41947 Disallow using the system.views collection name as the source or target names in the rename command

buildscripts/resmokeconfig/suites/causally_consistent_jscore_passthrough_auth.yml

@@ -125,7 +125,6 @@ selector:
   - jstests/core/views/view_with_invalid_dbname.js
   - jstests/core/views/views_creation.js
   - jstests/core/views/views_drop.js
-  - jstests/core/views/views_rename.js
 
   # The tests below use applyOps, SERVER-1439.
   - jstests/core/list_collections1.js

jstests/change_streams/whole_cluster_metadata_notifications.js

@@ -191,46 +191,6 @@ for (let collToInvalidate of [db1Coll, db2Coll]) {
         assert.commandWorked(collToInvalidate.insert({_id: 2}));
         assert.eq(cst.getOneChange(aggCursor).operationType, "insert");
 
-        // Test that renaming a "system" collection to a user collection *does* return a rename
-        // notification.
-        assert.commandWorked(
-            testDB.runCommand({create: "view1", viewOn: collToInvalidate.getName(), pipeline: []}));
-        assert.commandWorked(testDB.system.views.renameCollection("non_system_collection"));
-        cst.assertNextChangesEqual({
-            cursor: aggCursor,
-            expectedChanges: [{
-                operationType: "rename",
-                ns: {db: testDB.getName(), coll: "system.views"},
-                to: {db: testDB.getName(), coll: "non_system_collection"}
-            }],
-        });
-
-        // Test that renaming a "system" collection to a different "system" collection does not
-        // result in a notification in the change stream.
-        aggCursor = cst.startWatchingAllChangesForCluster();
-        assert.commandWorked(
-            testDB.runCommand({create: "view1", viewOn: collToInvalidate.getName(), pipeline: []}));
-        // Note that the target of the rename must be a valid "system" collection.
-        assert.commandWorked(testDB.system.views.renameCollection("system.users"));
-        // Verify that the change stream filters out the rename above, instead returning the
-        // next insert to the test collection.
-        assert.commandWorked(collToInvalidate.insert({_id: 1}));
-        change = cst.getOneChange(aggCursor);
-        assert.eq(change.operationType, "insert", tojson(change));
-        assert.eq(change.ns, {db: testDB.getName(), coll: collToInvalidate.getName()});
-
-        // Test that renaming a user collection to a "system" collection *does* return a rename
-        // notification.
-        assert.commandWorked(collToInvalidate.renameCollection("system.views"));
-        cst.assertNextChangesEqual({
-            cursor: aggCursor,
-            expectedChanges: [{
-                operationType: "rename",
-                ns: {db: testDB.getName(), coll: collToInvalidate.getName()},
-                to: {db: testDB.getName(), coll: "system.views"}
-            }],
-        });
-
         // Drop the "system.views" collection to avoid view catalog errors in subsequent tests.
         assertDropCollection(testDB, "system.views");
 

jstests/change_streams/whole_db_metadata_notifications.js

@@ -194,54 +194,6 @@ change = cst.getOneChange(aggCursor);
 assert.eq(change.operationType, "insert", tojson(change));
 assert.eq(change.ns, {db: testDB.getName(), coll: coll.getName()});
 
-// Test that renaming a "system" collection *does* return a notification if the target of
-// the rename is a non-system collection.
-assert.commandWorked(testDB.runCommand({create: "view1", viewOn: coll.getName(), pipeline: []}));
-assert.commandWorked(testDB.system.views.renameCollection("non_system_collection"));
-cst.assertNextChangesEqual({
-    cursor: aggCursor,
-    expectedChanges: [{
-        operationType: "rename",
-        ns: {db: testDB.getName(), coll: "system.views"},
-        to: {db: testDB.getName(), coll: "non_system_collection"}
-    }],
-});
-
-// Test that renaming a "system" collection to a different "system" collection does not
-// result in a notification in the change stream.
-aggCursor = cst.startWatchingChanges({pipeline: [{$changeStream: {}}], collection: 1});
-assert.commandWorked(testDB.runCommand({create: "view1", viewOn: coll.getName(), pipeline: []}));
-// Note that the target of the rename must be a valid "system" collection.
-assert.commandWorked(testDB.system.views.renameCollection("system.users"));
-// Verify that the change stream filters out the rename above, instead returning the next insert
-// to the test collection.
-assert.commandWorked(coll.insert({_id: 1}));
-change = cst.getOneChange(aggCursor);
-assert.eq(change.operationType, "insert", tojson(change));
-assert.eq(change.ns, {db: testDB.getName(), coll: coll.getName()});
-
-// Test that renaming a user collection to a "system" collection *is* returned in the change
-// stream.
-assert.commandWorked(coll.renameCollection("system.views"));
-cst.assertNextChangesEqual({
-    cursor: aggCursor,
-    expectedChanges: [{
-        operationType: "rename",
-        ns: {db: testDB.getName(), coll: coll.getName()},
-        to: {db: testDB.getName(), coll: "system.views"}
-    }],
-});
-
-// Drop the "system.views" collection to avoid view catalog errors in subsequent tests.
-assertDropCollection(testDB, "system.views");
-assertDropCollection(testDB, "non_system_collection");
-cst.assertNextChangesEqual({
-    cursor: aggCursor,
-    expectedChanges: [
-        {operationType: "drop", ns: {db: testDB.getName(), coll: "non_system_collection"}},
-    ]
-});
-
 // Dropping the database should generate a 'dropDatabase' notification followed by an
 // 'invalidate'.
 assert.commandWorked(testDB.dropDatabase());

jstests/core/views/views_rename.js

@@ -1,27 +0,0 @@
-// @tags: [
-//   assumes_superuser_permissions,
-//   requires_fastcount,
-//   requires_non_retryable_commands,
-// ]
-
-(function() {
-// SERVER-30406 Test that renaming system.views correctly invalidates the view catalog
-'use strict';
-
-const collName = "views_rename_test";
-let coll = db.getCollection(collName);
-
-db.view.drop();
-coll.drop();
-assert.commandWorked(db.createView("view", collName, []));
-assert.commandWorked(coll.insert({_id: 1}));
-assert.eq(db.view.find().count(), 1, "couldn't find document in view");
-assert.commandWorked(db.system.views.renameCollection("views", /*dropTarget*/ true));
-assert.eq(db.view.find().count(),
-          0,
-          "find on view should have returned no results after renaming away system.views");
-assert.commandWorked(db.views.renameCollection("system.views"));
-assert.eq(db.view.find().count(),
-          1,
-          "find on view should have worked again after renaming system.views back in place");
-})();

jstests/noPassthrough/view_catalog_deadlock_with_rename.js

@@ -1,36 +0,0 @@
-/**
- * This test demonstrates the deadlock we found in SERVER-40994: Rename locks
- * the source and target based on their resourceId order, and a delete op on an
- * non-existent collection (which also triggers a view catalog reload) locks the
- * same two namespaces in different order.
- *
- * The fix is to always lock 'system.views' collection in the end.
- */
-(function() {
-'use strict';
-
-const conn = MongoRunner.runMongod();
-const db = conn.getDB('test');
-
-assert.commandWorked(db.runCommand({insert: 'a', documents: [{x: 1}]}));
-assert.commandWorked(db.runCommand({insert: 'b', documents: [{x: 1}]}));
-
-assert.commandWorked(db.createView('viewA', 'a', []));
-
-// Will cause a view catalog reload.
-assert.commandWorked(db.runCommand(
-    {insert: 'system.views', documents: [{_id: 'test.viewB', viewOn: 'b', pipeline: []}]}));
-
-const renameSystemViews = startParallelShell(function() {
-    // This used to first lock 'test.system.views' and then 'test.aaabb' in X mode.
-    assert.commandWorked(
-        db.adminCommand({renameCollection: 'test.system.views', to: 'test.aaabb'}));
-}, conn.port);
-
-// This triggers view catalog reload. Therefore it first locked 'test.aaabb' in IX mode and then
-// 'test.system.views' in IS mode.
-assert.commandWorked(db.runCommand({delete: 'aaabb', deletes: [{q: {x: 2}, limit: 1}]}));
-
-renameSystemViews();
-MongoRunner.stopMongod(conn);
-})();

jstests/noPassthroughWithMongod/disallow_system_views_rename.js

@@ -0,0 +1,24 @@
+(function() {
+'use strict';
+assert.commandWorked(db.runCommand({insert: 'a', documents: [{x: 1}]}));
+
+// Disallow renaming to system.views
+assert.commandFailedWithCode(db.adminCommand({renameCollection: 'test.a', to: 'test.system.views'}),
+                             ErrorCodes.IllegalOperation);
+
+assert.commandWorked(db.createView('viewA', 'a', []));
+
+// Disallow renaming from system.views
+assert.commandFailedWithCode(
+    db.adminCommand({renameCollection: 'test.system.views', to: 'test.aaabb'}),
+    ErrorCodes.IllegalOperation);
+
+// User can still rename system.views (or to system.views) via applyOps command.
+assert.commandWorked(db.adminCommand({
+    applyOps: [{op: 'c', ns: 'test.$cmd', o: {renameCollection: 'test.system.views', to: 'test.b'}}]
+}));
+
+assert.commandWorked(db.adminCommand({
+    applyOps: [{op: 'c', ns: 'test.$cmd', o: {renameCollection: 'test.b', to: 'test.system.views'}}]
+}));
+})();

src/mongo/db/catalog/rename_collection.cpp

@@ -747,6 +747,12 @@ Status renameCollection(OperationContext* opCtx,
                                     << source);
     }
 
+    if (source.isSystemDotViews() || target.isSystemDotViews()) {
+        return Status(
+            ErrorCodes::IllegalOperation,
+            "renaming system.views collection or renaming to system.views is not allowed");
+    }
+
     const std::string dropTargetMsg =
         options.dropTarget ? " and drop " + target.toString() + "." : ".";
     log() << "renameCollectionForCommand: rename " << source << " to " << target << dropTargetMsg;