SERVER-32109 RenameNode no longer checks for no-op set.

Before this change, a {$rename: {from: "to"}} would check to see if
the "from" and "to" values were identical, allowing it to elide the
{$set: {to: <value-of-from-field>}} portion of the oplog update
entry. The equality check we used was not exact, however, resulting in
potentially incorrect results from $rename in edge cases.

We could use a precise equality check, but it's more expensive. More
likely than not, it's cheaper to just include the $set in the oplog
update for every $rename, so that's what we do now.

2 files changed, 24 insertions, 14 deletions

diff --git a/src/mongo/db/update/rename_node.cpp b/src/mongo/db/update/rename_node.cpp
index bae93e460de..3a725218763 100644
--- a/src/mongo/db/update/rename_node.cpp
+++ b/src/mongo/db/update/rename_node.cpp
@@ -48,7 +48,11 @@ namespace {
  * SetElementNode object. We create a class for this purpose (rather than a stand-alone function) so
  * that it can inherit from ModifierNode.
  *
- * Unlike SetNode, SetElementNode takes a mutablebson::Element as its input.
+ * Unlike SetNode, SetElementNode takes a mutablebson::Element as its input. Additionally,
+ * SetElementNode::updateExistingElement() does not check for the possibility that we are
+ * overwriting the target value with an identical source value (a no-op). That check would require
+ * us to convert _elemToSet from a mutablebson::Element to a BSONElement, which is not worth the
+ * extra time.
  */
 class SetElementNode : public ModifierNode {
 public:
@@ -67,18 +71,8 @@ public:
 protected:
     ModifierNode::ModifyResult updateExistingElement(
         mutablebson::Element* element, std::shared_ptr<FieldRef> elementPath) const final {
-        // In the case of a $rename where the source and destination have the same value, (e.g., we
-        // are applying {$rename: {a: b}} to the document {a: "foo", b: "foo"}), there's no need to
-        // modify the destination element. However, the source and destination values must be
-        // _exactly_ the same, which is why we do not use collation for this check.
-        StringData::ComparatorInterface* comparator = nullptr;
-        auto considerFieldName = false;
-        if (_elemToSet.compareWithElement(*element, comparator, considerFieldName) != 0) {
-            invariantOK(element->setValueElement(_elemToSet));
-            return ModifyResult::kNormalUpdate;
-        } else {
-            return ModifyResult::kNoOp;
-        }
+        invariantOK(element->setValueElement(_elemToSet));
+        return ModifyResult::kNormalUpdate;
     }
 
     void setValueForNewElement(mutablebson::Element* element) const final {
diff --git a/src/mongo/db/update/rename_node_test.cpp b/src/mongo/db/update/rename_node_test.cpp
index 103ca7f69a8..dc58e1c8174 100644
--- a/src/mongo/db/update/rename_node_test.cpp
+++ b/src/mongo/db/update/rename_node_test.cpp
@@ -154,7 +154,23 @@ TEST_F(RenameNodeTest, ToAndFromHaveSameValue) {
     ASSERT_FALSE(result.noop);
     ASSERT_TRUE(result.indexesAffected);
     ASSERT_EQUALS(fromjson("{b: 2}"), doc);
-    ASSERT_EQUALS(fromjson("{$unset: {a: true}}"), getLogDoc());
+    ASSERT_EQUALS(fromjson("{$set: {b: 2}, $unset: {a: true}}"), getLogDoc());
+}
+
+TEST_F(RenameNodeTest, RenameToFieldWithSameValueButDifferentType) {
+    auto update = fromjson("{$rename: {a: 'b'}}");
+    boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
+    RenameNode node;
+    ASSERT_OK(node.init(update["$rename"]["a"], expCtx));
+
+    mutablebson::Document doc(fromjson("{a: 1, b: NumberLong(1)}"));
+    setPathTaken("b");
+    addIndexedPath("a");
+    auto result = node.apply(getApplyParams(doc.root()["b"]));
+    ASSERT_FALSE(result.noop);
+    ASSERT_TRUE(result.indexesAffected);
+    ASSERT_EQUALS(fromjson("{b: 1}"), doc);
+    ASSERT_EQUALS(fromjson("{$set: {b: 1}, $unset: {a: true}}"), getLogDoc());
 }
 
 TEST_F(RenameNodeTest, FromDottedElement) {