SERVER-31496 MatchExpressionParser::parse() should not throw

4 files changed, 53 insertions, 36 deletions

diff --git a/jstests/aggregation/sources/graphLookup/error.js b/jstests/aggregation/sources/graphLookup/error.js
index 20dad85b842..62cec4ce87f 100644
--- a/jstests/aggregation/sources/graphLookup/error.js
+++ b/jstests/aggregation/sources/graphLookup/error.js
@@ -305,7 +305,7 @@ load("jstests/aggregation/extras/utils.js");  // For "assertErrorCode".
             restrictSearchWithMatch: {$expr: {$eq: ["$x", "$$unbound"]}}
         }
     };
-    assertErrorCode(local, pipeline, 17276, "cannot use $expr with unbound variable");
+    assertErrorCode(local, pipeline, 40186, "cannot use $expr with unbound variable");
 
     // $graphLookup can only consume at most 100MB of memory.
     var foreign = db.foreign;
diff --git a/jstests/core/expr.js b/jstests/core/expr.js
index d2c6d9216ff..63ffea5e7e4 100644
--- a/jstests/core/expr.js
+++ b/jstests/core/expr.js
@@ -204,6 +204,18 @@
     assert.eq(1, writeRes.nRemoved);
     assert.writeError(coll.remove({_id: 0, $expr: {$eq: ["$a", "$$unbound"]}}));
 
+    // Any writes preceding the write that fails to parse are executed.
+    coll.drop();
+    assert.writeOK(coll.insert({_id: 0}));
+    assert.writeOK(coll.insert({_id: 1}));
+    writeRes = db.runCommand({
+        delete: coll.getName(),
+        deletes: [{q: {_id: 0}, limit: 1}, {q: {$expr: "$$unbound"}, limit: 1}]
+    });
+    assert.commandWorked(writeRes);
+    assert.eq(writeRes.writeErrors[0].code, 17276, tojson(writeRes));
+    assert.eq(writeRes.n, 1, tojson(writeRes));
+
     //
     // $expr in update.
     //
@@ -236,4 +248,16 @@
                                       {$set: {"a.$[i].b": 6}},
                                       {arrayFilters: [{"i.b": 5, $expr: {$eq: ["$i.b", 5]}}]}));
     }
+
+    // Any writes preceding the write that fails to parse are executed.
+    coll.drop();
+    assert.writeOK(coll.insert({_id: 0}));
+    assert.writeOK(coll.insert({_id: 1}));
+    writeRes = db.runCommand({
+        update: coll.getName(),
+        updates: [{q: {_id: 0}, u: {$set: {b: 6}}}, {q: {$expr: "$$unbound"}, u: {$set: {b: 6}}}]
+    });
+    assert.commandWorked(writeRes);
+    assert.eq(writeRes.writeErrors[0].code, 17276, tojson(writeRes));
+    assert.eq(writeRes.n, 1, tojson(writeRes));
 })();
diff --git a/src/mongo/db/matcher/expression_parser.h b/src/mongo/db/matcher/expression_parser.h
index 6091d3f1426..76f1e19c863 100644
--- a/src/mongo/db/matcher/expression_parser.h
+++ b/src/mongo/db/matcher/expression_parser.h
@@ -129,8 +129,12 @@ public:
         const ExtensionsCallback& extensionsCallback = ExtensionsCallbackNoop(),
         AllowedFeatureSet allowedFeatures = kDefaultSpecialFeatures) {
         invariant(expCtx.get());
-        return MatchExpressionParser(&extensionsCallback)
-            ._parse(obj, expCtx, allowedFeatures, DocumentParseLevel::kPredicateTopLevel);
+        try {
+            return MatchExpressionParser(&extensionsCallback)
+                ._parse(obj, expCtx, allowedFeatures, DocumentParseLevel::kPredicateTopLevel);
+        } catch (const DBException& ex) {
+            return {ex.toStatus()};
+        }
     }
 
     /**
diff --git a/src/mongo/db/matcher/expression_parser_geo_test.cpp b/src/mongo/db/matcher/expression_parser_geo_test.cpp
index 7410fb1b01a..642080960fd 100644
--- a/src/mongo/db/matcher/expression_parser_geo_test.cpp
+++ b/src/mongo/db/matcher/expression_parser_geo_test.cpp
@@ -78,12 +78,11 @@ TEST(MatchExpressionParserGeoNear, ParseNearExtraField) {
         "$geometry:{type:\"Point\", coordinates:[0,0]}}, foo: 1}}");
 
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-    ASSERT_THROWS(MatchExpressionParser::parse(query,
+    ASSERT_NOT_OK(MatchExpressionParser::parse(query,
                                                expCtx,
                                                ExtensionsCallbackNoop(),
                                                MatchExpressionParser::kAllowAllSpecialFeatures)
-                      .status_with_transitional_ignore(),
-                  AssertionException);
+                      .getStatus());
 }
 
 // For $near, $nearSphere, and $geoNear syntax of:
@@ -132,32 +131,29 @@ TEST(MatchExpressionParserGeoNear, ParseInvalidNear) {
     {
         BSONObj query = fromjson("{loc: {$near: [0,0], $maxDistance: {}}}");
         boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-        ASSERT_THROWS(MatchExpressionParser::parse(query,
+        ASSERT_NOT_OK(MatchExpressionParser::parse(query,
                                                    expCtx,
                                                    ExtensionsCallbackNoop(),
                                                    MatchExpressionParser::kAllowAllSpecialFeatures)
-                          .status_with_transitional_ignore(),
-                      AssertionException);
+                          .getStatus());
     }
     {
         BSONObj query = fromjson("{loc: {$near: [0,0], $minDistance: {}}}");
         boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-        ASSERT_THROWS(MatchExpressionParser::parse(query,
+        ASSERT_NOT_OK(MatchExpressionParser::parse(query,
                                                    expCtx,
                                                    ExtensionsCallbackNoop(),
                                                    MatchExpressionParser::kAllowAllSpecialFeatures)
-                          .status_with_transitional_ignore(),
-                      AssertionException);
+                          .getStatus());
     }
     {
         BSONObj query = fromjson("{loc: {$near: [0,0], $eq: 40}}");
         boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-        ASSERT_THROWS(MatchExpressionParser::parse(query,
+        ASSERT_NOT_OK(MatchExpressionParser::parse(query,
                                                    expCtx,
                                                    ExtensionsCallbackNoop(),
                                                    MatchExpressionParser::kAllowAllSpecialFeatures)
-                          .status_with_transitional_ignore(),
-                      AssertionException);
+                          .getStatus());
     }
     {
         BSONObj query = fromjson("{loc: {$eq: 40, $near: [0,0]}}");
@@ -173,12 +169,11 @@ TEST(MatchExpressionParserGeoNear, ParseInvalidNear) {
         BSONObj query = fromjson(
             "{loc: {$near: [0,0], $geoWithin: {$geometry: {type: \"Polygon\", coordinates: []}}}}");
         boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-        ASSERT_THROWS(MatchExpressionParser::parse(query,
+        ASSERT_NOT_OK(MatchExpressionParser::parse(query,
                                                    expCtx,
                                                    ExtensionsCallbackNoop(),
                                                    MatchExpressionParser::kAllowAllSpecialFeatures)
-                          .status_with_transitional_ignore(),
-                      AssertionException);
+                          .getStatus());
     }
     {
         BSONObj query = fromjson("{loc: {$near: {$foo: 1}}}");
@@ -242,32 +237,29 @@ TEST(MatchExpressionParserGeoNear, ParseInvalidGeoNear) {
     {
         BSONObj query = fromjson("{loc: {$geoNear: [0,0], $eq: 1}}");
         boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-        ASSERT_THROWS(MatchExpressionParser::parse(query,
+        ASSERT_NOT_OK(MatchExpressionParser::parse(query,
                                                    expCtx,
                                                    ExtensionsCallbackNoop(),
                                                    MatchExpressionParser::kAllowAllSpecialFeatures)
-                          .status_with_transitional_ignore(),
-                      AssertionException);
+                          .getStatus());
     }
     {
         BSONObj query = fromjson("{loc: {$geoNear: [0,0], $maxDistance: {}}}");
         boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-        ASSERT_THROWS(MatchExpressionParser::parse(query,
+        ASSERT_NOT_OK(MatchExpressionParser::parse(query,
                                                    expCtx,
                                                    ExtensionsCallbackNoop(),
                                                    MatchExpressionParser::kAllowAllSpecialFeatures)
-                          .status_with_transitional_ignore(),
-                      AssertionException);
+                          .getStatus());
     }
     {
         BSONObj query = fromjson("{loc: {$geoNear: [0,0], $minDistance: {}}}");
         boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-        ASSERT_THROWS(MatchExpressionParser::parse(query,
+        ASSERT_NOT_OK(MatchExpressionParser::parse(query,
                                                    expCtx,
                                                    ExtensionsCallbackNoop(),
                                                    MatchExpressionParser::kAllowAllSpecialFeatures)
-                          .status_with_transitional_ignore(),
-                      AssertionException);
+                          .getStatus());
     }
 }
 
@@ -311,32 +303,29 @@ TEST(MatchExpressionParserGeoNear, ParseInvalidNearSphere) {
     {
         BSONObj query = fromjson("{loc: {$nearSphere: [0,0], $maxDistance: {}}}");
         boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-        ASSERT_THROWS(MatchExpressionParser::parse(query,
+        ASSERT_NOT_OK(MatchExpressionParser::parse(query,
                                                    expCtx,
                                                    ExtensionsCallbackNoop(),
                                                    MatchExpressionParser::kAllowAllSpecialFeatures)
-                          .status_with_transitional_ignore(),
-                      AssertionException);
+                          .getStatus());
     }
     {
         BSONObj query = fromjson("{loc: {$nearSphere: [0,0], $minDistance: {}}}");
         boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-        ASSERT_THROWS(MatchExpressionParser::parse(query,
+        ASSERT_NOT_OK(MatchExpressionParser::parse(query,
                                                    expCtx,
                                                    ExtensionsCallbackNoop(),
                                                    MatchExpressionParser::kAllowAllSpecialFeatures)
-                          .status_with_transitional_ignore(),
-                      AssertionException);
+                          .getStatus());
     }
     {
         BSONObj query = fromjson("{loc: {$nearSphere: [0,0], $eq: 1}}");
         boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-        ASSERT_THROWS(MatchExpressionParser::parse(query,
+        ASSERT_NOT_OK(MatchExpressionParser::parse(query,
                                                    expCtx,
                                                    ExtensionsCallbackNoop(),
                                                    MatchExpressionParser::kAllowAllSpecialFeatures)
-                          .status_with_transitional_ignore(),
-                      AssertionException);
+                          .getStatus());
     }
 }