SERVER-29731 upconvertRequest shouldn't separate data and metadata

5 files changed, 63 insertions, 82 deletions

diff --git a/src/mongo/rpc/legacy_request.cpp b/src/mongo/rpc/legacy_request.cpp
index 2c22b17edf2..94df4545497 100644
--- a/src/mongo/rpc/legacy_request.cpp
+++ b/src/mongo/rpc/legacy_request.cpp
@@ -52,10 +52,7 @@ OpMsgRequest opMsgRequestFromLegacyRequest(const Message& message) {
                           << ") for $cmd type ns - can only be 1 or -1",
             qm.ntoreturn == 1 || qm.ntoreturn == -1);
 
-    auto bodyAndMetadata = rpc::upconvertRequestMetadata(qm.query, qm.queryOptions);
-
-    return OpMsgRequest::fromDBAndBody(
-        ns.db(), std::move(std::get<0>(bodyAndMetadata)), std::get<1>(bodyAndMetadata));
+    return OpMsgRequest::fromDBAndBody(ns.db(), rpc::upconvertRequest(qm.query, qm.queryOptions));
 }
 
 }  // namespace rpc
diff --git a/src/mongo/rpc/metadata.cpp b/src/mongo/rpc/metadata.cpp
index 2ccea81633a..42296681e7e 100644
--- a/src/mongo/rpc/metadata.cpp
+++ b/src/mongo/rpc/metadata.cpp
@@ -119,15 +119,11 @@ void readRequestMetadata(OperationContext* opCtx, const BSONObj& metadataObj) {
     }
 }
 
-CommandAndMetadata upconvertRequestMetadata(BSONObj legacyCmdObj, int queryFlags) {
-    // We can reuse the same metadata BOB for every upconvert call, but we need to keep
-    // making new command BOBs as each metadata bob will need to remove fields. We can not use
-    // mutablebson here because the ReadPreference upconvert routine performs
-    // manipulations (replacing a root with its child) that mutablebson doesn't
-    // support.
+BSONObj upconvertRequest(BSONObj cmdObj, int queryFlags) {
+    cmdObj = cmdObj.getOwned();  // Usually this is a no-op since it is already owned.
 
     auto readPrefContainer = BSONObj();
-    const StringData firstFieldName = legacyCmdObj.firstElementFieldName();
+    const StringData firstFieldName = cmdObj.firstElementFieldName();
     if (firstFieldName == "$query" || firstFieldName == "query") {
         // Commands sent over OP_QUERY specify read preference by putting it at the top level and
         // putting the command in a nested field called either query or $query.
@@ -136,35 +132,30 @@ CommandAndMetadata upconvertRequestMetadata(BSONObj legacyCmdObj, int queryFlags
         uassert(ErrorCodes::InvalidOptions,
                 "cannot use $maxTimeMS query option with commands; use maxTimeMS command option "
                 "instead",
-                !legacyCmdObj.hasField("$maxTimeMS"));
-        readPrefContainer = legacyCmdObj;
-        legacyCmdObj = legacyCmdObj.firstElement().Obj().getOwned();
-    } else if (auto queryOptions = legacyCmdObj["$queryOptions"]) {
+                !cmdObj.hasField("$maxTimeMS"));
+
+        if (auto readPref = cmdObj["$readPreference"])
+            readPrefContainer = readPref.wrap();
+
+        cmdObj = cmdObj.firstElement().Obj().shareOwnershipWith(cmdObj);
+    } else if (auto queryOptions = cmdObj["$queryOptions"]) {
         // Mongos rewrites commands with $readPreference to put it in a field nested inside of
         // $queryOptions. Its command implementations often forward commands in that format to
         // shards. This function is responsible for rewriting it to a format that the shards
         // understand.
-        readPrefContainer = queryOptions.Obj().getOwned();
-        legacyCmdObj = legacyCmdObj.removeField("$queryOptions");
+        readPrefContainer = queryOptions.Obj().shareOwnershipWith(cmdObj);
+        cmdObj = cmdObj.removeField("$queryOptions");
     }
 
-    BSONObjBuilder metadataBob;
-    if (auto readPref = readPrefContainer["$readPreference"]) {
-        metadataBob.append(readPref);
-    } else if (queryFlags & QueryOption_SlaveOk) {
-        ReadPreferenceSetting(ReadPreference::SecondaryPreferred).toContainingBSON(&metadataBob);
-    }
-
-    BSONObjBuilder logicalTimeCommandBob;
-    for (auto elem : legacyCmdObj) {
-        if (elem.fieldNameStringData() == LogicalTimeMetadata::fieldName()) {
-            metadataBob.append(elem);
-        } else {
-            logicalTimeCommandBob.append(elem);
-        }
+    if (!readPrefContainer.isEmpty()) {
+        cmdObj = BSONObjBuilder(std::move(cmdObj)).appendElements(readPrefContainer).obj();
+    } else if (!cmdObj.hasField("$readPreference") && (queryFlags & QueryOption_SlaveOk)) {
+        BSONObjBuilder bodyBuilder(std::move(cmdObj));
+        ReadPreferenceSetting(ReadPreference::SecondaryPreferred).toContainingBSON(&bodyBuilder);
+        cmdObj = bodyBuilder.obj();
     }
 
-    return std::make_tuple(logicalTimeCommandBob.obj(), metadataBob.obj());
+    return cmdObj;
 }
 
 }  // namespace rpc
diff --git a/src/mongo/rpc/metadata.h b/src/mongo/rpc/metadata.h
index efacbd7d64a..850cf662ea3 100644
--- a/src/mongo/rpc/metadata.h
+++ b/src/mongo/rpc/metadata.h
@@ -56,21 +56,15 @@ BSONObj makeEmptyMetadata();
 void readRequestMetadata(OperationContext* opCtx, const BSONObj& metadataObj);
 
 /**
- * A command object and a corresponding metadata object.
- */
-using CommandAndMetadata = std::tuple<BSONObj, BSONObj>;
-
-/**
  * A legacy command object and a corresponding query flags bitfield. The legacy command object
  * may contain metadata fields, so it cannot safely be passed to a command's run method.
  */
 using LegacyCommandAndFlags = std::tuple<BSONObj, int>;
 
 /**
- * Given a legacy command object and a query flags bitfield, attempts to parse and remove
- * the metadata from the command object and construct a corresponding metadata object.
+ * Upconverts a legacy command object and a query flags bitfield. Does not add a $db field.
  */
-CommandAndMetadata upconvertRequestMetadata(BSONObj legacyCmdObj, int queryFlags);
+BSONObj upconvertRequest(BSONObj legacyCmdObj, int queryFlags);
 
 /**
  * A function type for writing request metadata. The function takes a pointer to an optional
diff --git a/src/mongo/rpc/metadata/logical_time_metadata_test.cpp b/src/mongo/rpc/metadata/logical_time_metadata_test.cpp
index b6cc15d1166..b5a112dea8e 100644
--- a/src/mongo/rpc/metadata/logical_time_metadata_test.cpp
+++ b/src/mongo/rpc/metadata/logical_time_metadata_test.cpp
@@ -183,9 +183,9 @@ TEST(LogicalTimeMetadataTest, UpconvertPass) {
     auto commandObj = builder.done();
     BSONObjBuilder metadataBob;
     BSONObjBuilder commandBob;
-    auto converted = upconvertRequestMetadata(commandObj, 0);
-    ASSERT_BSONOBJ_EQ(BSON("aaa" << 1 << "bbb" << 1), std::get<0>(converted));
-    ASSERT_BSONOBJ_EQ(BSON("$clusterTime" << logicalTimeMetadata), std::get<1>(converted));
+    auto converted = upconvertRequest(commandObj, 0);
+    ASSERT_BSONOBJ_EQ(BSON("aaa" << 1 << "bbb" << 1 << "$clusterTime" << logicalTimeMetadata),
+                      converted);
 }
 
 }  // namespace rpc
diff --git a/src/mongo/rpc/metadata_test.cpp b/src/mongo/rpc/metadata_test.cpp
index 1b538d4717d..1663fedb64e 100644
--- a/src/mongo/rpc/metadata_test.cpp
+++ b/src/mongo/rpc/metadata_test.cpp
@@ -42,10 +42,9 @@ using mongo::unittest::assertGet;
 
 void checkUpconvert(const BSONObj& legacyCommand,
                     const int legacyQueryFlags,
-                    const BSONObj& upconvertedCommand,
-                    const BSONObj& upconvertedMetadata) {
+                    const BSONObj& upconvertedCommand) {
 
-    auto converted = upconvertRequestMetadata(legacyCommand, legacyQueryFlags);
+    auto converted = upconvertRequest(legacyCommand, legacyQueryFlags);
     // We don't care about the order of the fields in the metadata object
     const auto sorted = [](const BSONObj& obj) {
         BSONObjIteratorSorted iter(obj);
@@ -56,8 +55,7 @@ void checkUpconvert(const BSONObj& legacyCommand,
         return bob.obj();
     };
 
-    ASSERT_BSONOBJ_EQ(upconvertedCommand, std::get<0>(converted));
-    ASSERT_BSONOBJ_EQ(sorted(upconvertedMetadata), sorted(std::get<1>(converted)));
+    ASSERT_BSONOBJ_EQ(sorted(upconvertedCommand), sorted(converted));
 }
 
 TEST(Metadata, UpconvertValidMetadata) {
@@ -67,9 +65,8 @@ TEST(Metadata, UpconvertValidMetadata) {
                                  << BSON("mode"
                                          << "secondary")),
                    mongo::QueryOption_SlaveOk,
-                   BSON("ping" << 1),
-                   BSON("$readPreference" << BSON("mode"
-                                                  << "secondary")));
+                   BSON("ping" << 1 << "$readPreference" << BSON("mode"
+                                                                 << "secondary")));
 
     // Wrapped in 'query', with readPref.
     checkUpconvert(BSON("query" << BSON("pong" << 1 << "foo"
@@ -82,14 +79,15 @@ TEST(Metadata, UpconvertValidMetadata) {
                                                 << "ny"))),
                    0,
                    BSON("pong" << 1 << "foo"
-                               << "bar"),
-                   BSON("$readPreference" << BSON("mode"
-                                                  << "primary"
-                                                  << "tags"
-                                                  << BSON("dc"
-                                                          << "ny"))));
+                               << "bar"
+                               << "$readPreference"
+                               << BSON("mode"
+                                       << "primary"
+                                       << "tags"
+                                       << BSON("dc"
+                                               << "ny"))));
     // Unwrapped, no readPref, no slaveOk
-    checkUpconvert(BSON("ping" << 1), 0, BSON("ping" << 1), BSONObj());
+    checkUpconvert(BSON("ping" << 1), 0, BSON("ping" << 1));
 
     // Readpref wrapped in $queryOptions
     checkUpconvert(BSON("pang"
@@ -102,41 +100,42 @@ TEST(Metadata, UpconvertValidMetadata) {
                                                                   << "city")))),
                    0,
                    BSON("pang"
-                        << "pong"),
-                   BSON("$readPreference" << BSON("mode"
-                                                  << "nearest"
-                                                  << "tags"
-                                                  << BSON("rack"
-                                                          << "city"))));
+                        << "pong"
+                        << "$readPreference"
+                        << BSON("mode"
+                                << "nearest"
+                                << "tags"
+                                << BSON("rack"
+                                        << "city"))));
 }
 
 TEST(Metadata, UpconvertInvalidMetadata) {
     // has $maxTimeMS option
-    ASSERT_THROWS_CODE(upconvertRequestMetadata(BSON("query" << BSON("foo"
-                                                                     << "bar")
-                                                             << "$maxTimeMS"
-                                                             << 200),
-                                                0),
+    ASSERT_THROWS_CODE(upconvertRequest(BSON("query" << BSON("foo"
+                                                             << "bar")
+                                                     << "$maxTimeMS"
+                                                     << 200),
+                                        0),
                        UserException,
                        ErrorCodes::InvalidOptions);
-    ASSERT_THROWS_CODE(upconvertRequestMetadata(BSON("$query" << BSON("foo"
-                                                                      << "bar")
-                                                              << "$maxTimeMS"
-                                                              << 200),
-                                                0),
+    ASSERT_THROWS_CODE(upconvertRequest(BSON("$query" << BSON("foo"
+                                                              << "bar")
+                                                      << "$maxTimeMS"
+                                                      << 200),
+                                        0),
                        UserException,
                        ErrorCodes::InvalidOptions);
 
     // invalid wrapped query
-    ASSERT_THROWS(upconvertRequestMetadata(BSON("$query" << 1), 0), UserException);
-    ASSERT_THROWS(upconvertRequestMetadata(BSON("$query"
-                                                << ""),
-                                           0),
+    ASSERT_THROWS(upconvertRequest(BSON("$query" << 1), 0), UserException);
+    ASSERT_THROWS(upconvertRequest(BSON("$query"
+                                        << ""),
+                                   0),
                   UserException);
-    ASSERT_THROWS(upconvertRequestMetadata(BSON("query" << 1), 0), UserException);
-    ASSERT_THROWS(upconvertRequestMetadata(BSON("query"
-                                                << ""),
-                                           0),
+    ASSERT_THROWS(upconvertRequest(BSON("query" << 1), 0), UserException);
+    ASSERT_THROWS(upconvertRequest(BSON("query"
+                                        << ""),
+                                   0),
                   UserException);
 }