From 16b59bd6236a90ad3d807a66ec2fe529e09cf0ec Mon Sep 17 00:00:00 2001 From: Ted Tuckman Date: Tue, 6 Oct 2020 10:50:17 -0400 Subject: SERVER-50778 Compare array indexes numerically for updates --- jstests/core/update_bit_examples.js | 9 +++++++- jstests/multiVersion/bit_update_mixed_fcv.js | 30 +++++++++++++++++++++++++ src/mongo/db/update/path_support.h | 22 ++++++++++++++++++ src/mongo/db/update/update_array_node.h | 2 +- src/mongo/db/update/update_internal_node.cpp | 11 +++++---- src/mongo/db/update/update_internal_node.h | 10 ++++++--- src/mongo/db/update/update_object_node.h | 5 +++-- src/mongo/db/update/update_object_node_test.cpp | 2 +- 8 files changed, 79 insertions(+), 12 deletions(-) create mode 100644 jstests/multiVersion/bit_update_mixed_fcv.js diff --git a/jstests/core/update_bit_examples.js b/jstests/core/update_bit_examples.js index 0b8f868ea17..1f1c03596e3 100644 --- a/jstests/core/update_bit_examples.js +++ b/jstests/core/update_bit_examples.js @@ -1,7 +1,7 @@ // Cannot implicitly shard accessed collections because of following errmsg: A single // update/delete on a sharded collection must contain an exact match on _id or contain the shard // key. -// @tags: [assumes_unsharded_collection, requires_non_retryable_writes] +// @tags: [assumes_unsharded_collection, requires_non_retryable_writes, requires_fcv_47] // Basic examples for $bit var res; @@ -32,3 +32,10 @@ assert.eq(coll.findOne().a, 4); // SERVER-19706 Empty bit operation. res = coll.update({}, {$bit: {a: {}}}); assert.writeError(res); + +// Make sure $bit on index arrays 9 and 10 when padding is needed works. +assert.commandWorked(coll.insert({_id: 2, a: [0]})); +assert.commandWorked( + coll.update({_id: 2}, {$bit: {"a.9": {or: NumberInt(0)}, "a.10": {or: NumberInt(0)}}})); +res = coll.find({_id: 2}).toArray(); +assert.eq(res[0]["a"], [0, null, null, null, null, null, null, null, null, 0, 0]); diff --git a/jstests/multiVersion/bit_update_mixed_fcv.js b/jstests/multiVersion/bit_update_mixed_fcv.js new file mode 100644 index 00000000000..53884be32c6 --- /dev/null +++ b/jstests/multiVersion/bit_update_mixed_fcv.js @@ -0,0 +1,30 @@ +/** + * Tests that $bit updates succeed if primaries compare updates numerically and secondaries compare + * updates lexicographically for array indexes. + */ + +(function() { +"use strict"; + +const rst = new ReplSetTest({ + nodes: 2, + nodeOptions: {binVersion: "latest"}, +}); + +rst.startSet(); +rst.initiate(); + +let primary = rst.getPrimary(); +let testDB = primary.getDB(jsTestName()); +let coll = testDB.test; + +// assert.commandWorked(testDB.adminCommand({setFeatureCompatibilityVersion: lastLTSFCV})); +testDB.adminCommand({setFeatureCompatibilityVersion: lastLTSFCV}); + +assert.commandWorked(coll.insert({_id: 0, arr: [0]})); + +assert.commandWorked( + coll.update({_id: 0}, {$bit: {"a.9": {or: NumberInt(0)}, "a.10": {or: NumberInt(0)}}})); + +rst.stopSet(); +})(); diff --git a/src/mongo/db/update/path_support.h b/src/mongo/db/update/path_support.h index 808f2df5dff..c6df4afe3c8 100644 --- a/src/mongo/db/update/path_support.h +++ b/src/mongo/db/update/path_support.h @@ -32,6 +32,7 @@ #include #include +#include "mongo/base/parse_number.h" #include "mongo/base/status.h" #include "mongo/bson/mutable/element.h" #include "mongo/db/field_ref.h" @@ -50,6 +51,27 @@ static const size_t kMaxPaddingAllowed = 1500000; // Convenience type to hold equality matches at particular paths from a MatchExpression typedef std::map EqualityMatches; +struct cmpPathsAndArrayIndexes { + // Assumes paths are valid. + bool operator()(const std::string& a, const std::string& b) const { + NumberParser parser; + long numA; + long numB; + auto status = parser(a, &numA); + if (!status.isOK()) + return a < b; + status = parser(b, &numB); + if (!status.isOK()) + return a < b; + // If the numbers are the same, and the strings are the same, 'numA' < 'numB' is equivalent + // to 'a' < 'b'. However, the strings could still be different (for example, "1" and "01") + // and we need to treat them differently. + if (numA == numB) + return a < b; + return numA < numB; + } +}; + /** * Finds the longest portion of 'prefix' that exists in document rooted at 'root' and is * "viable." A viable path is one that, if fully created on a given doc, would not diff --git a/src/mongo/db/update/update_array_node.h b/src/mongo/db/update/update_array_node.h index c6e90c1d9c3..ff48b8dba59 100644 --- a/src/mongo/db/update/update_array_node.h +++ b/src/mongo/db/update/update_array_node.h @@ -99,7 +99,7 @@ public: private: const std::map>& _arrayFilters; - std::map> _children; + std::map, pathsupport::cmpPathsAndArrayIndexes> _children; // When calling apply() causes us to merge elements of '_children', we store the result of the // merge in case we need it for another array element or document. diff --git a/src/mongo/db/update/update_internal_node.cpp b/src/mongo/db/update/update_internal_node.cpp index 95cc60eda8e..fe5e14ab336 100644 --- a/src/mongo/db/update/update_internal_node.cpp +++ b/src/mongo/db/update/update_internal_node.cpp @@ -34,12 +34,15 @@ namespace mongo { // static -std::map> UpdateInternalNode::createUpdateNodeMapByMerging( - const std::map>& leftMap, - const std::map>& rightMap, +std::map, pathsupport::cmpPathsAndArrayIndexes> +UpdateInternalNode::createUpdateNodeMapByMerging( + const std::map, pathsupport::cmpPathsAndArrayIndexes>& + leftMap, + const std::map, pathsupport::cmpPathsAndArrayIndexes>& + rightMap, FieldRef* pathTaken, bool wrapFieldNameAsArrayFilterIdentifier) { - std::map> mergedMap; + std::map, pathsupport::cmpPathsAndArrayIndexes> mergedMap; // Get the union of the field names we know about among the leftMap and rightMap. stdx::unordered_set allFields; diff --git a/src/mongo/db/update/update_internal_node.h b/src/mongo/db/update/update_internal_node.h index 9cadd04fc57..a3403879513 100644 --- a/src/mongo/db/update/update_internal_node.h +++ b/src/mongo/db/update/update_internal_node.h @@ -34,6 +34,7 @@ #include "mongo/base/clonable_ptr.h" #include "mongo/db/field_ref.h" +#include "mongo/db/update/path_support.h" #include "mongo/db/update/update_node.h" namespace mongo { @@ -72,9 +73,12 @@ protected: * wrapFieldNameAsArrayFilterIdentifier is true, field names are wrapped as $[] for * error reporting. */ - static std::map> createUpdateNodeMapByMerging( - const std::map>& leftMap, - const std::map>& rightMap, + static std::map, pathsupport::cmpPathsAndArrayIndexes> + createUpdateNodeMapByMerging( + const std::map, pathsupport::cmpPathsAndArrayIndexes>& + leftMap, + const std::map, pathsupport::cmpPathsAndArrayIndexes>& + rightMap, FieldRef* pathTaken, bool wrapFieldNameAsArrayFilterIdentifier = false); diff --git a/src/mongo/db/update/update_object_node.h b/src/mongo/db/update/update_object_node.h index d7f2e56e9de..8c347968e74 100644 --- a/src/mongo/db/update/update_object_node.h +++ b/src/mongo/db/update/update_object_node.h @@ -39,6 +39,7 @@ #include "mongo/bson/bsonelement.h" #include "mongo/db/matcher/expression_with_placeholder.h" #include "mongo/db/update/modifier_table.h" +#include "mongo/db/update/path_support.h" #include "mongo/db/update/update_internal_node.h" #include "mongo/stdx/unordered_map.h" @@ -126,12 +127,12 @@ public: visitor->visit(this); } - const std::map>& getChildren() const { + const auto& getChildren() const { return _children; } private: - std::map> _children; + std::map, pathsupport::cmpPathsAndArrayIndexes> _children; clonable_ptr _positionalChild; // When calling apply() causes us to merge an element of '_children' with '_positionalChild', we diff --git a/src/mongo/db/update/update_object_node_test.cpp b/src/mongo/db/update/update_object_node_test.cpp index a8d09f4f891..2e5906e8f30 100644 --- a/src/mongo/db/update/update_object_node_test.cpp +++ b/src/mongo/db/update/update_object_node_test.cpp @@ -2565,7 +2565,7 @@ TEST_F(UpdateObjectNodeTest, ApplyMultipleArrayUpdates) { doc.getObject()); ASSERT_FALSE(doc.isInPlaceModeEnabled()); - assertOplogEntry(fromjson("{$set: {'a.10': 10, 'a.2': 2}}"), + assertOplogEntry(fromjson("{$set: {'a.2': 2, 'a.10': 10}}"), fromjson("{$v: 2, diff: {sa: {a: true, u2: 2, u10: 10}}}")); } -- cgit v1.2.1