SERVER-39323 refactor index spec validation code to make it more modularized and flexible.

- Remove a "$freelist" collection reference, which was removed with MMAPv1
- Modularize a {buildIndexes:false} check that causes a IndexAlreadyExists error to be returned
- Split IndexCatalog::removeExistingIndexes into removeExistingIndexes and removeExistingIndexesNoChecks,
  to be more explicit about the internal behavior.

9 files changed, 160 insertions, 68 deletions

diff --git a/src/mongo/db/catalog/index_catalog.h b/src/mongo/db/catalog/index_catalog.h
index 5ba372df577..304ef4a09f8 100644
--- a/src/mongo/db/catalog/index_catalog.h
+++ b/src/mongo/db/catalog/index_catalog.h
@@ -351,21 +351,41 @@ public:
     virtual StatusWith<BSONObj> createIndexOnEmptyCollection(OperationContext* const opCtx,
                                                              const BSONObj spec) = 0;
 
+    /**
+     * Checks the spec 'original' to make sure nothing is incorrectly set and cleans up any legacy
+     * fields. Lastly, checks whether the spec conflicts with ready and in-progress indexes.
+     *
+     * Returns an error Status or the cleaned up version of the non-conflicting spec. Returns
+     * IndexAlreadyExists if the index either already exists or is already being built.
+     */
     virtual StatusWith<BSONObj> prepareSpecForCreate(OperationContext* const opCtx,
                                                      const BSONObj& original) const = 0;
 
     /**
-     * Returns a copy of 'indexSpecsToBuild' that does not contain index specifications already
-     * existing in this index catalog. If this isn't done, an index build using 'indexSpecsToBuild'
-     * may fail with error code IndexAlreadyExists.
+     * Returns a copy of 'indexSpecsToBuild' that does not contain index specifications that already
+     * exist or are already being built. If this is not done, an index build using
+     * 'indexSpecsToBuild' may fail with error code IndexAlreadyExists. If {buildIndexes:false} is
+     * set in the replica set config, also filters non-_id index specs out of the results.
      *
-     * If 'throwOnErrors' is set to true, any validation errors on a non-duplicate index
-     * will result in this function throwing an exception.
+     * Additionally verifies the specs are valid and corrects any legacy fields. Throws on any spec
+     * validation errors or conflicts other than IndexAlreadyExists, which indicates that the index
+     * spec either already exists or is already being built and is what this function filters out.
      */
     virtual std::vector<BSONObj> removeExistingIndexes(
-        OperationContext* const opCtx,
-        const std::vector<BSONObj>& indexSpecsToBuild,
-        bool throwOnErrors = false) const = 0;
+        OperationContext* const opCtx, const std::vector<BSONObj>& indexSpecsToBuild) const = 0;
+
+    /**
+     * Filters out ready and in-progress indexes that already exist and returns the remaining
+     * indexes. Additionally filters out non-_id indexes if the replica set member config has
+     * {buildIndexes:false} set.
+     *
+     * Does no correctness verification of the provided specs, nor modifications for legacy reasons.
+     *
+     * This should only be used when we are confident in the specs, such as when specs are received
+     * via replica set cloning or chunk migrations.
+     */
+    virtual std::vector<BSONObj> removeExistingIndexesNoChecks(
+        OperationContext* const opCtx, const std::vector<BSONObj>& indexSpecsToBuild) const = 0;
 
     /**
      * Drops all indexes in the index catalog, optionally dropping the id index depending on the
diff --git a/src/mongo/db/catalog/index_catalog_impl.cpp b/src/mongo/db/catalog/index_catalog_impl.cpp
index 435e71a5888..f946ebf7c63 100644
--- a/src/mongo/db/catalog/index_catalog_impl.cpp
+++ b/src/mongo/db/catalog/index_catalog_impl.cpp
@@ -149,7 +149,7 @@ IndexCatalogEntry* IndexCatalogImpl::_setupInMemoryStructures(
     bool initFromDisk,
     bool isReadyIndex) {
     Status status = _isSpecOk(opCtx, descriptor->infoObj());
-    if (!status.isOK() && status != ErrorCodes::IndexAlreadyExists) {
+    if (!status.isOK()) {
         severe() << "Found an invalid index " << descriptor->infoObj() << " on the "
                  << _collection->ns() << " collection: " << redact(status);
         fassertFailedNoTrace(28782);
@@ -284,43 +284,98 @@ string IndexCatalogImpl::_getAccessMethodName(const BSONObj& keyPattern) const {
 
 // ---------------------------
 
-StatusWith<BSONObj> IndexCatalogImpl::prepareSpecForCreate(OperationContext* opCtx,
-                                                           const BSONObj& original) const {
+StatusWith<BSONObj> IndexCatalogImpl::_validateAndFixIndexSpec(OperationContext* opCtx,
+                                                               const BSONObj& original) const {
     Status status = _isSpecOk(opCtx, original);
-    if (!status.isOK())
-        return StatusWith<BSONObj>(status);
+    if (!status.isOK()) {
+        return status;
+    }
 
-    auto fixed = _fixIndexSpec(opCtx, _collection, original);
-    if (!fixed.isOK()) {
-        return fixed;
+    auto swFixed = _fixIndexSpec(opCtx, _collection, original);
+    if (!swFixed.isOK()) {
+        return swFixed;
     }
 
     // we double check with new index spec
-    status = _isSpecOk(opCtx, fixed.getValue());
-    if (!status.isOK())
-        return StatusWith<BSONObj>(status);
+    status = _isSpecOk(opCtx, swFixed.getValue());
+    if (!status.isOK()) {
+        return status;
+    }
 
-    status = _doesSpecConflictWithExisting(opCtx, fixed.getValue());
-    if (!status.isOK())
-        return StatusWith<BSONObj>(status);
+    return swFixed;
+}
+
+Status IndexCatalogImpl::_isNonIDIndexAndNotAllowedToBuild(OperationContext* opCtx,
+                                                           const BSONObj& spec) const {
+    const BSONObj key = spec.getObjectField("key");
+    invariant(!key.isEmpty());
+    if (!IndexDescriptor::isIdIndexPattern(key)) {
+        // Check whether the replica set member's config has {buildIndexes:false} set, which means
+        // we are not allowed to build non-_id indexes on this server.
+        if (!repl::ReplicationCoordinator::get(opCtx)->buildsIndexes()) {
+            // We return an IndexAlreadyExists error so that the caller can catch it and silently
+            // skip building it.
+            return Status(ErrorCodes::IndexAlreadyExists,
+                          "this replica set member's 'buildIndexes' setting is set to false");
+        }
+    }
 
-    return fixed;
+    return Status::OK();
+}
+
+StatusWith<BSONObj> IndexCatalogImpl::prepareSpecForCreate(OperationContext* opCtx,
+                                                           const BSONObj& original) const {
+    auto swValidatedAndFixed = _validateAndFixIndexSpec(opCtx, original);
+    if (!swValidatedAndFixed.isOK()) {
+        return swValidatedAndFixed.getStatus();
+    }
+
+    // Check whether this is a non-_id index and there are any settings disallowing this server
+    // from building non-_id indexes.
+    Status status = _isNonIDIndexAndNotAllowedToBuild(opCtx, swValidatedAndFixed.getValue());
+    if (!status.isOK()) {
+        return status;
+    }
+
+    status = _doesSpecConflictWithExisting(opCtx, swValidatedAndFixed.getValue());
+    if (!status.isOK()) {
+        return status;
+    }
+
+    return swValidatedAndFixed.getValue();
+}
+
+std::vector<BSONObj> IndexCatalogImpl::removeExistingIndexesNoChecks(
+    OperationContext* const opCtx, const std::vector<BSONObj>& indexSpecsToBuild) const {
+    std::vector<BSONObj> result;
+    // Filter out ready and in-progress index builds, and any non-_id indexes if 'buildIndexes' is
+    // set to false in the replica set's config.
+    for (const auto& spec : indexSpecsToBuild) {
+        // returned to be built by the caller.
+        if (ErrorCodes::OK != _isNonIDIndexAndNotAllowedToBuild(opCtx, spec)) {
+            continue;
+        }
+
+        // _doesSpecConflictWithExisting currently does more work than we require here: we are only
+        // interested in the index already exists error.
+        if (ErrorCodes::IndexAlreadyExists == _doesSpecConflictWithExisting(opCtx, spec)) {
+            continue;
+        }
+
+        result.push_back(spec);
+    }
+    return result;
 }
 
 std::vector<BSONObj> IndexCatalogImpl::removeExistingIndexes(
-    OperationContext* const opCtx,
-    const std::vector<BSONObj>& indexSpecsToBuild,
-    bool throwOnErrors) const {
+    OperationContext* const opCtx, const std::vector<BSONObj>& indexSpecsToBuild) const {
     std::vector<BSONObj> result;
     for (const auto& spec : indexSpecsToBuild) {
         auto prepareResult = prepareSpecForCreate(opCtx, spec);
         if (prepareResult == ErrorCodes::IndexAlreadyExists) {
             continue;
         }
-        // Intentionally ignoring other error codes unless 'throwOnErrors' is true.
-        if (throwOnErrors) {
-            uassertStatusOK(prepareResult);
-        }
+        uassertStatusOK(prepareResult);
         result.push_back(spec);
     }
     return result;
@@ -447,11 +502,6 @@ Status IndexCatalogImpl::_isSpecOk(OperationContext* opCtx, const BSONObj& spec)
     if (nss.isOplog())
         return Status(ErrorCodes::CannotCreateIndex, "cannot have an index on the oplog");
 
-    if (nss.coll() == "$freelist") {
-        // this isn't really proper, but we never want it and its not an error per se
-        return Status(ErrorCodes::IndexAlreadyExists, "cannot index freelist");
-    }
-
     const BSONElement specNamespace = spec["ns"];
     if (specNamespace.type() != String)
         return Status(ErrorCodes::CannotCreateIndex,
@@ -621,13 +671,6 @@ Status IndexCatalogImpl::_isSpecOk(OperationContext* opCtx, const BSONObj& spec)
             return Status(ErrorCodes::CannotCreateIndex,
                           "_id index must have the collection default collation");
         }
-    } else {
-        // for non _id indexes, we check to see if replication has turned off all indexes
-        // we _always_ created _id index
-        if (!repl::ReplicationCoordinator::get(opCtx)->buildsIndexes()) {
-            // this is not exactly the right error code, but I think will make the most sense
-            return Status(ErrorCodes::IndexAlreadyExists, "no indexes per repl");
-        }
     }
 
     // --- only storage engine checks allowed below this ----
@@ -666,8 +709,8 @@ Status IndexCatalogImpl::_doesSpecConflictWithExisting(OperationContext* opCtx,
     const BSONObj collation = spec.getObjectField("collation");
 
     {
-        // Check both existing and in-progress indexes (2nd param = true)
-        const IndexDescriptor* desc = findIndexByName(opCtx, name, true);
+        const IndexDescriptor* desc =
+            findIndexByName(opCtx, name, true /*includeUnfinishedIndexes*/);
         if (desc) {
             // index already exists with same name
 
@@ -711,10 +754,8 @@ Status IndexCatalogImpl::_doesSpecConflictWithExisting(OperationContext* opCtx,
     }
 
     {
-        // Check both existing and in-progress indexes.
-        const bool findInProgressIndexes = true;
-        const IndexDescriptor* desc =
-            findIndexByKeyPatternAndCollationSpec(opCtx, key, collation, findInProgressIndexes);
+        const IndexDescriptor* desc = findIndexByKeyPatternAndCollationSpec(
+            opCtx, key, collation, true /*includeUnfinishedIndexes*/);
         if (desc) {
             LOG(2) << "Index already exists with a different name: " << name << " pattern: " << key
                    << " collation: " << collation;
diff --git a/src/mongo/db/catalog/index_catalog_impl.h b/src/mongo/db/catalog/index_catalog_impl.h
index 26f47b801ad..1bfde7395ac 100644
--- a/src/mongo/db/catalog/index_catalog_impl.h
+++ b/src/mongo/db/catalog/index_catalog_impl.h
@@ -191,9 +191,13 @@ public:
     StatusWith<BSONObj> prepareSpecForCreate(OperationContext* opCtx,
                                              const BSONObj& original) const override;
 
-    std::vector<BSONObj> removeExistingIndexes(OperationContext* const opCtx,
-                                               const std::vector<BSONObj>& indexSpecsToBuild,
-                                               bool throwOnErrors) const override;
+    std::vector<BSONObj> removeExistingIndexes(
+        OperationContext* const opCtx,
+        const std::vector<BSONObj>& indexSpecsToBuild) const override;
+
+    std::vector<BSONObj> removeExistingIndexesNoChecks(
+        OperationContext* const opCtx,
+        const std::vector<BSONObj>& indexSpecsToBuild) const override;
 
     /**
      * Drops all indexes in the index catalog, optionally dropping the id index depending on the
@@ -420,18 +424,39 @@ private:
                                                 bool initFromDisk,
                                                 bool isReadyIndex);
 
-    // Apply a set of transformations to the user-provided index object 'spec' to make it
-    // conform to the standard for insertion.  This function adds the 'v' field if it didn't
-    // exist, removes the '_id' field if it exists, applies plugin-level transformations if
-    // appropriate, etc.
+    /**
+     * Applies a set of transformations to the user-provided index object 'spec' to make it
+     * conform to the standard for insertion.  Removes the '_id' field if it exists, applies
+     * plugin-level transformations if appropriate, etc.
+     */
     StatusWith<BSONObj> _fixIndexSpec(OperationContext* opCtx,
                                       Collection* collection,
                                       const BSONObj& spec) const;
 
     Status _isSpecOk(OperationContext* opCtx, const BSONObj& spec) const;
 
+    /**
+     * Validates the 'original' index specification, alters any legacy fields and does plugin-level
+     * transformations for text and geo indexes. Returns a clean spec ready to be built, or an
+     * error.
+     */
+    StatusWith<BSONObj> _validateAndFixIndexSpec(OperationContext* opCtx,
+                                                 const BSONObj& original) const;
+
+    /**
+     * Checks whether there are any spec conflicts with existing ready indexes or in-progress index
+     * builds. Also checks whether any limits set on this server would be exceeded by building the
+     * index.
+     */
     Status _doesSpecConflictWithExisting(OperationContext* opCtx, const BSONObj& spec) const;
 
+    /**
+     * Returns true if the replica set member's config has {buildIndexes:false} set, which means
+     * we are not allowed to build non-_id indexes on this server, AND this index spec is for a
+     * non-_id index.
+     */
+    Status _isNonIDIndexAndNotAllowedToBuild(OperationContext* opCtx, const BSONObj& spec) const;
+
     int _magic;
     Collection* const _collection;
     const int _maxNumIndexesAllowed;
diff --git a/src/mongo/db/catalog/index_catalog_noop.h b/src/mongo/db/catalog/index_catalog_noop.h
index 445b26f384c..a1cb16bd9f4 100644
--- a/src/mongo/db/catalog/index_catalog_noop.h
+++ b/src/mongo/db/catalog/index_catalog_noop.h
@@ -147,9 +147,15 @@ public:
         return original;
     }
 
-    std::vector<BSONObj> removeExistingIndexes(OperationContext* const opCtx,
-                                               const std::vector<BSONObj>& indexSpecsToBuild,
-                                               bool throwOnErrors) const override {
+    std::vector<BSONObj> removeExistingIndexes(
+        OperationContext* const opCtx,
+        const std::vector<BSONObj>& indexSpecsToBuild) const override {
+        return {};
+    }
+
+    std::vector<BSONObj> removeExistingIndexesNoChecks(
+        OperationContext* const opCtx,
+        const std::vector<BSONObj>& indexSpecsToBuild) const override {
         return {};
     }
 
diff --git a/src/mongo/db/cloner.cpp b/src/mongo/db/cloner.cpp
index 3f49093f124..b52ef891603 100644
--- a/src/mongo/db/cloner.cpp
+++ b/src/mongo/db/cloner.cpp
@@ -394,7 +394,7 @@ void Cloner::copyIndexes(OperationContext* opCtx,
     }
 
     auto indexCatalog = collection->getIndexCatalog();
-    auto prunedIndexesToBuild = indexCatalog->removeExistingIndexes(opCtx, indexesToBuild);
+    auto prunedIndexesToBuild = indexCatalog->removeExistingIndexesNoChecks(opCtx, indexesToBuild);
     if (prunedIndexesToBuild.empty()) {
         return;
     }
diff --git a/src/mongo/db/commands/create_indexes.cpp b/src/mongo/db/commands/create_indexes.cpp
index 7e93a5d4366..380a4050958 100644
--- a/src/mongo/db/commands/create_indexes.cpp
+++ b/src/mongo/db/commands/create_indexes.cpp
@@ -203,17 +203,17 @@ boost::optional<CommitQuorumOptions> parseAndGetCommitQuorum(OperationContext* o
 
 /**
  * Returns a vector of index specs with the filled in collection default options and removes any
- * indexes that already exist on the collection. If the returned vector is empty after returning, no
- * new indexes need to be built. Throws on error.
+ * indexes that already exist on the collection -- both ready indexes and in-progress builds. If the
+ * returned vector is empty after returning, no new indexes need to be built. Throws on error.
  */
 std::vector<BSONObj> resolveDefaultsAndRemoveExistingIndexes(OperationContext* opCtx,
                                                              const Collection* collection,
-                                                             std::vector<BSONObj> validatedSpecs) {
-    auto swDefaults = collection->addCollationDefaultsToIndexSpecsForCreate(opCtx, validatedSpecs);
+                                                             std::vector<BSONObj> indexSpecs) {
+    auto swDefaults = collection->addCollationDefaultsToIndexSpecsForCreate(opCtx, indexSpecs);
     uassertStatusOK(swDefaults.getStatus());
 
     auto indexCatalog = collection->getIndexCatalog();
-    return indexCatalog->removeExistingIndexes(opCtx, swDefaults.getValue(), /*throwOnError=*/true);
+    return indexCatalog->removeExistingIndexes(opCtx, swDefaults.getValue());
 }
 
 void checkUniqueIndexConstraints(OperationContext* opCtx,
diff --git a/src/mongo/db/index_builds_coordinator.cpp b/src/mongo/db/index_builds_coordinator.cpp
index 243e5755d20..23062bd9aea 100644
--- a/src/mongo/db/index_builds_coordinator.cpp
+++ b/src/mongo/db/index_builds_coordinator.cpp
@@ -1130,8 +1130,7 @@ std::vector<BSONObj> IndexBuildsCoordinator::_addDefaultsAndFilterExistingIndexe
         uassertStatusOK(collection->addCollationDefaultsToIndexSpecsForCreate(opCtx, indexSpecs));
 
     auto indexCatalog = collection->getIndexCatalog();
-    auto filteredSpecs = indexCatalog->removeExistingIndexes(
-        opCtx, specsWithCollationDefaults, /*throwOnError=*/true);
+    auto filteredSpecs = indexCatalog->removeExistingIndexes(opCtx, specsWithCollationDefaults);
 
     for (const BSONObj& spec : filteredSpecs) {
         if (spec[kUniqueFieldName].trueValue()) {
diff --git a/src/mongo/db/repl/collection_bulk_loader_impl.cpp b/src/mongo/db/repl/collection_bulk_loader_impl.cpp
index 6acaeaffbe5..7449a3761d7 100644
--- a/src/mongo/db/repl/collection_bulk_loader_impl.cpp
+++ b/src/mongo/db/repl/collection_bulk_loader_impl.cpp
@@ -82,7 +82,8 @@ Status CollectionBulkLoaderImpl::init(const std::vector<BSONObj>& secondaryIndex
             UnreplicatedWritesBlock uwb(_opCtx.get());
             // This enforces the buildIndexes setting in the replica set configuration.
             auto indexCatalog = coll->getIndexCatalog();
-            auto specs = indexCatalog->removeExistingIndexes(_opCtx.get(), secondaryIndexSpecs);
+            auto specs =
+                indexCatalog->removeExistingIndexesNoChecks(_opCtx.get(), secondaryIndexSpecs);
             if (specs.size()) {
                 _secondaryIndexesBlock->ignoreUniqueConstraint();
                 auto status =
diff --git a/src/mongo/db/s/migration_destination_manager.cpp b/src/mongo/db/s/migration_destination_manager.cpp
index 3b0501316a5..9c2a40795f5 100644
--- a/src/mongo/db/s/migration_destination_manager.cpp
+++ b/src/mongo/db/s/migration_destination_manager.cpp
@@ -619,7 +619,7 @@ void MigrationDestinationManager::cloneCollectionIndexesAndOptions(OperationCont
         // possible).
         auto checkEmptyOrGetMissingIndexesFromDonor = [&](Collection* collection) {
             auto indexCatalog = collection->getIndexCatalog();
-            auto indexSpecs = indexCatalog->removeExistingIndexes(opCtx, donorIndexSpecs);
+            auto indexSpecs = indexCatalog->removeExistingIndexesNoChecks(opCtx, donorIndexSpecs);
             if (!indexSpecs.empty()) {
                 // Only allow indexes to be copied if the collection does not have any documents.
                 uassert(ErrorCodes::CannotCreateCollection,