SERVER-25004 isolate parsing logic in collmod

(cherry-picked from commit 948f978a782167ddb6efec9f9adf5ad317eb4d0d)

4 files changed, 175 insertions, 111 deletions

diff --git a/src/mongo/db/catalog/coll_mod.cpp b/src/mongo/db/catalog/coll_mod.cpp
index 0f2304a6f49..df3e2e203e1 100644
--- a/src/mongo/db/catalog/coll_mod.cpp
+++ b/src/mongo/db/catalog/coll_mod.cpp
@@ -41,6 +41,99 @@
 #include "mongo/db/service_context.h"
 
 namespace mongo {
+
+struct CollModRequest {
+    const IndexDescriptor* idx = nullptr;
+    BSONElement indexExpireAfterSeconds = {};
+    BSONElement collValidator = {};
+    std::string collValidationAction = {};
+    std::string collValidationLevel = {};
+    BSONElement usePowerOf2Sizes = {};
+    BSONElement noPadding = {};
+};
+
+StatusWith<CollModRequest> parseCollModRequest(OperationContext* txn,
+                                               const NamespaceString& nss,
+                                               Collection* coll,
+                                               const BSONObj& cmdObj) {
+    CollModRequest cmr;
+
+    BSONForEach(e, cmdObj) {
+        if (str::equals("collMod", e.fieldName())) {
+            // no-op
+        } else if (str::startsWith(e.fieldName(), "$")) {
+            // no-op ignore top-level fields prefixed with $. They are for the command processor
+        } else if (LiteParsedQuery::cmdOptionMaxTimeMS == e.fieldNameStringData()) {
+            // no-op
+        } else if (str::equals("index", e.fieldName())) {
+            BSONObj indexObj = e.Obj();
+            BSONObj keyPattern = indexObj.getObjectField("keyPattern");
+
+            if (keyPattern.isEmpty()) {
+                return Status(ErrorCodes::InvalidOptions, "no keyPattern specified");
+            }
+            cmr.indexExpireAfterSeconds = indexObj["expireAfterSeconds"];
+            if (cmr.indexExpireAfterSeconds.eoo()) {
+                return Status(ErrorCodes::InvalidOptions, "no expireAfterSeconds field");
+            }
+            if (!cmr.indexExpireAfterSeconds.isNumber()) {
+                return Status(ErrorCodes::InvalidOptions,
+                              "expireAfterSeconds field must be a number");
+            }
+
+            const IndexDescriptor* idx =
+                coll->getIndexCatalog()->findIndexByKeyPattern(txn, keyPattern);
+            if (idx == NULL) {
+                return Status(ErrorCodes::InvalidOptions,
+                              str::stream() << "cannot find index " << keyPattern << " for ns "
+                                            << nss.ns());
+            }
+            cmr.idx = idx;
+
+            BSONElement oldExpireSecs = cmr.idx->infoObj().getField("expireAfterSeconds");
+
+            if (oldExpireSecs.eoo()) {
+                return Status(ErrorCodes::InvalidOptions, "no expireAfterSeconds field to update");
+            }
+            if (!oldExpireSecs.isNumber()) {
+                return Status(ErrorCodes::InvalidOptions,
+                              "existing expireAfterSeconds field is not a number");
+            }
+        } else if (str::equals("validator", e.fieldName())) {
+            auto statusW = coll->parseValidator(e.Obj());
+            if (!statusW.isOK())
+                return statusW.getStatus();
+
+            cmr.collValidator = e;
+        } else if (str::equals("validationLevel", e.fieldName())) {
+            auto statusW = coll->parseValidationLevel(e.String());
+            if (!statusW.isOK())
+                return statusW.getStatus();
+
+            cmr.collValidationLevel = e.String();
+        } else if (str::equals("validationAction", e.fieldName())) {
+            auto statusW = coll->parseValidationAction(e.String());
+            if (!statusW.isOK())
+                return statusW.getStatus();
+
+            cmr.collValidationAction = e.String();
+        } else {
+            // As of SERVER-17312 we only support these two options. When SERVER-17320 is
+            // resolved this will need to be enhanced to handle other options.
+            const StringData name = e.fieldNameStringData();
+            if (name == "usePowerOf2Sizes")
+                cmr.usePowerOf2Sizes = e;
+            else if (name == "noPadding")
+                cmr.noPadding = e;
+            else
+                return Status(ErrorCodes::InvalidOptions,
+                              str::stream() << "unknown option to collMod: " << name);
+        }
+    }
+
+    return {std::move(cmr)};
+}
+
 Status collMod(OperationContext* txn,
                const NamespaceString& nss,
                const BSONObj& cmdObj,
@@ -49,7 +142,7 @@ Status collMod(OperationContext* txn,
     ScopedTransaction transaction(txn, MODE_IX);
     AutoGetDb autoDb(txn, dbName, MODE_X);
     Database* const db = autoDb.getDb();
-    Collection* coll = db ? db->getCollection(nss) : NULL;
+    Collection* coll = db ? db->getCollection(nss) : nullptr;
 
     // This can kill all cursors so don't allow running it while a background operation is in
     // progress.
@@ -71,122 +164,80 @@ Status collMod(OperationContext* txn,
                                     << nss.ns());
     }
 
+    auto statusW = parseCollModRequest(txn, nss, coll, cmdObj);
+    if (!statusW.isOK()) {
+        return statusW.getStatus();
+    }
+
+    CollModRequest cmr = statusW.getValue();
+
     WriteUnitOfWork wunit(txn);
 
-    Status errorStatus = Status::OK();
+    if (!cmr.indexExpireAfterSeconds.eoo()) {
+        BSONElement& newExpireSecs = cmr.indexExpireAfterSeconds;
+        BSONElement oldExpireSecs = cmr.idx->infoObj().getField("expireAfterSeconds");
+
+        if (oldExpireSecs != newExpireSecs) {
+            result->appendAs(oldExpireSecs, "expireAfterSeconds_old");
+            // Change the value of "expireAfterSeconds" on disk.
+            coll->getCatalogEntry()->updateTTLSetting(
+                txn, cmr.idx->indexName(), newExpireSecs.safeNumberLong());
+            // Notify the index catalog that the definition of this index changed.
+            cmr.idx = coll->getIndexCatalog()->refreshEntry(txn, cmr.idx);
+            result->appendAs(newExpireSecs, "expireAfterSeconds_new");
+        }
+    }
 
-    BSONForEach(e, cmdObj) {
-        if (str::equals("collMod", e.fieldName())) {
-            // no-op
-        } else if (str::startsWith(e.fieldName(), "$")) {
-            // no-op ignore top-level fields prefixed with $. They are for the command processor
-        } else if (LiteParsedQuery::cmdOptionMaxTimeMS == e.fieldNameStringData()) {
-            // no-op
-        } else if (str::equals("index", e.fieldName())) {
-            BSONObj indexObj = e.Obj();
-            BSONObj keyPattern = indexObj.getObjectField("keyPattern");
+    if (!cmr.collValidator.eoo())
+        coll->setValidator(txn, cmr.collValidator.Obj());
 
-            if (keyPattern.isEmpty()) {
-                errorStatus = Status(ErrorCodes::InvalidOptions, "no keyPattern specified");
-                continue;
-            }
+    if (!cmr.collValidationAction.empty())
+        coll->setValidationAction(txn, cmr.collValidationAction);
 
-            BSONElement newExpireSecs = indexObj["expireAfterSeconds"];
-            if (newExpireSecs.eoo()) {
-                errorStatus = Status(ErrorCodes::InvalidOptions, "no expireAfterSeconds field");
-                continue;
-            }
-            if (!newExpireSecs.isNumber()) {
-                errorStatus =
-                    Status(ErrorCodes::InvalidOptions, "expireAfterSeconds field must be a number");
-                continue;
-            }
+    if (!cmr.collValidationLevel.empty())
+        coll->setValidationLevel(txn, cmr.collValidationLevel);
 
-            const IndexDescriptor* idx =
-                coll->getIndexCatalog()->findIndexByKeyPattern(txn, keyPattern);
-            if (idx == NULL) {
-                errorStatus = Status(ErrorCodes::InvalidOptions,
-                                     str::stream() << "cannot find index " << keyPattern
-                                                   << " for ns " << nss.ns());
-                continue;
-            }
-            BSONElement oldExpireSecs = idx->infoObj().getField("expireAfterSeconds");
-            if (oldExpireSecs.eoo()) {
-                errorStatus =
-                    Status(ErrorCodes::InvalidOptions, "no expireAfterSeconds field to update");
-                continue;
-            }
-            if (!oldExpireSecs.isNumber()) {
-                errorStatus = Status(ErrorCodes::InvalidOptions,
-                                     "existing expireAfterSeconds field is not a number");
-                continue;
-            }
+    auto setCollectionOption = [&](BSONElement& COElement) {
+        typedef CollectionOptions CO;
+        const StringData name = COElement.fieldNameStringData();
 
-            if (oldExpireSecs != newExpireSecs) {
-                result->appendAs(oldExpireSecs, "expireAfterSeconds_old");
-                // Change the value of "expireAfterSeconds" on disk.
-                coll->getCatalogEntry()->updateTTLSetting(
-                    txn, idx->indexName(), newExpireSecs.numberLong());
-                // Notify the index catalog that the definition of this index changed.
-                idx = coll->getIndexCatalog()->refreshEntry(txn, idx);
-                result->appendAs(newExpireSecs, "expireAfterSeconds_new");
-            }
-        } else if (str::equals("validator", e.fieldName())) {
-            auto status = coll->setValidator(txn, e.Obj());
-            if (!status.isOK())
-                errorStatus = std::move(status);
-        } else if (str::equals("validationLevel", e.fieldName())) {
-            auto status = coll->setValidationLevel(txn, e.String());
-            if (!status.isOK())
-                errorStatus = std::move(status);
-        } else if (str::equals("validationAction", e.fieldName())) {
-            auto status = coll->setValidationAction(txn, e.String());
-            if (!status.isOK())
-                errorStatus = std::move(status);
-        } else {
-            // As of SERVER-17312 we only support these two options. When SERVER-17320 is
-            // resolved this will need to be enhanced to handle other options.
-            typedef CollectionOptions CO;
-            const StringData name = e.fieldNameStringData();
-            const int flag = (name == "usePowerOf2Sizes")
-                ? CO::Flag_UsePowerOf2Sizes
-                : (name == "noPadding") ? CO::Flag_NoPadding : 0;
-            if (!flag) {
-                errorStatus = Status(ErrorCodes::InvalidOptions,
-                                     str::stream() << "unknown option to collMod: " << name);
-                continue;
-            }
+        int flag = (name == "usePowerOf2Sizes") ? CO::Flag_UsePowerOf2Sizes
+                                                : (name == "noPadding") ? CO::Flag_NoPadding : 0;
 
-            CollectionCatalogEntry* cce = coll->getCatalogEntry();
+        CollectionCatalogEntry* cce = coll->getCatalogEntry();
 
-            const int oldFlags = cce->getCollectionOptions(txn).flags;
-            const bool oldSetting = oldFlags & flag;
-            const bool newSetting = e.trueValue();
+        const int oldFlags = cce->getCollectionOptions(txn).flags;
+        const bool oldSetting = oldFlags & flag;
+        const bool newSetting = COElement.trueValue();
 
-            result->appendBool(name.toString() + "_old", oldSetting);
-            result->appendBool(name.toString() + "_new", newSetting);
+        result->appendBool(name.toString() + "_old", oldSetting);
+        result->appendBool(name.toString() + "_new", newSetting);
 
-            const int newFlags = newSetting ? (oldFlags | flag)    // set flag
-                                            : (oldFlags & ~flag);  // clear flag
+        const int newFlags = newSetting ? (oldFlags | flag)    // set flag
+                                        : (oldFlags & ~flag);  // clear flag
 
-            // NOTE we do this unconditionally to ensure that we note that the user has
-            // explicitly set flags, even if they are just setting the default.
-            cce->updateFlags(txn, newFlags);
+        // NOTE we do this unconditionally to ensure that we note that the user has
+        // explicitly set flags, even if they are just setting the default.
+        cce->updateFlags(txn, newFlags);
 
-            const CollectionOptions newOptions = cce->getCollectionOptions(txn);
-            invariant(newOptions.flags == newFlags);
-            invariant(newOptions.flagsSet);
-        }
+        const CollectionOptions newOptions = cce->getCollectionOptions(txn);
+        invariant(newOptions.flags == newFlags);
+        invariant(newOptions.flagsSet);
+    };
+
+    if (!cmr.usePowerOf2Sizes.eoo()) {
+        setCollectionOption(cmr.usePowerOf2Sizes);
     }
 
-    if (!errorStatus.isOK()) {
-        return errorStatus;
+    if (!cmr.noPadding.eoo()) {
+        setCollectionOption(cmr.noPadding);
     }
 
     getGlobalServiceContext()->getOpObserver()->onCollMod(
         txn, (dbName.toString() + ".$cmd").c_str(), cmdObj);
 
     wunit.commit();
+
     return Status::OK();
 }
 }  // namespace mongo
diff --git a/src/mongo/db/catalog/coll_mod.h b/src/mongo/db/catalog/coll_mod.h
index eb8644b74d1..bb86bbc50d1 100644
--- a/src/mongo/db/catalog/coll_mod.h
+++ b/src/mongo/db/catalog/coll_mod.h
@@ -27,13 +27,22 @@
  */
 
 #include "mongo/base/status.h"
+#include "mongo/base/status_with.h"
 
 namespace mongo {
 class BSONObj;
 class BSONObjBuilder;
+class Collection;
 class NamespaceString;
 class OperationContext;
 
+struct CollModRequest;
+
+StatusWith<CollModRequest> parseCollModRequest(OperationContext* txn,
+                                               const NamespaceString& nss,
+                                               Collection* coll,
+                                               const BSONObj& cmdObj);
+
 /**
  * Performs the collection modification described in "cmdObj" on the collection "ns".
  */
diff --git a/src/mongo/db/catalog/collection.cpp b/src/mongo/db/catalog/collection.cpp
index 0043b20a4c5..42fc0ea7fee 100644
--- a/src/mongo/db/catalog/collection.cpp
+++ b/src/mongo/db/catalog/collection.cpp
@@ -180,9 +180,9 @@ Collection::Collection(OperationContext* txn,
       _validatorDoc(_details->getCollectionOptions(txn).validator.getOwned()),
       _validator(uassertStatusOK(parseValidator(_validatorDoc))),
       _validationAction(uassertStatusOK(
-          _parseValidationAction(_details->getCollectionOptions(txn).validationAction))),
+          parseValidationAction(_details->getCollectionOptions(txn).validationAction))),
       _validationLevel(uassertStatusOK(
-          _parseValidationLevel(_details->getCollectionOptions(txn).validationLevel))),
+          parseValidationLevel(_details->getCollectionOptions(txn).validationLevel))),
       _cursorManager(fullNS),
       _cappedNotifier(_recordStore->isCapped() ? new CappedInsertNotifier() : nullptr),
       _mustTakeCappedLockOnInsert(isCapped() && !_ns.isSystemDotProfile() && !_ns.isOplog()) {
@@ -823,7 +823,7 @@ Status Collection::setValidator(OperationContext* txn, BSONObj validatorDoc) {
     return Status::OK();
 }
 
-StatusWith<Collection::ValidationLevel> Collection::_parseValidationLevel(StringData newLevel) {
+StatusWith<Collection::ValidationLevel> Collection::parseValidationLevel(StringData newLevel) {
     if (newLevel == "") {
         // default
         return STRICT_V;
@@ -839,7 +839,7 @@ StatusWith<Collection::ValidationLevel> Collection::_parseValidationLevel(String
     }
 }
 
-StatusWith<Collection::ValidationAction> Collection::_parseValidationAction(StringData newAction) {
+StatusWith<Collection::ValidationAction> Collection::parseValidationAction(StringData newAction) {
     if (newAction == "") {
         // default
         return ERROR_V;
@@ -878,7 +878,7 @@ StringData Collection::getValidationAction() const {
 Status Collection::setValidationLevel(OperationContext* txn, StringData newLevel) {
     invariant(txn->lockState()->isCollectionLockedForMode(ns().toString(), MODE_X));
 
-    StatusWith<ValidationLevel> status = _parseValidationLevel(newLevel);
+    StatusWith<ValidationLevel> status = parseValidationLevel(newLevel);
     if (!status.isOK()) {
         return status.getStatus();
     }
@@ -893,7 +893,7 @@ Status Collection::setValidationLevel(OperationContext* txn, StringData newLevel
 Status Collection::setValidationAction(OperationContext* txn, StringData newAction) {
     invariant(txn->lockState()->isCollectionLockedForMode(ns().toString(), MODE_X));
 
-    StatusWith<ValidationAction> status = _parseValidationAction(newAction);
+    StatusWith<ValidationAction> status = parseValidationAction(newAction);
     if (!status.isOK()) {
         return status.getStatus();
     }
diff --git a/src/mongo/db/catalog/collection.h b/src/mongo/db/catalog/collection.h
index 6823045b952..becfe366ab3 100644
--- a/src/mongo/db/catalog/collection.h
+++ b/src/mongo/db/catalog/collection.h
@@ -38,6 +38,7 @@
 #include "mongo/base/status_with.h"
 #include "mongo/base/string_data.h"
 #include "mongo/bson/mutable/damage_vector.h"
+#include "mongo/db/catalog/coll_mod.h"
 #include "mongo/db/catalog/collection_info_cache.h"
 #include "mongo/db/catalog/cursor_manager.h"
 #include "mongo/db/catalog/index_catalog.h"
@@ -362,6 +363,16 @@ public:
      */
     void temp_cappedTruncateAfter(OperationContext* txn, RecordId end, bool inclusive);
 
+    enum ValidationAction { WARN, ERROR_V };
+    enum ValidationLevel { OFF, MODERATE, STRICT_V };
+
+    /**
+     * Returns a non-ok Status if validator is not legal for this collection.
+     */
+    StatusWithMatchExpression parseValidator(const BSONObj& validator) const;
+
+    static StatusWith<ValidationLevel> parseValidationLevel(StringData);
+    static StatusWith<ValidationAction> parseValidationAction(StringData);
     /**
      * Sets the validator for this collection.
      *
@@ -428,11 +439,6 @@ private:
      */
     Status checkValidation(OperationContext* txn, const BSONObj& document) const;
 
-    /**
-     * Returns a non-ok Status if validator is not legal for this collection.
-     */
-    StatusWithMatchExpression parseValidator(const BSONObj& validator) const;
-
     Status recordStoreGoingToMove(OperationContext* txn,
                                   const RecordId& oldLocation,
                                   const char* oldBuffer,
@@ -470,11 +476,9 @@ private:
     BSONObj _validatorDoc;
     // Points into _validatorDoc. Null means no filter.
     std::unique_ptr<MatchExpression> _validator;
-    enum ValidationAction { WARN, ERROR_V } _validationAction;
-    enum ValidationLevel { OFF, MODERATE, STRICT_V } _validationLevel;
 
-    static StatusWith<ValidationLevel> _parseValidationLevel(StringData);
-    static StatusWith<ValidationAction> _parseValidationAction(StringData);
+    ValidationAction _validationAction;
+    ValidationLevel _validationLevel;
 
     // this is mutable because read only users of the Collection class
     // use it keep state.  This seems valid as const correctness of Collection