SERVER-40248 Return correct operation to apply for update oplog entries

7 files changed, 151 insertions, 36 deletions

diff --git a/src/mongo/db/repl/SConscript b/src/mongo/db/repl/SConscript
index f849bffbdf6..2e426a1a3cc 100644
--- a/src/mongo/db/repl/SConscript
+++ b/src/mongo/db/repl/SConscript
@@ -817,6 +817,16 @@ env.CppUnitTest(
 )
 
 env.CppUnitTest(
+    target='oplog_entry_test',
+    source=[
+        'oplog_entry_test.cpp',
+    ],
+    LIBDEPS=[
+        'idempotency_test_fixture',
+    ],
+)
+
+env.CppUnitTest(
     target='idempotency_document_structure_test',
     source=[
         'idempotency_document_structure_test.cpp',
diff --git a/src/mongo/db/repl/apply_ops_test.cpp b/src/mongo/db/repl/apply_ops_test.cpp
index 019bdd59b4c..a5c16c41cdb 100644
--- a/src/mongo/db/repl/apply_ops_test.cpp
+++ b/src/mongo/db/repl/apply_ops_test.cpp
@@ -471,7 +471,7 @@ TEST_F(ApplyOpsTest, ExtractOperationsReturnsOperationsWithSameOpTimeAsApplyOps)
                                                               << operation3.toBSON();
         ASSERT_EQUALS(ui3, *operation3.getUuid());
         ASSERT_EQUALS(ns3, operation3.getNss());
-        ASSERT_BSONOBJ_EQ(BSON("_id" << 3), operation3.getOperationToApply());
+        ASSERT_BSONOBJ_EQ(BSON("x" << 1), operation3.getOperationToApply());
 
         auto optionalUpsertBool = operation3.getUpsert();
         ASSERT(optionalUpsertBool);
diff --git a/src/mongo/db/repl/oplog_entry.cpp b/src/mongo/db/repl/oplog_entry.cpp
index 0ed3b9afed6..b5f71f9fe50 100644
--- a/src/mongo/db/repl/oplog_entry.cpp
+++ b/src/mongo/db/repl/oplog_entry.cpp
@@ -286,8 +286,9 @@ bool OplogEntry::shouldPrepare() const {
 BSONElement OplogEntry::getIdElement() const {
     invariant(isCrudOpType());
     if (getOpType() == OpTypeEnum::kUpdate) {
-        // We cannot use getOperationToApply() here because the BSONObj will go out out of scope
-        // after we return the BSONElement.
+        // We cannot use getObjectContainingDocumentKey() here because the BSONObj will go out
+        // of scope after we return the BSONElement.
+        fassert(31080, getObject2() != boost::none);
         return getObject2()->getField("_id");
     } else {
         return getObject()["_id"];
@@ -295,15 +296,17 @@ BSONElement OplogEntry::getIdElement() const {
 }
 
 BSONObj OplogEntry::getOperationToApply() const {
-    if (getOpType() != OpTypeEnum::kUpdate) {
-        return getObject();
-    }
+    return getObject();
+}
 
-    if (auto optionalObj = getObject2()) {
-        return *optionalObj;
+BSONObj OplogEntry::getObjectContainingDocumentKey() const {
+    invariant(isCrudOpType());
+    if (getOpType() == OpTypeEnum::kUpdate) {
+        fassert(31081, getObject2() != boost::none);
+        return *getObject2();
+    } else {
+        return getObject();
     }
-
-    return {};
 }
 
 OplogEntry::CommandType OplogEntry::getCommandType() const {
diff --git a/src/mongo/db/repl/oplog_entry.h b/src/mongo/db/repl/oplog_entry.h
index c5a1af7e7dd..7aeffa97a70 100644
--- a/src/mongo/db/repl/oplog_entry.h
+++ b/src/mongo/db/repl/oplog_entry.h
@@ -173,14 +173,23 @@ public:
     BSONElement getIdElement() const;
 
     /**
-     * Returns the document representing the operation to apply.
-     * For commands and insert/delete operations, this will be the document in the 'o' field.
-     * For update operations, this will be the document in the 'o2' field.
-     * An empty document returned by this function indicates that we have a malformed OplogEntry.
+     * Returns the document representing the operation to apply. This is the 'o' field for all
+     * operations, including updates. For updates this is not guaranteed to include the _id or the
+     * shard key.
      */
     BSONObj getOperationToApply() const;
 
     /**
+     * Returns an object containing the _id of the target document for a CRUD operation. In a
+     * sharded cluster this object also contains the shard key. This object may contain more fields
+     * in the target document than the _id and shard key.
+     * For insert/delete operations, this will be the document in the 'o' field.
+     * For update operations, this will be the document in the 'o2' field.
+     * Should not be called for non-CRUD operations.
+     */
+    BSONObj getObjectContainingDocumentKey() const;
+
+    /**
      * Returns the type of command of the oplog entry. If it is not a command, returns kNotCommand.
      */
     CommandType getCommandType() const;
diff --git a/src/mongo/db/repl/oplog_entry_test.cpp b/src/mongo/db/repl/oplog_entry_test.cpp
new file mode 100644
index 00000000000..e1d4c614118
--- /dev/null
+++ b/src/mongo/db/repl/oplog_entry_test.cpp
@@ -0,0 +1,109 @@
+/**
+ *    Copyright (C) 2019-present MongoDB, Inc.
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the Server Side Public License, version 1,
+ *    as published by MongoDB, Inc.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    Server Side Public License for more details.
+ *
+ *    You should have received a copy of the Server Side Public License
+ *    along with this program. If not, see
+ *    <http://www.mongodb.com/licensing/server-side-public-license>.
+ *
+ *    As a special exception, the copyright holders give permission to link the
+ *    code of portions of this program with the OpenSSL library under certain
+ *    conditions as described in each individual source file and distribute
+ *    linked combinations including the program with the OpenSSL library. You
+ *    must comply with the Server Side Public License in all respects for
+ *    all of the code used other than as permitted herein. If you modify file(s)
+ *    with this exception, you may extend this exception to your version of the
+ *    file(s), but you are not obligated to do so. If you do not wish to do so,
+ *    delete this exception statement from your version. If you delete this
+ *    exception statement from all source files in the program, then also delete
+ *    it in the license file.
+ */
+
+#include "mongo/platform/basic.h"
+
+#include "mongo/db/repl/idempotency_test_fixture.h"
+#include "mongo/unittest/unittest.h"
+
+namespace mongo {
+namespace repl {
+namespace {
+
+const OpTime entryOpTime{Timestamp(3, 4), 5};
+const NamespaceString nss{"foo", "bar"};
+const int docId = 17;
+
+TEST(OplogEntryTest, Update) {
+    const BSONObj doc = BSON("_id" << docId);
+    const BSONObj update = BSON("$set" << BSON("a" << 4));
+    const auto entry = makeUpdateDocumentOplogEntry(entryOpTime, nss, doc, update);
+
+    ASSERT_FALSE(entry.isCommand());
+    ASSERT_FALSE(entry.isPartialTransaction());
+    ASSERT(entry.isCrudOpType());
+    ASSERT_FALSE(entry.shouldPrepare());
+    ASSERT_BSONOBJ_EQ(entry.getIdElement().wrap("_id"), doc);
+    ASSERT_BSONOBJ_EQ(entry.getOperationToApply(), update);
+    ASSERT_BSONOBJ_EQ(entry.getObjectContainingDocumentKey(), doc);
+    ASSERT(entry.getCommandType() == OplogEntry::CommandType::kNotCommand);
+    ASSERT_EQ(entry.getOpTime(), entryOpTime);
+}
+
+TEST(OplogEntryTest, Insert) {
+    const BSONObj doc = BSON("_id" << docId << "a" << 5);
+    const auto entry = makeInsertDocumentOplogEntry(entryOpTime, nss, doc);
+
+    ASSERT_FALSE(entry.isCommand());
+    ASSERT_FALSE(entry.isPartialTransaction());
+    ASSERT(entry.isCrudOpType());
+    ASSERT_FALSE(entry.shouldPrepare());
+    ASSERT_BSONOBJ_EQ(entry.getIdElement().wrap("_id"), BSON("_id" << docId));
+    ASSERT_BSONOBJ_EQ(entry.getOperationToApply(), doc);
+    ASSERT_BSONOBJ_EQ(entry.getObjectContainingDocumentKey(), doc);
+    ASSERT(entry.getCommandType() == OplogEntry::CommandType::kNotCommand);
+    ASSERT_EQ(entry.getOpTime(), entryOpTime);
+}
+
+TEST(OplogEntryTest, Delete) {
+    const BSONObj doc = BSON("_id" << docId);
+    const auto entry = makeDeleteDocumentOplogEntry(entryOpTime, nss, doc);
+
+    ASSERT_FALSE(entry.isCommand());
+    ASSERT_FALSE(entry.isPartialTransaction());
+    ASSERT(entry.isCrudOpType());
+    ASSERT_FALSE(entry.shouldPrepare());
+    ASSERT_BSONOBJ_EQ(entry.getIdElement().wrap("_id"), doc);
+    ASSERT_BSONOBJ_EQ(entry.getOperationToApply(), doc);
+    ASSERT_BSONOBJ_EQ(entry.getObjectContainingDocumentKey(), doc);
+    ASSERT(entry.getCommandType() == OplogEntry::CommandType::kNotCommand);
+    ASSERT_EQ(entry.getOpTime(), entryOpTime);
+}
+
+TEST(OplogEntryTest, Create) {
+    CollectionOptions opts;
+    opts.capped = true;
+    opts.cappedSize = 15;
+
+    const auto entry = makeCreateCollectionOplogEntry(entryOpTime, nss, opts.toBSON());
+
+    ASSERT(entry.isCommand());
+    ASSERT_FALSE(entry.isPartialTransaction());
+    ASSERT_FALSE(entry.isCrudOpType());
+    ASSERT_FALSE(entry.shouldPrepare());
+    ASSERT_BSONOBJ_EQ(entry.getOperationToApply(),
+                      BSON("create" << nss.coll() << "capped" << true << "size" << 15));
+    ASSERT(entry.getCommandType() == OplogEntry::CommandType::kCreate);
+    ASSERT_EQ(entry.getOpTime(), entryOpTime);
+}
+
+
+}  // namespace
+}  // namespace repl
+}  // namespace mongo
diff --git a/src/mongo/db/repl/rs_rollback_test.cpp b/src/mongo/db/repl/rs_rollback_test.cpp
index 88931143977..23385e13f7f 100644
--- a/src/mongo/db/repl/rs_rollback_test.cpp
+++ b/src/mongo/db/repl/rs_rollback_test.cpp
@@ -2037,24 +2037,7 @@ TEST(RSRollbackTest, LocalEntryWithoutOIsFatal) {
                   RSFatalException);
 }
 
-TEST(RSRollbackTest, LocalUpdateEntryWithoutO2IsFatal) {
-    const auto validOplogEntry = BSON("op"
-                                      << "u"
-                                      << "ui"
-                                      << UUID::gen()
-                                      << "ts"
-                                      << Timestamp(1, 1)
-                                      << "t"
-                                      << 1LL
-                                      << "ns"
-                                      << "test.t"
-                                      << "o"
-                                      << BSON("_id" << 1 << "a" << 1)
-                                      << "o2"
-                                      << BSON("_id" << 1));
-    FixUpInfo fui;
-    ASSERT_OK(updateFixUpInfoFromLocalOplogEntry(
-        nullptr /* opCtx */, OplogInterfaceMock(), fui, validOplogEntry, false));
+DEATH_TEST_F(RSRollbackTest, LocalUpdateEntryWithoutO2IsFatal, "Fatal Assertion") {
     const auto invalidOplogEntry = BSON("op"
                                         << "u"
                                         << "ui"
@@ -2067,9 +2050,10 @@ TEST(RSRollbackTest, LocalUpdateEntryWithoutO2IsFatal) {
                                         << "test.t"
                                         << "o"
                                         << BSON("_id" << 1 << "a" << 1));
-    ASSERT_THROWS(updateFixUpInfoFromLocalOplogEntry(
-                      nullptr /* opCtx */, OplogInterfaceMock(), fui, invalidOplogEntry, false),
-                  RSFatalException);
+    FixUpInfo fui;
+    updateFixUpInfoFromLocalOplogEntry(
+        nullptr /* opCtx */, OplogInterfaceMock(), fui, invalidOplogEntry, false)
+        .ignore();
 }
 
 TEST(RSRollbackTest, LocalUpdateEntryWithEmptyO2IsFatal) {
diff --git a/src/mongo/db/s/migration_chunk_cloner_source_legacy.cpp b/src/mongo/db/s/migration_chunk_cloner_source_legacy.cpp
index 57ba2ccd650..9e6e44388fd 100644
--- a/src/mongo/db/s/migration_chunk_cloner_source_legacy.cpp
+++ b/src/mongo/db/s/migration_chunk_cloner_source_legacy.cpp
@@ -92,7 +92,7 @@ bool shouldApplyOplogToSession(const repl::OplogEntry& oplog,
                                const ShardKeyPattern& keyPattern) {
     // Skip appending CRUD operations that don't pertain to the ChunkRange being migrated.
     if (oplog.isCrudOpType()) {
-        auto shardKey = keyPattern.extractShardKeyFromDoc(oplog.getOperationToApply());
+        auto shardKey = keyPattern.extractShardKeyFromDoc(oplog.getObjectContainingDocumentKey());
         if (!range.containsKey(shardKey)) {
             return false;
         }