Skip to content

Commit b1d1b24

Browse files
author
Tess Avitabile
committed
SERVER-31496 MatchExpressionParser::parse() should not throw
1 parent fc249a3 commit b1d1b24

File tree

4 files changed

+53
-36
lines changed

4 files changed

+53
-36
lines changed

‎jstests/aggregation/sources/graphLookup/error.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ load("jstests/aggregation/extras/utils.js"); // For "assertErrorCode".
305305
restrictSearchWithMatch: {$expr: {$eq: ["$x", "$$unbound"]}}
306306
}
307307
};
308-
assertErrorCode(local, pipeline, 17276, "cannot use $expr with unbound variable");
308+
assertErrorCode(local, pipeline, 40186, "cannot use $expr with unbound variable");
309309

310310
// $graphLookup can only consume at most 100MB of memory.
311311
var foreign = db.foreign;

‎jstests/core/expr.js‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,18 @@
204204
assert.eq(1, writeRes.nRemoved);
205205
assert.writeError(coll.remove({_id: 0, $expr: {$eq: ["$a", "$$unbound"]}}));
206206

207+
// Any writes preceding the write that fails to parse are executed.
208+
coll.drop();
209+
assert.writeOK(coll.insert({_id: 0}));
210+
assert.writeOK(coll.insert({_id: 1}));
211+
writeRes = db.runCommand({
212+
delete: coll.getName(),
213+
deletes: [{q: {_id: 0}, limit: 1}, {q: {$expr: "$$unbound"}, limit: 1}]
214+
});
215+
assert.commandWorked(writeRes);
216+
assert.eq(writeRes.writeErrors[0].code, 17276, tojson(writeRes));
217+
assert.eq(writeRes.n, 1, tojson(writeRes));
218+
207219
//
208220
// $expr in update.
209221
//
@@ -236,4 +248,16 @@
236248
{$set: {"a.$[i].b": 6}},
237249
{arrayFilters: [{"i.b": 5, $expr: {$eq: ["$i.b", 5]}}]}));
238250
}
251+
252+
// Any writes preceding the write that fails to parse are executed.
253+
coll.drop();
254+
assert.writeOK(coll.insert({_id: 0}));
255+
assert.writeOK(coll.insert({_id: 1}));
256+
writeRes = db.runCommand({
257+
update: coll.getName(),
258+
updates: [{q: {_id: 0}, u: {$set: {b: 6}}}, {q: {$expr: "$$unbound"}, u: {$set: {b: 6}}}]
259+
});
260+
assert.commandWorked(writeRes);
261+
assert.eq(writeRes.writeErrors[0].code, 17276, tojson(writeRes));
262+
assert.eq(writeRes.n, 1, tojson(writeRes));
239263
})();

‎src/mongo/db/matcher/expression_parser.h‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,12 @@ class MatchExpressionParser {
129129
const ExtensionsCallback& extensionsCallback = ExtensionsCallbackNoop(),
130130
AllowedFeatureSet allowedFeatures = kDefaultSpecialFeatures) {
131131
invariant(expCtx.get());
132-
return MatchExpressionParser(&extensionsCallback)
133-
._parse(obj, expCtx, allowedFeatures, DocumentParseLevel::kPredicateTopLevel);
132+
try {
133+
return MatchExpressionParser(&extensionsCallback)
134+
._parse(obj, expCtx, allowedFeatures, DocumentParseLevel::kPredicateTopLevel);
135+
} catch (const DBException& ex) {
136+
return {ex.toStatus()};
137+
}
134138
}
135139

136140
/**

‎src/mongo/db/matcher/expression_parser_geo_test.cpp‎

Lines changed: 22 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -78,12 +78,11 @@ TEST(MatchExpressionParserGeoNear, ParseNearExtraField) {
7878
"$geometry:{type:\"Point\", coordinates:[0,0]}}, foo: 1}}");
7979

8080
boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
81-
ASSERT_THROWS(MatchExpressionParser::parse(query,
81+
ASSERT_NOT_OK(MatchExpressionParser::parse(query,
8282
expCtx,
8383
ExtensionsCallbackNoop(),
8484
MatchExpressionParser::kAllowAllSpecialFeatures)
85-
.status_with_transitional_ignore(),
86-
AssertionException);
85+
.getStatus());
8786
}
8887

8988
// For $near, $nearSphere, and $geoNear syntax of:
@@ -132,32 +131,29 @@ TEST(MatchExpressionParserGeoNear, ParseInvalidNear) {
132131
{
133132
BSONObj query = fromjson("{loc: {$near: [0,0], $maxDistance: {}}}");
134133
boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
135-
ASSERT_THROWS(MatchExpressionParser::parse(query,
134+
ASSERT_NOT_OK(MatchExpressionParser::parse(query,
136135
expCtx,
137136
ExtensionsCallbackNoop(),
138137
MatchExpressionParser::kAllowAllSpecialFeatures)
139-
.status_with_transitional_ignore(),
140-
AssertionException);
138+
.getStatus());
141139
}
142140
{
143141
BSONObj query = fromjson("{loc: {$near: [0,0], $minDistance: {}}}");
144142
boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
145-
ASSERT_THROWS(MatchExpressionParser::parse(query,
143+
ASSERT_NOT_OK(MatchExpressionParser::parse(query,
146144
expCtx,
147145
ExtensionsCallbackNoop(),
148146
MatchExpressionParser::kAllowAllSpecialFeatures)
149-
.status_with_transitional_ignore(),
150-
AssertionException);
147+
.getStatus());
151148
}
152149
{
153150
BSONObj query = fromjson("{loc: {$near: [0,0], $eq: 40}}");
154151
boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
155-
ASSERT_THROWS(MatchExpressionParser::parse(query,
152+
ASSERT_NOT_OK(MatchExpressionParser::parse(query,
156153
expCtx,
157154
ExtensionsCallbackNoop(),
158155
MatchExpressionParser::kAllowAllSpecialFeatures)
159-
.status_with_transitional_ignore(),
160-
AssertionException);
156+
.getStatus());
161157
}
162158
{
163159
BSONObj query = fromjson("{loc: {$eq: 40, $near: [0,0]}}");
@@ -173,12 +169,11 @@ TEST(MatchExpressionParserGeoNear, ParseInvalidNear) {
173169
BSONObj query = fromjson(
174170
"{loc: {$near: [0,0], $geoWithin: {$geometry: {type: \"Polygon\", coordinates: []}}}}");
175171
boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
176-
ASSERT_THROWS(MatchExpressionParser::parse(query,
172+
ASSERT_NOT_OK(MatchExpressionParser::parse(query,
177173
expCtx,
178174
ExtensionsCallbackNoop(),
179175
MatchExpressionParser::kAllowAllSpecialFeatures)
180-
.status_with_transitional_ignore(),
181-
AssertionException);
176+
.getStatus());
182177
}
183178
{
184179
BSONObj query = fromjson("{loc: {$near: {$foo: 1}}}");
@@ -242,32 +237,29 @@ TEST(MatchExpressionParserGeoNear, ParseInvalidGeoNear) {
242237
{
243238
BSONObj query = fromjson("{loc: {$geoNear: [0,0], $eq: 1}}");
244239
boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
245-
ASSERT_THROWS(MatchExpressionParser::parse(query,
240+
ASSERT_NOT_OK(MatchExpressionParser::parse(query,
246241
expCtx,
247242
ExtensionsCallbackNoop(),
248243
MatchExpressionParser::kAllowAllSpecialFeatures)
249-
.status_with_transitional_ignore(),
250-
AssertionException);
244+
.getStatus());
251245
}
252246
{
253247
BSONObj query = fromjson("{loc: {$geoNear: [0,0], $maxDistance: {}}}");
254248
boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
255-
ASSERT_THROWS(MatchExpressionParser::parse(query,
249+
ASSERT_NOT_OK(MatchExpressionParser::parse(query,
256250
expCtx,
257251
ExtensionsCallbackNoop(),
258252
MatchExpressionParser::kAllowAllSpecialFeatures)
259-
.status_with_transitional_ignore(),
260-
AssertionException);
253+
.getStatus());
261254
}
262255
{
263256
BSONObj query = fromjson("{loc: {$geoNear: [0,0], $minDistance: {}}}");
264257
boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
265-
ASSERT_THROWS(MatchExpressionParser::parse(query,
258+
ASSERT_NOT_OK(MatchExpressionParser::parse(query,
266259
expCtx,
267260
ExtensionsCallbackNoop(),
268261
MatchExpressionParser::kAllowAllSpecialFeatures)
269-
.status_with_transitional_ignore(),
270-
AssertionException);
262+
.getStatus());
271263
}
272264
}
273265

@@ -311,32 +303,29 @@ TEST(MatchExpressionParserGeoNear, ParseInvalidNearSphere) {
311303
{
312304
BSONObj query = fromjson("{loc: {$nearSphere: [0,0], $maxDistance: {}}}");
313305
boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
314-
ASSERT_THROWS(MatchExpressionParser::parse(query,
306+
ASSERT_NOT_OK(MatchExpressionParser::parse(query,
315307
expCtx,
316308
ExtensionsCallbackNoop(),
317309
MatchExpressionParser::kAllowAllSpecialFeatures)
318-
.status_with_transitional_ignore(),
319-
AssertionException);
310+
.getStatus());
320311
}
321312
{
322313
BSONObj query = fromjson("{loc: {$nearSphere: [0,0], $minDistance: {}}}");
323314
boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
324-
ASSERT_THROWS(MatchExpressionParser::parse(query,
315+
ASSERT_NOT_OK(MatchExpressionParser::parse(query,
325316
expCtx,
326317
ExtensionsCallbackNoop(),
327318
MatchExpressionParser::kAllowAllSpecialFeatures)
328-
.status_with_transitional_ignore(),
329-
AssertionException);
319+
.getStatus());
330320
}
331321
{
332322
BSONObj query = fromjson("{loc: {$nearSphere: [0,0], $eq: 1}}");
333323
boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
334-
ASSERT_THROWS(MatchExpressionParser::parse(query,
324+
ASSERT_NOT_OK(MatchExpressionParser::parse(query,
335325
expCtx,
336326
ExtensionsCallbackNoop(),
337327
MatchExpressionParser::kAllowAllSpecialFeatures)
338-
.status_with_transitional_ignore(),
339-
AssertionException);
328+
.getStatus());
340329
}
341330
}
342331

0 commit comments

Comments
 (0)