diff options
author | Tess Avitabile <tess.avitabile@mongodb.com> | 2017-11-09 15:42:32 -0500 |
---|---|---|
committer | Tess Avitabile <tess.avitabile@mongodb.com> | 2017-11-15 09:29:21 -0500 |
commit | 50521fcdcb57dad03b68d8ef070b970f227329cf (patch) | |
tree | feaddf8b8e9e65ee2a361cdcde4b23a50f94d211 /src/mongo/db/update | |
parent | e7f03f979c98ea7cc90151839d6d91276f5d32ed (diff) | |
download | mongo-50521fcdcb57dad03b68d8ef070b970f227329cf.tar.gz |
SERVER-31894 Update system should not use mutablebson::Element operator[](StringData name) for arrays
Diffstat (limited to 'src/mongo/db/update')
-rw-r--r-- | src/mongo/db/update/addtoset_node_test.cpp | 2 | ||||
-rw-r--r-- | src/mongo/db/update/arithmetic_node_test.cpp | 8 | ||||
-rw-r--r-- | src/mongo/db/update/rename_node_test.cpp | 2 | ||||
-rw-r--r-- | src/mongo/db/update/set_node_test.cpp | 14 | ||||
-rw-r--r-- | src/mongo/db/update/unset_node_test.cpp | 8 | ||||
-rw-r--r-- | src/mongo/db/update/update_array_node_test.cpp | 4 | ||||
-rw-r--r-- | src/mongo/db/update/update_driver.cpp | 13 | ||||
-rw-r--r-- | src/mongo/db/update/update_object_node.cpp | 40 | ||||
-rw-r--r-- | src/mongo/db/update/update_object_node_test.cpp | 29 |
9 files changed, 84 insertions, 36 deletions
diff --git a/src/mongo/db/update/addtoset_node_test.cpp b/src/mongo/db/update/addtoset_node_test.cpp index 7fe1e95ca45..83e26170f9a 100644 --- a/src/mongo/db/update/addtoset_node_test.cpp +++ b/src/mongo/db/update/addtoset_node_test.cpp @@ -395,7 +395,7 @@ TEST_F(AddToSetNodeTest, ApplyNestedArray) { mutablebson::Document doc(fromjson("{ _id : 1, a : [ 1, [ ] ] }")); setPathTaken("a.1"); addIndexedPath("a"); - auto result = node.apply(getApplyParams(doc.root()["a"]["1"])); + auto result = node.apply(getApplyParams(doc.root()["a"][1])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{ _id : 1, a : [ 1, [ 1 ] ] }"), doc); diff --git a/src/mongo/db/update/arithmetic_node_test.cpp b/src/mongo/db/update/arithmetic_node_test.cpp index f8cd87927c6..23a8bc5f1a7 100644 --- a/src/mongo/db/update/arithmetic_node_test.cpp +++ b/src/mongo/db/update/arithmetic_node_test.cpp @@ -240,7 +240,7 @@ TEST_F(ArithmeticNodeTest, ApplyPositional) { setPathTaken("a.1"); setMatchedField("1"); addIndexedPath("a"); - auto result = node.apply(getApplyParams(doc.root()["a"]["1"])); + auto result = node.apply(getApplyParams(doc.root()["a"][1])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a: [0, 7, 2]}"), doc); @@ -759,7 +759,7 @@ TEST_F(ArithmeticNodeTest, ApplyNoOpArrayIndex) { mutablebson::Document doc(fromjson("{a: [{b: 0},{b: 1},{b: 2}]}")); setPathTaken("a.2.b"); addIndexedPath("a"); - auto result = node.apply(getApplyParams(doc.root()["a"]["2"]["b"])); + auto result = node.apply(getApplyParams(doc.root()["a"][2]["b"])); ASSERT_TRUE(result.noop); ASSERT_FALSE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a: [{b: 0},{b: 1},{b: 2}]}"), doc); @@ -776,7 +776,7 @@ TEST_F(ArithmeticNodeTest, TypePromotionInArrayIsNotANoOp) { fromjson("{a: [{b: NumberInt(0)},{b: NumberInt(1)},{b: NumberInt(2)}]}")); setPathTaken("a.2.b"); addIndexedPath("a"); - auto result = node.apply(getApplyParams(doc.root()["a"]["2"]["b"])); + auto result = node.apply(getApplyParams(doc.root()["a"][2]["b"])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a: [{b: 0},{b: 1},{b: NumberLong(2)}]}"), doc); @@ -807,7 +807,7 @@ TEST_F(ArithmeticNodeTest, ApplyInPlaceArrayIndex) { mutablebson::Document doc(fromjson("{a: [{b: 0},{b: 1},{b: 1}]}")); setPathTaken("a.2.b"); addIndexedPath("a"); - auto result = node.apply(getApplyParams(doc.root()["a"]["2"]["b"])); + auto result = node.apply(getApplyParams(doc.root()["a"][2]["b"])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a: [{b: 0},{b: 1},{b: 3}]}"), doc); diff --git a/src/mongo/db/update/rename_node_test.cpp b/src/mongo/db/update/rename_node_test.cpp index e78ddf091d8..103ca7f69a8 100644 --- a/src/mongo/db/update/rename_node_test.cpp +++ b/src/mongo/db/update/rename_node_test.cpp @@ -281,7 +281,7 @@ TEST_F(RenameNodeTest, MoveToArrayElement) { mutablebson::Document doc(fromjson("{_id: 'test_object', a: [1, 2], b: 2}")); setPathTaken("a.1"); addIndexedPath("a"); - ASSERT_THROWS_CODE_AND_WHAT(node.apply(getApplyParams(doc.root()["a"]["1"])), + ASSERT_THROWS_CODE_AND_WHAT(node.apply(getApplyParams(doc.root()["a"][1])), AssertionException, ErrorCodes::BadValue, "The destination field cannot be an array element, 'a.1' in doc " diff --git a/src/mongo/db/update/set_node_test.cpp b/src/mongo/db/update/set_node_test.cpp index 705be2f6071..5c4bcf9be3d 100644 --- a/src/mongo/db/update/set_node_test.cpp +++ b/src/mongo/db/update/set_node_test.cpp @@ -138,7 +138,7 @@ TEST_F(SetNodeTest, ApplyPositional) { setPathTaken("a.1"); setMatchedField("1"); addIndexedPath("a"); - auto result = node.apply(getApplyParams(doc.root()["a"]["1"])); + auto result = node.apply(getApplyParams(doc.root()["a"][1])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a: [0, 6, 2]}"), doc); @@ -528,7 +528,7 @@ TEST_F(SetNodeTest, ApplyNoOpArrayIndex) { mutablebson::Document doc(fromjson("{a: [{b: 0},{b: 1},{b: 2}]}")); setPathTaken("a.2.b"); addIndexedPath("a.2.b"); - auto result = node.apply(getApplyParams(doc.root()["a"]["2"]["b"])); + auto result = node.apply(getApplyParams(doc.root()["a"][2]["b"])); ASSERT_TRUE(result.noop); ASSERT_FALSE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a: [{b: 0},{b: 1},{b: 2}]}"), doc); @@ -544,7 +544,7 @@ TEST_F(SetNodeTest, TypeChangeInArrayIsNotANoOp) { mutablebson::Document doc(fromjson("{a: [{b: 0},{b: 1},{b: 2.0}]}")); setPathTaken("a.2.b"); addIndexedPath("a.2.b"); - auto result = node.apply(getApplyParams(doc.root()["a"]["2"]["b"])); + auto result = node.apply(getApplyParams(doc.root()["a"][2]["b"])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a: [{b: 0},{b: 1},{b: NumberInt(2)}]}"), doc); @@ -575,7 +575,7 @@ TEST_F(SetNodeTest, ApplyInPlaceArrayIndex) { mutablebson::Document doc(fromjson("{a: [{b: 0},{b: 1},{b: 1}]}")); setPathTaken("a.2.b"); addIndexedPath("a.2.b"); - auto result = node.apply(getApplyParams(doc.root()["a"]["2"]["b"])); + auto result = node.apply(getApplyParams(doc.root()["a"][2]["b"])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a: [{b: 0},{b: 1},{b: 2}]}"), doc); @@ -757,7 +757,7 @@ TEST_F(SetNodeTest, ApplyNoOpComplex) { mutablebson::Document doc(fromjson("{a: [{b: {c: 0, d: 0}}, {b: {c: 1, d: 1}}]}}")); setPathTaken("a.1.b"); addIndexedPath("a.1.b"); - auto result = node.apply(getApplyParams(doc.root()["a"]["1"]["b"])); + auto result = node.apply(getApplyParams(doc.root()["a"][1]["b"])); ASSERT_TRUE(result.noop); ASSERT_FALSE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a: [{b: {c: 0, d: 0}}, {b: {c: 1, d: 1}}]}}"), doc); @@ -773,7 +773,7 @@ TEST_F(SetNodeTest, ApplySameStructure) { mutablebson::Document doc(fromjson("{a: [{b: {c: 0, d: 0}}, {b: {c: 1, xxx: 1}}]}}")); setPathTaken("a.1.b"); addIndexedPath("a.1.b"); - auto result = node.apply(getApplyParams(doc.root()["a"]["1"]["b"])); + auto result = node.apply(getApplyParams(doc.root()["a"][1]["b"])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a: [{b: {c: 0, d: 0}}, {b: {c: 1, d: 1}}]}}"), doc); @@ -896,7 +896,7 @@ TEST_F(SetNodeTest, ReplayArrayFieldNotAppendedIntermediateFromReplication) { setPathTaken("a.0"); addIndexedPath("a.1.b"); setFromOplogApplication(true); - auto result = node.apply(getApplyParams(doc.root()["a"]["0"])); + auto result = node.apply(getApplyParams(doc.root()["a"][0])); ASSERT_TRUE(result.noop); ASSERT_FALSE(result.indexesAffected); ASSERT_EQUALS(fromjson("{_id: 0, a: [1, {b: [1]}]}"), doc); diff --git a/src/mongo/db/update/unset_node_test.cpp b/src/mongo/db/update/unset_node_test.cpp index 1c68c8520dd..e25b434a9dc 100644 --- a/src/mongo/db/update/unset_node_test.cpp +++ b/src/mongo/db/update/unset_node_test.cpp @@ -200,7 +200,7 @@ TEST_F(UnsetNodeTest, UnsetArrayElement) { mutablebson::Document doc(fromjson("{a:[1], b:1}")); setPathTaken("a.0"); addIndexedPath("a"); - auto result = node.apply(getApplyParams(doc.root()["a"]["0"])); + auto result = node.apply(getApplyParams(doc.root()["a"][0])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a:[null], b:1}"), doc); @@ -218,7 +218,7 @@ TEST_F(UnsetNodeTest, UnsetPositional) { setPathTaken("a.1"); setMatchedField("1"); addIndexedPath("a"); - auto result = node.apply(getApplyParams(doc.root()["a"]["1"])); + auto result = node.apply(getApplyParams(doc.root()["a"][1])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a: [0, null, 2]}"), doc); @@ -252,7 +252,7 @@ TEST_F(UnsetNodeTest, UnsetFromObjectInArray) { mutablebson::Document doc(fromjson("{a: [{b: 1}]}")); setPathTaken("a.0.b"); addIndexedPath("a"); - auto result = node.apply(getApplyParams(doc.root()["a"]["0"]["b"])); + auto result = node.apply(getApplyParams(doc.root()["a"][0]["b"])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{a:[{}]}"), doc); @@ -269,7 +269,7 @@ TEST_F(UnsetNodeTest, CanUnsetInvalidField) { mutablebson::Document doc(fromjson("{b: 1, a: [{$b: 1}]}")); setPathTaken("a.0.$b"); addIndexedPath("a"); - auto result = node.apply(getApplyParams(doc.root()["a"]["0"]["$b"])); + auto result = node.apply(getApplyParams(doc.root()["a"][0]["$b"])); ASSERT_FALSE(result.noop); ASSERT_TRUE(result.indexesAffected); ASSERT_EQUALS(fromjson("{b: 1, a: [{}]}"), doc); diff --git a/src/mongo/db/update/update_array_node_test.cpp b/src/mongo/db/update/update_array_node_test.cpp index 59204998b81..243541a741d 100644 --- a/src/mongo/db/update/update_array_node_test.cpp +++ b/src/mongo/db/update/update_array_node_test.cpp @@ -141,8 +141,8 @@ DEATH_TEST_F(UpdateArrayNodeTest, foundIdentifiers)); mutablebson::Document doc(fromjson("{a: [{c: 0}, {c: 0}, {c: 1}]}")); - doc.root()["a"]["1"]["c"].setValueInt(1).transitional_ignore(); - doc.root()["a"]["2"]["c"].setValueInt(0).transitional_ignore(); + ASSERT_OK(doc.root()["a"][1]["c"].setValueInt(1)); + ASSERT_OK(doc.root()["a"][2]["c"].setValueInt(0)); addIndexedPath("a"); root.apply(getApplyParams(doc.root())); } diff --git a/src/mongo/db/update/update_driver.cpp b/src/mongo/db/update/update_driver.cpp index a09dc6635ec..7caaa4ab8a6 100644 --- a/src/mongo/db/update/update_driver.cpp +++ b/src/mongo/db/update/update_driver.cpp @@ -46,6 +46,7 @@ #include "mongo/db/update/storage_validation.h" #include "mongo/util/embedded_builder.h" #include "mongo/util/mongoutils/str.h" +#include "mongo/util/stringutils.h" namespace mongo { @@ -448,7 +449,17 @@ Status UpdateDriver::update(StringData matchedField, // Find the updated field in the updated document. auto newElem = doc->root(); for (size_t i = 0; i < (*path)->numParts(); ++i) { - newElem = newElem[(*path)->getPart(i)]; + if (newElem.getType() == BSONType::Array) { + auto indexFromField = parseUnsignedBase10Integer((*path)->getPart(i)); + if (indexFromField) { + newElem = newElem.findNthChild(*indexFromField); + } else { + newElem = newElem.getDocument().end(); + } + } else { + newElem = newElem[(*path)->getPart(i)]; + } + if (!newElem.ok()) { break; } diff --git a/src/mongo/db/update/update_object_node.cpp b/src/mongo/db/update/update_object_node.cpp index 542ed8cd201..ebf0b6cf8a1 100644 --- a/src/mongo/db/update/update_object_node.cpp +++ b/src/mongo/db/update/update_object_node.cpp @@ -81,6 +81,21 @@ StatusWith<std::string> parseArrayFilterIdentifier( } /** + * Gets the child of 'element' named 'field', if it exists. Otherwise returns a non-ok element. + */ +mutablebson::Element getChild(mutablebson::Element element, StringData field) { + if (element.getType() == BSONType::Object) { + return element[field]; + } else if (element.getType() == BSONType::Array) { + auto indexFromField = parseUnsignedBase10Integer(field); + if (indexFromField) { + return element.findNthChild(*indexFromField); + } + } + return element.getDocument().end(); +} + +/** * Applies 'child' to the child of 'applyParams->element' named 'field' (which will create it, if it * does not exist). If 'applyParams->pathToCreate' is created, then 'applyParams->pathToCreate' is * moved to the end of 'applyParams->pathTaken', and 'applyParams->element' is advanced to the end @@ -101,18 +116,8 @@ void applyChild(const UpdateNode& child, if (!applyParams->pathToCreate->empty()) { // We're already traversing a path with elements that don't exist yet, so we will definitely // need to append. - } else if (applyParams->element.getType() == BSONType::Object) { - childElement = applyParams->element[field]; - } else if (applyParams->element.getType() == BSONType::Array) { - boost::optional<size_t> indexFromField = parseUnsignedBase10Integer(field); - if (indexFromField) { - childElement = applyParams->element.findNthChild(*indexFromField); - } else { - // We're trying to traverse an array element, but the path specifies a name instead of - // an index. We append the name to 'pathToCreate' for now, even though we know we won't - // be able to create it. If the update eventually needs to create the path, - // pathsupport::createPathAt() will provide a sensible PathNotViable UserError. - } + } else { + childElement = getChild(applyParams->element, field); } if (childElement.ok()) { @@ -124,7 +129,8 @@ void applyChild(const UpdateNode& child, // We are traversing path components that do not exist in our document. Any update modifier // that creates new path components (i.e., any modifiers that return true for // allowCreation()) will need to create this component, so we append it to the - // 'pathToCreate' FieldRef. + // 'pathToCreate' FieldRef. If the component cannot be created, pathsupport::createPathAt() + // will provide a sensible PathNotViable UserError. childElement = applyParams->element; applyParams->pathToCreate->appendPart(field); } @@ -147,7 +153,8 @@ void applyChild(const UpdateNode& child, // to the end of 'pathTaken'. We should advance 'element' to the end of 'pathTaken'. if (applyParams->pathTaken->numParts() > pathTakenSizeBefore) { for (auto i = pathTakenSizeBefore; i < applyParams->pathTaken->numParts(); ++i) { - applyParams->element = applyParams->element[applyParams->pathTaken->getPart(i)]; + applyParams->element = + getChild(applyParams->element, applyParams->pathTaken->getPart(i)); invariant(applyParams->element.ok()); } } else if (!applyParams->pathToCreate->empty()) { @@ -155,14 +162,15 @@ void applyChild(const UpdateNode& child, // If the child is a leaf node, it may have created 'pathToCreate' without moving // 'pathToCreate' to the end of 'pathTaken'. We should move 'pathToCreate' to the end of // 'pathTaken' and advance 'element' to the end of 'pathTaken'. - childElement = applyParams->element[applyParams->pathToCreate->getPart(0)]; + childElement = getChild(applyParams->element, applyParams->pathToCreate->getPart(0)); if (childElement.ok()) { applyParams->element = childElement; applyParams->pathTaken->appendPart(applyParams->pathToCreate->getPart(0)); // Either the path was fully created or not created at all. for (size_t i = 1; i < applyParams->pathToCreate->numParts(); ++i) { - applyParams->element = applyParams->element[applyParams->pathToCreate->getPart(i)]; + applyParams->element = + getChild(applyParams->element, applyParams->pathToCreate->getPart(i)); invariant(applyParams->element.ok()); applyParams->pathTaken->appendPart(applyParams->pathToCreate->getPart(i)); } diff --git a/src/mongo/db/update/update_object_node_test.cpp b/src/mongo/db/update/update_object_node_test.cpp index 9bd9ff81d52..1d1edff4132 100644 --- a/src/mongo/db/update/update_object_node_test.cpp +++ b/src/mongo/db/update/update_object_node_test.cpp @@ -2513,6 +2513,35 @@ TEST_F(UpdateObjectNodeTest, ApplyMultipleArrayUpdates) { ASSERT_BSONOBJ_EQ(fromjson("{$set: {'a.10': 10, 'a.2': 2}}"), getLogDoc().getObject()); } +TEST_F(UpdateObjectNodeTest, ApplyMultipleUpdatesToDocumentInArray) { + auto setUpdate = fromjson("{$set: {'a.2.b': 1, 'a.2.c': 1}}"); + boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); + std::map<StringData, std::unique_ptr<ExpressionWithPlaceholder>> arrayFilters; + std::set<std::string> foundIdentifiers; + UpdateObjectNode root; + ASSERT_OK(UpdateObjectNode::parseAndMerge(&root, + modifiertable::ModifierType::MOD_SET, + setUpdate["$set"]["a.2.b"], + expCtx, + arrayFilters, + foundIdentifiers)); + ASSERT_OK(UpdateObjectNode::parseAndMerge(&root, + modifiertable::ModifierType::MOD_SET, + setUpdate["$set"]["a.2.c"], + expCtx, + arrayFilters, + foundIdentifiers)); + + mutablebson::Document doc(fromjson("{a: []}")); + addIndexedPath("a"); + auto result = root.apply(getApplyParams(doc.root())); + ASSERT_TRUE(result.indexesAffected); + ASSERT_FALSE(result.noop); + ASSERT_BSONOBJ_EQ(fromjson("{a: [null, null, {b: 1, c: 1}]}"), doc.getObject()); + ASSERT_FALSE(doc.isInPlaceModeEnabled()); + ASSERT_BSONOBJ_EQ(fromjson("{$set: {'a.2.b': 1, 'a.2.c': 1}}"), getLogDoc().getObject()); +} + TEST_F(UpdateObjectNodeTest, ApplyUpdateToNonViablePathInArray) { auto setUpdate = fromjson("{$set: {'a.b': 3}}"); boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); |