SERVER-32048 Ensure updates that implicitly create an array element generate new null index keys

3 files changed, 83 insertions, 7 deletions

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();
+                    }
                 }
             }