diff options
Diffstat (limited to 'src/mongo')
31 files changed, 121 insertions, 224 deletions
diff --git a/src/mongo/db/command_generic_argument.cpp b/src/mongo/db/command_generic_argument.cpp index 15f0a7d8227..5969b9e1e0f 100644 --- a/src/mongo/db/command_generic_argument.cpp +++ b/src/mongo/db/command_generic_argument.cpp @@ -52,7 +52,7 @@ struct SpecialArgRecord { // If that changes, it should be added. When you add to this list, consider whether you // should also change the filterCommandRequestForPassthrough() function. // clang-format off -static constexpr std::array<SpecialArgRecord, 26> specials{{ +static constexpr std::array<SpecialArgRecord, 27> specials{{ // /-isGeneric // | /-stripFromRequest // | | /-stripFromReply @@ -81,7 +81,8 @@ static constexpr std::array<SpecialArgRecord, 26> specials{{ {"$gleStats"_sd, 0, 0, 1}, {"operationTime"_sd, 0, 0, 1}, {"lastCommittedOpTime"_sd, 0, 0, 1}, - {"readOnly"_sd, 0, 0, 1}}}; + {"readOnly"_sd, 0, 0, 1}, + {"comment"_sd, 1, 0, 0}}}; // clang-format on template <bool SpecialArgRecord::*pmo> diff --git a/src/mongo/db/commands/explain_cmd.cpp b/src/mongo/db/commands/explain_cmd.cpp index 277fb80b3c7..78fa4d67217 100644 --- a/src/mongo/db/commands/explain_cmd.cpp +++ b/src/mongo/db/commands/explain_cmd.cpp @@ -150,6 +150,13 @@ std::unique_ptr<CommandInvocation> CmdExplain::parse(OperationContext* opCtx, "explain command requires a nested object", cmdObj.firstElement().type() == Object); auto explainedObj = cmdObj.firstElement().Obj(); + + // Extract 'comment' field from the 'explainedObj' only if there is no top-level comment. + auto commentField = explainedObj["comment"]; + if (!opCtx->getComment() && commentField) { + opCtx->setComment(commentField.wrap()); + } + if (auto innerDb = explainedObj["$db"]) { uassert(ErrorCodes::InvalidNamespace, str::stream() << "Mismatched $db in explain command. Expected " << dbname diff --git a/src/mongo/db/commands/find_cmd.cpp b/src/mongo/db/commands/find_cmd.cpp index 1694fe28f97..4800ed178d9 100644 --- a/src/mongo/db/commands/find_cmd.cpp +++ b/src/mongo/db/commands/find_cmd.cpp @@ -101,9 +101,8 @@ boost::intrusive_ptr<ExpressionContext> makeExpressionContext(OperationContext* auto expCtx = make_intrusive<ExpressionContext>(opCtx, boost::none, // explain - StringData{queryRequest.getComment()}, - false, // fromMongos - false, // needsMerge + false, // fromMongos + false, // needsMerge queryRequest.allowDiskUse(), false, // bypassDocumentValidation queryRequest.nss(), diff --git a/src/mongo/db/commands/getmore_cmd.cpp b/src/mongo/db/commands/getmore_cmd.cpp index cceb0f15fa8..345ac9f0049 100644 --- a/src/mongo/db/commands/getmore_cmd.cpp +++ b/src/mongo/db/commands/getmore_cmd.cpp @@ -200,6 +200,13 @@ void setUpOperationContextStateForGetMore(OperationContext* opCtx, applyCursorReadConcern(opCtx, cursor.getReadConcernArgs()); opCtx->setWriteConcern(cursor.getWriteConcernOptions()); setUpOperationDeadline(opCtx, cursor, request, disableAwaitDataFailpointActive); + + // If the originating command had a 'comment' field, we extract it and set it on opCtx. Note + // that if the 'getMore' command itself has a 'comment' field, we give precedence to it. + auto comment = cursor.getOriginatingCommandObj()["comment"]; + if (!opCtx->getComment() && comment) { + opCtx->setComment(comment.wrap()); + } } /** diff --git a/src/mongo/db/commands/map_reduce_agg.cpp b/src/mongo/db/commands/map_reduce_agg.cpp index 774237627fa..c8ede42d18d 100644 --- a/src/mongo/db/commands/map_reduce_agg.cpp +++ b/src/mongo/db/commands/map_reduce_agg.cpp @@ -211,11 +211,10 @@ auto makeExpressionContext(OperationContext* opCtx, const MapReduce& parsedMr) { // the $group stage of the translated pipeline to spill to disk. return make_intrusive<ExpressionContext>( opCtx, - boost::none, // explain - std::string{}, // comment - false, // fromMongos - false, // needsmerge - true, // allowDiskUse + boost::none, // explain + false, // fromMongos + false, // needsmerge + true, // allowDiskUse parsedMr.getBypassDocumentValidation().get_value_or(false), parsedMr.getNamespace(), parsedMr.getCollation().get_value_or(BSONObj()), diff --git a/src/mongo/db/curop.cpp b/src/mongo/db/curop.cpp index 945dbfc9bdb..cf08c38548d 100644 --- a/src/mongo/db/curop.cpp +++ b/src/mongo/db/curop.cpp @@ -302,7 +302,7 @@ void CurOp::reportCurrentOpForClient(OperationContext* opCtx, lsid->serialize(&lsidBuilder); } - CurOp::get(clientOpCtx)->reportState(infoBuilder, truncateOps); + CurOp::get(clientOpCtx)->reportState(clientOpCtx, infoBuilder, truncateOps); } if (auto diagnostic = DiagnosticInfo::get(*client)) { @@ -458,11 +458,7 @@ bool CurOp::completeAndLogOperation(OperationContext* opCtx, // Gets the time spent blocked on prepare conflicts. _debug.prepareConflictDurationMicros = durationCount<Microseconds>( PrepareConflictTracker::get(opCtx).getPrepareConflictDuration()); - - log(component) << _debug.report(client, - *this, - (lockerInfo ? &lockerInfo->stats : nullptr), - opCtx->lockState()->getFlowControlStats()); + log(component) << _debug.report(opCtx, (lockerInfo ? &lockerInfo->stats : nullptr)); } // Return 'true' if this operation should also be added to the profiler. @@ -488,6 +484,11 @@ Command::ReadWriteType CurOp::getReadWriteType() const { namespace { +BSONObj appendCommentField(OperationContext* opCtx, const BSONObj& cmdObj) { + return opCtx->getComment() && !cmdObj["comment"] ? cmdObj.addField(*opCtx->getComment()) + : cmdObj; +} + /** * Appends {<name>: obj} to the provided builder. If obj is greater than maxSize, appends a string * summary of obj as { <name>: { $truncated: "obj" } }. If a comment parameter is present, add it to @@ -561,7 +562,7 @@ BSONObj CurOp::truncateAndSerializeGenericCursor(GenericCursor* cursor, return serialized; } -void CurOp::reportState(BSONObjBuilder* builder, bool truncateOps) { +void CurOp::reportState(OperationContext* opCtx, BSONObjBuilder* builder, bool truncateOps) { if (_start) { builder->append("secs_running", durationCount<Seconds>(elapsedTimeTotal())); builder->append("microsecs_running", durationCount<Microseconds>(elapsedTimeTotal())); @@ -576,7 +577,9 @@ void CurOp::reportState(BSONObjBuilder* builder, bool truncateOps) { // is true, limit the size of each op to 1000 bytes. Otherwise, do not truncate. const boost::optional<size_t> maxQuerySize{truncateOps, 1000}; - appendAsObjOrString("command", _opDescription, maxQuerySize, builder); + appendAsObjOrString( + "command", appendCommentField(opCtx, _opDescription), maxQuerySize, builder); + if (!_planSummary.empty()) { builder->append("planSummary", _planSummary); @@ -635,10 +638,10 @@ StringData getProtoString(int op) { if (y) \ s << " " x ":" << (*y) -string OpDebug::report(Client* client, - const CurOp& curop, - const SingleThreadedLockStats* lockStats, - FlowControlTicketholder::CurOp flowControlStats) const { +string OpDebug::report(OperationContext* opCtx, const SingleThreadedLockStats* lockStats) const { + Client* client = opCtx->getClient(); + auto& curop = *CurOp::get(opCtx); + auto flowControlStats = opCtx->lockState()->getFlowControlStats(); StringBuilder s; if (iscommand) s << "command "; @@ -655,7 +658,7 @@ string OpDebug::report(Client* client, } } - auto query = curop.opDescription(); + auto query = appendCommentField(opCtx, curop.opDescription()); if (!query.isEmpty()) { s << " command: "; if (iscommand) { @@ -776,10 +779,11 @@ string OpDebug::report(Client* client, if (y) \ b.appendNumber(x, (*y)) -void OpDebug::append(const CurOp& curop, +void OpDebug::append(OperationContext* opCtx, const SingleThreadedLockStats& lockStats, FlowControlTicketholder::CurOp flowControlStats, BSONObjBuilder& b) const { + auto& curop = *CurOp::get(opCtx); const size_t maxElementSize = 50 * 1024; b.append("op", logicalOpToString(logicalOp)); @@ -787,7 +791,8 @@ void OpDebug::append(const CurOp& curop, NamespaceString nss = NamespaceString(curop.getNS()); b.append("ns", nss.ns()); - appendAsObjOrString("command", curop.opDescription(), maxElementSize, &b); + appendAsObjOrString( + "command", appendCommentField(opCtx, curop.opDescription()), maxElementSize, &b); auto originatingCommand = curop.originatingCommand(); if (!originatingCommand.isEmpty()) { diff --git a/src/mongo/db/curop.h b/src/mongo/db/curop.h index 0019bbb85ad..7cb2395df5f 100644 --- a/src/mongo/db/curop.h +++ b/src/mongo/db/curop.h @@ -143,10 +143,7 @@ public: OpDebug() = default; - std::string report(Client* client, - const CurOp& curop, - const SingleThreadedLockStats* lockStats, - FlowControlTicketholder::CurOp flowControlStats) const; + std::string report(OperationContext* opCtx, const SingleThreadedLockStats* lockStats) const; /** * Appends information about the current operation to "builder" @@ -154,7 +151,7 @@ public: * @param curop reference to the CurOp that owns this OpDebug * @param lockStats lockStats object containing locking information about the operation */ - void append(const CurOp& curop, + void append(OperationContext* opCtx, const SingleThreadedLockStats& lockStats, FlowControlTicketholder::CurOp flowControlStats, BSONObjBuilder& builder) const; @@ -553,7 +550,7 @@ public: * If called from a thread other than the one executing the operation associated with this * CurOp, it is necessary to lock the associated Client object before executing this method. */ - void reportState(BSONObjBuilder* builder, bool truncateOps = false); + void reportState(OperationContext* opCtx, BSONObjBuilder* builder, bool truncateOps = false); /** * Sets the message for this CurOp. diff --git a/src/mongo/db/curop_test.cpp b/src/mongo/db/curop_test.cpp index abda927f3c8..84d3db43edf 100644 --- a/src/mongo/db/curop_test.cpp +++ b/src/mongo/db/curop_test.cpp @@ -201,7 +201,7 @@ TEST(CurOpTest, OptionalAdditiveMetricsNotDisplayedIfUninitialized) { opCtx.get(), NamespaceString("myDb.coll"), nullptr, command, NetworkOp::dbQuery); BSONObjBuilder builder; - od.append(*curop, ls, {}, builder); + od.append(opCtx.get(), ls, {}, builder); auto bs = builder.done(); // Append should always include these basic fields. @@ -213,7 +213,7 @@ TEST(CurOpTest, OptionalAdditiveMetricsNotDisplayedIfUninitialized) { ASSERT_EQ(static_cast<size_t>(bs.nFields()), basicFields.size()); // 'reportString' should only contain basic fields. - std::string reportString = od.report(serviceContext.getClient(), *curop, nullptr, {}); + std::string reportString = od.report(opCtx.get(), nullptr); std::string expectedReportString = "query myDb.coll command: { a: 3 } numYields:0 0ms"; ASSERT_EQ(reportString, expectedReportString); diff --git a/src/mongo/db/introspect.cpp b/src/mongo/db/introspect.cpp index 52c1435e4a9..114a39305d5 100644 --- a/src/mongo/db/introspect.cpp +++ b/src/mongo/db/introspect.cpp @@ -93,7 +93,7 @@ void profile(OperationContext* opCtx, NetworkOp op) { Locker::LockerInfo lockerInfo; opCtx->lockState()->getLockerInfo(&lockerInfo, CurOp::get(opCtx)->getLockStatsBase()); CurOp::get(opCtx)->debug().append( - *CurOp::get(opCtx), lockerInfo.stats, opCtx->lockState()->getFlowControlStats(), b); + opCtx, lockerInfo.stats, opCtx->lockState()->getFlowControlStats(), b); } b.appendDate("ts", jsTime()); diff --git a/src/mongo/db/operation_context.h b/src/mongo/db/operation_context.h index a834f89ae1d..43553efae9b 100644 --- a/src/mongo/db/operation_context.h +++ b/src/mongo/db/operation_context.h @@ -373,6 +373,15 @@ public: _inMultiDocumentTransaction = true; } + void setComment(const BSONObj& comment) { + _comment = comment.getOwned(); + } + + boost::optional<BSONElement> getComment() { + // The '_comment' object, if present, will only ever have one field. + return _comment ? boost::optional<BSONElement>(_comment->firstElement()) : boost::none; + } + private: IgnoreInterruptsState pushIgnoreInterrupts() override { IgnoreInterruptsState iis{_ignoreInterrupts, @@ -498,6 +507,10 @@ private: bool _writesAreReplicated = true; bool _shouldParticipateInFlowControl = true; bool _inMultiDocumentTransaction = false; + + // If populated, this is an owned singleton BSONObj whose only field, 'comment', is a copy of + // the 'comment' field from the input command object. + boost::optional<BSONObj> _comment; }; namespace repl { diff --git a/src/mongo/db/pipeline/aggregation_request.cpp b/src/mongo/db/pipeline/aggregation_request.cpp index 04c806bb992..8b0aa1c4cb7 100644 --- a/src/mongo/db/pipeline/aggregation_request.cpp +++ b/src/mongo/db/pipeline/aggregation_request.cpp @@ -58,7 +58,6 @@ constexpr StringData AggregationRequest::kCollationName; constexpr StringData AggregationRequest::kExplainName; constexpr StringData AggregationRequest::kAllowDiskUseName; constexpr StringData AggregationRequest::kHintName; -constexpr StringData AggregationRequest::kCommentName; constexpr StringData AggregationRequest::kExchangeName; constexpr long long AggregationRequest::kDefaultBatchSize; @@ -145,13 +144,6 @@ StatusWith<AggregationRequest> AggregationRequest::parseFromBSON( << " must be specified as a string representing an index" << " name, or an object representing an index's key pattern"); } - } else if (kCommentName == fieldName) { - if (elem.type() != BSONType::String) { - return {ErrorCodes::TypeMismatch, - str::stream() << kCommentName << " must be a string, not a " - << typeName(elem.type())}; - } - request.setComment(elem.str()); } else if (kExplainName == fieldName) { if (elem.type() != BSONType::Bool) { return {ErrorCodes::TypeMismatch, @@ -308,8 +300,6 @@ Document AggregationRequest::serializeToCommandObj() const { _explainMode ? Value(Document()) : Value(Document{{kBatchSizeName, _batchSize}})}, // Only serialize a hint if one was specified. {kHintName, _hint.isEmpty() ? Value() : Value(_hint)}, - // Only serialize a comment if one was specified. - {kCommentName, _comment.empty() ? Value() : Value(_comment)}, // Only serialize readConcern if specified. {repl::ReadConcernArgs::kReadConcernFieldName, _readConcern.isEmpty() ? Value() : Value(_readConcern)}, diff --git a/src/mongo/db/pipeline/aggregation_request.h b/src/mongo/db/pipeline/aggregation_request.h index 99550df8785..053797eb389 100644 --- a/src/mongo/db/pipeline/aggregation_request.h +++ b/src/mongo/db/pipeline/aggregation_request.h @@ -61,7 +61,6 @@ public: static constexpr StringData kExplainName = "explain"_sd; static constexpr StringData kAllowDiskUseName = "allowDiskUse"_sd; static constexpr StringData kHintName = "hint"_sd; - static constexpr StringData kCommentName = "comment"_sd; static constexpr StringData kExchangeName = "exchange"_sd; static constexpr StringData kRuntimeConstants = "runtimeConstants"_sd; @@ -189,10 +188,6 @@ public: return _hint; } - const std::string& getComment() const { - return _comment; - } - boost::optional<ExplainOptions::Verbosity> getExplain() const { return _explainMode; } @@ -241,10 +236,6 @@ public: _hint = hint.getOwned(); } - void setComment(const std::string& comment) { - _comment = comment; - } - void setExplain(boost::optional<ExplainOptions::Verbosity> verbosity) { _explainMode = verbosity; } @@ -309,9 +300,6 @@ private: // {$hint: <String>}, where <String> is the index name hinted. BSONObj _hint; - // The comment parameter attached to this aggregation, empty if not set. - std::string _comment; - BSONObj _readConcern; // The unwrapped readPreference object, if one was given to us by the mongos command processor. diff --git a/src/mongo/db/pipeline/aggregation_request_test.cpp b/src/mongo/db/pipeline/aggregation_request_test.cpp index 10da16a2a85..55b0992ec8f 100644 --- a/src/mongo/db/pipeline/aggregation_request_test.cpp +++ b/src/mongo/db/pipeline/aggregation_request_test.cpp @@ -60,7 +60,7 @@ TEST(AggregationRequestTest, ShouldParseAllKnownOptions) { "{pipeline: [{$match: {a: 'abc'}}], explain: false, allowDiskUse: true, fromMongos: true, " "needsMerge: true, bypassDocumentValidation: true, collation: {locale: 'en_US'}, cursor: " "{batchSize: 10}, hint: {a: 1}, maxTimeMS: 100, readConcern: {level: 'linearizable'}, " - "$queryOptions: {$readPreference: 'nearest'}, comment: 'agg_comment', exchange: {policy: " + "$queryOptions: {$readPreference: 'nearest'}, exchange: {policy: " "'roundrobin', consumers:NumberInt(2)}}"); auto request = unittest::assertGet(AggregationRequest::parseFromBSON(nss, inputBson)); ASSERT_FALSE(request.getExplain()); @@ -70,7 +70,6 @@ TEST(AggregationRequestTest, ShouldParseAllKnownOptions) { ASSERT_TRUE(request.shouldBypassDocumentValidation()); ASSERT_EQ(request.getBatchSize(), 10); ASSERT_BSONOBJ_EQ(request.getHint(), BSON("a" << 1)); - ASSERT_EQ(request.getComment(), "agg_comment"); ASSERT_BSONOBJ_EQ(request.getCollation(), BSON("locale" << "en_US")); @@ -157,7 +156,6 @@ TEST(AggregationRequestTest, ShouldNotSerializeOptionalValuesIfEquivalentToDefau request.setBypassDocumentValidation(false); request.setCollation(BSONObj()); request.setHint(BSONObj()); - request.setComment(""); request.setMaxTimeMS(0u); request.setUnwrappedReadPref(BSONObj()); request.setReadConcern(BSONObj()); @@ -180,8 +178,6 @@ TEST(AggregationRequestTest, ShouldSerializeOptionalValuesIfSet) { request.setMaxTimeMS(10u); const auto hintObj = BSON("a" << 1); request.setHint(hintObj); - const auto comment = std::string("agg_comment"); - request.setComment(comment); const auto collationObj = BSON("locale" << "en_US"); request.setCollation(collationObj); @@ -203,7 +199,6 @@ TEST(AggregationRequestTest, ShouldSerializeOptionalValuesIfSet) { {AggregationRequest::kCursorName, Value(Document({{AggregationRequest::kBatchSizeName, 10}}))}, {AggregationRequest::kHintName, hintObj}, - {AggregationRequest::kCommentName, comment}, {repl::ReadConcernArgs::kReadConcernFieldName, readConcernObj}, {QueryRequest::kUnwrappedReadPrefField, readPrefObj}, {QueryRequest::cmdOptionMaxTimeMS, 10}}; @@ -311,14 +306,6 @@ TEST(AggregationRequestTest, ShouldRejectHintAsArray) { AggregationRequest::parseFromBSON(NamespaceString("a.collection"), inputBson).getStatus()); } -TEST(AggregationRequestTest, ShouldRejectNonStringComment) { - NamespaceString nss("a.collection"); - const BSONObj inputBson = - fromjson("{pipeline: [{$match: {a: 'abc'}}], cursor: {}, comment: 1}"); - ASSERT_NOT_OK( - AggregationRequest::parseFromBSON(NamespaceString("a.collection"), inputBson).getStatus()); -} - TEST(AggregationRequestTest, ShouldRejectExplainIfNumber) { NamespaceString nss("a.collection"); const BSONObj inputBson = diff --git a/src/mongo/db/pipeline/expression_context.cpp b/src/mongo/db/pipeline/expression_context.cpp index 489caa64725..e281178ab3e 100644 --- a/src/mongo/db/pipeline/expression_context.cpp +++ b/src/mongo/db/pipeline/expression_context.cpp @@ -53,7 +53,6 @@ ExpressionContext::ExpressionContext(OperationContext* opCtx, boost::optional<UUID> collUUID) : ExpressionContext(opCtx, request.getExplain(), - request.getComment(), request.isFromMongos(), request.needsMerge(), request.shouldAllowDiskUse(), @@ -69,7 +68,6 @@ ExpressionContext::ExpressionContext(OperationContext* opCtx, ExpressionContext::ExpressionContext( OperationContext* opCtx, const boost::optional<ExplainOptions::Verbosity>& explain, - StringData comment, bool fromMongos, bool needsMerge, bool allowDiskUse, @@ -82,7 +80,6 @@ ExpressionContext::ExpressionContext( StringMap<ExpressionContext::ResolvedNamespace> resolvedNamespaces, boost::optional<UUID> collUUID) : explain(explain), - comment(comment), fromMongos(fromMongos), needsMerge(needsMerge), allowDiskUse(allowDiskUse), @@ -192,7 +189,6 @@ intrusive_ptr<ExpressionContext> ExpressionContext::copyWith( auto expCtx = make_intrusive<ExpressionContext>(opCtx, explain, - comment, fromMongos, needsMerge, allowDiskUse, diff --git a/src/mongo/db/pipeline/expression_context.h b/src/mongo/db/pipeline/expression_context.h index 43e9b5a7e37..dbac88c28e1 100644 --- a/src/mongo/db/pipeline/expression_context.h +++ b/src/mongo/db/pipeline/expression_context.h @@ -116,7 +116,6 @@ public: */ ExpressionContext(OperationContext* opCtx, const boost::optional<ExplainOptions::Verbosity>& explain, - StringData comment, bool fromMongos, bool needsmerge, bool allowDiskUse, @@ -235,9 +234,6 @@ public: // The explain verbosity requested by the user, or boost::none if no explain was requested. boost::optional<ExplainOptions::Verbosity> explain; - // The comment provided by the user, or the empty string if no comment was provided. - std::string comment; - bool fromMongos = false; bool needsMerge = false; bool inMongos = false; diff --git a/src/mongo/db/pipeline/expression_context_for_test.h b/src/mongo/db/pipeline/expression_context_for_test.h index 8a204fa7039..b9f891b1603 100644 --- a/src/mongo/db/pipeline/expression_context_for_test.h +++ b/src/mongo/db/pipeline/expression_context_for_test.h @@ -54,7 +54,6 @@ public: ExpressionContextForTest(NamespaceString nss) : ExpressionContext(nullptr, // opCtx, nullptr while base class is constructed. boost::none, // explain - "", // comment false, // fromMongos, false, // needsMerge, false, // allowDiskUse, diff --git a/src/mongo/db/pipeline/mongos_process_interface.cpp b/src/mongo/db/pipeline/mongos_process_interface.cpp index cec0279587e..742583bc94b 100644 --- a/src/mongo/db/pipeline/mongos_process_interface.cpp +++ b/src/mongo/db/pipeline/mongos_process_interface.cpp @@ -163,7 +163,6 @@ boost::optional<Document> MongoSInterface::lookupSingleDocument( cmdBuilder.append("find", nss.coll()); } cmdBuilder.append("filter", filterObj); - cmdBuilder.append("comment", expCtx->comment); if (readConcern) { cmdBuilder.append(repl::ReadConcernArgs::kReadConcernFieldName, *readConcern); } diff --git a/src/mongo/db/query/count_command_test.cpp b/src/mongo/db/query/count_command_test.cpp index b7ea431f678..ca1bde54ee0 100644 --- a/src/mongo/db/query/count_command_test.cpp +++ b/src/mongo/db/query/count_command_test.cpp @@ -246,28 +246,6 @@ TEST(CountCommandTest, ConvertToAggregationWithQueryOptions) { SimpleBSONObjComparator::kInstance.makeEqualTo())); } -TEST(CountCommandTest, ConvertToAggregationWithComment) { - auto countCmd = CountCommand::parse(ctxt, - BSON("count" - << "TestColl" - << "$db" - << "TestDB" - << "comment" - << "aComment")); - auto agg = uassertStatusOK(countCommandAsAggregationCommand(countCmd, testns)); - - auto ar = uassertStatusOK(AggregationRequest::parseFromBSON(testns, agg)); - ASSERT_EQ(ar.getComment(), "aComment"); - - std::vector<BSONObj> expectedPipeline{BSON("$count" - << "count")}; - ASSERT(std::equal(expectedPipeline.begin(), - expectedPipeline.end(), - ar.getPipeline().begin(), - ar.getPipeline().end(), - SimpleBSONObjComparator::kInstance.makeEqualTo())); -} - TEST(CountCommandTest, ConvertToAggregationWithReadConcern) { auto countCmd = CountCommand::parse(ctxt, BSON("count" diff --git a/src/mongo/db/query/find.cpp b/src/mongo/db/query/find.cpp index 2f351c930d3..8cc25ffea1c 100644 --- a/src/mongo/db/query/find.cpp +++ b/src/mongo/db/query/find.cpp @@ -580,6 +580,12 @@ std::string runQuery(OperationContext* opCtx, // Set CurOp information. const auto upconvertedQuery = upconvertQueryEntry(q.query, nss, q.ntoreturn, q.ntoskip); + + // Extract the 'comment' parameter from the upconverted query, if it exists. + if (auto commentField = upconvertedQuery["comment"]) { + opCtx->setComment(commentField.wrap()); + } + beginQueryOp(opCtx, nss, upconvertedQuery, q.ntoreturn, q.ntoskip); // Parse the qm into a CanonicalQuery. diff --git a/src/mongo/db/query/parsed_distinct.cpp b/src/mongo/db/query/parsed_distinct.cpp index 226754acba4..5cfa30ebdf9 100644 --- a/src/mongo/db/query/parsed_distinct.cpp +++ b/src/mongo/db/query/parsed_distinct.cpp @@ -47,7 +47,6 @@ namespace mongo { const char ParsedDistinct::kKeyField[] = "key"; const char ParsedDistinct::kQueryField[] = "query"; const char ParsedDistinct::kCollationField[] = "collation"; -const char ParsedDistinct::kCommentField[] = "comment"; namespace { @@ -241,10 +240,6 @@ StatusWith<BSONObj> ParsedDistinct::asAggregationCommand() const { aggregationBuilder.append(QueryRequest::kUnwrappedReadPrefField, qr.getUnwrappedReadPref()); } - if (!qr.getComment().empty()) { - aggregationBuilder.append(kCommentField, qr.getComment()); - } - // Specify the 'cursor' option so that aggregation uses the cursor interface. aggregationBuilder.append("cursor", BSONObj()); @@ -284,10 +279,6 @@ StatusWith<ParsedDistinct> ParsedDistinct::parse(OperationContext* opCtx, qr->setCollation(collation.get()); } - if (auto comment = parsedDistinct.getComment()) { - qr->setComment(comment.get().toString()); - } - // The IDL parser above does not handle generic command arguments. Since the underlying query // request requires the following options, manually parse and verify them here. if (auto readConcernElt = cmdObj[repl::ReadConcernArgs::kReadConcernFieldName]) { diff --git a/src/mongo/db/query/parsed_distinct_test.cpp b/src/mongo/db/query/parsed_distinct_test.cpp index dd6e501ed24..0f0efd7a18a 100644 --- a/src/mongo/db/query/parsed_distinct_test.cpp +++ b/src/mongo/db/query/parsed_distinct_test.cpp @@ -67,7 +67,6 @@ TEST(ParsedDistinctTest, ConvertToAggregationNoQuery) { ASSERT_BSONOBJ_EQ(ar.getValue().getCollation(), BSONObj()); ASSERT(ar.getValue().getReadConcern().isEmpty()); ASSERT(ar.getValue().getUnwrappedReadPref().isEmpty()); - ASSERT(ar.getValue().getComment().empty()); ASSERT_EQUALS(ar.getValue().getMaxTimeMS(), 0u); std::vector<BSONObj> expectedPipeline{ @@ -107,7 +106,6 @@ TEST(ParsedDistinctTest, ConvertToAggregationDottedPathNoQuery) { ASSERT_BSONOBJ_EQ(ar.getValue().getCollation(), BSONObj()); ASSERT(ar.getValue().getReadConcern().isEmpty()); ASSERT(ar.getValue().getUnwrappedReadPref().isEmpty()); - ASSERT(ar.getValue().getComment().empty()); ASSERT_EQUALS(ar.getValue().getMaxTimeMS(), 0u); std::vector<BSONObj> expectedPipeline{ @@ -155,8 +153,6 @@ TEST(ParsedDistinctTest, ConvertToAggregationWithAllOptions) { << "$queryOptions" << BSON("readPreference" << "secondary") - << "comment" - << "aComment" << "maxTimeMS" << 100 << "$db" << "testdb"), ExtensionsCallbackNoop(), @@ -180,7 +176,6 @@ TEST(ParsedDistinctTest, ConvertToAggregationWithAllOptions) { ASSERT_BSONOBJ_EQ(ar.getValue().getUnwrappedReadPref(), BSON("readPreference" << "secondary")); - ASSERT_EQUALS(ar.getValue().getComment(), "aComment"); ASSERT_EQUALS(ar.getValue().getMaxTimeMS(), 100u); std::vector<BSONObj> expectedPipeline{ @@ -221,7 +216,6 @@ TEST(ParsedDistinctTest, ConvertToAggregationWithQuery) { ASSERT_BSONOBJ_EQ(ar.getValue().getCollation(), BSONObj()); ASSERT(ar.getValue().getReadConcern().isEmpty()); ASSERT(ar.getValue().getUnwrappedReadPref().isEmpty()); - ASSERT(ar.getValue().getComment().empty()); ASSERT_EQUALS(ar.getValue().getMaxTimeMS(), 0u); std::vector<BSONObj> expectedPipeline{ diff --git a/src/mongo/db/query/query_request.cpp b/src/mongo/db/query/query_request.cpp index a4867aca42f..d8ab73b8116 100644 --- a/src/mongo/db/query/query_request.cpp +++ b/src/mongo/db/query/query_request.cpp @@ -90,7 +90,6 @@ const char kLimitField[] = "limit"; const char kBatchSizeField[] = "batchSize"; const char kNToReturnField[] = "ntoreturn"; const char kSingleBatchField[] = "singleBatch"; -const char kCommentField[] = "comment"; const char kMaxField[] = "max"; const char kMinField[] = "min"; const char kReturnKeyField[] = "returnKey"; @@ -274,13 +273,6 @@ StatusWith<unique_ptr<QueryRequest>> QueryRequest::parseFromFindCommand(unique_p } qr->_allowDiskUse = el.boolean(); - } else if (fieldName == kCommentField) { - Status status = checkFieldType(el, String); - if (!status.isOK()) { - return status; - } - - qr->_comment = el.str(); } else if (fieldName == cmdOptionMaxTimeMS) { StatusWith<int> maxTimeMS = parseMaxTimeMS(el); if (!maxTimeMS.isOK()) { @@ -517,10 +509,6 @@ void QueryRequest::asFindCommandInternal(BSONObjBuilder* cmdBuilder) const { cmdBuilder->append(kSingleBatchField, true); } - if (!_comment.empty()) { - cmdBuilder->append(kCommentField, _comment); - } - if (_maxTimeMS > 0) { cmdBuilder->append(cmdOptionMaxTimeMS, _maxTimeMS); } @@ -944,14 +932,6 @@ Status QueryRequest::initFullQuery(const BSONObj& top) { return maxTimeMS.getStatus(); } _maxTimeMS = maxTimeMS.getValue(); - } else if (name == "comment") { - // 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); - } } } } @@ -1126,9 +1106,6 @@ StatusWith<BSONObj> QueryRequest::asAggregationCommand() const { if (!_hint.isEmpty()) { aggregationBuilder.append("hint", _hint); } - if (!_comment.empty()) { - aggregationBuilder.append("comment", _comment); - } if (!_readConcern.isEmpty()) { aggregationBuilder.append("readConcern", _readConcern); } diff --git a/src/mongo/db/query/query_request.h b/src/mongo/db/query/query_request.h index 17e915ace5c..a204e45c39b 100644 --- a/src/mongo/db/query/query_request.h +++ b/src/mongo/db/query/query_request.h @@ -259,14 +259,6 @@ public: _explain = explain; } - const std::string& getComment() const { - return _comment; - } - - void setComment(const std::string& comment) { - _comment = comment; - } - const BSONObj& getUnwrappedReadPref() const { return _unwrappedReadPref; } @@ -518,8 +510,6 @@ private: bool _explain = false; - std::string _comment; - // A user-specified maxTimeMS limit, or a value of '0' if not specified. int _maxTimeMS = 0; diff --git a/src/mongo/db/query/query_request_test.cpp b/src/mongo/db/query/query_request_test.cpp index e4f9989b44f..c2624e65ceb 100644 --- a/src/mongo/db/query/query_request_test.cpp +++ b/src/mongo/db/query/query_request_test.cpp @@ -430,7 +430,7 @@ TEST(QueryRequestTest, ParseFromCommandReadOnceDefaultsToFalse) { ASSERT(!qr->isReadOnce()); } -TEST(QueryRequestTest, ParseFromCommandCommentWithValidMinMax) { +TEST(QueryRequestTest, ParseFromCommandValidMinMax) { BSONObj cmdObj = fromjson( "{find: 'testns'," "comment: 'the comment'," @@ -441,7 +441,6 @@ TEST(QueryRequestTest, ParseFromCommandCommentWithValidMinMax) { unique_ptr<QueryRequest> qr( assertGet(QueryRequest::makeFromFindCommand(nss, cmdObj, isExplain))); - ASSERT_EQUALS("the comment", qr->getComment()); BSONObj expectedMin = BSON("a" << 1); ASSERT_EQUALS(0, expectedMin.woCompare(qr->getMin())); BSONObj expectedMax = BSON("a" << 2); @@ -607,17 +606,6 @@ TEST(QueryRequestTest, ParseFromCommandSingleBatchWrongType) { ASSERT_NOT_OK(result.getStatus()); } -TEST(QueryRequestTest, ParseFromCommandCommentWrongType) { - BSONObj cmdObj = fromjson( - "{find: 'testns'," - "filter: {a: 1}," - "comment: 1}"); - const NamespaceString nss("test.testns"); - bool isExplain = false; - auto result = QueryRequest::makeFromFindCommand(nss, cmdObj, isExplain); - ASSERT_NOT_OK(result.getStatus()); -} - TEST(QueryRequestTest, ParseFromCommandUnwrappedReadPrefWrongType) { BSONObj cmdObj = fromjson( "{find: 'testns'," @@ -1272,17 +1260,6 @@ TEST(QueryRequestTest, ConvertToAggregationWithReturnKeyFails) { ASSERT_NOT_OK(qr.asAggregationCommand()); } -TEST(QueryRequestTest, ConvertToAggregationWithCommentSucceeds) { - QueryRequest qr(testns); - qr.setComment("test"); - const auto aggCmd = qr.asAggregationCommand(); - ASSERT_OK(aggCmd); - - auto ar = AggregationRequest::parseFromBSON(testns, aggCmd.getValue()); - ASSERT_OK(ar.getStatus()); - ASSERT_EQ(qr.getComment(), ar.getValue().getComment()); -} - TEST(QueryRequestTest, ConvertToAggregationWithShowRecordIdFails) { QueryRequest qr(testns); qr.setShowRecordId(true); @@ -1470,31 +1447,6 @@ TEST(QueryRequestTest, ConvertToFindWithAllowDiskUseFalseSucceeds) { ASSERT_FALSE(findCmd[QueryRequest::kAllowDiskUseField]); } -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::fromLegacyQuery(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\" } }"); - ASSERT_BSONOBJ_EQ(qr->getFilter(), fromjson("{a: 1}")); -} - -TEST(QueryRequestTest, ParseFromLegacyStringMetaOpComment) { - BSONObj queryObj = fromjson( - "{$query: {a: 1}," - "$comment: 'ParseFromLegacyStringMetaOpComment'}"); - const NamespaceString nss("test.testns"); - unique_ptr<QueryRequest> qr( - assertGet(QueryRequest::fromLegacyQuery(nss, queryObj, BSONObj(), 0, 0, 0))); - - ASSERT_EQ(qr->getComment(), "ParseFromLegacyStringMetaOpComment"); - ASSERT_BSONOBJ_EQ(qr->getFilter(), fromjson("{a: 1}")); -} - TEST(QueryRequestTest, ParseFromLegacyQuery) { const auto kSkip = 1; const auto kNToReturn = 2; diff --git a/src/mongo/db/service_entry_point_common.cpp b/src/mongo/db/service_entry_point_common.cpp index 29a2bb973f5..1f38b66d6d9 100644 --- a/src/mongo/db/service_entry_point_common.cpp +++ b/src/mongo/db/service_entry_point_common.cpp @@ -99,9 +99,9 @@ namespace mongo { MONGO_FAIL_POINT_DEFINE(rsStopGetMore); MONGO_FAIL_POINT_DEFINE(respondWithNotPrimaryInCommandDispatch); MONGO_FAIL_POINT_DEFINE(skipCheckingForNotMasterInCommandDispatch); -MONGO_FAIL_POINT_DEFINE(waitAfterReadCommandFinishesExecution); MONGO_FAIL_POINT_DEFINE(sleepMillisAfterCommandExecutionBegins); MONGO_FAIL_POINT_DEFINE(waitAfterNewStatementBlocksBehindPrepare); +MONGO_FAIL_POINT_DEFINE(waitAfterCommandFinishesExecution); // Tracks the number of times a legacy unacknowledged write failed due to // not master error resulted in network disconnection. @@ -627,19 +627,24 @@ bool runCommandImpl(OperationContext* opCtx, } } - // This failpoint should affect both getMores and commands which are read-only and thus don't - // support writeConcern. - if (!shouldWaitForWriteConcern || command->getLogicalOp() == LogicalOp::opGetMore) { - waitAfterReadCommandFinishesExecution.execute([&](const BSONObj& data) { - auto db = data["db"].str(); - if (db.empty() || request.getDatabase() == db) { - CurOpFailpointHelpers::waitWhileFailPointEnabled( - &waitAfterReadCommandFinishesExecution, - opCtx, - "waitAfterReadCommandFinishesExecution"); - } - }); - } + // This fail point blocks all commands which are running on the specified namespace, or which + // are present in the given list of commands.If no namespace or command list are provided,then + // the failpoint will block all commands. + waitAfterCommandFinishesExecution.execute([&](const BSONObj& data) { + auto ns = data["ns"].valueStringDataSafe(); + auto commands = + data.hasField("commands") ? data["commands"].Array() : std::vector<BSONElement>(); + + // If 'ns' or 'commands' is not set, block for all the namespaces or commands respectively. + if ((ns.empty() || invocation->ns().ns() == ns) && + (commands.empty() || + std::any_of(commands.begin(), commands.end(), [&request](auto& element) { + return element.valueStringDataSafe() == request.getCommandName(); + }))) { + CurOpFailpointHelpers::waitWhileFailPointEnabled( + &waitAfterCommandFinishesExecution, opCtx, "waitAfterCommandFinishesExecution"); + } + }); behaviors.waitForLinearizableReadConcern(opCtx); @@ -754,6 +759,8 @@ void execCommandDatabase(OperationContext* opCtx, allowImplicitCollectionCreationField = element; } else if (fieldName == CommandHelpers::kHelpFieldName) { helpField = element; + } else if (fieldName == "comment") { + opCtx->setComment(element.wrap()); } else if (fieldName == QueryRequest::queryOptionMaxTimeMS) { uasserted(ErrorCodes::InvalidOptions, "no such command option $maxTimeMs; use maxTimeMS instead"); diff --git a/src/mongo/db/views/resolved_view.cpp b/src/mongo/db/views/resolved_view.cpp index c5780407243..a6ac9d6b4d9 100644 --- a/src/mongo/db/views/resolved_view.cpp +++ b/src/mongo/db/views/resolved_view.cpp @@ -106,7 +106,6 @@ AggregationRequest ResolvedView::asExpandedViewAggregation( } expandedRequest.setHint(request.getHint()); - expandedRequest.setComment(request.getComment()); expandedRequest.setMaxTimeMS(request.getMaxTimeMS()); expandedRequest.setReadConcern(request.getReadConcern()); expandedRequest.setUnwrappedReadPref(request.getUnwrappedReadPref()); diff --git a/src/mongo/db/views/resolved_view_test.cpp b/src/mongo/db/views/resolved_view_test.cpp index a4b5111419a..0273c5319c5 100644 --- a/src/mongo/db/views/resolved_view_test.cpp +++ b/src/mongo/db/views/resolved_view_test.cpp @@ -173,15 +173,6 @@ TEST(ResolvedViewTest, ExpandingAggRequestPreservesDefaultCollationOfView) { << "fr_CA")); } -TEST(ResolvedViewTest, ExpandingAggRequestPreservesComment) { - const ResolvedView resolvedView{backingNss, emptyPipeline, kSimpleCollation}; - AggregationRequest aggRequest(viewNss, {}); - aggRequest.setComment("agg_comment"); - - auto result = resolvedView.asExpandedViewAggregation(aggRequest); - ASSERT_EQ(result.getComment(), "agg_comment"); -} - TEST(ResolvedViewTest, FromBSONFailsIfMissingResolvedView) { BSONObj badCmdResponse = BSON("x" << 1); ASSERT_THROWS_CODE(ResolvedView::fromBSON(badCmdResponse), AssertionException, 40248); diff --git a/src/mongo/executor/remote_command_request.cpp b/src/mongo/executor/remote_command_request.cpp index 38b745bebee..d0fa3d6e081 100644 --- a/src/mongo/executor/remote_command_request.cpp +++ b/src/mongo/executor/remote_command_request.cpp @@ -59,7 +59,12 @@ RemoteCommandRequestBase::RemoteCommandRequestBase(RequestId requestId, const BSONObj& metadataObj, OperationContext* opCtx, Milliseconds timeoutMillis) - : id(requestId), dbname(theDbName), metadata(metadataObj), cmdObj(theCmdObj), opCtx(opCtx) { + : id(requestId), dbname(theDbName), metadata(metadataObj), opCtx(opCtx) { + // If there is a comment associated with the current operation, append it to the command that we + // are about to dispatch to the shards. + cmdObj = opCtx && opCtx->getComment() && !theCmdObj["comment"] + ? theCmdObj.addField(*opCtx->getComment()) + : theCmdObj; timeout = opCtx ? std::min<Milliseconds>(opCtx->getRemainingMaxTimeMillis(), timeoutMillis) : timeoutMillis; } diff --git a/src/mongo/s/commands/cluster_explain_cmd.cpp b/src/mongo/s/commands/cluster_explain_cmd.cpp index 09882eeb642..3736947d113 100644 --- a/src/mongo/s/commands/cluster_explain_cmd.cpp +++ b/src/mongo/s/commands/cluster_explain_cmd.cpp @@ -176,6 +176,13 @@ std::unique_ptr<CommandInvocation> ClusterExplainCmd::parse(OperationContext* op // arguments into the inner command since it is what is passed to the virtual // CommandInvocation::explain() method. const BSONObj explainedObj = makeExplainedObj(cmdObj, dbName); + + // Extract 'comment' field from the 'explainedObj' only if there is no top-level comment. + auto commentField = explainedObj["comment"]; + if (!opCtx->getComment() && commentField) { + opCtx->setComment(commentField.wrap()); + } + const std::string cmdName = explainedObj.firstElementFieldName(); auto explainedCommand = CommandHelpers::findCommand(cmdName); uassert(ErrorCodes::CommandNotFound, diff --git a/src/mongo/s/commands/strategy.cpp b/src/mongo/s/commands/strategy.cpp index 5949128fe88..802a1d69ab2 100644 --- a/src/mongo/s/commands/strategy.cpp +++ b/src/mongo/s/commands/strategy.cpp @@ -365,6 +365,11 @@ void runCommand(OperationContext* opCtx, } opCtx->checkForInterrupt(); // May trigger maxTimeAlwaysTimeOut fail point. + // If the command includes a 'comment' field, set it on the current OpCtx. + if (auto commentField = request.body["comment"]) { + opCtx->setComment(commentField.wrap()); + } + auto invocation = command->parse(opCtx, request); // Set the logical optype, command object and namespace as soon as we identify the command. If @@ -634,6 +639,12 @@ DbResponse Strategy::queryOp(OperationContext* opCtx, const NamespaceString& nss Client* const client = opCtx->getClient(); AuthorizationSession* const authSession = AuthorizationSession::get(client); + // The legacy '$comment' operator gets converted to 'comment' by upconvertQueryEntry(). We + // set the comment in 'opCtx' so that it can be passed on to the respective shards. + if (auto commentField = upconvertedQuery["comment"]) { + opCtx->setComment(commentField.wrap()); + } + Status status = authSession->checkAuthForFind(nss, false); audit::logQueryAuthzCheck(client, nss, q.query, status.code()); uassertStatusOK(status); diff --git a/src/mongo/s/query/cluster_find.cpp b/src/mongo/s/query/cluster_find.cpp index e697d32f150..69fe465d4cb 100644 --- a/src/mongo/s/query/cluster_find.cpp +++ b/src/mongo/s/query/cluster_find.cpp @@ -371,6 +371,12 @@ Status setUpOperationContextStateForGetMore(OperationContext* opCtx, ReadPreferenceSetting::get(opCtx) = *readPref; } + // If the originating command had a 'comment' field, we extract it and set it on opCtx. Note + // that if the 'getMore' command itself has a 'comment' field, we give precedence to it. + auto comment = cursor->getOriginatingCommand()["comment"]; + if (!opCtx->getComment() && comment) { + opCtx->setComment(comment.wrap()); + } if (cursor->isTailableAndAwaitData()) { // For tailable + awaitData cursors, the request may have indicated a maximum amount of time // to wait for new data. If not, default it to 1 second. We track the deadline instead via |