From 9f73260db3bfa261b2f02880bcbf6010cbc10ea6 Mon Sep 17 00:00:00 2001 From: Nikita Lapkov Date: Thu, 5 Nov 2020 18:42:51 +0000 Subject: SERVER-32960 Make $mod truncate float values consistently and use long long for its arguments --- jstests/core/mod.js | 175 +++++++++++++++++++++ jstests/core/mod1.js | 29 ---- jstests/core/mod_with_where.js | 38 +++++ src/mongo/db/matcher/expression_leaf.cpp | 4 +- src/mongo/db/matcher/expression_leaf.h | 17 +- src/mongo/db/matcher/expression_parser.cpp | 3 +- .../db/matcher/expression_parser_leaf_test.cpp | 59 +++++++ 7 files changed, 288 insertions(+), 37 deletions(-) create mode 100644 jstests/core/mod.js delete mode 100644 jstests/core/mod1.js create mode 100644 jstests/core/mod_with_where.js diff --git a/jstests/core/mod.js b/jstests/core/mod.js new file mode 100644 index 00000000000..bf68927f905 --- /dev/null +++ b/jstests/core/mod.js @@ -0,0 +1,175 @@ +// Tests the behavior of $mod for match expressions. + +(function() { + "use strict"; + + const coll = db.mod; + coll.drop(); + + function assertDocumentsFromMod(divider, remainder, expectedDocuments) { + const actualDocuments = + coll.find({a: {$mod: [divider, remainder]}}).sort({_id: 1}).toArray(); + assert.eq(expectedDocuments, actualDocuments); + } + + // Check mod with divisor and remainder which do not fit into int type. + assert.writeOK(coll.insert([ + {_id: 1, a: 4000000000}, + {_id: 2, a: 15000000000}, + {_id: 3, a: -4000000000}, + {_id: 4, a: -15000000000}, + {_id: 5, a: 4000000000.12345}, + {_id: 6, a: 15000000000.12345}, + {_id: 7, a: -4000000000.12345}, + {_id: 8, a: -15000000000.12345}, + {_id: 9, a: NumberDecimal(4000000000.12345)}, + {_id: 10, a: NumberDecimal(15000000000.12345)}, + {_id: 11, a: NumberDecimal(-4000000000.12345)}, + {_id: 12, a: NumberDecimal(-15000000000.12345)}, + ])); + + assertDocumentsFromMod(4000000000, 0, [ + {_id: 1, a: 4000000000}, + {_id: 3, a: -4000000000}, + {_id: 5, a: 4000000000.12345}, + {_id: 7, a: -4000000000.12345}, + {_id: 9, a: NumberDecimal(4000000000.12345)}, + {_id: 11, a: NumberDecimal(-4000000000.12345)}, + ]); + + assertDocumentsFromMod(-4000000000, 0, [ + {_id: 1, a: 4000000000}, + {_id: 3, a: -4000000000}, + {_id: 5, a: 4000000000.12345}, + {_id: 7, a: -4000000000.12345}, + {_id: 9, a: NumberDecimal(4000000000.12345)}, + {_id: 11, a: NumberDecimal(-4000000000.12345)}, + ]); + + assertDocumentsFromMod(10000000000, 5000000000, [ + {_id: 2, a: 15000000000}, + {_id: 6, a: 15000000000.12345}, + {_id: 10, a: NumberDecimal(15000000000.12345)}, + ]); + + assertDocumentsFromMod(10000000000, -5000000000, [ + {_id: 4, a: -15000000000}, + {_id: 8, a: -15000000000.12345}, + {_id: 12, a: NumberDecimal(-15000000000.12345)}, + ]); + + assertDocumentsFromMod(-10000000000, 5000000000, [ + {_id: 2, a: 15000000000}, + {_id: 6, a: 15000000000.12345}, + {_id: 10, a: NumberDecimal(15000000000.12345)}, + ]); + + assertDocumentsFromMod(-10000000000, -5000000000, [ + {_id: 4, a: -15000000000}, + {_id: 8, a: -15000000000.12345}, + {_id: 12, a: NumberDecimal(-15000000000.12345)}, + ]); + + assert(coll.drop()); + + // Check truncation of input argument for mod operator. + assert.writeOK(coll.insert([ + {_id: 1, a: 4.2}, + {_id: 2, a: 4.5}, + {_id: 3, a: 4.7}, + {_id: 4, a: NumberDecimal(4.2)}, + {_id: 5, a: NumberDecimal(4.5)}, + {_id: 6, a: NumberDecimal(4.7)}, + ])); + + assertDocumentsFromMod(4, 0, [ + {_id: 1, a: 4.2}, + {_id: 2, a: 4.5}, + {_id: 3, a: 4.7}, + {_id: 4, a: NumberDecimal(4.2)}, + {_id: 5, a: NumberDecimal(4.5)}, + {_id: 6, a: NumberDecimal(4.7)}, + ]); + + assert(coll.drop()); + + // Check more basic mod usage. + assert.writeOK(coll.insert([ + {_id: 1, str: "abc123", a: 0}, + {_id: 2, str: "xyz123", a: 5}, + {_id: 3, str: "ijk123", a: 12}, + {_id: 4, s: "array", a: [0, 7]}, + {_id: 5, s: "array", a: [1, 7]}, + {_id: 6, s: "array", a: [-5]}, + ])); + + assert.eq(1, coll.find({a: {$mod: [-10, -5]}}).itcount()); + assert.eq(1, coll.find({a: {$mod: [4, -1]}}).itcount()); + assert.eq(4, coll.find({a: {$mod: [5, 0]}}).itcount()); + assert.eq(3, coll.find({a: {$mod: [12, 0]}}).itcount()); + assert.eq(1, coll.find({a: {$mod: [12, 1]}}).itcount()); + + assert.writeOK(coll.insert([ + {_id: 7, arr: [1, [2, 3], [[4]]]}, + {_id: 8, arr: [{b: [1, 2, 3]}, {b: [5]}]}, + {_id: 9, arr: [{b: [-1]}, {b: [5]}]}, + {_id: 10, arr: "string"}, + ])); + + // Check nested arrays and dotted paths. + assert.eq(1, coll.find({arr: {$mod: [1, 0]}}).itcount()); + assert.eq(0, coll.find({arr: {$mod: [4, 0]}}).itcount()); + assert.eq(1, coll.find({"arr.b": {$mod: [3, 0]}}).itcount()); + assert.eq(2, coll.find({"arr.b": {$mod: [5, 0]}}).itcount()); + assert.eq(0, coll.find({"arr.b.c": {$mod: [5, 0]}}).itcount()); + + // Check with different data types. + assert.eq(6, coll.find({a: {$mod: [1, NumberLong(0)]}}).itcount()); + assert.eq(3, coll.find({a: {$mod: [NumberLong(5), NumberDecimal(2.1)]}}).itcount()); + assert.eq(3, coll.find({a: {$mod: [NumberDecimal(5.001), NumberDecimal(2.1)]}}).itcount()); + assert.eq(1, coll.find({a: {$mod: [NumberInt(7), NumberInt(1)]}}).itcount()); + + // Check on fields that are not numbers or do not exist. + assert.eq(0, coll.find({str: {$mod: [10, 1]}}).itcount()); + assert.eq(0, coll.find({s: {$mod: [10, 1]}}).itcount()); + assert.eq(0, coll.find({noField: {$mod: [10, 1]}}).itcount()); + + // Check divide by zero. + assert.commandFailedWithCode(db.runCommand({find: coll.getName(), filter: {a: {$mod: [0, 1]}}}), + ErrorCodes.BadValue); + assert.commandFailedWithCode(db.runCommand({find: coll.getName(), filter: {a: {$mod: [0, 0]}}}), + ErrorCodes.BadValue); + + // Check failures with different data types. + assert.commandFailedWithCode(db.runCommand({ + find: coll.getName(), + filter: {a: {$mod: [NumberDecimal(0.001), NumberDecimal(1.001)]}} + }), + ErrorCodes.BadValue); + assert.commandFailedWithCode(db.runCommand({ + find: coll.getName(), + filter: {a: {$mod: [NumberInt(1), NumberInt(0), NumberInt(0)]}} + }), + ErrorCodes.BadValue); + assert.commandFailedWithCode( + db.runCommand({find: coll.getName(), filter: {a: {$mod: [NumberDecimal(0.1)]}}}), + ErrorCodes.BadValue); + assert.commandFailedWithCode( + db.runCommand( + {find: coll.getName(), filter: {a: {$mod: [NumberDecimal(0), NumberDecimal(0)]}}}), + ErrorCodes.BadValue); + + // Check incorrect arity. + assert.commandFailedWithCode( + db.runCommand({find: coll.getName(), filter: {a: {$mod: [10, 1, 1]}}}), + ErrorCodes.BadValue); + assert.commandFailedWithCode(db.runCommand({find: coll.getName(), filter: {a: {$mod: [10]}}}), + ErrorCodes.BadValue); + + // Check remainder, divisor not a number. + assert.commandFailedWithCode( + db.runCommand({find: coll.getName(), filter: {a: {$mod: ["a", 0]}}}), ErrorCodes.BadValue); + assert.commandFailedWithCode( + db.runCommand({find: coll.getName(), filter: {a: {$mod: ["a", "b"]}}}), + ErrorCodes.BadValue); +}()); diff --git a/jstests/core/mod1.js b/jstests/core/mod1.js deleted file mode 100644 index 11be6b1b293..00000000000 --- a/jstests/core/mod1.js +++ /dev/null @@ -1,29 +0,0 @@ - -t = db.mod1; -t.drop(); - -t.save({a: 1}); -t.save({a: 2}); -t.save({a: 11}); -t.save({a: 20}); -t.save({a: "asd"}); -t.save({a: "adasdas"}); - -assert.eq(2, t.find("this.a % 10 == 1").itcount(), "A1"); -assert.eq(2, t.find({a: {$mod: [10, 1]}}).itcount(), "A2"); -assert.eq(0, - t.find({a: {$mod: [10, 1]}}).explain("executionStats").executionStats.totalKeysExamined, - "A3"); - -t.ensureIndex({a: 1}); - -assert.eq(2, t.find("this.a % 10 == 1").itcount(), "B1"); -assert.eq(2, t.find({a: {$mod: [10, 1]}}).itcount(), "B2"); - -assert.eq(1, t.find("this.a % 10 == 0").itcount(), "B3"); -assert.eq(1, t.find({a: {$mod: [10, 0]}}).itcount(), "B4"); -assert.eq(4, - t.find({a: {$mod: [10, 1]}}).explain("executionStats").executionStats.totalKeysExamined, - "B5"); - -assert.eq(1, t.find({a: {$gt: 5, $mod: [10, 1]}}).itcount()); diff --git a/jstests/core/mod_with_where.js b/jstests/core/mod_with_where.js new file mode 100644 index 00000000000..5f60ec8d0e7 --- /dev/null +++ b/jstests/core/mod_with_where.js @@ -0,0 +1,38 @@ +// Tests the behavior of $mod for match expressions. +// @tags: [ +// assumes_balancer_off, +// # Uses $where operator +// requires_scripting, +// ] + +(function() { + "use strict"; + + const coll = db.mod_with_where; + coll.drop(); + + assert.writeOK(coll.insert([{a: 1}, {a: 2}, {a: 11}, {a: 20}, {a: "asd"}, {a: "adasdas"}])); + + // Check basic mod usage. + assert.eq(2, coll.find("this.a % 10 == 1").itcount(), "A1"); + assert.eq(2, coll.find({a: {$mod: [10, 1]}}).itcount(), "A2"); + assert.eq( + 0, + coll.find({a: {$mod: [10, 1]}}).explain("executionStats").executionStats.totalKeysExamined, + "A3"); + + assert.commandWorked(coll.createIndex({a: 1})); + + // Check mod with an index. + assert.eq(2, coll.find("this.a % 10 == 1").itcount(), "B1"); + assert.eq(2, coll.find({a: {$mod: [10, 1]}}).itcount(), "B2"); + assert.eq(1, coll.find("this.a % 10 == 0").itcount(), "B3"); + assert.eq(1, coll.find({a: {$mod: [10, 0]}}).itcount(), "B4"); + assert.eq( + 4, + coll.find({a: {$mod: [10, 1]}}).explain("executionStats").executionStats.totalKeysExamined, + "B5"); + assert.eq(1, coll.find({a: {$gt: 5, $mod: [10, 1]}}).itcount()); + + assert(coll.drop()); +}()); diff --git a/src/mongo/db/matcher/expression_leaf.cpp b/src/mongo/db/matcher/expression_leaf.cpp index fd9d082c518..eb1e77d88d9 100644 --- a/src/mongo/db/matcher/expression_leaf.cpp +++ b/src/mongo/db/matcher/expression_leaf.cpp @@ -289,7 +289,7 @@ void RegexMatchExpression::shortDebugString(StringBuilder& debug) const { // --------- -Status ModMatchExpression::init(StringData path, int divisor, int remainder) { +Status ModMatchExpression::init(StringData path, long long divisor, long long remainder) { if (divisor == 0) return Status(ErrorCodes::BadValue, "divisor cannot be 0"); _divisor = divisor; @@ -300,7 +300,7 @@ Status ModMatchExpression::init(StringData path, int divisor, int remainder) { bool ModMatchExpression::matchesSingleElement(const BSONElement& e, MatchDetails* details) const { if (!e.isNumber()) return false; - return mongoSafeMod(e.numberLong(), static_cast(_divisor)) == _remainder; + return mongoSafeMod(truncateToLong(e), _divisor) == _remainder; } void ModMatchExpression::debugString(StringBuilder& debug, int level) const { diff --git a/src/mongo/db/matcher/expression_leaf.h b/src/mongo/db/matcher/expression_leaf.h index f7e1ea078dc..90e8463d196 100644 --- a/src/mongo/db/matcher/expression_leaf.h +++ b/src/mongo/db/matcher/expression_leaf.h @@ -335,7 +335,7 @@ class ModMatchExpression : public LeafMatchExpression { public: ModMatchExpression() : LeafMatchExpression(MOD) {} - Status init(StringData path, int divisor, int remainder); + Status init(StringData path, long long divisor, long long remainder); virtual std::unique_ptr shallowClone() const { std::unique_ptr m = stdx::make_unique(); @@ -354,20 +354,27 @@ public: virtual bool equivalent(const MatchExpression* other) const; - int getDivisor() const { + long long getDivisor() const { return _divisor; } - int getRemainder() const { + long long getRemainder() const { return _remainder; } + static long long truncateToLong(const BSONElement& element) { + if (element.type() == BSONType::NumberDecimal) { + return element.numberDecimal().toLong(Decimal128::kRoundTowardZero); + } + return element.numberLong(); + } + private: ExpressionOptimizerFunc getOptimizer() const final { return [](std::unique_ptr expression) { return expression; }; } - int _divisor; - int _remainder; + long long _divisor; + long long _remainder; }; class ExistsMatchExpression : public LeafMatchExpression { diff --git a/src/mongo/db/matcher/expression_parser.cpp b/src/mongo/db/matcher/expression_parser.cpp index e1068520e2e..68b1702c7c5 100644 --- a/src/mongo/db/matcher/expression_parser.cpp +++ b/src/mongo/db/matcher/expression_parser.cpp @@ -565,7 +565,8 @@ StatusWithMatchExpression parseMOD(StringData name, BSONElement e) { return {Status(ErrorCodes::BadValue, "malformed mod, too many elements")}; auto temp = stdx::make_unique(); - auto s = temp->init(name, d.numberInt(), r.numberInt()); + auto s = temp->init( + name, ModMatchExpression::truncateToLong(d), ModMatchExpression::truncateToLong(r)); if (!s.isOK()) return s; return {std::move(temp)}; diff --git a/src/mongo/db/matcher/expression_parser_leaf_test.cpp b/src/mongo/db/matcher/expression_parser_leaf_test.cpp index a1a79f8f450..18a25a410ae 100644 --- a/src/mongo/db/matcher/expression_parser_leaf_test.cpp +++ b/src/mongo/db/matcher/expression_parser_leaf_test.cpp @@ -349,6 +349,65 @@ TEST(MatchExpressionParserLeafTest, SimpleModNotNumber) { << "a"))); } +TEST(MatchExpressionParserLeafTest, ModFloatTruncate) { + struct TestCase { + BSONObj _query; + long long _divider; + long long _remainder; + }; + + const auto positiveLargerThanInt = 3 * static_cast(std::numeric_limits::max()); + const auto negativeSmallerThanInt = 3 * static_cast(std::numeric_limits::min()); + std::vector testCases = { + {BSON("x" << BSON("$mod" << BSON_ARRAY(3 << 2))), 3, 2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(3LL << 2LL))), 3, 2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(3.2 << 2.2))), 3, 2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(3.7 << 2.7))), 3, 2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(Decimal128("3") << Decimal128("2")))), 3, 2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(Decimal128("3.2") << Decimal128("2.2")))), 3, 2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(Decimal128("3.7") << Decimal128("2.7")))), 3, 2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(positiveLargerThanInt << positiveLargerThanInt))), + positiveLargerThanInt, + positiveLargerThanInt}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(static_cast(positiveLargerThanInt) + << static_cast(positiveLargerThanInt)))), + positiveLargerThanInt, + positiveLargerThanInt}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(Decimal128(positiveLargerThanInt) + << Decimal128(positiveLargerThanInt)))), + positiveLargerThanInt, + positiveLargerThanInt}, + + {BSON("x" << BSON("$mod" << BSON_ARRAY(-3 << -2))), -3, -2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(-3LL << -2LL))), -3, -2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(-3.2 << -2.2))), -3, -2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(-3.7 << -2.7))), -3, -2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(Decimal128("-3") << Decimal128("-2")))), -3, -2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(Decimal128("-3.2") << Decimal128("-2.2")))), -3, -2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(Decimal128("-3.7") << Decimal128("-2.7")))), -3, -2}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(negativeSmallerThanInt << negativeSmallerThanInt))), + negativeSmallerThanInt, + negativeSmallerThanInt}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(static_cast(negativeSmallerThanInt) + << static_cast(negativeSmallerThanInt)))), + negativeSmallerThanInt, + negativeSmallerThanInt}, + {BSON("x" << BSON("$mod" << BSON_ARRAY(Decimal128(negativeSmallerThanInt) + << Decimal128(negativeSmallerThanInt)))), + negativeSmallerThanInt, + negativeSmallerThanInt}, + }; + + for (const auto& testCase : testCases) { + boost::intrusive_ptr expCtx(new ExpressionContextForTest()); + StatusWithMatchExpression result = MatchExpressionParser::parse(testCase._query, expCtx); + ASSERT_OK(result.getStatus()); + auto modExpr = static_cast(result.getValue().get()); + ASSERT_EQ(modExpr->getDivisor(), testCase._divider); + ASSERT_EQ(modExpr->getRemainder(), testCase._remainder); + } +} + TEST(MatchExpressionParserLeafTest, IdCollation) { BSONObj query = BSON("$id" << "string"); -- cgit v1.2.1