diff options
author | Tess Avitabile <tess.avitabile@mongodb.com> | 2017-11-29 19:16:50 -0500 |
---|---|---|
committer | Tess Avitabile <tess.avitabile@mongodb.com> | 2017-12-11 16:53:27 -0500 |
commit | 80b311e0a8537a47eb723ed4b6c010b5d1006551 (patch) | |
tree | 277f5eaeb68fb67c561f597580e3d049b9373cc5 | |
parent | 26caeb66ad2a6733313f1e9c25afb812fd4a284e (diff) | |
download | mongo-80b311e0a8537a47eb723ed4b6c010b5d1006551.tar.gz |
SERVER-32048 Ensure updates that implicitly create an array element generate new null index keys
(cherry picked from commit 69f94d7172801725bad43dba99ca0e5400eb293c)
-rw-r--r-- | jstests/core/update_affects_indexes.js | 94 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_add_to_set.cpp | 8 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_add_to_set_test.cpp | 37 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_bit.cpp | 8 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_bit_test.cpp | 37 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_compare.cpp | 7 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_compare_test.cpp | 37 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_current_date.cpp | 10 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_current_date_test.cpp | 37 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_inc.cpp | 7 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_inc_test.cpp | 37 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_interface.h | 6 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_push.cpp | 9 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_push_test.cpp | 37 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_set.cpp | 6 | ||||
-rw-r--r-- | src/mongo/db/ops/modifier_set_test.cpp | 37 | ||||
-rw-r--r-- | src/mongo/db/update/modifier_node.cpp | 12 | ||||
-rw-r--r-- | src/mongo/db/update/set_node_test.cpp | 58 | ||||
-rw-r--r-- | src/mongo/db/update/update_driver.cpp | 20 |
19 files changed, 497 insertions, 7 deletions
diff --git a/jstests/core/update_affects_indexes.js b/jstests/core/update_affects_indexes.js new file mode 100644 index 00000000000..93bc4086c62 --- /dev/null +++ b/jstests/core/update_affects_indexes.js @@ -0,0 +1,94 @@ +// This is a regression test for SERVER-32048. It checks that index keys are correctly updated when +// an update modifier implicitly creates a new array element. +(function() { + "use strict"; + + let coll = db.update_affects_indexes; + coll.drop(); + let indexKeyPattern = {"a.b": 1}; + assert.commandWorked(coll.createIndex(indexKeyPattern)); + + // Tests that the document 'docId' has all the index keys in 'expectedKeys' and none of the + // index keys in 'unexpectedKeys'. + function assertExpectedIndexKeys(docId, expectedKeys, unexpectedKeys) { + for (let key of expectedKeys) { + let res = coll.find(docId).hint(indexKeyPattern).min(key).returnKey().toArray(); + assert.eq(1, res.length, tojson(res)); + assert.eq(key, res[0]); + } + + for (let key of unexpectedKeys) { + let res = coll.find(docId).hint(indexKeyPattern).min(key).returnKey().toArray(); + if (res.length > 0) { + assert.eq(1, res.length, tojson(res)); + assert.neq(key, res[0]); + } + } + } + + // $set implicitly creates array element at end of array. + assert.writeOK(coll.insert({_id: 0, a: [{b: 0}]})); + assertExpectedIndexKeys({_id: 0}, [{"a.b": 0}], [{"a.b": null}]); + assert.writeOK(coll.update({_id: 0}, {$set: {"a.1.c": 0}})); + assertExpectedIndexKeys({_id: 0}, [{"a.b": 0}, {"a.b": null}], []); + + // $set implicitly creates array element beyond end of array. + assert.writeOK(coll.insert({_id: 1, a: [{b: 0}]})); + assertExpectedIndexKeys({_id: 1}, [{"a.b": 0}], [{"a.b": null}]); + assert.writeOK(coll.update({_id: 1}, {$set: {"a.3.c": 0}})); + assertExpectedIndexKeys({_id: 1}, [{"a.b": 0}, {"a.b": null}], []); + + // $set implicitly creates array element in empty array (no index key changes needed). + assert.writeOK(coll.insert({_id: 2, a: []})); + assertExpectedIndexKeys({_id: 2}, [{"a.b": null}], []); + assert.writeOK(coll.update({_id: 2}, {$set: {"a.0.c": 0}})); + assertExpectedIndexKeys({_id: 2}, [{"a.b": null}], []); + + // $inc implicitly creates array element at end of array. + assert.writeOK(coll.insert({_id: 3, a: [{b: 0}]})); + assertExpectedIndexKeys({_id: 3}, [{"a.b": 0}], [{"a.b": null}]); + assert.writeOK(coll.update({_id: 3}, {$inc: {"a.1.c": 0}})); + assertExpectedIndexKeys({_id: 3}, [{"a.b": 0}, {"a.b": null}], []); + + // $mul implicitly creates array element at end of array. + assert.writeOK(coll.insert({_id: 4, a: [{b: 0}]})); + assertExpectedIndexKeys({_id: 4}, [{"a.b": 0}], [{"a.b": null}]); + assert.writeOK(coll.update({_id: 4}, {$mul: {"a.1.c": 0}})); + assertExpectedIndexKeys({_id: 4}, [{"a.b": 0}, {"a.b": null}], []); + + // $addToSet implicitly creates array element at end of array. + assert.writeOK(coll.insert({_id: 5, a: [{b: 0}]})); + assertExpectedIndexKeys({_id: 5}, [{"a.b": 0}], [{"a.b": null}]); + assert.writeOK(coll.update({_id: 5}, {$addToSet: {"a.1.c": 0}})); + assertExpectedIndexKeys({_id: 5}, [{"a.b": 0}, {"a.b": null}], []); + + // $bit implicitly creates array element at end of array. + assert.writeOK(coll.insert({_id: 6, a: [{b: 0}]})); + assertExpectedIndexKeys({_id: 6}, [{"a.b": 0}], [{"a.b": null}]); + assert.writeOK(coll.update({_id: 6}, {$bit: {"a.1.c": {and: NumberInt(1)}}})); + assertExpectedIndexKeys({_id: 6}, [{"a.b": 0}, {"a.b": null}], []); + + // $min implicitly creates array element at end of array. + assert.writeOK(coll.insert({_id: 7, a: [{b: 0}]})); + assertExpectedIndexKeys({_id: 7}, [{"a.b": 0}], [{"a.b": null}]); + assert.writeOK(coll.update({_id: 7}, {$min: {"a.1.c": 0}})); + assertExpectedIndexKeys({_id: 7}, [{"a.b": 0}, {"a.b": null}], []); + + // $max implicitly creates array element at end of array. + assert.writeOK(coll.insert({_id: 8, a: [{b: 0}]})); + assertExpectedIndexKeys({_id: 8}, [{"a.b": 0}], [{"a.b": null}]); + assert.writeOK(coll.update({_id: 8}, {$max: {"a.1.c": 0}})); + assertExpectedIndexKeys({_id: 8}, [{"a.b": 0}, {"a.b": null}], []); + + // $currentDate implicitly creates array element at end of array. + assert.writeOK(coll.insert({_id: 9, a: [{b: 0}]})); + assertExpectedIndexKeys({_id: 9}, [{"a.b": 0}], [{"a.b": null}]); + assert.writeOK(coll.update({_id: 9}, {$currentDate: {"a.1.c": true}})); + assertExpectedIndexKeys({_id: 9}, [{"a.b": 0}, {"a.b": null}], []); + + // $push implicitly creates array element at end of array. + assert.writeOK(coll.insert({_id: 10, a: [{b: 0}]})); + assertExpectedIndexKeys({_id: 10}, [{"a.b": 0}], [{"a.b": null}]); + assert.writeOK(coll.update({_id: 10}, {$push: {"a.1.c": 0}})); + assertExpectedIndexKeys({_id: 10}, [{"a.b": 0}, {"a.b": null}], []); +}()); diff --git a/src/mongo/db/ops/modifier_add_to_set.cpp b/src/mongo/db/ops/modifier_add_to_set.cpp index 32016419151..dc68fe11e84 100644 --- a/src/mongo/db/ops/modifier_add_to_set.cpp +++ b/src/mongo/db/ops/modifier_add_to_set.cpp @@ -197,6 +197,8 @@ Status ModifierAddToSet::prepare(mb::Element root, StringData matchedField, Exec // Locate the field name in 'root'. Status status = pathsupport::findLongestPrefix( _fieldRef, root, &_preparedState->idxFound, &_preparedState->elemFound); + const auto elemFoundIsArray = + _preparedState->elemFound.ok() && _preparedState->elemFound.getType() == BSONType::Array; // FindLongestPrefix may say the path does not exist at all, which is fine here, or // that the path was not viable or otherwise wrong, in which case, the mod cannot @@ -220,6 +222,12 @@ Status ModifierAddToSet::prepare(mb::Element root, StringData matchedField, Exec if (!_preparedState->elemFound.ok() || _preparedState->idxFound < (_fieldRef.numParts() - 1)) { // If no target element exists, we will simply be creating a new array. _preparedState->addAll = true; + + if (elemFoundIsArray) { + // Report that an existing array will gain a new element as a result of this mod. + execInfo->indexOfArrayWithNewElement[0] = _preparedState->idxFound; + } + return Status::OK(); } diff --git a/src/mongo/db/ops/modifier_add_to_set_test.cpp b/src/mongo/db/ops/modifier_add_to_set_test.cpp index 68fe447dbbd..e8a0b4944c2 100644 --- a/src/mongo/db/ops/modifier_add_to_set_test.cpp +++ b/src/mongo/db/ops/modifier_add_to_set_test.cpp @@ -442,4 +442,41 @@ TEST(Collation, AddToSetWithEachRespectsCollation) { ASSERT_EQUALS(doc, fromjson("{ a : ['abc', 'bdc'] }")); } + +TEST(IndexedMod, PrepareReportCreatedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod mod(fromjson("{$addToSet: {'a.1.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_TRUE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_EQUALS(*execInfo.indexOfArrayWithNewElement[0], 0u); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportModifiedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod mod(fromjson("{$addToSet: {'a.0.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.0.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportCreatedNumericObjectField) { + Document doc(fromjson("{a: {'0': {b: 0}}}")); + Mod mod(fromjson("{$addToSet: {'a.1.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} } // namespace diff --git a/src/mongo/db/ops/modifier_bit.cpp b/src/mongo/db/ops/modifier_bit.cpp index 51bfed75a64..75c096545cf 100644 --- a/src/mongo/db/ops/modifier_bit.cpp +++ b/src/mongo/db/ops/modifier_bit.cpp @@ -164,6 +164,8 @@ Status ModifierBit::prepare(mutablebson::Element root, // Locate the field name in 'root'. Status status = pathsupport::findLongestPrefix( _fieldRef, root, &_preparedState->idxFound, &_preparedState->elemFound); + const auto elemFoundIsArray = + _preparedState->elemFound.ok() && _preparedState->elemFound.getType() == BSONType::Array; // FindLongestPrefix may say the path does not exist at all, which is fine here, or @@ -189,6 +191,12 @@ Status ModifierBit::prepare(mutablebson::Element root, // If no target element exists, the value we will write is the result of applying // the operation to a zero-initialized integer element. _preparedState->newValue = apply(SafeNum(static_cast<int32_t>(0))); + + if (elemFoundIsArray) { + // Report that an existing array will gain a new element as a result of this mod. + execInfo->indexOfArrayWithNewElement[0] = _preparedState->idxFound; + } + return Status::OK(); } diff --git a/src/mongo/db/ops/modifier_bit_test.cpp b/src/mongo/db/ops/modifier_bit_test.cpp index 204e2bd16a6..41633e22865 100644 --- a/src/mongo/db/ops/modifier_bit_test.cpp +++ b/src/mongo/db/ops/modifier_bit_test.cpp @@ -758,4 +758,41 @@ TEST(DbUpdateTests, Bit1_4_Combined) { ASSERT_EQUALS(BSON("$set" << BSON("x" << ((3 | 2) & 8))), logDoc); } +TEST(IndexedMod, PrepareReportCreatedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod mod(fromjson("{$bit: {'a.1.c': {and: NumberInt(1)}}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_TRUE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_EQUALS(*execInfo.indexOfArrayWithNewElement[0], 0u); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportModifiedArrayElement) { + Document doc(fromjson("{a: [{b: NumberInt(0)}]}")); + Mod mod(fromjson("{$bit: {'a.0.c': {or: NumberInt(1)}}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.0.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportCreatedNumericObjectField) { + Document doc(fromjson("{a: {'0': {b: 0}}}")); + Mod mod(fromjson("{$bit: {'a.1.c': {and: NumberInt(1)}}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + } // namespace diff --git a/src/mongo/db/ops/modifier_compare.cpp b/src/mongo/db/ops/modifier_compare.cpp index a63d224a94f..0ea27b22575 100644 --- a/src/mongo/db/ops/modifier_compare.cpp +++ b/src/mongo/db/ops/modifier_compare.cpp @@ -110,6 +110,8 @@ Status ModifierCompare::prepare(mutablebson::Element root, // be created during the apply. Status status = pathsupport::findLongestPrefix( _updatePath, root, &_preparedState->idxFound, &_preparedState->elemFound); + const auto elemFoundIsArray = + _preparedState->elemFound.ok() && _preparedState->elemFound.getType() == BSONType::Array; // FindLongestPrefix may say the path does not exist at all, which is fine here, or // that the path was not viable or otherwise wrong, in which case, the mod cannot @@ -128,6 +130,11 @@ Status ModifierCompare::prepare(mutablebson::Element root, _preparedState->idxFound == (_updatePath.numParts() - 1)); if (!destExists) { execInfo->noOp = false; + + if (elemFoundIsArray) { + // Report that an existing array will gain a new element as a result of this mod. + execInfo->indexOfArrayWithNewElement[0] = _preparedState->idxFound; + } } else { const int compareVal = _preparedState->elemFound.compareWithBSONElement(_val, _collator, false); diff --git a/src/mongo/db/ops/modifier_compare_test.cpp b/src/mongo/db/ops/modifier_compare_test.cpp index 839697c899a..583a952697c 100644 --- a/src/mongo/db/ops/modifier_compare_test.cpp +++ b/src/mongo/db/ops/modifier_compare_test.cpp @@ -347,4 +347,41 @@ TEST(Collation, MaxRespectsCollationFromSetCollator) { ASSERT_OK(mod.apply()); ASSERT_EQUALS(fromjson("{a : 'abd'}"), doc); } + +TEST(IndexedMod, PrepareReportCreatedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod mod(fromjson("{$min: {'a.1.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_TRUE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_EQUALS(*execInfo.indexOfArrayWithNewElement[0], 0u); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportModifiedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod mod(fromjson("{$min: {'a.0.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.0.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportCreatedNumericObjectField) { + Document doc(fromjson("{a: {'0': {b: 0}}}")); + Mod mod(fromjson("{$min: {'a.1.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} } // namespace diff --git a/src/mongo/db/ops/modifier_current_date.cpp b/src/mongo/db/ops/modifier_current_date.cpp index babf1a2b82a..02f67fa7207 100644 --- a/src/mongo/db/ops/modifier_current_date.cpp +++ b/src/mongo/db/ops/modifier_current_date.cpp @@ -165,6 +165,8 @@ Status ModifierCurrentDate::prepare(mutablebson::Element root, // be created during the apply. Status status = pathsupport::findLongestPrefix( _updatePath, root, &_preparedState->idxFound, &_preparedState->elemFound); + const auto elemFoundIsArray = + _preparedState->elemFound.ok() && _preparedState->elemFound.getType() == BSONType::Array; // FindLongestPrefix may say the path does not exist at all, which is fine here, or // that the path was not viable or otherwise wrong, in which case, the mod cannot @@ -179,6 +181,14 @@ Status ModifierCurrentDate::prepare(mutablebson::Element root, // there is any conflict among mods. execInfo->fieldRef[0] = &_updatePath; + if (!_preparedState->elemFound.ok() || + _preparedState->idxFound < (_updatePath.numParts() - 1)) { + if (elemFoundIsArray) { + // Report that an existing array will gain a new element as a result of this mod. + execInfo->indexOfArrayWithNewElement[0] = _preparedState->idxFound; + } + } + return Status::OK(); } diff --git a/src/mongo/db/ops/modifier_current_date_test.cpp b/src/mongo/db/ops/modifier_current_date_test.cpp index cd0f872d018..b91aa5505c6 100644 --- a/src/mongo/db/ops/modifier_current_date_test.cpp +++ b/src/mongo/db/ops/modifier_current_date_test.cpp @@ -389,4 +389,41 @@ TEST_F(DottedTimestampInput, EmptyStartDoc) { validateOplogEntry(oplogFormat, logDoc); } +TEST_F(BoolInput, PrepareReportCreatedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod mod(fromjson("{$currentDate: {'a.1.c': true}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_TRUE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_EQUALS(*execInfo.indexOfArrayWithNewElement[0], 0u); + ASSERT_FALSE(execInfo.noOp); +} + +TEST_F(BoolInput, PrepareDoNotReportModifiedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod mod(fromjson("{$currentDate: {'a.0.c': true}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.0.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + +TEST_F(BoolInput, PrepareDoNotReportCreatedNumericObjectField) { + Document doc(fromjson("{a: {'0': {b: 0}}}")); + Mod mod(fromjson("{$currentDate: {'a.1.c': true}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + } // namespace diff --git a/src/mongo/db/ops/modifier_inc.cpp b/src/mongo/db/ops/modifier_inc.cpp index 1edca18a4fa..e708afeb941 100644 --- a/src/mongo/db/ops/modifier_inc.cpp +++ b/src/mongo/db/ops/modifier_inc.cpp @@ -135,6 +135,8 @@ Status ModifierInc::prepare(mutablebson::Element root, // be created during the apply. Status status = pathsupport::findLongestPrefix( _fieldRef, root, &_preparedState->idxFound, &_preparedState->elemFound); + const auto elemFoundIsArray = + _preparedState->elemFound.ok() && _preparedState->elemFound.getType() == BSONType::Array; // FindLongestPrefix may say the path does not exist at all, which is fine here, or // that the path was not viable or otherwise wrong, in which case, the mod cannot @@ -166,6 +168,11 @@ Status ModifierInc::prepare(mutablebson::Element root, if (_mode == MODE_MUL) _preparedState->newValue *= SafeNum(static_cast<int32_t>(0)); + if (elemFoundIsArray) { + // Report that an existing array will gain a new element as a result of this mod. + execInfo->indexOfArrayWithNewElement[0] = _preparedState->idxFound; + } + return Status::OK(); } diff --git a/src/mongo/db/ops/modifier_inc_test.cpp b/src/mongo/db/ops/modifier_inc_test.cpp index ec0a1b38690..5f941a3ae8b 100644 --- a/src/mongo/db/ops/modifier_inc_test.cpp +++ b/src/mongo/db/ops/modifier_inc_test.cpp @@ -633,4 +633,41 @@ TEST(Multiplication, ApplyMissingElementDouble) { ASSERT_EQUALS(mongo::NumberDouble, doc.root().rightChild().getType()); } +TEST(IndexedMod, PrepareReportCreatedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod mod(fromjson("{$inc: {'a.1.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_TRUE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_EQUALS(*execInfo.indexOfArrayWithNewElement[0], 0u); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportModifiedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod mod(fromjson("{$inc: {'a.0.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.0.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportCreatedNumericObjectField) { + Document doc(fromjson("{a: {'0': {b: 0}}}")); + Mod mod(fromjson("{$inc: {'a.1.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + } // namespace diff --git a/src/mongo/db/ops/modifier_interface.h b/src/mongo/db/ops/modifier_interface.h index cb104069ca9..b5705b536fe 100644 --- a/src/mongo/db/ops/modifier_interface.h +++ b/src/mongo/db/ops/modifier_interface.h @@ -205,11 +205,17 @@ struct ModifierInterface::ExecInfo { ExecInfo() : noOp(false), context(ANY_CONTEXT) { for (int i = 0; i < MAX_NUM_FIELDS; i++) { fieldRef[i] = NULL; + indexOfArrayWithNewElement[i] = boost::none; } } // The fields of concern to the driver: no other op may modify the fields listed here. FieldRef* fieldRef[MAX_NUM_FIELDS]; // not owned here + + // For each modified field ref, the index of the path component representing an existing array + // that gained a new element. + boost::optional<size_t> indexOfArrayWithNewElement[MAX_NUM_FIELDS]; + bool noOp; UpdateContext context; }; diff --git a/src/mongo/db/ops/modifier_push.cpp b/src/mongo/db/ops/modifier_push.cpp index c062a4e9c34..65b7cad0bbc 100644 --- a/src/mongo/db/ops/modifier_push.cpp +++ b/src/mongo/db/ops/modifier_push.cpp @@ -384,6 +384,8 @@ Status ModifierPush::prepare(mutablebson::Element root, // be created during the apply. Status status = pathsupport::findLongestPrefix( _fieldRef, root, &_preparedState->idxFound, &_preparedState->elemFound); + const auto elemFoundIsArray = + _preparedState->elemFound.ok() && _preparedState->elemFound.getType() == BSONType::Array; // FindLongestPrefix may say the path does not exist at all, which is fine here, or // that the path was not viable or otherwise wrong, in which case, the mod cannot @@ -413,6 +415,13 @@ Status ModifierPush::prepare(mutablebson::Element root, // there is any conflict among mods. execInfo->fieldRef[0] = &_fieldRef; + if (!_preparedState->elemFound.ok() || _preparedState->idxFound < (_fieldRef.numParts() - 1)) { + if (elemFoundIsArray) { + // Report that an existing array will gain a new element as a result of this mod. + execInfo->indexOfArrayWithNewElement[0] = _preparedState->idxFound; + } + } + return Status::OK(); } diff --git a/src/mongo/db/ops/modifier_push_test.cpp b/src/mongo/db/ops/modifier_push_test.cpp index 7c2d65bb485..d461f992afb 100644 --- a/src/mongo/db/ops/modifier_push_test.cpp +++ b/src/mongo/db/ops/modifier_push_test.cpp @@ -1504,4 +1504,41 @@ TEST(ToPosition, NegativePositionPushMultipleElements) { ASSERT_EQUALS(fromjson("{$set: {'a': [0, 1, 2, 5, 6, 7, 3, 4]}}"), logDoc); } +TEST(IndexedMod, PrepareReportCreatedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod mod(fromjson("{$push: {'a.1.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_TRUE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_EQUALS(*execInfo.indexOfArrayWithNewElement[0], 0u); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportModifiedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod mod(fromjson("{$push: {'a.0.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.0.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportCreatedNumericObjectField) { + Document doc(fromjson("{a: {'0': {b: 0}}}")); + Mod mod(fromjson("{$push: {'a.1.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(mod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + } // unnamed namespace diff --git a/src/mongo/db/ops/modifier_set.cpp b/src/mongo/db/ops/modifier_set.cpp index 60f2fa07608..5e3b89d1c1a 100644 --- a/src/mongo/db/ops/modifier_set.cpp +++ b/src/mongo/db/ops/modifier_set.cpp @@ -127,6 +127,8 @@ Status ModifierSet::prepare(mutablebson::Element root, // be created during the apply. Status status = pathsupport::findLongestPrefix( _fieldRef, root, &_preparedState->idxFound, &_preparedState->elemFound); + const auto elemFoundIsArray = + _preparedState->elemFound.ok() && _preparedState->elemFound.getType() == BSONType::Array; // FindLongestPrefix may say the path does not exist at all, which is fine here, or // that the path was not viable or otherwise wrong, in which case, the mod cannot @@ -155,6 +157,10 @@ Status ModifierSet::prepare(mutablebson::Element root, // If the field path is not fully present, then this mod cannot be in place, nor is it a noOp. if (!_preparedState->elemFound.ok() || _preparedState->idxFound < (_fieldRef.numParts() - 1)) { + if (elemFoundIsArray) { + // Report that an existing array will gain a new element as a result of this mod. + execInfo->indexOfArrayWithNewElement[0] = _preparedState->idxFound; + } return Status::OK(); } diff --git a/src/mongo/db/ops/modifier_set_test.cpp b/src/mongo/db/ops/modifier_set_test.cpp index f4ca9c86359..e60648e2aaf 100644 --- a/src/mongo/db/ops/modifier_set_test.cpp +++ b/src/mongo/db/ops/modifier_set_test.cpp @@ -433,6 +433,43 @@ TEST(IndexedMod, PrepareNonViablePath) { ASSERT_NOT_OK(setMod.prepare(doc.root(), "", &execInfo)); } +TEST(IndexedMod, PrepareReportCreatedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod setMod(fromjson("{$set: {'a.1.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(setMod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_TRUE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_EQUALS(*execInfo.indexOfArrayWithNewElement[0], 0u); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportModifiedArrayElement) { + Document doc(fromjson("{a: [{b: 0}]}")); + Mod setMod(fromjson("{$set: {'a.0.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(setMod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.0.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + +TEST(IndexedMod, PrepareDoNotReportCreatedNumericObjectField) { + Document doc(fromjson("{a: {'0': {b: 0}}}")); + Mod setMod(fromjson("{$set: {'a.1.c': 2}}")); + + ModifierInterface::ExecInfo execInfo; + ASSERT_OK(setMod.prepare(doc.root(), "", &execInfo)); + + ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a.1.c"); + ASSERT_FALSE(execInfo.indexOfArrayWithNewElement[0]); + ASSERT_FALSE(execInfo.noOp); +} + TEST(IndexedMod, PrepareApplyInPlace) { Document doc(fromjson("{a: [{b: 0},{b: 1},{b: 1}]}")); Mod setMod(fromjson("{$set: {'a.2.b': 2}}")); diff --git a/src/mongo/db/update/modifier_node.cpp b/src/mongo/db/update/modifier_node.cpp index 82b66014a51..a85b636ac0d 100644 --- a/src/mongo/db/update/modifier_node.cpp +++ b/src/mongo/db/update/modifier_node.cpp @@ -280,8 +280,16 @@ UpdateNode::ApplyResult ModifierNode::applyToNonexistentElement(ApplyParams appl ApplyResult applyResult; - // Determine if indexes are affected. - if (!applyParams.indexData || !applyParams.indexData->mightBeIndexed(fullPath)) { + // Determine if indexes are affected. If we did not create a new element in an array, check + // whether the full path affects indexes. If we did create a new element in an array, check + // whether the array itself might affect any indexes. This is necessary because if there is + // an index {"a.b": 1}, and we set "a.1.c" and implicitly create an array element in "a", + // then we may need to add a null key to the index, even though "a.1.c" does not appear to + // affect the index. + if (!applyParams.indexData || + !applyParams.indexData->mightBeIndexed(applyParams.element.getType() != BSONType::Array + ? fullPath + : applyParams.pathTaken->dottedField())) { applyResult.indexesAffected = false; } diff --git a/src/mongo/db/update/set_node_test.cpp b/src/mongo/db/update/set_node_test.cpp index 5c4bcf9be3d..03b354041b8 100644 --- a/src/mongo/db/update/set_node_test.cpp +++ b/src/mongo/db/update/set_node_test.cpp @@ -1275,6 +1275,64 @@ TEST_F(SetNodeTest, ApplyCanCreatePrefixOfImmutablePath) { ASSERT_EQUALS(fromjson("{$set: {a: 2}}"), getLogDoc()); } +TEST_F(SetNodeTest, ApplySetFieldInNonExistentArrayElementAffectsIndexOnSiblingField) { + auto update = fromjson("{$set: {'a.1.c': 2}}"); + boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); + SetNode node; + ASSERT_OK(node.init(update["$set"]["a.1.c"], expCtx)); + + mutablebson::Document doc(fromjson("{a: [{b: 0}]}")); + setPathToCreate("1.c"); + setPathTaken("a"); + addIndexedPath("a.b"); + auto result = node.apply(getApplyParams(doc.root()["a"])); + ASSERT_FALSE(result.noop); + ASSERT_TRUE(result.indexesAffected); + ASSERT_EQUALS(fromjson("{a: [{b: 0}, {c: 2}]}"), doc); + ASSERT_FALSE(doc.isInPlaceModeEnabled()); + ASSERT_EQUALS(countChildren(getLogDoc().root()), 1u); + ASSERT_EQUALS(fromjson("{$set: {'a.1.c': 2}}"), getLogDoc()); +} + +TEST_F(SetNodeTest, ApplySetFieldInExistingArrayElementDoesNotAffectIndexOnSiblingField) { + auto update = fromjson("{$set: {'a.0.c': 2}}"); + boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); + SetNode node; + ASSERT_OK(node.init(update["$set"]["a.0.c"], expCtx)); + + mutablebson::Document doc(fromjson("{a: [{b: 0}]}")); + setPathToCreate("c"); + setPathTaken("a.0"); + addIndexedPath("a.b"); + auto result = node.apply(getApplyParams(doc.root()["a"][0])); + ASSERT_FALSE(result.noop); + ASSERT_FALSE(result.indexesAffected); + ASSERT_EQUALS(fromjson("{a: [{b: 0, c: 2}]}"), doc); + ASSERT_FALSE(doc.isInPlaceModeEnabled()); + ASSERT_EQUALS(countChildren(getLogDoc().root()), 1u); + ASSERT_EQUALS(fromjson("{$set: {'a.0.c': 2}}"), getLogDoc()); +} + +TEST_F(SetNodeTest, ApplySetFieldInNonExistentNumericFieldDoesNotAffectIndexOnSiblingField) { + auto update = fromjson("{$set: {'a.1.c': 2}}"); + boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); + SetNode node; + ASSERT_OK(node.init(update["$set"]["a.1.c"], expCtx)); + + mutablebson::Document doc(fromjson("{a: {'0': {b: 0}}}")); + setPathToCreate("1.c"); + setPathTaken("a"); + addIndexedPath("a.b"); + addIndexedPath("a.1.b"); + auto result = node.apply(getApplyParams(doc.root()["a"])); + ASSERT_FALSE(result.noop); + ASSERT_FALSE(result.indexesAffected); + ASSERT_EQUALS(fromjson("{a: {'0': {b: 0}, '1': {c: 2}}}"), doc); + ASSERT_FALSE(doc.isInPlaceModeEnabled()); + ASSERT_EQUALS(countChildren(getLogDoc().root()), 1u); + ASSERT_EQUALS(fromjson("{$set: {'a.1.c': 2}}"), getLogDoc()); +} + TEST_F(SetNodeTest, ApplySetOnInsertIsNoopWhenInsertIsFalse) { auto update = fromjson("{$setOnInsert: {a: 2}}"); boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); diff --git a/src/mongo/db/update/update_driver.cpp b/src/mongo/db/update/update_driver.cpp index 7ae41480779..88d80341981 100644 --- a/src/mongo/db/update/update_driver.cpp +++ b/src/mongo/db/update/update_driver.cpp @@ -414,11 +414,21 @@ Status UpdateDriver::update(StringData matchedField, // is not a no-op and it is in a valid context -- then we switch back to a // non-in-place mode. // - // TODO: make mightBeIndexed and fieldRef like each other. - if (!_affectIndices && !execInfo.noOp && _indexedFields && - _indexedFields->mightBeIndexed(execInfo.fieldRef[i]->dottedField())) { - _affectIndices = true; - doc->disableInPlaceUpdates(); + // To determine if indexes are affected: If we did not create a new element in an + // array, check whether the full path affects indexes. If we did create a new + // element in an array, check whether the array itself might affect any indexes. + // This is necessary because if there is an index {"a.b": 1}, and we set "a.1.c" and + // implicitly create an array element in "a", then we may need to add a null key to + // the index {"a.b": 1}, even though "a.1.c" does not appear to affect the index. + if (!_affectIndices && !execInfo.noOp && _indexedFields) { + auto pathLengthForIndexCheck = execInfo.indexOfArrayWithNewElement[i] + ? *execInfo.indexOfArrayWithNewElement[i] + 1 + : execInfo.fieldRef[i]->numParts(); + if (_indexedFields->mightBeIndexed( + execInfo.fieldRef[i]->dottedSubstring(0, pathLengthForIndexCheck))) { + _affectIndices = true; + doc->disableInPlaceUpdates(); + } } } |