From d6bf4c26878a61513b0a1cc92e2955330a02d0a8 Mon Sep 17 00:00:00 2001 From: Nick Zolnierz Date: Thu, 24 Aug 2017 15:13:53 -0400 Subject: SERVER-30664: extend ExpressionWithPlaceholder to accept more expressions --- src/mongo/db/matcher/expression_parser.cpp | 5 +- .../db/matcher/expression_serialization_test.cpp | 54 ++++++-- .../db/matcher/expression_with_placeholder.cpp | 89 ++++++------- src/mongo/db/matcher/expression_with_placeholder.h | 32 ++++- .../matcher/expression_with_placeholder_test.cpp | 148 ++++++++++++++++++--- ...pression_internal_schema_allowed_properties.cpp | 13 +- ...ion_internal_schema_allowed_properties_test.cpp | 8 ++ ...xpression_internal_schema_match_array_index.cpp | 8 +- .../expression_internal_schema_match_array_index.h | 2 +- .../schema/expression_parser_schema_test.cpp | 17 ++- src/mongo/db/ops/parsed_update.cpp | 13 +- src/mongo/db/update/update_array_node.cpp | 2 +- 12 files changed, 281 insertions(+), 110 deletions(-) (limited to 'src/mongo') diff --git a/src/mongo/db/matcher/expression_parser.cpp b/src/mongo/db/matcher/expression_parser.cpp index 3901971ae64..f945115c370 100644 --- a/src/mongo/db/matcher/expression_parser.cpp +++ b/src/mongo/db/matcher/expression_parser.cpp @@ -1416,14 +1416,15 @@ StatusWith> parseExprWithPlaceholder( return result.getStatus(); } - if (result.getValue()->getPlaceholder() != expectedPlaceholder) { + auto placeholder = result.getValue()->getPlaceholder(); + if (placeholder && (*placeholder != expectedPlaceholder)) { return {ErrorCodes::FailedToParse, str::stream() << expressionName << " expected a name placeholder of " << expectedPlaceholder << ", but '" << exprWithPlaceholderElem.fieldName() << "' has a mismatching placeholder '" - << result.getValue()->getPlaceholder() + << *placeholder << "'"}; } return result; diff --git a/src/mongo/db/matcher/expression_serialization_test.cpp b/src/mongo/db/matcher/expression_serialization_test.cpp index a7e8b4403e3..2fb9fd6dc22 100644 --- a/src/mongo/db/matcher/expression_serialization_test.cpp +++ b/src/mongo/db/matcher/expression_serialization_test.cpp @@ -1585,14 +1585,13 @@ TEST(SerializeInternalSchema, ExpressionInternalSchemaCondSerializesCorrectly) { TEST(SerializeInternalSchema, ExpressionInternalSchemaMinPropertiesSerializesCorrectly) { boost::intrusive_ptr expCtx(new ExpressionContextForTest()); - const CollatorInterface* collator = nullptr; Matcher original(fromjson("{$_internalSchemaMinProperties: 1}"), - collator, + kSimpleCollator, expCtx, ExtensionsCallbackNoop(), MatchExpressionParser::kAllowAllSpecialFeatures); Matcher reserialized(serialize(original.getMatchExpression()), - collator, + kSimpleCollator, expCtx, ExtensionsCallbackNoop(), MatchExpressionParser::kAllowAllSpecialFeatures); @@ -1602,14 +1601,13 @@ TEST(SerializeInternalSchema, ExpressionInternalSchemaMinPropertiesSerializesCor TEST(SerializeInternalSchema, ExpressionInternalSchemaMaxPropertiesSerializesCorrectly) { boost::intrusive_ptr expCtx(new ExpressionContextForTest()); - const CollatorInterface* collator = nullptr; Matcher original(fromjson("{$_internalSchemaMaxProperties: 1}"), - collator, + kSimpleCollator, expCtx, ExtensionsCallbackNoop(), MatchExpressionParser::kAllowAllSpecialFeatures); Matcher reserialized(serialize(original.getMatchExpression()), - collator, + kSimpleCollator, expCtx, ExtensionsCallbackNoop(), MatchExpressionParser::kAllowAllSpecialFeatures); @@ -1643,15 +1641,14 @@ TEST(SerializeInternalSchema, ExpressionInternalSchemaFmodSerializesCorrectly) { TEST(SerializeInternalSchema, ExpressionInternalSchemaMatchArrayIndexSerializesCorrectly) { boost::intrusive_ptr expCtx(new ExpressionContextForTest()); - constexpr CollatorInterface* collator = nullptr; Matcher original(fromjson("{a: {$_internalSchemaMatchArrayIndex:" "{index: 2, namePlaceholder: 'i', expression: {i: {$lt: 3}}}}}"), - collator, + kSimpleCollator, expCtx, ExtensionsCallbackNoop(), MatchExpressionParser::kAllowAllSpecialFeatures); Matcher reserialized(serialize(original.getMatchExpression()), - collator, + kSimpleCollator, expCtx, ExtensionsCallbackNoop(), MatchExpressionParser::kAllowAllSpecialFeatures); @@ -1662,14 +1659,22 @@ TEST(SerializeInternalSchema, ExpressionInternalSchemaMatchArrayIndexSerializesC } TEST(SerializeInternalSchema, ExpressionInternalSchemaAllowedPropertiesSerializesCorrectly) { + boost::intrusive_ptr expCtx(new ExpressionContextForTest()); Matcher original(fromjson(R"({$_internalSchemaAllowedProperties: { properties: ['a'], otherwise: {i: {$gt: 10}}, namePlaceholder: 'i', patternProperties: [{regex: /b/, expression: {i: {$type: 'number'}}}] }})"), - kSimpleCollator); - Matcher reserialized(serialize(original.getMatchExpression()), kSimpleCollator); + kSimpleCollator, + expCtx, + ExtensionsCallbackNoop(), + MatchExpressionParser::kAllowAllSpecialFeatures); + Matcher reserialized(serialize(original.getMatchExpression()), + kSimpleCollator, + expCtx, + ExtensionsCallbackNoop(), + MatchExpressionParser::kAllowAllSpecialFeatures); ASSERT_BSONOBJ_EQ(*reserialized.getQuery(), fromjson(R"({$_internalSchemaAllowedProperties: { properties: ['a'], namePlaceholder: 'i', @@ -1678,5 +1683,32 @@ TEST(SerializeInternalSchema, ExpressionInternalSchemaAllowedPropertiesSerialize }})")); ASSERT_BSONOBJ_EQ(*reserialized.getQuery(), serialize(reserialized.getMatchExpression())); } + +TEST(SerializeInternalSchema, + ExpressionInternalSchemaAllowedPropertiesEmptyOtherwiseSerializesCorrectly) { + boost::intrusive_ptr expCtx(new ExpressionContextForTest()); + Matcher original(fromjson(R"({$_internalSchemaAllowedProperties: { + properties: [], + otherwise: {}, + namePlaceholder: 'i', + patternProperties: [] + }})"), + kSimpleCollator, + expCtx, + ExtensionsCallbackNoop(), + MatchExpressionParser::kAllowAllSpecialFeatures); + Matcher reserialized(serialize(original.getMatchExpression()), + kSimpleCollator, + expCtx, + ExtensionsCallbackNoop(), + MatchExpressionParser::kAllowAllSpecialFeatures); + ASSERT_BSONOBJ_EQ(*reserialized.getQuery(), fromjson(R"({$_internalSchemaAllowedProperties: { + properties: [], + namePlaceholder: 'i', + patternProperties: [], + otherwise: {} + }})")); + ASSERT_BSONOBJ_EQ(*reserialized.getQuery(), serialize(reserialized.getMatchExpression())); +} } // namespace } // namespace mongo diff --git a/src/mongo/db/matcher/expression_with_placeholder.cpp b/src/mongo/db/matcher/expression_with_placeholder.cpp index 97158b6ab33..35f8cc1b2bb 100644 --- a/src/mongo/db/matcher/expression_with_placeholder.cpp +++ b/src/mongo/db/matcher/expression_with_placeholder.cpp @@ -37,55 +37,42 @@ namespace mongo { namespace { /** -* Finds the top-level field that 'expr' is over. This must be unique and not the empty string. -*/ -StatusWith parseTopLevelFieldName(MatchExpression* expr) { - switch (expr->getCategory()) { - case MatchExpression::MatchCategory::kLeaf: - case MatchExpression::MatchCategory::kArrayMatching: { - auto firstDotPos = expr->path().find('.'); - if (firstDotPos == std::string::npos) { - return expr->path(); - } - return expr->path().substr(0, firstDotPos); + * Finds the top-level field that 'expr' is over. Returns boost::none if the expression does not + * have a top-level field name, or a non-OK status if there are multiple top-level field names. + */ +StatusWith> parseTopLevelFieldName(MatchExpression* expr) { + if (auto pathExpr = dynamic_cast(expr)) { + auto firstDotPos = pathExpr->path().find('.'); + if (firstDotPos == std::string::npos) { + return {pathExpr->path()}; } - case MatchExpression::MatchCategory::kLogical: { - if (expr->numChildren() == 0) { - return Status(ErrorCodes::FailedToParse, "No top-level field name found."); + return {pathExpr->path().substr(0, firstDotPos)}; + } else if (expr->getCategory() == MatchExpression::MatchCategory::kLogical) { + boost::optional placeholder; + for (size_t i = 0; i < expr->numChildren(); ++i) { + auto statusWithId = parseTopLevelFieldName(expr->getChild(i)); + if (!statusWithId.isOK()) { + return statusWithId.getStatus(); } - StringData placeholder; - for (size_t i = 0; i < expr->numChildren(); ++i) { - auto statusWithId = parseTopLevelFieldName(expr->getChild(i)); - if (!statusWithId.isOK()) { - return statusWithId.getStatus(); - } - - if (placeholder == StringData()) { - placeholder = statusWithId.getValue(); - continue; - } - - if (placeholder != statusWithId.getValue()) { - return Status( - ErrorCodes::FailedToParse, - str::stream() - << "Each array filter must use a single top-level field name, found '" - << placeholder - << "' and '" - << statusWithId.getValue() - << "'"); - } + if (!placeholder) { + placeholder = statusWithId.getValue(); + continue; + } + + if (statusWithId.getValue() && placeholder != statusWithId.getValue()) { + return Status(ErrorCodes::FailedToParse, + str::stream() << "Expected a single top-level field name, found '" + << *placeholder + << "' and '" + << *statusWithId.getValue() + << "'"); } - return placeholder; - } - case MatchExpression::MatchCategory::kOther: { - return Status(ErrorCodes::FailedToParse, - str::stream() << "Match expression does not support placeholders."); } + return placeholder; } - MONGO_UNREACHABLE; + return {boost::none}; } } // namespace @@ -114,13 +101,17 @@ StatusWith> ExpressionWithPlaceholder if (!statusWithId.isOK()) { return statusWithId.getStatus(); } - auto placeholder = statusWithId.getValue().toString(); - if (!std::regex_match(placeholder, placeholderRegex)) { - return Status(ErrorCodes::BadValue, - str::stream() << "The top-level field name must be an alphanumeric " - "string beginning with a lowercase letter, found '" - << placeholder - << "'"); + + boost::optional placeholder; + if (statusWithId.getValue()) { + placeholder = statusWithId.getValue()->toString(); + if (!std::regex_match(*placeholder, placeholderRegex)) { + return Status(ErrorCodes::BadValue, + str::stream() << "The top-level field name must be an alphanumeric " + "string beginning with a lowercase letter, found '" + << *placeholder + << "'"); + } } auto exprWithPlaceholder = diff --git a/src/mongo/db/matcher/expression_with_placeholder.h b/src/mongo/db/matcher/expression_with_placeholder.h index a17ab8ccc93..27047def6d2 100644 --- a/src/mongo/db/matcher/expression_with_placeholder.h +++ b/src/mongo/db/matcher/expression_with_placeholder.h @@ -28,6 +28,7 @@ #pragma once +#include #include #include "mongo/db/matcher/expression_parser.h" @@ -55,28 +56,47 @@ public: /** * Construct a new ExpressionWithPlaceholder. 'filter' must point to a valid MatchExpression. */ - ExpressionWithPlaceholder(std::string placeholder, std::unique_ptr filter) + ExpressionWithPlaceholder(boost::optional placeholder, + std::unique_ptr filter) : _placeholder(std::move(placeholder)), _filter(std::move(filter)) { invariant(static_cast(_filter)); } /** - * Returns true if this expression has both a placeholder and filter - * equivalent to 'other'. + * Returns true if this expression has both a placeholder and filter equivalent to 'other'. */ bool equivalent(const ExpressionWithPlaceholder* other) const; - StringData getPlaceholder() const { - return _placeholder; + /** + * Uses this filter to match against 'elem' as if it is wrapped in a BSONObj with a single + * field whose name is given by getPlaceholder(). If the placeholder name does not exist, then + * the filter expression does not refer to any specific paths. + */ + bool matchesBSONElement(BSONElement elem, MatchDetails* details = nullptr) const { + return _filter->matchesBSONElement(elem, details); + } + + /** + * If this object has a placeholder, returns a view of the placeholder as a StringData. + */ + boost::optional getPlaceholder() const { + if (_placeholder) { + return StringData(*_placeholder); + } + return boost::none; } MatchExpression* getFilter() const { return _filter.get(); } + std::unique_ptr shallowClone() const { + return stdx::make_unique(_placeholder, _filter->shallowClone()); + } + private: // The top-level field that _filter is over. - const std::string _placeholder; + const boost::optional _placeholder; const std::unique_ptr _filter; }; diff --git a/src/mongo/db/matcher/expression_with_placeholder_test.cpp b/src/mongo/db/matcher/expression_with_placeholder_test.cpp index bb8abef57ef..06460455dc7 100644 --- a/src/mongo/db/matcher/expression_with_placeholder_test.cpp +++ b/src/mongo/db/matcher/expression_with_placeholder_test.cpp @@ -43,7 +43,8 @@ TEST(ExpressionWithPlaceholderTest, ParseBasic) { const CollatorInterface* collator = nullptr; auto rawFilter = fromjson("{i: 0}"); auto filter = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); - ASSERT_EQ(filter->getPlaceholder(), "i"); + ASSERT(filter->getPlaceholder()); + ASSERT_EQ(*filter->getPlaceholder(), "i"); ASSERT_TRUE(filter->getFilter()->matchesBSON(fromjson("{i: 0}"))); ASSERT_FALSE(filter->getFilter()->matchesBSON(fromjson("{i: 1}"))); } @@ -52,7 +53,8 @@ TEST(ExpressionWithPlaceholderTest, ParseDottedField) { const CollatorInterface* collator = nullptr; auto rawFilter = fromjson("{'i.a': 0, 'i.b': 1}"); auto filter = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); - ASSERT_EQ(filter->getPlaceholder(), "i"); + ASSERT(filter->getPlaceholder()); + ASSERT_EQ(*filter->getPlaceholder(), "i"); ASSERT_TRUE(filter->getFilter()->matchesBSON(fromjson("{i: {a: 0, b: 1}}"))); ASSERT_FALSE(filter->getFilter()->matchesBSON(fromjson("{i: {a: 0, b: 0}}"))); } @@ -61,7 +63,8 @@ TEST(ExpressionWithPlaceholderTest, ParseLogicalQuery) { const CollatorInterface* collator = nullptr; auto rawFilter = fromjson("{$and: [{i: {$gte: 0}}, {i: {$lte: 0}}]}"); auto filter = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); - ASSERT_EQ(filter->getPlaceholder(), "i"); + ASSERT(filter->getPlaceholder()); + ASSERT_EQ(*filter->getPlaceholder(), "i"); ASSERT_TRUE(filter->getFilter()->matchesBSON(fromjson("{i: 0}"))); ASSERT_FALSE(filter->getFilter()->matchesBSON(fromjson("{i: 1}"))); } @@ -70,7 +73,8 @@ TEST(ExpressionWithPlaceholderTest, ParseElemMatch) { const CollatorInterface* collator = nullptr; auto rawFilter = fromjson("{i: {$elemMatch: {a: 0}}}"); auto filter = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); - ASSERT_EQ(filter->getPlaceholder(), "i"); + ASSERT(filter->getPlaceholder()); + ASSERT_EQ(*filter->getPlaceholder(), "i"); ASSERT_TRUE(filter->getFilter()->matchesBSON(fromjson("{i: [{a: 0}]}"))); ASSERT_FALSE(filter->getFilter()->matchesBSON(fromjson("{i: [{a: 1}]}"))); } @@ -79,7 +83,8 @@ TEST(ExpressionWithPlaceholderTest, ParseCollation) { CollatorInterfaceMock collator(CollatorInterfaceMock::MockType::kAlwaysEqual); auto rawFilter = fromjson("{i: 'abc'}"); auto filter = assertGet(ExpressionWithPlaceholder::parse(rawFilter, &collator)); - ASSERT_EQ(filter->getPlaceholder(), "i"); + ASSERT(filter->getPlaceholder()); + ASSERT_EQ(*filter->getPlaceholder(), "i"); ASSERT_TRUE(filter->getFilter()->matchesBSON(fromjson("{i: 'cba'}"))); ASSERT_FALSE(filter->getFilter()->matchesBSON(fromjson("{i: 0}"))); } @@ -88,7 +93,8 @@ TEST(ExpressionWithPlaceholderTest, ParseIdContainsNumbersAndCapitals) { const CollatorInterface* collator = nullptr; auto rawFilter = fromjson("{iA3: 0}"); auto filter = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); - ASSERT_EQ(filter->getPlaceholder(), "iA3"); + ASSERT(filter->getPlaceholder()); + ASSERT_EQ(*filter->getPlaceholder(), "iA3"); ASSERT_TRUE(filter->getFilter()->matchesBSON(fromjson("{'iA3': 0}"))); ASSERT_FALSE(filter->getFilter()->matchesBSON(fromjson("{'iA3': 1}"))); } @@ -100,18 +106,63 @@ TEST(ExpressionWithPlaceholderTest, BadMatchExpressionFailsToParse) { ASSERT_NOT_OK(status.getStatus()); } -TEST(ExpressionWithPlaceholderTest, EmptyMatchExpressionFailsToParse) { - const CollatorInterface* collator = nullptr; +TEST(ExpressionWithPlaceholderTest, EmptyMatchExpressionParsesSuccessfully) { + constexpr CollatorInterface* collator = nullptr; auto rawFilter = fromjson("{}"); - auto status = ExpressionWithPlaceholder::parse(rawFilter, collator); - ASSERT_NOT_OK(status.getStatus()); + auto result = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); + ASSERT_FALSE(result->getPlaceholder()); } -TEST(ExpressionWithPlaceholderTest, NestedEmptyMatchExpressionFailsToParse) { - const CollatorInterface* collator = nullptr; - auto rawFilter = fromjson("{$or: [{i: 0}, {$and: [{}]}]}"); - auto status = ExpressionWithPlaceholder::parse(rawFilter, collator); - ASSERT_NOT_OK(status.getStatus()); +TEST(ExpressionWithPlaceholderTest, NestedEmptyMatchExpressionParsesSuccessfully) { + constexpr CollatorInterface* collator = nullptr; + auto rawFilter = fromjson("{$or: [{$and: [{}]}]}"); + auto result = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); + ASSERT_FALSE(result->getPlaceholder()); +} + +TEST(ExpressionWithPlaceholderTest, + NestedMatchExpressionParsesSuccessfullyWhenSomeClausesHaveNoFieldName) { + constexpr CollatorInterface* collator = nullptr; + auto rawFilter = fromjson("{$or: [{$and: [{}]}, {i: 0}, {i: 1}, {$and: [{}]}]}"); + auto result = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); + ASSERT(result->getPlaceholder()); + ASSERT_EQ(*result->getPlaceholder(), "i"_sd); +} + +TEST(ExpressionWithPlaceholderTest, SuccessfullyParsesExpressionsWithTypeOther) { + constexpr CollatorInterface* collator = nullptr; + auto rawFilter = + fromjson("{a: {$_internalSchemaObjectMatch: {$_internalSchemaMinProperties: 5}}}"); + auto result = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); + ASSERT(result->getPlaceholder()); + ASSERT_EQ(*result->getPlaceholder(), "a"_sd); + + rawFilter = fromjson("{a: {$_internalSchemaType: 'string'}}"); + result = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); + ASSERT(result->getPlaceholder()); + ASSERT_EQ(*result->getPlaceholder(), "a"_sd); + + rawFilter = fromjson("{$_internalSchemaMinProperties: 1}}"); + result = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); + ASSERT_FALSE(result->getPlaceholder()); + + rawFilter = fromjson("{$_internalSchemaCond: [{a: {$exists: true}}, {b: 1}, {c: 1}]}"); + result = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); + ASSERT_FALSE(result->getPlaceholder()); +} + +TEST(ExpressionWithPlaceholderTest, SuccessfullyParsesAlwaysTrue) { + constexpr CollatorInterface* collator = nullptr; + auto rawFilter = fromjson("{$alwaysTrue: 1}"); + auto result = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); + ASSERT_FALSE(result->getPlaceholder()); +} + +TEST(ExpressionWithPlaceholderTest, SuccessfullyParsesAlwaysFalse) { + constexpr CollatorInterface* collator = nullptr; + auto rawFilter = fromjson("{$alwaysFalse: 1}"); + auto result = assertGet(ExpressionWithPlaceholder::parse(rawFilter, collator)); + ASSERT_FALSE(result->getPlaceholder()); } TEST(ExpressionWithPlaceholderTest, EmptyFieldNameFailsToParse) { @@ -205,6 +256,60 @@ TEST(ExpressionWithPlaceholderTest, EquivalentIfPlaceholderAndExpressionMatch) { expressionWithPlaceholder2.getValue().get())); } +TEST(ExpressionWithPlaceholderTest, EmptyMatchExpressionsAreEquivalent) { + constexpr auto collator = nullptr; + auto rawFilter1 = fromjson("{}"); + auto expressionWithPlaceholder1 = ExpressionWithPlaceholder::parse(rawFilter1, collator); + ASSERT_OK(expressionWithPlaceholder1.getStatus()); + + auto rawFilter2 = fromjson("{}"); + auto expressionWithPlaceholder2 = ExpressionWithPlaceholder::parse(rawFilter2, collator); + ASSERT_OK(expressionWithPlaceholder2.getStatus()); + ASSERT(expressionWithPlaceholder1.getValue()->equivalent( + expressionWithPlaceholder2.getValue().get())); +} + +TEST(ExpressionWithPlaceholderTest, NestedEmptyMatchExpressionsAreEquivalent) { + constexpr auto collator = nullptr; + auto rawFilter1 = fromjson("{$or: [{$and: [{}]}]}"); + auto expressionWithPlaceholder1 = ExpressionWithPlaceholder::parse(rawFilter1, collator); + ASSERT_OK(expressionWithPlaceholder1.getStatus()); + + auto rawFilter2 = fromjson("{$or: [{$and: [{}]}]}"); + auto expressionWithPlaceholder2 = ExpressionWithPlaceholder::parse(rawFilter2, collator); + ASSERT_OK(expressionWithPlaceholder2.getStatus()); + ASSERT(expressionWithPlaceholder1.getValue()->equivalent( + expressionWithPlaceholder2.getValue().get())); +} + +TEST(ExpressionWithPlaceholderTest, SameObjectMatchesAreEquivalent) { + constexpr auto collator = nullptr; + auto rawFilter1 = + fromjson("{a: {$_internalSchemaObjectMatch: {$_internalSchemaMaxProperties: 2}}}"); + auto expressionWithPlaceholder1 = ExpressionWithPlaceholder::parse(rawFilter1, collator); + ASSERT_OK(expressionWithPlaceholder1.getStatus()); + + auto rawFilter2 = + fromjson("{a: {$_internalSchemaObjectMatch: {$_internalSchemaMaxProperties: 2}}}"); + auto expressionWithPlaceholder2 = ExpressionWithPlaceholder::parse(rawFilter2, collator); + ASSERT_OK(expressionWithPlaceholder2.getStatus()); + ASSERT(expressionWithPlaceholder1.getValue()->equivalent( + expressionWithPlaceholder2.getValue().get())); +} + +TEST(ExpressionWithPlaceholderTest, AlwaysTruesAreEquivalent) { + constexpr auto collator = nullptr; + auto rawFilter1 = fromjson("{$alwaysTrue: 1}"); + auto expressionWithPlaceholder1 = ExpressionWithPlaceholder::parse(rawFilter1, collator); + ASSERT_OK(expressionWithPlaceholder1.getStatus()); + + auto rawFilter2 = fromjson("{$alwaysTrue: 1}"); + auto expressionWithPlaceholder2 = ExpressionWithPlaceholder::parse(rawFilter2, collator); + ASSERT_OK(expressionWithPlaceholder2.getStatus()); + ASSERT(expressionWithPlaceholder1.getValue()->equivalent( + expressionWithPlaceholder2.getValue().get())); +} + TEST(ExpressionWithPlaceholderTest, NotEquivalentIfPlaceholderDoesNotMatch) { constexpr auto collator = nullptr; auto rawFilter1 = fromjson("{i: {$type: 'array'}}"); @@ -218,6 +323,19 @@ TEST(ExpressionWithPlaceholderTest, NotEquivalentIfPlaceholderDoesNotMatch) { expressionWithPlaceholder2.getValue().get())); } +TEST(ExpressionWithPlaceholder, NotEquivalentIfOnePlaceholderIsEmpty) { + constexpr auto collator = nullptr; + auto rawFilter1 = fromjson("{}"); + auto expressionWithPlaceholder1 = ExpressionWithPlaceholder::parse(rawFilter1, collator); + ASSERT_OK(expressionWithPlaceholder1.getStatus()); + + auto rawFilter2 = fromjson("{i: 5}"); + auto expressionWithPlaceholder2 = ExpressionWithPlaceholder::parse(rawFilter2, collator); + ASSERT_OK(expressionWithPlaceholder2.getStatus()); + ASSERT_FALSE(expressionWithPlaceholder1.getValue()->equivalent( + expressionWithPlaceholder2.getValue().get())); +} + TEST(ExpressionWithPlaceholderTest, NotEquivalentIfExpressionDoesNotMatch) { constexpr auto collator = nullptr; auto rawFilter1 = fromjson("{i: {$lte: 5}}"); diff --git a/src/mongo/db/matcher/schema/expression_internal_schema_allowed_properties.cpp b/src/mongo/db/matcher/schema/expression_internal_schema_allowed_properties.cpp index 21ae9713a1d..c293fdfd112 100644 --- a/src/mongo/db/matcher/schema/expression_internal_schema_allowed_properties.cpp +++ b/src/mongo/db/matcher/schema/expression_internal_schema_allowed_properties.cpp @@ -99,7 +99,7 @@ bool InternalSchemaAllowedPropertiesMatchExpression::_matchesBSONObj(const BSONO for (auto&& constraint : _patternProperties) { if (constraint.first.regex->PartialMatch(property.fieldName())) { checkOtherwise = false; - if (!constraint.second->getFilter()->matchesSingleElement(property)) { + if (!constraint.second->matchesBSONElement(property)) { return false; } } @@ -110,7 +110,7 @@ bool InternalSchemaAllowedPropertiesMatchExpression::_matchesBSONObj(const BSONO checkOtherwise = false; } - if (checkOtherwise && !_otherwise->getFilter()->matchesSingleElement(property)) { + if (checkOtherwise && !_otherwise->matchesBSONElement(property)) { return false; } } @@ -152,19 +152,14 @@ std::unique_ptr InternalSchemaAllowedPropertiesMatchExpression: clonedPatternProperties.reserve(_patternProperties.size()); for (auto&& constraint : _patternProperties) { clonedPatternProperties.emplace_back(Pattern(constraint.first.rawRegex), - stdx::make_unique( - constraint.second->getPlaceholder().toString(), - constraint.second->getFilter()->shallowClone())); + constraint.second->shallowClone()); } - auto clonedOtherwise = stdx::make_unique( - _otherwise->getPlaceholder().toString(), _otherwise->getFilter()->shallowClone()); - auto clone = stdx::make_unique(); clone->init(_properties, _namePlaceholder, std::move(clonedPatternProperties), - std::move(clonedOtherwise)); + _otherwise->shallowClone()); return {std::move(clone)}; } } // namespace mongo diff --git a/src/mongo/db/matcher/schema/expression_internal_schema_allowed_properties_test.cpp b/src/mongo/db/matcher/schema/expression_internal_schema_allowed_properties_test.cpp index e19a5bc2637..5572831cb8e 100644 --- a/src/mongo/db/matcher/schema/expression_internal_schema_allowed_properties_test.cpp +++ b/src/mongo/db/matcher/schema/expression_internal_schema_allowed_properties_test.cpp @@ -106,5 +106,13 @@ TEST(InternalSchemaAllowedPropertiesMatchExpression, EquivalentToClone) { ASSERT_OK(expr.getStatus()); auto clone = expr.getValue()->shallowClone(); ASSERT_TRUE(expr.getValue()->equivalent(clone.get())); + + filter = fromjson( + "{$_internalSchemaAllowedProperties: {properties: [], namePlaceholder: 'i'," + "patternProperties: [], otherwise: {}}}"); + expr = MatchExpressionParser::parse(filter, kSimpleCollator); + ASSERT_OK(expr.getStatus()); + clone = expr.getValue()->shallowClone(); + ASSERT_TRUE(expr.getValue()->equivalent(clone.get())); } } // namespace mongo diff --git a/src/mongo/db/matcher/schema/expression_internal_schema_match_array_index.cpp b/src/mongo/db/matcher/schema/expression_internal_schema_match_array_index.cpp index 7183e25cc28..9debc6046f9 100644 --- a/src/mongo/db/matcher/schema/expression_internal_schema_match_array_index.cpp +++ b/src/mongo/db/matcher/schema/expression_internal_schema_match_array_index.cpp @@ -72,7 +72,7 @@ void InternalSchemaMatchArrayIndexMatchExpression::serialize(BSONObjBuilder* bui { BSONObjBuilder matchArrayElemSubobj(pathSubobj.subobjStart(kName)); matchArrayElemSubobj.append("index", _index); - matchArrayElemSubobj.append("namePlaceholder", _expression->getPlaceholder()); + matchArrayElemSubobj.append("namePlaceholder", _expression->getPlaceholder().value_or("")); { BSONObjBuilder subexprSubObj(matchArrayElemSubobj.subobjStart("expression")); _expression->getFilter()->serialize(&subexprSubObj); @@ -86,11 +86,7 @@ void InternalSchemaMatchArrayIndexMatchExpression::serialize(BSONObjBuilder* bui std::unique_ptr InternalSchemaMatchArrayIndexMatchExpression::shallowClone() const { auto clone = stdx::make_unique(); - invariantOK(clone->init( - path(), - _index, - stdx::make_unique(_expression->getPlaceholder().toString(), - _expression->getFilter()->shallowClone()))); + invariantOK(clone->init(path(), _index, _expression->shallowClone())); return std::move(clone); } } // namespace mongo diff --git a/src/mongo/db/matcher/schema/expression_internal_schema_match_array_index.h b/src/mongo/db/matcher/schema/expression_internal_schema_match_array_index.h index 48da97b2015..c3536e7deaa 100644 --- a/src/mongo/db/matcher/schema/expression_internal_schema_match_array_index.h +++ b/src/mongo/db/matcher/schema/expression_internal_schema_match_array_index.h @@ -68,7 +68,7 @@ public: element = iterator.next(); } - return _expression->getFilter()->matchesSingleElement(element, details); + return _expression->matchesBSONElement(element, details); } void serialize(BSONObjBuilder* builder) const final; diff --git a/src/mongo/db/matcher/schema/expression_parser_schema_test.cpp b/src/mongo/db/matcher/schema/expression_parser_schema_test.cpp index a029ce91d6a..4b6c8009df6 100644 --- a/src/mongo/db/matcher/schema/expression_parser_schema_test.cpp +++ b/src/mongo/db/matcher/schema/expression_parser_schema_test.cpp @@ -768,12 +768,6 @@ TEST(MatchExpressionParserSchemaTest, AllowedPropertiesFailsParsingIfOtherwiseNo "patternProperties: [], otherwise: {i: {$invalid: 1}}}}}}"); ASSERT_EQ(MatchExpressionParser::parse(query, kSimpleCollator).getStatus(), ErrorCodes::BadValue); - - query = fromjson( - "{$_internalSchemaAllowedProperties:" - "{properties: [], namePlaceholder: 'i', patternProperties: [], otherwise: {}}}}}"); - ASSERT_EQ(MatchExpressionParser::parse(query, kSimpleCollator).getStatus(), - ErrorCodes::FailedToParse); } TEST(MatchExpressionParserSchemaTest, AllowedPropertiesParsesSuccessfully) { @@ -803,5 +797,16 @@ TEST(MatchExpressionParserSchemaTest, AllowedPropertiesAcceptsEmptyPropertiesAnd ASSERT_TRUE(allowedProperties.getValue()->matchesBSON(BSONObj())); } + +TEST(MatchExpressionParserSchemaTest, AllowedPropertiesAcceptsEmptyOtherwiseExpression) { + auto query = fromjson( + "{$_internalSchemaAllowedProperties:" + "{properties: [], namePlaceholder: 'i', patternProperties: [], otherwise: {}}}}}"); + auto allowedProperties = MatchExpressionParser::parse(query, kSimpleCollator); + ASSERT_OK(allowedProperties.getStatus()); + + ASSERT_TRUE(allowedProperties.getValue()->matchesBSON(BSONObj())); +} + } // namespace } // namespace mongo diff --git a/src/mongo/db/ops/parsed_update.cpp b/src/mongo/db/ops/parsed_update.cpp index 30a2ed15728..313f511d3fd 100644 --- a/src/mongo/db/ops/parsed_update.cpp +++ b/src/mongo/db/ops/parsed_update.cpp @@ -160,15 +160,20 @@ Status ParsedUpdate::parseArrayFilters() { << arrayFilterStatus.getStatus().reason()); } auto arrayFilter = std::move(arrayFilterStatus.getValue()); - - if (_arrayFilters.find(arrayFilter->getPlaceholder()) != _arrayFilters.end()) { + auto fieldName = arrayFilter->getPlaceholder(); + if (!fieldName) { + return Status( + ErrorCodes::FailedToParse, + "Cannot use an expression without a top-level field name in arrayFilters"); + } + if (_arrayFilters.find(*fieldName) != _arrayFilters.end()) { return Status(ErrorCodes::FailedToParse, str::stream() << "Found multiple array filters with the same top-level field name " - << arrayFilter->getPlaceholder()); + << *fieldName); } - _arrayFilters[arrayFilter->getPlaceholder()] = std::move(arrayFilter); + _arrayFilters[*fieldName] = std::move(arrayFilter); } return Status::OK(); diff --git a/src/mongo/db/update/update_array_node.cpp b/src/mongo/db/update/update_array_node.cpp index 02f904d77b2..c909d7c96cb 100644 --- a/src/mongo/db/update/update_array_node.cpp +++ b/src/mongo/db/update/update_array_node.cpp @@ -85,7 +85,7 @@ UpdateNode::ApplyResult UpdateArrayNode::apply(ApplyParams applyParams) const { } else { auto filter = _arrayFilters.find(update.first); invariant(filter != _arrayFilters.end()); - if (filter->second->getFilter()->matchesBSONElement(arrayElement)) { + if (filter->second->matchesBSONElement(arrayElement)) { matchingElements[i].push_back(update.second.get()); } } -- cgit v1.2.1