SERVER-27438 Prevent mongos from dropping legacy $comment meta-operator

Closes #1135

Signed-off-by: James Wahlin <james.wahlin@10gen.com>

6 files changed, 149 insertions, 2 deletions

diff --git a/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml b/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
index 1b7f628dad0..d00fc7de418 100644
--- a/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
+++ b/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
@@ -9,6 +9,8 @@ selector:
     - jstests/sharding/printShardingStatus.js
     # Behavior change to addShard
     - jstests/sharding/addshard_idempotent.js
+    # SERVER-27438: Preserves $comment in OP_QUERY and tests for this change.
+    - jstests/sharding/mongos_query_comment.js
 
 executor:
   js_test:
diff --git a/jstests/libs/profiler.js b/jstests/libs/profiler.js
index b7c4a3f89e7..c0fc210c928 100644
--- a/jstests/libs/profiler.js
+++ b/jstests/libs/profiler.js
@@ -14,4 +14,13 @@ function getProfilerProtocolStringForCommand(conn) {
     }
 
     return "op_command";
+}
+
+// Throws an assertion if the profiler does not contain exactly one entry matching <filter>.
+// Optional arguments <errorMsgFilter> and <errorMsgProj> limit profiler output if this asserts.
+function profilerHasSingleMatchingEntryOrThrow(inputDb, filter, errorMsgFilter, errorMsgProj) {
+    assert.eq(inputDb.system.profile.find(filter).itcount(),
+              1,
+              "Expected exactly one op matching: " + tojson(filter) + " in profiler " +
+                  tojson(inputDb.system.profile.find(errorMsgFilter, errorMsgProj).toArray()));
 }
\ No newline at end of file
diff --git a/jstests/sharding/mongos_query_comment.js b/jstests/sharding/mongos_query_comment.js
new file mode 100644
index 00000000000..af035c5de30
--- /dev/null
+++ b/jstests/sharding/mongos_query_comment.js
@@ -0,0 +1,79 @@
+/**
+ * Test that a legacy query via mongos retains the $comment query meta-operator when transformed
+ * into a find command for the shards. In addition, verify that the find command comment parameter
+ * and query operator are passed to the shards correctly, and that an attempt to attach a non-string
+ * comment to the find command fails.
+ */
+(function() {
+    "use strict";
+
+    // For profilerHasSingleMatchingEntryOrThrow.
+    load("jstests/libs/profiler.js");
+
+    const st = new ShardingTest({name: "mongos_comment_test", mongos: 1, shards: 1});
+
+    const shard = st.shard0;
+    const mongos = st.s;
+
+    // Need references to the database via both mongos and mongod so that we can enable profiling &
+    // test queries on the shard.
+    const mongosDB = mongos.getDB("mongos_comment");
+    const shardDB = shard.getDB("mongos_comment");
+
+    assert.commandWorked(mongosDB.dropDatabase());
+
+    const mongosColl = mongosDB.test;
+    const shardColl = shardDB.test;
+
+    const collNS = mongosColl.getFullName();
+
+    for (let i = 0; i < 5; ++i) {
+        assert.writeOK(mongosColl.insert({_id: i, a: i}));
+    }
+
+    // The profiler will be used to verify that comments are present on the shard.
+    assert.commandWorked(shardDB.setProfilingLevel(2));
+    const profiler = shardDB.system.profile;
+
+    //
+    // Set legacy read mode for the mongos and shard connections.
+    //
+    mongosDB.getMongo().forceReadMode("legacy");
+    shardDB.getMongo().forceReadMode("legacy");
+
+    // TEST CASE: A legacy string $comment meta-operator is propagated to the shards via mongos.
+    assert.eq(mongosColl.find({$query: {a: 1}, $comment: "TEST"}).itcount(), 1);
+    profilerHasSingleMatchingEntryOrThrow(shardDB,
+                                          {op: "query", ns: collNS, "query.comment": "TEST"});
+
+    // TEST CASE: A legacy BSONObj $comment is converted to a string and propagated via mongos.
+    assert.eq(mongosColl.find({$query: {a: 1}, $comment: {c: 2, d: {e: "TEST"}}}).itcount(), 1);
+    profilerHasSingleMatchingEntryOrThrow(
+        shardDB, {op: "query", ns: collNS, "query.comment": "{ c: 2.0, d: { e: \"TEST\" } }"});
+
+    // TEST CASE: Legacy BSONObj $comment is NOT converted to a string when issued on the mongod.
+    assert.eq(shardColl.find({$query: {a: 1}, $comment: {c: 3, d: {e: "TEST"}}}).itcount(), 1);
+    profilerHasSingleMatchingEntryOrThrow(
+        shardDB, {op: "query", ns: collNS, "query.comment": {c: 3, d: {e: "TEST"}}});
+
+    //
+    // Revert to "commands" read mode for the find command test cases below.
+    //
+    mongosDB.getMongo().forceReadMode("commands");
+    shardDB.getMongo().forceReadMode("commands");
+
+    // TEST CASE: Verify that string find.comment and non-string find.filter.$comment propagate.
+    assert.eq(mongosColl.find({a: 1, $comment: {b: "TEST"}}).comment("TEST").itcount(), 1);
+    profilerHasSingleMatchingEntryOrThrow(
+        shardDB,
+        {op: "query", ns: collNS, "query.comment": "TEST", "query.filter.$comment": {b: "TEST"}});
+
+    // TEST CASE: Verify that find command with a non-string comment parameter is rejected.
+    assert.commandFailedWithCode(
+        mongosDB.runCommand(
+            {"find": mongosColl.getName(), "filter": {a: 1}, "comment": {b: "TEST"}}),
+        9,
+        "Non-string find command comment did not return an error.");
+
+    st.stop();
+})();
\ No newline at end of file
diff --git a/src/mongo/db/query/query_request.cpp b/src/mongo/db/query/query_request.cpp
index 821282f78eb..b0f1334de8d 100644
--- a/src/mongo/db/query/query_request.cpp
+++ b/src/mongo/db/query/query_request.cpp
@@ -37,6 +37,7 @@
 #include "mongo/db/dbmessage.h"
 #include "mongo/db/namespace_string.h"
 #include "mongo/db/repl/read_concern_args.h"
+#include "mongo/stdx/memory.h"
 #include "mongo/util/assert_util.h"
 #include "mongo/util/mongoutils/str.h"
 
@@ -113,7 +114,7 @@ QueryRequest::QueryRequest(NamespaceString nss) : _nss(std::move(nss)) {}
 StatusWith<unique_ptr<QueryRequest>> QueryRequest::makeFromFindCommand(NamespaceString nss,
                                                                        const BSONObj& cmdObj,
                                                                        bool isExplain) {
-    unique_ptr<QueryRequest> qr(new QueryRequest(std::move(nss)));
+    auto qr = stdx::make_unique<QueryRequest>(nss);
     qr->_explain = isExplain;
 
     // Parse the command BSON by looping through one element at a time.
@@ -702,7 +703,7 @@ bool QueryRequest::isQueryIsolated(const BSONObj& query) {
 
 // static
 StatusWith<unique_ptr<QueryRequest>> QueryRequest::fromLegacyQueryMessage(const QueryMessage& qm) {
-    unique_ptr<QueryRequest> qr(new QueryRequest(NamespaceString(qm.ns)));
+    auto qr = stdx::make_unique<QueryRequest>(NamespaceString(qm.ns));
 
     Status status = qr->init(qm.ntoskip, qm.ntoreturn, qm.queryOptions, qm.query, qm.fields, true);
     if (!status.isOK()) {
@@ -712,6 +713,22 @@ StatusWith<unique_ptr<QueryRequest>> QueryRequest::fromLegacyQueryMessage(const
     return std::move(qr);
 }
 
+StatusWith<unique_ptr<QueryRequest>> QueryRequest::fromLegacyQueryForTest(NamespaceString nss,
+                                                                          const BSONObj& queryObj,
+                                                                          const BSONObj& proj,
+                                                                          int ntoskip,
+                                                                          int ntoreturn,
+                                                                          int queryOptions) {
+    auto qr = stdx::make_unique<QueryRequest>(nss);
+
+    Status status = qr->init(ntoskip, ntoreturn, queryOptions, queryObj, proj, true);
+    if (!status.isOK()) {
+        return status;
+    }
+
+    return std::move(qr);
+}
+
 Status QueryRequest::init(int ntoskip,
                           int ntoreturn,
                           int queryOptions,
@@ -862,6 +879,13 @@ Status QueryRequest::initFullQuery(const BSONObj& top) {
                     return maxTimeMS.getStatus();
                 }
                 _maxTimeMS = maxTimeMS.getValue();
+            } else if (str::equals("comment", name)) {
+                // Legacy $comment can be any BSON element. Convert to string if it isn't already.
+                if (e.type() == BSONType::String) {
+                    _comment = e.str();
+                } else {
+                    _comment = e.toString(false);
+                }
             }
         }
     }
diff --git a/src/mongo/db/query/query_request.h b/src/mongo/db/query/query_request.h
index 01d356a5837..376d5ece98c 100644
--- a/src/mongo/db/query/query_request.h
+++ b/src/mongo/db/query/query_request.h
@@ -393,6 +393,16 @@ public:
      */
     static StatusWith<std::unique_ptr<QueryRequest>> fromLegacyQueryMessage(const QueryMessage& qm);
 
+    /**
+     * Parse the provided legacy query object and parameters to construct a QueryRequest.
+     */
+    static StatusWith<std::unique_ptr<QueryRequest>> fromLegacyQueryForTest(NamespaceString nss,
+                                                                            const BSONObj& queryObj,
+                                                                            const BSONObj& proj,
+                                                                            int ntoskip,
+                                                                            int ntoreturn,
+                                                                            int queryOptions);
+
 private:
     Status init(int ntoskip,
                 int ntoreturn,
diff --git a/src/mongo/db/query/query_request_test.cpp b/src/mongo/db/query/query_request_test.cpp
index b8e45d810e4..bd40a94ef33 100644
--- a/src/mongo/db/query/query_request_test.cpp
+++ b/src/mongo/db/query/query_request_test.cpp
@@ -1304,5 +1304,28 @@ TEST(QueryRequestTest, ConvertToAggregationWithCollationSucceeds) {
     ASSERT_BSONOBJ_EQ(ar.getValue().getCollation(), BSON("f" << 1));
 }
 
+TEST(QueryRequestTest, ParseFromLegacyObjMetaOpComment) {
+    BSONObj queryObj = fromjson(
+        "{$query: {a: 1},"
+        "$comment: {b: 2, c: {d: 'ParseFromLegacyObjMetaOpComment'}}}");
+    const NamespaceString nss("test.testns");
+    unique_ptr<QueryRequest> qr(
+        assertGet(QueryRequest::fromLegacyQueryForTest(nss, queryObj, BSONObj(), 0, 0, 0)));
+
+    // Ensure that legacy comment meta-operator is parsed to a string comment
+    ASSERT_EQ(qr->getComment(), "{ b: 2, c: { d: \"ParseFromLegacyObjMetaOpComment\" } }");
+}
+
+TEST(QueryRequestTest, ParseFromLegacyStringMetaOpComment) {
+    BSONObj queryObj = fromjson(
+        "{$query: {a: 1},"
+        "$comment: 'ParseFromLegacyStringMetaOpComment'}");
+    const NamespaceString nss("test.testns");
+    unique_ptr<QueryRequest> qr(
+        assertGet(QueryRequest::fromLegacyQueryForTest(nss, queryObj, BSONObj(), 0, 0, 0)));
+
+    ASSERT_EQ(qr->getComment(), "ParseFromLegacyStringMetaOpComment");
+}
+
 }  // namespace mongo
 }  // namespace