diff options
-rw-r--r-- | jstests/core/ddl/drop_collection.js | 21 | ||||
-rw-r--r-- | jstests/core/query/explain/explain_upsert.js | 3 | ||||
-rw-r--r-- | jstests/noPassthrough/durable_view_catalog.js | 2 | ||||
-rw-r--r-- | jstests/replsets/noop_writes_wait_for_write_concern.js | 14 | ||||
-rw-r--r-- | src/mongo/db/catalog/collection_catalog.cpp | 7 | ||||
-rw-r--r-- | src/mongo/db/catalog/drop_collection.cpp | 75 | ||||
-rw-r--r-- | src/mongo/db/catalog/rename_collection_test.cpp | 3 | ||||
-rw-r--r-- | src/mongo/db/views/view_catalog_test.cpp | 2 |
8 files changed, 71 insertions, 56 deletions
diff --git a/jstests/core/ddl/drop_collection.js b/jstests/core/ddl/drop_collection.js index 3dd4bafbe1b..4ac5d140364 100644 --- a/jstests/core/ddl/drop_collection.js +++ b/jstests/core/ddl/drop_collection.js @@ -25,18 +25,12 @@ function assertCollectionDropped(ns) { const coll = db[jsTestName() + "_coll"]; -const isMongos = db.adminCommand({isdbgrid: 1}).isdbgrid; - jsTest.log("Drop Unexistent collection."); { // Drop the collection - let dropRes = db.runCommand({drop: coll.getName()}); - if (isMongos) { - assert.commandWorked(dropRes); - } else { - // TODO SERVER-43894 Make dropping a nonexistent collection a noop - assert.commandWorkedOrFailedWithCode(dropRes, ErrorCodes.NamespaceNotFound); - } + // NamespaceNotFound will be returned by mongod versions earlier than 7.0. + assert.commandWorkedOrFailedWithCode(db.runCommand({drop: coll.getName()}), + ErrorCodes.NamespaceNotFound); assertCollectionDropped(coll.getFullName()); } @@ -51,13 +45,8 @@ jsTest.log("Drop existing collection."); assertCollectionDropped(coll.getFullName()); // Test idempotency - let dropRes = db.runCommand({drop: coll.getName()}); - if (isMongos) { - assert.commandWorked(dropRes); - } else { - // TODO SERVER-43894 Make dropping a nonexistent collection a noop - assert.commandWorkedOrFailedWithCode(dropRes, ErrorCodes.NamespaceNotFound); - } + // NamespaceNotFound will be returned by mongod versions earlier than 7.0. + assert.commandWorkedOrFailedWithCode(db.runCommand({drop: coll.getName()})); assertCollectionDropped(coll.getFullName()); } diff --git a/jstests/core/query/explain/explain_upsert.js b/jstests/core/query/explain/explain_upsert.js index 6da5921b8f0..802cf4b5343 100644 --- a/jstests/core/query/explain/explain_upsert.js +++ b/jstests/core/query/explain/explain_upsert.js @@ -17,7 +17,7 @@ assert.commandWorked(explain); // Collection should still not exist. assert.eq(0, t.count()); -assert(!t.drop()); +assert(!db.getCollectionInfos({name: t.getName()}).length); // Add a document to the collection. t.insert({a: 3}); @@ -27,3 +27,4 @@ explain = db.runCommand( {explain: {update: t.getName(), updates: [{q: {a: 1}, u: {a: 1}, upsert: true}]}}); assert.commandWorked(explain); assert.eq(1, t.count()); +assert(db.getCollectionInfos({name: t.getName()}).length); diff --git a/jstests/noPassthrough/durable_view_catalog.js b/jstests/noPassthrough/durable_view_catalog.js index 39db5b0b855..0ecbd2e6a43 100644 --- a/jstests/noPassthrough/durable_view_catalog.js +++ b/jstests/noPassthrough/durable_view_catalog.js @@ -79,7 +79,7 @@ assert.commandFailedWithCode(viewsDB.runCommand({collMod: "view2", viewOn: "view // Checks that dropping a nonexistent view or collection is not affected by an invalid view existing // in the view catalog. -assert.commandFailedWithCode(viewsDB.runCommand({drop: "view4"}), ErrorCodes.NamespaceNotFound); +assert.commandWorked(viewsDB.runCommand({drop: "view4"})); assert.commandFailedWithCode(viewsDB.runCommand({listCollections: 1}), ErrorCodes.InvalidViewDefinition); diff --git a/jstests/replsets/noop_writes_wait_for_write_concern.js b/jstests/replsets/noop_writes_wait_for_write_concern.js index bdcc6df093c..1fc4bfdcb13 100644 --- a/jstests/replsets/noop_writes_wait_for_write_concern.js +++ b/jstests/replsets/noop_writes_wait_for_write_concern.js @@ -222,7 +222,7 @@ commands.push({ assert.commandWorkedIgnoringWriteConcernErrors(db.runCommand({drop: collName})); }, confirmFunc: function(res) { - assert.commandFailedWithCode(res, ErrorCodes.NamespaceNotFound); + assert.commandWorkedIgnoringWriteConcernErrors(res); } }); @@ -256,6 +256,18 @@ function testCommandWithWriteConcern(cmd) { cmd.req.writeConcern = {w: 3, wtimeout: 1000}; jsTest.log("Testing " + tojson(cmd.req)); + // Don't run drop cmd on older versions. Starting in v7.0 drop returns OK on non-existent + // collections, instead of a NamespaceNotFound error. + if (cmd.req["drop"] !== undefined) { + const primaryShell = new Mongo(primary.host); + const primaryBinVersion = primaryShell.getDB("admin").serverStatus()["version"]; + if (primaryBinVersion != MongoRunner.getBinVersionFor("latest")) { + jsTest.log( + "Skipping test: drop on non-existent collections in older versions returns an error."); + return; + } + } + dropTestCollection(); cmd.setupFunc(); diff --git a/src/mongo/db/catalog/collection_catalog.cpp b/src/mongo/db/catalog/collection_catalog.cpp index c131f69dad0..75f4b02f4c2 100644 --- a/src/mongo/db/catalog/collection_catalog.cpp +++ b/src/mongo/db/catalog/collection_catalog.cpp @@ -734,11 +734,8 @@ Status CollectionCatalog::dropView(OperationContext* opCtx, const NamespaceStrin invariant(_viewsForDatabase.contains(viewName.dbName())); const ViewsForDatabase& viewsForDb = *_getViewsForDatabase(opCtx, viewName.dbName()); assertViewCatalogValid(viewsForDb); - - // Make sure the view exists before proceeding. - if (auto viewPtr = viewsForDb.lookup(viewName); !viewPtr) { - return {ErrorCodes::NamespaceNotFound, - str::stream() << "cannot drop missing view: " << viewName.ns()}; + if (!viewsForDb.lookup(viewName)) { + return Status::OK(); } Status result = Status::OK(); diff --git a/src/mongo/db/catalog/drop_collection.cpp b/src/mongo/db/catalog/drop_collection.cpp index 4d4c792dcd7..c005bddb28a 100644 --- a/src/mongo/db/catalog/drop_collection.cpp +++ b/src/mongo/db/catalog/drop_collection.cpp @@ -59,24 +59,24 @@ namespace { MONGO_FAIL_POINT_DEFINE(hangDropCollectionBeforeLockAcquisition); MONGO_FAIL_POINT_DEFINE(hangDuringDropCollection); -Status _checkNssAndReplState(OperationContext* opCtx, - const CollectionPtr& coll, - const NamespaceString& nss, - const boost::optional<UUID>& expectedUUID = boost::none) { +/** + * Checks that the collection has the 'expectedUUID' if given. + * Checks that writes are allowed to 'coll' -- e.g. whether this server is PRIMARY. + */ +Status _checkUUIDAndReplState(OperationContext* opCtx, + const CollectionPtr& coll, + const NamespaceString& nss, + const boost::optional<UUID>& expectedUUID = boost::none) { try { checkCollectionUUIDMismatch(opCtx, nss, coll, expectedUUID); } catch (const DBException& ex) { return ex.toStatus(); } - if (!coll) { - return Status(ErrorCodes::NamespaceNotFound, "ns not found"); - } - if (opCtx->writesAreReplicated() && - !repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, coll->ns())) { + !repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, nss)) { return Status(ErrorCodes::NotWritablePrimary, - str::stream() << "Not primary while dropping collection " << coll->ns()); + str::stream() << "Not primary while dropping collection " << nss); } return Status::OK(); @@ -125,11 +125,7 @@ Status _dropView(OperationContext* opCtx, const NamespaceString& collectionName, const boost::optional<UUID>& expectedUUID, DropReply* reply) { - if (!db) { - Status status = Status(ErrorCodes::NamespaceNotFound, "ns not found"); - audit::logDropView(opCtx->getClient(), collectionName, "", {}, status.code()); - return status; - } + invariant(db); // Views don't have UUIDs so if the expectedUUID is specified, we will always throw. try { @@ -141,9 +137,9 @@ Status _dropView(OperationContext* opCtx, auto view = CollectionCatalog::get(opCtx)->lookupViewWithoutValidatingDurable(opCtx, collectionName); if (!view) { - Status status = Status(ErrorCodes::NamespaceNotFound, "ns not found"); - audit::logDropView(opCtx->getClient(), collectionName, "", {}, status.code()); - return status; + audit::logDropView( + opCtx->getClient(), collectionName, "", {}, ErrorCodes::NamespaceNotFound); + return Status::OK(); } // Validates the view or throws an "invalid view" error. @@ -207,9 +203,13 @@ Status _abortIndexBuildsAndDrop(OperationContext* opCtx, CollectionPtr coll = CollectionCatalog::get(opCtx)->lookupCollectionByNamespace(opCtx, startingNss); - Status status = _checkNssAndReplState(opCtx, coll, startingNss, expectedUUID); + + // Even if the collection doesn't exist, UUID mismatches must return an error. + Status status = _checkUUIDAndReplState(opCtx, coll, startingNss, expectedUUID); if (!status.isOK()) { return status; + } else if (!coll) { + return Status::OK(); } warnEncryptedCollectionsIfNeeded(opCtx, coll); @@ -262,9 +262,13 @@ Status _abortIndexBuildsAndDrop(OperationContext* opCtx, opCtx->recoveryUnit()->abandonSnapshot(); coll = CollectionCatalog::get(opCtx)->lookupCollectionByUUID(opCtx, collectionUUID); - status = _checkNssAndReplState(opCtx, coll, startingNss, expectedUUID); + + // Even if the collection doesn't exist, UUID mismatches must return an error. + status = _checkUUIDAndReplState(opCtx, coll, startingNss, expectedUUID); if (!status.isOK()) { return status; + } else if (!coll) { + return Status::OK(); } // Check if any new index builds were started while releasing the collection lock @@ -311,9 +315,13 @@ Status _dropCollectionForApplyOps(OperationContext* opCtx, Lock::CollectionLock collLock(opCtx, collectionName, MODE_X); const CollectionPtr& coll = CollectionCatalog::get(opCtx)->lookupCollectionByNamespace(opCtx, collectionName); - Status status = _checkNssAndReplState(opCtx, coll, collectionName); + + // Even if the collection doesn't exist, UUID mismatches must return an error. + Status status = _checkUUIDAndReplState(opCtx, coll, collectionName); if (!status.isOK()) { return status; + } else if (!coll) { + return Status::OK(); } if (MONGO_unlikely(hangDuringDropCollection.shouldFail())) { @@ -369,7 +377,7 @@ Status _dropCollection(OperationContext* opCtx, collectionName.coll().toString(), boost::none), "Database does not exist"} - : Status(ErrorCodes::NamespaceNotFound, "ns not found"); + : Status::OK(); } if (CollectionCatalog::get(opCtx)->lookupCollectionByNamespace(opCtx, collectionName)) { @@ -457,15 +465,18 @@ Status _dropCollection(OperationContext* opCtx, return dropTimeseries(bucketsNs, false); } + // There is no collection or view at the namespace. Check whether a UUID was given + // and error if so because the caller expects the collection to exist. If no UUID + // was given, then it is OK to return success. try { checkCollectionUUIDMismatch(opCtx, collectionName, nullptr, expectedUUID); } catch (const DBException& ex) { return ex.toStatus(); } - Status status = Status(ErrorCodes::NamespaceNotFound, "ns not found"); - audit::logDropView(opCtx->getClient(), collectionName, "", {}, status.code()); - return status; + audit::logDropView( + opCtx->getClient(), collectionName, "", {}, ErrorCodes::NamespaceNotFound); + return Status::OK(); } if (view->timeseries() && CollectionCatalog::get(opCtx)->lookupCollectionByNamespace(opCtx, view->viewOn())) { @@ -478,9 +489,15 @@ Status _dropCollection(OperationContext* opCtx, return _dropView(opCtx, db, collectionName, expectedUUID, reply); }); } catch (ExceptionFor<ErrorCodes::NamespaceNotFound>&) { - // The shell requires that NamespaceNotFound error codes return the "ns not found" - // string. - return Status(ErrorCodes::NamespaceNotFound, "ns not found"); + // Any unhandled namespace not found errors should be converted into success. Unless the + // caller specified a UUID and expects the collection to exist. + try { + checkCollectionUUIDMismatch(opCtx, collectionName, nullptr, expectedUUID); + } catch (const DBException& ex) { + return ex.toStatus(); + } + + return Status::OK(); } } } // namespace @@ -559,7 +576,7 @@ Status dropCollectionForApplyOps(OperationContext* opCtx, AutoGetDb autoDb(opCtx, collectionName.dbName(), MODE_IX); Database* db = autoDb.getDb(); if (!db) { - return Status(ErrorCodes::NamespaceNotFound, "ns not found"); + return Status::OK(); } const CollectionPtr& coll = diff --git a/src/mongo/db/catalog/rename_collection_test.cpp b/src/mongo/db/catalog/rename_collection_test.cpp index 8605bb21e37..40de0350973 100644 --- a/src/mongo/db/catalog/rename_collection_test.cpp +++ b/src/mongo/db/catalog/rename_collection_test.cpp @@ -804,8 +804,7 @@ TEST_F(RenameCollectionTest, RenameCollectionForApplyOpsSourceAndTargetDoNotExis auto uuid = UUID::gen(); auto cmd = BSON("renameCollection" << _sourceNss.ns() << "to" << _targetNss.ns() << "dropTarget" << "true"); - ASSERT_EQUALS(ErrorCodes::NamespaceNotFound, - renameCollectionForApplyOps(_opCtx.get(), uuid, boost::none, cmd, {})); + ASSERT_OK(renameCollectionForApplyOps(_opCtx.get(), uuid, boost::none, cmd, {})); ASSERT_FALSE(_collectionExists(_opCtx.get(), _sourceNss)); ASSERT_FALSE(_collectionExists(_opCtx.get(), _targetNss)); } diff --git a/src/mongo/db/views/view_catalog_test.cpp b/src/mongo/db/views/view_catalog_test.cpp index 7533b434c1d..1b1232dba64 100644 --- a/src/mongo/db/views/view_catalog_test.cpp +++ b/src/mongo/db/views/view_catalog_test.cpp @@ -469,7 +469,7 @@ TEST_F(ViewCatalogFixture, CannotCreateViewIfItsFullyResolvedPipelineWouldExceed TEST_F(ViewCatalogFixture, DropMissingView) { NamespaceString viewName = NamespaceString::createNamespaceString_forTest("db.view"); - ASSERT_NOT_OK(dropView(operationContext(), viewName)); + ASSERT_OK(dropView(operationContext(), viewName)); } TEST_F(ViewCatalogFixture, ModifyMissingView) { |