SERVER-32348 Make UpdateDriver::parse() throw an exception instead of returning error Status

9 files changed, 67 insertions, 68 deletions

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 d755d61c556..62c8c9fae74 100644
--- a/src/mongo/db/auth/authz_manager_external_state_mock.cpp
+++ b/src/mongo/db/auth/authz_manager_external_state_mock.cpp
@@ -186,12 +186,10 @@ Status AuthzManagerExternalStateMock::updateOne(OperationContext* opCtx,
     boost::intrusive_ptr<ExpressionContext> expCtx(new ExpressionContext(opCtx, collator));
     UpdateDriver driver(std::move(expCtx));
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
-    Status status = driver.parse(updatePattern, arrayFilters);
-    if (!status.isOK())
-        return status;
+    driver.parse(updatePattern, arrayFilters);
 
     BSONObjCollection::iterator iter;
-    status = _findOneIter(opCtx, collectionName, query, &iter);
+    Status status = _findOneIter(opCtx, collectionName, query, &iter);
     mmb::Document document;
     if (status.isOK()) {
         document.reset(*iter, mmb::Document::kInPlaceDisabled);
diff --git a/src/mongo/db/auth/role_graph_update.cpp b/src/mongo/db/auth/role_graph_update.cpp
index 89a67b44638..16f4f1cb566 100644
--- a/src/mongo/db/auth/role_graph_update.cpp
+++ b/src/mongo/db/auth/role_graph_update.cpp
@@ -208,9 +208,7 @@ Status handleOplogUpdate(OperationContext* opCtx,
 
     // Oplog updates do not have array filters.
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
-    status = driver.parse(updatePattern, arrayFilters);
-    if (!status.isOK())
-        return status;
+    driver.parse(updatePattern, arrayFilters);
 
     mutablebson::Document roleDocument;
     status = RoleGraph::getBSONForRole(roleGraph, roleToUpdate, roleDocument.root());
diff --git a/src/mongo/db/ops/parsed_update.cpp b/src/mongo/db/ops/parsed_update.cpp
index d7a31d0b936..f3da8cb18c3 100644
--- a/src/mongo/db/ops/parsed_update.cpp
+++ b/src/mongo/db/ops/parsed_update.cpp
@@ -72,9 +72,7 @@ Status ParsedUpdate::parseRequest() {
     // may determine whether or not we need to produce a CanonicalQuery at all.  For example, if
     // the update involves the positional-dollar operator, we must have a CanonicalQuery even if
     // it isn't required for query execution.
-    status = parseUpdate();
-    if (!status.isOK())
-        return status;
+    parseUpdate();
     status = parseQuery();
     if (!status.isOK())
         return status;
@@ -139,12 +137,12 @@ Status ParsedUpdate::parseQueryToCQ() {
     return statusWithCQ.getStatus();
 }
 
-Status ParsedUpdate::parseUpdate() {
+void ParsedUpdate::parseUpdate() {
     _driver.setCollator(_collator.get());
     _driver.setLogOp(true);
     _driver.setFromOplogApplication(_request->isFromOplogApplication());
 
-    return _driver.parse(_request->getUpdates(), _arrayFilters, _request->isMulti());
+    _driver.parse(_request->getUpdates(), _arrayFilters, _request->isMulti());
 }
 
 Status ParsedUpdate::parseArrayFilters() {
diff --git a/src/mongo/db/ops/parsed_update.h b/src/mongo/db/ops/parsed_update.h
index 8e006f461d1..f6334650244 100644
--- a/src/mongo/db/ops/parsed_update.h
+++ b/src/mongo/db/ops/parsed_update.h
@@ -131,7 +131,7 @@ private:
     /**
      * Parses the update-descriptor portion of the update request.
      */
-    Status parseUpdate();
+    void parseUpdate();
 
     /**
      * Parses the array filters portion of the update request.
diff --git a/src/mongo/db/ops/update.cpp b/src/mongo/db/ops/update.cpp
index 85664098e4d..4cacd35d047 100644
--- a/src/mongo/db/ops/update.cpp
+++ b/src/mongo/db/ops/update.cpp
@@ -114,19 +114,14 @@ BSONObj applyUpdateOperators(OperationContext* opCtx,
     boost::intrusive_ptr<ExpressionContext> expCtx(new ExpressionContext(opCtx, collator));
     UpdateDriver driver(std::move(expCtx));
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
-    Status status = driver.parse(operators, arrayFilters);
-    if (!status.isOK()) {
-        uasserted(16838, status.reason());
-    }
+    driver.parse(operators, arrayFilters);
 
     mutablebson::Document doc(from, mutablebson::Document::kInPlaceDisabled);
 
     const bool validateForStorage = false;
     const FieldRefSet emptyImmutablePaths;
-    status = driver.update(StringData(), &doc, validateForStorage, emptyImmutablePaths);
-    if (!status.isOK()) {
-        uasserted(16839, status.reason());
-    }
+
+    uassertStatusOK(driver.update(StringData(), &doc, validateForStorage, emptyImmutablePaths));
 
     return doc.getObject();
 }
diff --git a/src/mongo/db/update/update_driver.cpp b/src/mongo/db/update/update_driver.cpp
index d8b824316cb..761f914be83 100644
--- a/src/mongo/db/update/update_driver.cpp
+++ b/src/mongo/db/update/update_driver.cpp
@@ -149,7 +149,7 @@ bool parseUpdateExpression(
 UpdateDriver::UpdateDriver(const boost::intrusive_ptr<ExpressionContext>& expCtx)
     : _expCtx(expCtx) {}
 
-Status UpdateDriver::parse(
+void UpdateDriver::parse(
     const BSONObj& updateExpr,
     const std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>>& arrayFilters,
     const bool multi) {
@@ -157,16 +157,14 @@ Status UpdateDriver::parse(
 
     // Check if the update expression is a full object replacement.
     if (isDocReplacement(updateExpr)) {
-        if (multi) {
-            return Status(ErrorCodes::FailedToParse, "multi update only works with $ operators");
-        }
+        uassert(ErrorCodes::FailedToParse, "multi update only works with $ operators", !multi);
 
         _root = stdx::make_unique<ObjectReplaceNode>(updateExpr);
 
         // Register the fact that this driver will only do full object replacements.
         _replacementMode = true;
 
-        return Status::OK();
+        return;
     }
 
     // Register the fact that this driver is not doing a full object replacement.
@@ -178,20 +176,16 @@ Status UpdateDriver::parse(
     // we parse $v and check its value for compatibility.
     BSONElement updateSemanticsElement = updateExpr[LogBuilder::kUpdateSemanticsFieldName];
     if (updateSemanticsElement) {
-        if (!_fromOplogApplication) {
-            return {ErrorCodes::FailedToParse, "The $v update field is only recognized internally"};
-        }
-        auto statusWithUpdateSemantics = updateSemanticsFromElement(updateSemanticsElement);
-        if (!statusWithUpdateSemantics.isOK()) {
-            return statusWithUpdateSemantics.getStatus();
-        }
+        uassert(ErrorCodes::FailedToParse,
+                "The $v update field is only recognized internally",
+                _fromOplogApplication);
+
+        uassertStatusOK(updateSemanticsFromElement(updateSemanticsElement));
     }
 
     auto root = stdx::make_unique<UpdateObjectNode>();
     _positional = parseUpdateExpression(updateExpr, root.get(), _expCtx, arrayFilters);
     _root = std::move(root);
-
-    return Status::OK();
 }
 
 Status UpdateDriver::populateDocumentWithQueryFields(OperationContext* opCtx,
diff --git a/src/mongo/db/update/update_driver.h b/src/mongo/db/update/update_driver.h
index d754a4f58f4..8345958641b 100644
--- a/src/mongo/db/update/update_driver.h
+++ b/src/mongo/db/update/update_driver.h
@@ -51,13 +51,12 @@ public:
     UpdateDriver(const boost::intrusive_ptr<ExpressionContext>& expCtx);
 
     /**
-     * Parses the 'updateExpr' update expression into the '_root' member variable. Uasserts or
-     * returns a non-ok status if 'updateExpr' fails to parse.
+     * Parses the 'updateExpr' update expression into the '_root' member variable. Uasserts
+     * if 'updateExpr' fails to parse.
      */
-    Status parse(
-        const BSONObj& updateExpr,
-        const std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>>& arrayFilters,
-        const bool multi = false);
+    void parse(const BSONObj& updateExpr,
+               const std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>>& arrayFilters,
+               const bool multi = false);
 
     /**
      * Fills in document with any fields in the query which are valid.
diff --git a/src/mongo/db/update/update_driver_test.cpp b/src/mongo/db/update/update_driver_test.cpp
index 1911c417f0a..10d9c8105bd 100644
--- a/src/mongo/db/update/update_driver_test.cpp
+++ b/src/mongo/db/update/update_driver_test.cpp
@@ -45,6 +45,15 @@
 #include "mongo/db/update_index_data.h"
 #include "mongo/unittest/unittest.h"
 
+#define ASSERT_DOES_NOT_THROW(EXPRESSION)                                          \
+    try {                                                                          \
+        EXPRESSION;                                                                \
+    } catch (const AssertionException& e) {                                        \
+        ::mongoutils::str::stream err;                                             \
+        err << "Threw an exception incorrectly: " << e.toString();                 \
+        ::mongo::unittest::TestAssertionFailure(__FILE__, __LINE__, err).stream(); \
+    }
+
 namespace mongo {
 namespace {
 
@@ -54,7 +63,7 @@ TEST(Parse, Normal) {
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
     UpdateDriver driver(expCtx);
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
-    ASSERT_OK(driver.parse(fromjson("{$set:{a:1}}"), arrayFilters));
+    ASSERT_DOES_NOT_THROW(driver.parse(fromjson("{$set:{a:1}}"), arrayFilters));
     ASSERT_FALSE(driver.isDocReplacement());
 }
 
@@ -62,7 +71,7 @@ TEST(Parse, MultiMods) {
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
     UpdateDriver driver(expCtx);
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
-    ASSERT_OK(driver.parse(fromjson("{$set:{a:1, b:1}}"), arrayFilters));
+    ASSERT_DOES_NOT_THROW(driver.parse(fromjson("{$set:{a:1, b:1}}"), arrayFilters));
     ASSERT_FALSE(driver.isDocReplacement());
 }
 
@@ -70,7 +79,7 @@ TEST(Parse, MixingMods) {
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
     UpdateDriver driver(expCtx);
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
-    ASSERT_OK(driver.parse(fromjson("{$set:{a:1}, $unset:{b:1}}"), arrayFilters));
+    ASSERT_DOES_NOT_THROW(driver.parse(fromjson("{$set:{a:1}, $unset:{b:1}}"), arrayFilters));
     ASSERT_FALSE(driver.isDocReplacement());
 }
 
@@ -78,7 +87,7 @@ TEST(Parse, ObjectReplacment) {
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
     UpdateDriver driver(expCtx);
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
-    ASSERT_OK(driver.parse(fromjson("{obj: \"obj replacement\"}"), arrayFilters));
+    ASSERT_DOES_NOT_THROW(driver.parse(fromjson("{obj: \"obj replacement\"}"), arrayFilters));
     ASSERT_TRUE(driver.isDocReplacement());
 }
 
@@ -87,7 +96,7 @@ TEST(Parse, EmptyMod) {
     UpdateDriver driver(expCtx);
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
     ASSERT_THROWS_CODE_AND_WHAT(
-        driver.parse(fromjson("{$set:{}}"), arrayFilters).transitional_ignore(),
+        driver.parse(fromjson("{$set:{}}"), arrayFilters),
         AssertionException,
         ErrorCodes::FailedToParse,
         "'$set' is empty. You must specify a field like so: {$set: {<field>: ...}}");
@@ -97,23 +106,21 @@ TEST(Parse, WrongMod) {
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
     UpdateDriver driver(expCtx);
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
-    ASSERT_THROWS_CODE_AND_WHAT(
-        driver.parse(fromjson("{$xyz:{a:1}}"), arrayFilters).transitional_ignore(),
-        AssertionException,
-        ErrorCodes::FailedToParse,
-        "Unknown modifier: $xyz");
+    ASSERT_THROWS_CODE_AND_WHAT(driver.parse(fromjson("{$xyz:{a:1}}"), arrayFilters),
+                                AssertionException,
+                                ErrorCodes::FailedToParse,
+                                "Unknown modifier: $xyz");
 }
 
 TEST(Parse, WrongType) {
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
     UpdateDriver driver(expCtx);
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
-    ASSERT_THROWS_CODE_AND_WHAT(
-        driver.parse(fromjson("{$set:[{a:1}]}"), arrayFilters).transitional_ignore(),
-        AssertionException,
-        ErrorCodes::FailedToParse,
-        "Modifiers operate on fields but we found type array instead. For "
-        "example: {$mod: {<field>: ...}} not {$set: [ { a: 1 } ]}");
+    ASSERT_THROWS_CODE_AND_WHAT(driver.parse(fromjson("{$set:[{a:1}]}"), arrayFilters),
+                                AssertionException,
+                                ErrorCodes::FailedToParse,
+                                "Modifiers operate on fields but we found type array instead. For "
+                                "example: {$mod: {<field>: ...}} not {$set: [ { a: 1 } ]}");
 }
 
 TEST(Parse, ModsWithLaterObjReplacement) {
@@ -121,8 +128,7 @@ TEST(Parse, ModsWithLaterObjReplacement) {
     UpdateDriver driver(expCtx);
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
     ASSERT_THROWS_CODE_AND_WHAT(
-        driver.parse(fromjson("{$set:{a:1}, obj: \"obj replacement\"}"), arrayFilters)
-            .transitional_ignore(),
+        driver.parse(fromjson("{$set:{a:1}, obj: \"obj replacement\"}"), arrayFilters),
         AssertionException,
         ErrorCodes::FailedToParse,
         "Unknown modifier: obj");
@@ -132,7 +138,7 @@ TEST(Parse, SetOnInsert) {
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
     UpdateDriver driver(expCtx);
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
-    ASSERT_OK(driver.parse(fromjson("{$setOnInsert:{a:1}}"), arrayFilters));
+    ASSERT_DOES_NOT_THROW(driver.parse(fromjson("{$setOnInsert:{a:1}}"), arrayFilters));
     ASSERT_FALSE(driver.isDocReplacement());
 }
 
@@ -143,7 +149,7 @@ TEST(Collator, SetCollationUpdatesModifierInterfaces) {
     UpdateDriver driver(expCtx);
     std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
 
-    ASSERT_OK(driver.parse(updateDocument, arrayFilters));
+    ASSERT_DOES_NOT_THROW(driver.parse(updateDocument, arrayFilters));
 
     const bool validateForStorage = true;
     const FieldRefSet emptyImmutablePaths;
@@ -169,8 +175,8 @@ public:
         : _opCtx(_serviceContext.makeOperationContext()),
           _driverOps(new UpdateDriver(new ExpressionContext(_opCtx.get(), nullptr))),
           _driverRepl(new UpdateDriver(new ExpressionContext(_opCtx.get(), nullptr))) {
-        _driverOps->parse(fromjson("{$set:{'_':1}}"), _arrayFilters).transitional_ignore();
-        _driverRepl->parse(fromjson("{}"), _arrayFilters).transitional_ignore();
+        _driverOps->parse(fromjson("{$set:{'_':1}}"), _arrayFilters);
+        _driverRepl->parse(fromjson("{}"), _arrayFilters);
     }
 
     mutablebson::Document& doc() {
diff --git a/src/mongo/dbtests/query_stage_update.cpp b/src/mongo/dbtests/query_stage_update.cpp
index fb8d4b57994..7deed7fea80 100644
--- a/src/mongo/dbtests/query_stage_update.cpp
+++ b/src/mongo/dbtests/query_stage_update.cpp
@@ -54,6 +54,15 @@
 #include "mongo/dbtests/dbtests.h"
 #include "mongo/stdx/memory.h"
 
+#define ASSERT_DOES_NOT_THROW(EXPRESSION)                                          \
+    try {                                                                          \
+        EXPRESSION;                                                                \
+    } catch (const AssertionException& e) {                                        \
+        ::mongoutils::str::stream err;                                             \
+        err << "Threw an exception incorrectly: " << e.toString();                 \
+        ::mongo::unittest::TestAssertionFailure(__FILE__, __LINE__, err).stream(); \
+    }
+
 namespace QueryStageUpdate {
 
 using std::unique_ptr;
@@ -210,7 +219,8 @@ public:
 
             const std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
 
-            ASSERT_OK(driver.parse(request.getUpdates(), arrayFilters, request.isMulti()));
+            ASSERT_DOES_NOT_THROW(
+                driver.parse(request.getUpdates(), arrayFilters, request.isMulti()));
 
             // Setup update params.
             UpdateStageParams params(&request, &driver, opDebug);
@@ -284,7 +294,8 @@ public:
 
             const std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
 
-            ASSERT_OK(driver.parse(request.getUpdates(), arrayFilters, request.isMulti()));
+            ASSERT_DOES_NOT_THROW(
+                driver.parse(request.getUpdates(), arrayFilters, request.isMulti()));
 
             // Configure the scan.
             CollectionScanParams collScanParams;
@@ -400,7 +411,7 @@ public:
 
         const std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
 
-        ASSERT_OK(driver.parse(request.getUpdates(), arrayFilters, request.isMulti()));
+        ASSERT_DOES_NOT_THROW(driver.parse(request.getUpdates(), arrayFilters, request.isMulti()));
 
         // Configure a QueuedDataStage to pass the first object in the collection back in a
         // RID_AND_OBJ state.
@@ -491,7 +502,7 @@ public:
 
         const std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
 
-        ASSERT_OK(driver.parse(request.getUpdates(), arrayFilters, request.isMulti()));
+        ASSERT_DOES_NOT_THROW(driver.parse(request.getUpdates(), arrayFilters, request.isMulti()));
 
         // Configure a QueuedDataStage to pass the first object in the collection back in a
         // RID_AND_OBJ state.
@@ -572,7 +583,7 @@ public:
 
         const std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters;
 
-        ASSERT_OK(driver.parse(request.getUpdates(), arrayFilters, request.isMulti()));
+        ASSERT_DOES_NOT_THROW(driver.parse(request.getUpdates(), arrayFilters, request.isMulti()));
 
         // Configure a QueuedDataStage to pass an OWNED_OBJ to the update stage.
         auto qds = make_unique<QueuedDataStage>(&_opCtx, ws.get());