SERVER-29136 Plumb update document key through to op log

8 files changed, 30 insertions, 71 deletions

diff --git a/src/mongo/db/auth/authz_manager_external_state_mock.cpp b/src/mongo/db/auth/authz_manager_external_state_mock.cpp
index 1d2c92ecd2a..ca77d4184a7 100644
--- a/src/mongo/db/auth/authz_manager_external_state_mock.cpp
+++ b/src/mongo/db/auth/authz_manager_external_state_mock.cpp
@@ -203,7 +203,7 @@ Status AuthzManagerExternalStateMock::updateOne(OperationContext* opCtx,
             return status;
         BSONObj newObj = document.getObject().copy();
         *iter = newObj;
-        BSONObj idQuery = driver.makeOplogEntryQuery(newObj, false);
+        BSONObj idQuery = newObj["_id"_sd].Obj();
 
         if (_authzManager) {
             _authzManager->logOp(opCtx, "u", collectionName, logObj, &idQuery);
diff --git a/src/mongo/db/exec/update.cpp b/src/mongo/db/exec/update.cpp
index 5125368b612..f3a12e32159 100644
--- a/src/mongo/db/exec/update.cpp
+++ b/src/mongo/db/exec/update.cpp
@@ -46,6 +46,7 @@
 #include "mongo/db/repl/replication_coordinator_global.h"
 #include "mongo/db/s/collection_metadata.h"
 #include "mongo/db/s/collection_sharding_state.h"
+#include "mongo/db/s/metadata_manager.h"
 #include "mongo/db/service_context.h"
 #include "mongo/db/update/storage_validation.h"
 #include "mongo/stdx/memory.h"
@@ -288,34 +289,35 @@ BSONObj UpdateStage::transformAndUpdate(const Snapshotted<BSONObj>& oldObj, Reco
         WriteUnitOfWork wunit(getOpCtx());
 
         RecordId newRecordId;
+        OplogUpdateEntryArgs args;
+        if (!request->isExplain()) {
+            invariant(_collection);
+            auto* css = CollectionShardingState::get(getOpCtx(), _collection->ns());
+            args.nss = _collection->ns();
+            args.uuid = _collection->uuid();
+            args.stmtId = request->getStmtId();
+            args.update = logObj;
+            args.criteria = css->getMetadata().extractDocumentKey(newObj);
+            uassert(16980,
+                    "Multi-update operations require all documents to have an '_id' field",
+                    !request->isMulti() || args.criteria.hasField("_id"_sd));
+            args.fromMigrate = request->isFromMigration();
+            args.storeDocOption = getStoreDocMode(*request);
+            if (args.storeDocOption == OplogUpdateEntryArgs::StoreDocOption::PreImage) {
+                args.preImageDoc = oldObj.value().getOwned();
+            }
+        }
 
         if (inPlace) {
-            // Don't actually do the write if this is an explain.
             if (!request->isExplain()) {
-                invariant(_collection);
                 newObj = oldObj.value();
                 const RecordData oldRec(oldObj.value().objdata(), oldObj.value().objsize());
-                BSONObj idQuery = driver->makeOplogEntryQuery(newObj, request->isMulti());
-                OplogUpdateEntryArgs args;
-                args.nss = _collection->ns();
-                args.uuid = _collection->uuid();
-                args.stmtId = request->getStmtId();
-                args.update = logObj;
-                args.criteria = idQuery;
-                args.fromMigrate = request->isFromMigration();
-                args.storeDocOption = getStoreDocMode(*request);
-
-                if (args.storeDocOption == OplogUpdateEntryArgs::StoreDocOption::PreImage) {
-                    args.preImageDoc = oldObj.value().getOwned();
-                }
+
+                Snapshotted<RecordData> snap(oldObj.snapshotId(), oldRec);
 
                 StatusWith<RecordData> newRecStatus = _collection->updateDocumentWithDamages(
-                    getOpCtx(),
-                    recordId,
-                    Snapshotted<RecordData>(oldObj.snapshotId(), oldRec),
-                    source,
-                    _damages,
-                    &args);
+                    getOpCtx(), recordId, std::move(snap), source, _damages, &args);
+
                 newObj = uassertStatusOK(std::move(newRecStatus)).releaseToBson();
             }
 
@@ -329,23 +331,7 @@ BSONObj UpdateStage::transformAndUpdate(const Snapshotted<BSONObj>& oldObj, Reco
                                   << BSONObjMaxUserSize,
                     newObj.objsize() <= BSONObjMaxUserSize);
 
-            // Don't actually do the write if this is an explain.
             if (!request->isExplain()) {
-                invariant(_collection);
-                BSONObj idQuery = driver->makeOplogEntryQuery(newObj, request->isMulti());
-                OplogUpdateEntryArgs args;
-                args.nss = _collection->ns();
-                args.uuid = _collection->uuid();
-                args.stmtId = request->getStmtId();
-                args.update = logObj;
-                args.criteria = idQuery;
-                args.fromMigrate = request->isFromMigration();
-                args.storeDocOption = getStoreDocMode(*request);
-
-                if (args.storeDocOption == OplogUpdateEntryArgs::StoreDocOption::PreImage) {
-                    args.preImageDoc = oldObj.value().getOwned();
-                }
-
                 newRecordId = _collection->updateDocument(getOpCtx(),
                                                           recordId,
                                                           oldObj,
diff --git a/src/mongo/db/op_observer.h b/src/mongo/db/op_observer.h
index 3ff05ff561d..53c2a79f0e5 100644
--- a/src/mongo/db/op_observer.h
+++ b/src/mongo/db/op_observer.h
@@ -72,7 +72,7 @@ struct OplogUpdateEntryArgs {
     BSONObj criteria;
 
     // True if this update comes from a chunk migration.
-    bool fromMigrate;
+    bool fromMigrate = false;
 
     StoreDocOption storeDocOption = StoreDocOption::None;
 };
diff --git a/src/mongo/db/op_observer_impl.cpp b/src/mongo/db/op_observer_impl.cpp
index be7b8c9be8c..9a6bd8e5be7 100644
--- a/src/mongo/db/op_observer_impl.cpp
+++ b/src/mongo/db/op_observer_impl.cpp
@@ -177,7 +177,7 @@ repl::OpTime replLogDelete(OperationContext* opCtx,
                            OptionalCollectionUUID uuid,
                            Session* session,
                            StmtId stmtId,
-                           CollectionShardingState::DeleteState deleteState,
+                           const CollectionShardingState::DeleteState& deleteState,
                            bool fromMigrate,
                            const boost::optional<BSONObj>& deletedDoc) {
     OperationSessionInfo sessionInfo;
diff --git a/src/mongo/db/s/collection_sharding_state.cpp b/src/mongo/db/s/collection_sharding_state.cpp
index a5b6073205d..787b59e73bd 100644
--- a/src/mongo/db/s/collection_sharding_state.cpp
+++ b/src/mongo/db/s/collection_sharding_state.cpp
@@ -320,14 +320,11 @@ void CollectionShardingState::onUpdateOp(OperationContext* opCtx,
 }
 
 auto CollectionShardingState::makeDeleteState(BSONObj const& doc) -> DeleteState {
-    BSONObj documentKey = getMetadata().extractDocumentKey(doc).getOwned();
-    invariant(documentKey.hasField("_id"_sd));
-    return {std::move(documentKey),
+    return {getMetadata().extractDocumentKey(doc).getOwned(),
             _sourceMgr && _sourceMgr->getCloner()->isDocumentInMigratingChunk(doc)};
 }
 
-void CollectionShardingState::onDeleteOp(OperationContext* opCtx,
-                                         const CollectionShardingState::DeleteState& deleteState) {
+void CollectionShardingState::onDeleteOp(OperationContext* opCtx, DeleteState const& deleteState) {
     dassert(opCtx->lockState()->isCollectionLockedForMode(_nss.ns(), MODE_IX));
 
     if (serverGlobalParams.clusterRole == ClusterRole::ShardServer) {
diff --git a/src/mongo/db/s/metadata_manager.cpp b/src/mongo/db/s/metadata_manager.cpp
index c7d5c74052e..9e4e2efd88c 100644
--- a/src/mongo/db/s/metadata_manager.cpp
+++ b/src/mongo/db/s/metadata_manager.cpp
@@ -293,7 +293,8 @@ BSONObj ScopedCollectionMetadata::extractDocumentKey(BSONObj const& doc) const {
     if (auto id = doc["_id"_sd]) {
         return key.isEmpty() ? id.wrap() : BSONObjBuilder(std::move(key)).append(id).obj();
     }
-    return key;
+    // For legacy documents that lack an _id, use the document itself as its key.
+    return doc;
 }
 
 ScopedCollectionMetadata::~ScopedCollectionMetadata() {
diff --git a/src/mongo/db/update/update_driver.cpp b/src/mongo/db/update/update_driver.cpp
index 3c6124e4d8e..cfcc2d891fd 100644
--- a/src/mongo/db/update/update_driver.cpp
+++ b/src/mongo/db/update/update_driver.cpp
@@ -558,25 +558,6 @@ void UpdateDriver::setCollator(const CollatorInterface* collator) {
     _modOptions.collator = collator;
 }
 
-BSONObj UpdateDriver::makeOplogEntryQuery(const BSONObj& doc, bool multi) const {
-    BSONObjBuilder idPattern;
-    BSONElement id;
-    // NOTE: If the matching object lacks an id, we'll log
-    // with the original pattern.  This isn't replay-safe.
-    // It might make sense to suppress the log instead
-    // if there's no id.
-    if (doc.getObjectID(id)) {
-        idPattern.append(id);
-        return idPattern.obj();
-    } else {
-        uassert(16980,
-                str::stream() << "Multi-update operations require all documents to "
-                                 "have an '_id' field. "
-                              << doc.toString(),
-                !multi);
-        return doc;
-    }
-}
 void UpdateDriver::clear() {
     for (vector<ModifierInterface*>::iterator it = _mods.begin(); it != _mods.end(); ++it) {
         delete *it;
diff --git a/src/mongo/db/update/update_driver.h b/src/mongo/db/update/update_driver.h
index 823e8eafeeb..3a74b1ca753 100644
--- a/src/mongo/db/update/update_driver.h
+++ b/src/mongo/db/update/update_driver.h
@@ -98,12 +98,6 @@ public:
                                            mutablebson::Document& doc) const;
 
     /**
-     * return a BSONObj with the _id field of the doc passed in, or the doc itself.
-     * If no _id and multi, error.
-     */
-    BSONObj makeOplogEntryQuery(const BSONObj& doc, bool multi) const;
-
-    /**
      * Executes the update over 'doc'. If any modifier is positional, use 'matchedField' (index of
      * the array item matched). If 'doc' allows the modifiers to be applied in place and no index
      * updating is involved, then the modifiers may be applied "in place" over 'doc'.