SERVER-21647 fix $rename to not re-order fields when the destination field is already present

(cherry picked from commit 994060e622c22783c29ff3d1c243c298fcd3442c)

Conflicts:
    src/mongo/bson/mutable/document.cpp
    src/mongo/bson/mutable/element.h
    src/mongo/bson/mutable/mutable_bson_test.cpp
    src/mongo/db/ops/modifier_rename.cpp
    src/mongo/db/ops/modifier_rename_test.cpp

5 files changed, 236 insertions, 4 deletions

diff --git a/src/mongo/bson/mutable/document.cpp b/src/mongo/bson/mutable/document.cpp
index 6ef13e1de43..acd09f8df79 100644
--- a/src/mongo/bson/mutable/document.cpp
+++ b/src/mongo/bson/mutable/document.cpp
@@ -1898,6 +1898,30 @@ namespace mutablebson {
         }
     }
 
+    Status Element::setValueElement(ConstElement setFrom) {
+        verify(ok());
+
+        // Can't set to your own root element, since this would create a circular document.
+        if (_doc->root() == setFrom) {
+            return Status(ErrorCodes::IllegalOperation,
+                          "Attempt to set an element to its own document's root");
+        }
+
+        // Setting to self is a no-op.
+        //
+        // Setting the root is always an error so we want to fall through to the error handling in
+        // this case.
+        if (*this == setFrom && _repIdx != kRootRepIdx) {
+            return Status::OK();
+        }
+
+        Document::Impl& impl = getDocument().getImpl();
+        ElementRep thisRep = impl.getElementRep(_repIdx);
+        const StringData fieldName = impl.getFieldNameForNewElement(thisRep);
+        Element newValue = getDocument().makeElementWithNewFieldName(fieldName, setFrom);
+        return setValue(newValue._repIdx);
+    }
+
     BSONType Element::getType() const {
         verify(ok());
         const Document::Impl& impl = getDocument().getImpl();
diff --git a/src/mongo/bson/mutable/element.h b/src/mongo/bson/mutable/element.h
index 189b840068c..284c265f223 100644
--- a/src/mongo/bson/mutable/element.h
+++ b/src/mongo/bson/mutable/element.h
@@ -454,6 +454,12 @@ namespace mutablebson {
          */
         Status setValueSafeNum(const SafeNum value);
 
+        /** Set the value of this Element to the value from another Element.
+         *
+         * The name of this Element is not modified.
+         */
+        Status setValueElement(ConstElement setFrom);
+
 
         //
         // Accessors
diff --git a/src/mongo/bson/mutable/mutable_bson_test.cpp b/src/mongo/bson/mutable/mutable_bson_test.cpp
index 3f8b2f40c74..82adff8cbfc 100644
--- a/src/mongo/bson/mutable/mutable_bson_test.cpp
+++ b/src/mongo/bson/mutable/mutable_bson_test.cpp
@@ -1379,6 +1379,168 @@ namespace {
         ASSERT_EQUALS(mongo::fromjson(outJson), doc.getObject());
     }
 
+    TEST(Document, SetValueElementFromSeparateDocument) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : 4 }");
+        mmb::Document doc1(inObj);
+
+        mongo::BSONObj inObj2 = mongo::fromjson("{ b : 5 }");
+        const mmb::Document doc2(inObj2);
+
+        mmb::Element setTo = doc1.root().leftChild();
+        mmb::ConstElement setFrom = doc2.root().leftChild();
+        ASSERT_OK(setTo.setValueElement(setFrom));
+
+        ASSERT_EQUALS(mongo::fromjson("{ a : 5 }"), doc1.getObject());
+
+        // Doc containing the 'setFrom' element should be unchanged.
+        ASSERT_EQUALS(inObj2, doc2.getObject());
+    }
+
+    TEST(Document, SetValueElementIsNoopWhenSetToSelf) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : 4 }");
+        mmb::Document doc(inObj);
+
+        mmb::Element element = doc.root().leftChild();
+        ASSERT_OK(element.setValueElement(element));
+
+        ASSERT_EQUALS(inObj, doc.getObject());
+    }
+
+    TEST(Document, SetValueElementIsNoopWhenSetToSelfFromCopy) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : 4 }");
+        mmb::Document doc(inObj);
+
+        mmb::Element element = doc.root().leftChild();
+        mmb::ConstElement elementCopy = element;
+        ASSERT_OK(element.setValueElement(elementCopy));
+
+        ASSERT_EQUALS(inObj, doc.getObject());
+    }
+
+    TEST(Document, SetValueElementIsNoopWhenSetToSelfNonRootElement) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : { b : { c: 4 } } }");
+        mmb::Document doc(inObj);
+
+        mmb::Element element = doc.root().leftChild().leftChild().leftChild();
+        ASSERT_EQUALS("c", element.getFieldName());
+        ASSERT_OK(element.setValueElement(element));
+
+        ASSERT_EQUALS(inObj, doc.getObject());
+    }
+
+    TEST(Document, SetValueElementSetToNestedObject) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : 4 }");
+        mmb::Document doc1(inObj);
+
+        mongo::BSONObj inObj2 = mongo::fromjson("{ b : { c : 5, d : 6 } }");
+        const mmb::Document doc2(inObj2);
+
+        mmb::Element setTo = doc1.root().leftChild();
+        mmb::ConstElement setFrom = doc2.root().leftChild();
+        ASSERT_OK(setTo.setValueElement(setFrom));
+
+        ASSERT_EQUALS(mongo::fromjson("{ a : { c : 5, d : 6 } }"), doc1.getObject());
+
+        // Doc containing the 'setFrom' element should be unchanged.
+        ASSERT_EQUALS(inObj2, doc2.getObject());
+    }
+
+    TEST(Document, SetValueElementNonRootElements) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : { b : 5, c : 6 } }");
+        mmb::Document doc1(inObj);
+
+        mongo::BSONObj inObj2 = mongo::fromjson("{ d : { e : 8, f : 9 } }");
+        const mmb::Document doc2(inObj2);
+
+        mmb::Element setTo = doc1.root().leftChild().rightChild();
+        ASSERT_EQUALS("c", setTo.getFieldName());
+        mmb::ConstElement setFrom = doc2.root().leftChild().leftChild();
+        ASSERT_EQUALS("e", setFrom.getFieldName());
+        ASSERT_OK(setTo.setValueElement(setFrom));
+
+        ASSERT_EQUALS(mongo::fromjson("{ a : { b : 5, c : 8 } }"), doc1.getObject());
+
+        // Doc containing the 'setFrom' element should be unchanged.
+        ASSERT_EQUALS(inObj2, doc2.getObject());
+    }
+
+    TEST(Document, SetValueElementSetRootToSelfErrors) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : 4 }");
+        mmb::Document doc(inObj);
+
+        mmb::Element element = doc.root();
+        ASSERT_NOT_OK(element.setValueElement(element));
+        ASSERT_EQUALS(inObj, doc.getObject());
+    }
+
+    TEST(Document, SetValueElementSetRootToAnotherDocRootErrors) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : 4 }");
+        mmb::Document doc1(inObj);
+
+        mongo::BSONObj inObj2 = mongo::fromjson("{ b : 5 }");
+        const mmb::Document doc2(inObj2);
+
+        mmb::Element setTo = doc1.root();
+        mmb::ConstElement setFrom = doc2.root();
+        ASSERT_NOT_OK(setTo.setValueElement(setFrom));
+
+        ASSERT_EQUALS(inObj, doc1.getObject());
+        ASSERT_EQUALS(inObj2, doc2.getObject());
+    }
+
+    TEST(Document, SetValueElementSetRootToNotRootInSelfErrors) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : 4 }");
+        mmb::Document doc(inObj);
+
+        mmb::Element setTo = doc.root();
+        mmb::ConstElement setFrom = doc.root().leftChild();
+        ASSERT_NOT_OK(setTo.setValueElement(setFrom));
+        ASSERT_EQUALS(inObj, doc.getObject());
+    }
+
+    TEST(Document, SetValueElementSetRootToNotRootInAnotherDocErrors) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : 4 }");
+        mmb::Document doc1(inObj);
+
+        mongo::BSONObj inObj2 = mongo::fromjson("{ b : 5 }");
+        const mmb::Document doc2(inObj2);
+
+        mmb::Element setTo = doc1.root();
+        mmb::ConstElement setFrom = doc2.root().leftChild();
+        ASSERT_NOT_OK(setTo.setValueElement(setFrom));
+
+        ASSERT_EQUALS(inObj, doc1.getObject());
+        ASSERT_EQUALS(inObj2, doc2.getObject());
+    }
+
+    TEST(Document, SetValueElementSetToOwnRootErrors) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : { b : 4 } }");
+        mmb::Document doc(inObj);
+
+        mmb::Element setTo = doc.root().leftChild().leftChild();
+        ASSERT_EQUALS("b", setTo.getFieldName());
+        mmb::ConstElement setFrom = doc.root();
+
+        ASSERT_NOT_OK(setTo.setValueElement(setFrom));
+        ASSERT_EQUALS(inObj, doc.getObject());
+    }
+
+    TEST(Document, SetValueElementSetToOtherDocRoot) {
+        mongo::BSONObj inObj = mongo::fromjson("{ a : { b : 4 } }");
+        mmb::Document doc1(inObj);
+
+        mongo::BSONObj inObj2 = mongo::fromjson("{ c : 5 } }");
+        mmb::Document doc2(inObj2);
+
+        mmb::Element setTo = doc1.root().leftChild().leftChild();
+        ASSERT_EQUALS("b", setTo.getFieldName());
+        mmb::ConstElement setFrom = doc2.root();
+
+        ASSERT_OK(setTo.setValueElement(setFrom));
+        ASSERT_EQUALS(mongo::fromjson("{ a : { b : { c : 5 } } }"), doc1.getObject());
+        ASSERT_EQUALS(inObj2, doc2.getObject());
+    }
+
     TEST(Document, CreateElementWithEmptyFieldName) {
         mmb::Document doc;
         mmb::Element noname = doc.makeElementObject(mongo::StringData());
diff --git a/src/mongo/db/ops/modifier_rename.cpp b/src/mongo/db/ops/modifier_rename.cpp
index c2a86e5cb0d..3942d11cea7 100644
--- a/src/mongo/db/ops/modifier_rename.cpp
+++ b/src/mongo/db/ops/modifier_rename.cpp
@@ -235,10 +235,8 @@ namespace mongo {
                                 (_preparedState->toIdxFound == (_toFieldRef.numParts()-1));
 
         if (destExists) {
-            removeStatus = _preparedState->toElemFound.remove();
-            if (!removeStatus.isOK()) {
-                return removeStatus;
-            }
+            // Set destination element to the value of the source element.
+            return _preparedState->toElemFound.setValueElement(_preparedState->fromElemFound);
         }
 
         // Creates the final element that's going to be the in 'doc'.
diff --git a/src/mongo/db/ops/modifier_rename_test.cpp b/src/mongo/db/ops/modifier_rename_test.cpp
index 606337bdd7c..cec279f5975 100644
--- a/src/mongo/db/ops/modifier_rename_test.cpp
+++ b/src/mongo/db/ops/modifier_rename_test.cpp
@@ -219,6 +219,48 @@ namespace {
         ASSERT_EQUALS(logDoc, logObj);
     }
 
+    TEST(SimpleReplace, RenameToExistingFieldDoesNotReorderFields) {
+        Document doc(fromjson("{a: 1, b: 2, c: 3}"));
+        Mod setMod(fromjson("{$rename: {a: 'b'}}"));
+
+        ModifierInterface::ExecInfo execInfo;
+        ASSERT_OK(setMod.prepare(doc.root(), "", &execInfo));
+        ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "a");
+        ASSERT_EQUALS(execInfo.fieldRef[1]->dottedField(), "b");
+        ASSERT_FALSE(execInfo.noOp);
+
+        ASSERT_OK(setMod.apply());
+        ASSERT_FALSE(doc.isInPlaceModeEnabled());
+        ASSERT_EQUALS(doc, fromjson("{b: 1, c: 3}"));
+
+        Document logDoc;
+        LogBuilder logBuilder(logDoc.root());
+        BSONObj logObj = fromjson("{$set: {b: 1}, $unset: {a: true}}");
+        ASSERT_OK(setMod.log(&logBuilder));
+        ASSERT_EQUALS(logDoc, logObj);
+    }
+
+    TEST(SimpleReplace, RenameToExistingNestedFieldDoesNotReorderFields) {
+        Document doc(fromjson("{a: {b: {c: 1, d: 2}}, b: 3, c: {d: 4}}"));
+        Mod setMod(fromjson("{$rename: {'c.d': 'a.b.c'}}"));
+
+        ModifierInterface::ExecInfo execInfo;
+        ASSERT_OK(setMod.prepare(doc.root(), "", &execInfo));
+        ASSERT_EQUALS(execInfo.fieldRef[0]->dottedField(), "c.d");
+        ASSERT_EQUALS(execInfo.fieldRef[1]->dottedField(), "a.b.c");
+        ASSERT_FALSE(execInfo.noOp);
+
+        ASSERT_OK(setMod.apply());
+        ASSERT_FALSE(doc.isInPlaceModeEnabled());
+        ASSERT_EQUALS(doc, fromjson("{a: {b: {c: 4, d: 2}}, b: 3, c: {}}"));
+
+        Document logDoc;
+        LogBuilder logBuilder(logDoc.root());
+        BSONObj logObj = fromjson("{$set: {'a.b.c': 4}, $unset: {'c.d': true}}");
+        ASSERT_OK(setMod.log(&logBuilder));
+        ASSERT_EQUALS(logDoc, logObj);
+    }
+
     TEST(DottedTo, MissingCompleteTo) {
         Document doc(fromjson("{a: 2, b: 1, c: {}}"));
         Mod setMod(fromjson("{$rename: {'a':'c.r.d'}}"));