diff options
author | Geert Bosch <geert@mongodb.com> | 2016-08-18 19:36:15 +0100 |
---|---|---|
committer | Geert Bosch <geert@mongodb.com> | 2016-08-22 17:04:08 +0100 |
commit | c14515630a02136b60e49c8e15e7135cf8153497 (patch) | |
tree | 057758a3c991dc988f75e4df6a0e61789ad3f3e7 | |
parent | e576de40f3629649c453f437ad18a2a86b433509 (diff) | |
download | mongo-c14515630a02136b60e49c8e15e7135cf8153497.tar.gz |
SERVER-24771 Use view namespace in cursors for aggregate/getMore
-rw-r--r-- | jstests/auth/views_authz.js | 15 | ||||
-rw-r--r-- | jstests/views/views_basic.js | 4 | ||||
-rw-r--r-- | src/mongo/db/commands/getmore_cmd.cpp | 87 | ||||
-rw-r--r-- | src/mongo/db/commands/pipeline_command.cpp | 90 | ||||
-rw-r--r-- | src/mongo/db/s/operation_sharding_state.cpp | 5 | ||||
-rw-r--r-- | src/mongo/db/s/operation_sharding_state.h | 5 |
6 files changed, 147 insertions, 59 deletions
diff --git a/jstests/auth/views_authz.js b/jstests/auth/views_authz.js index e187dff9c90..4a851583110 100644 --- a/jstests/auth/views_authz.js +++ b/jstests/auth/views_authz.js @@ -26,6 +26,7 @@ resource: {db: viewsDBName, collection: "view"}, actions: ["find", "createCollection", "collMod"] }, + {resource: {db: viewsDBName, collection: "view2"}, actions: ["find"]}, {resource: {db: viewsDBName, collection: "permitted"}, actions: ["find"]} ], roles: [] @@ -83,4 +84,18 @@ viewsDB.runCommand( {collMod: "view", viewOn: "permitted", pipeline: [{$facet: {b: [graphLookupStage]}}]}), ErrorCodes.Unauthorized); + + // Performing a find on a readable view returns a cursor that allows us to perform a getMore + // even if the underlying collection is unreadable. + assert.eq(1, adminDB.auth("admin", "admin")); + assert.commandWorked(viewsDB.createView("view2", "forbidden", [])); + for (let i = 0; i < 10; i++) { + assert.writeOK(viewsDB.forbidden.insert({x: 1})); + } + adminDB.logout(); + assert.commandFailedWithCode(viewsDB.runCommand({find: "forbidden"}), ErrorCodes.Unauthorized); + let res = viewsDB.runCommand({find: "view2", batchSize: 1}); + assert.commandWorked(res); + assert.eq(res.cursor.ns, "views_authz.view2"); + assert.commandWorked(viewsDB.runCommand({getMore: res.cursor.id, collection: "view2"})); }()); diff --git a/jstests/views/views_basic.js b/jstests/views/views_basic.js index b5459380f07..d6032b105df 100644 --- a/jstests/views/views_basic.js +++ b/jstests/views/views_basic.js @@ -14,7 +14,9 @@ assert.commandWorked(res); let cursor = new DBCommandCursor(db.getMongo(), res, 5); - assert(arrayEq(cursor.toArray(), expected)); + let actual = cursor.toArray(); + assert(arrayEq(actual, expected), + "actual: " + tojson(cursor.toArray()) + ", expected:" + tojson(expected)); }; // Insert some control documents. diff --git a/src/mongo/db/commands/getmore_cmd.cpp b/src/mongo/db/commands/getmore_cmd.cpp index 2fcf53e8e82..cbcf44f19d0 100644 --- a/src/mongo/db/commands/getmore_cmd.cpp +++ b/src/mongo/db/commands/getmore_cmd.cpp @@ -145,26 +145,12 @@ public: request.nss, request.cursorid, request.term.is_initialized()); } - bool run(OperationContext* txn, - const std::string& dbname, - BSONObj& cmdObj, - int options, - std::string& errmsg, - BSONObjBuilder& result) override { - // Counted as a getMore, not as a command. - globalOpCounters.gotGetMore(); - - if (txn->getClient()->isInDirectClient()) { - return appendCommandStatus( - result, - Status(ErrorCodes::IllegalOperation, "Cannot run getMore command from eval()")); - } - - StatusWith<GetMoreRequest> parseStatus = GetMoreRequest::parseFromBSON(dbname, cmdObj); - if (!parseStatus.isOK()) { - return appendCommandStatus(result, parseStatus.getStatus()); - } - const GetMoreRequest& request = parseStatus.getValue(); + bool runParsed(OperationContext* txn, + const NamespaceString& origNss, + const GetMoreRequest& request, + BSONObj& cmdObj, + std::string& errmsg, + BSONObjBuilder& result) { auto curOp = CurOp::get(txn); curOp->debug().cursorid = request.cursorid; @@ -208,9 +194,41 @@ public: if (request.nss.isListIndexesCursorNS() || request.nss.isListCollectionsCursorNS()) { cursorManager = CursorManager::getGlobalCursorManager(); } else { - ctx = stdx::make_unique<AutoGetCollectionForRead>(txn, request.nss); + ctx = stdx::make_unique<AutoGetCollectionOrViewForRead>(txn, request.nss); + auto viewCtx = static_cast<AutoGetCollectionOrViewForRead*>(ctx.get()); Collection* collection = ctx->getCollection(); if (!collection) { + // Rewrite a getMore on a view to a getMore on the original underlying collection. + // If the view no longer exists, or has been rewritten, the cursor id will be + // unknown, resulting in an appropriate error. + if (viewCtx->getView()) { + auto resolved = + viewCtx->getDb()->getViewCatalog()->resolveView(txn, request.nss); + if (!resolved.isOK()) { + return appendCommandStatus(result, resolved.getStatus()); + } + viewCtx->releaseLocksForView(); + + // Only one shardversion can be set at a time for an operation, so unset it + // here to allow setting it on the underlying namespace. + OperationShardingState::get(txn).unsetShardVersion(request.nss); + + GetMoreRequest newRequest(resolved.getValue().getNamespace(), + request.cursorid, + request.batchSize, + request.awaitDataTimeout, + request.term, + request.lastKnownCommittedOpTime); + + bool retVal = runParsed(txn, origNss, newRequest, cmdObj, errmsg, result); + { + // Set the namespace of the curop back to the view namespace so ctx records + // stats on this view namespace on destruction. + stdx::lock_guard<Client>(*txn->getClient()); + curOp->setNS_inlock(origNss.ns()); + } + return retVal; + } return appendCommandStatus(result, Status(ErrorCodes::OperationFailed, "collection dropped between getMore calls")); @@ -429,7 +447,9 @@ public: curOp->debug().cursorExhausted = true; } - nextBatch.done(respondWithId, request.nss.ns()); + // Respond with the originally requested namespace, even if this is a getMore over a view + // that was resolved to a different backing namespace. + nextBatch.done(respondWithId, origNss.ns()); // Ensure log and profiler include the number of results returned in this getMore's response // batch. @@ -451,6 +471,29 @@ public: return true; } + bool run(OperationContext* txn, + const std::string& dbname, + BSONObj& cmdObj, + int options, + std::string& errmsg, + BSONObjBuilder& result) override { + // Counted as a getMore, not as a command. + globalOpCounters.gotGetMore(); + + if (txn->getClient()->isInDirectClient()) { + return appendCommandStatus( + result, + Status(ErrorCodes::IllegalOperation, "Cannot run getMore command from eval()")); + } + + StatusWith<GetMoreRequest> parsedRequest = GetMoreRequest::parseFromBSON(dbname, cmdObj); + if (!parsedRequest.isOK()) { + return appendCommandStatus(result, parsedRequest.getStatus()); + } + auto request = parsedRequest.getValue(); + return runParsed(txn, request.nss, request, cmdObj, errmsg, result); + } + /** * Uses 'cursor' and 'request' to fill out 'nextBatch' with the batch of result documents to * be returned by this getMore. diff --git a/src/mongo/db/commands/pipeline_command.cpp b/src/mongo/db/commands/pipeline_command.cpp index 2aa42c8f736..4f64276be72 100644 --- a/src/mongo/db/commands/pipeline_command.cpp +++ b/src/mongo/db/commands/pipeline_command.cpp @@ -79,10 +79,12 @@ namespace { /** * Returns true if we need to keep a ClientCursor saved for this pipeline (for future getMore - * requests). Otherwise, returns false. + * requests). Otherwise, returns false. The passed 'nsForCursor' is only used to determine the + * namespace used in the returned cursor. In the case of views, this can be different from that + * in 'request'. */ bool handleCursorCommand(OperationContext* txn, - const string& ns, + const string& nsForCursor, ClientCursorPin* pin, PlanExecutor* exec, const AggregationRequest& request, @@ -139,7 +141,7 @@ bool handleCursorCommand(OperationContext* txn, 17391, str::stream() << "Aggregation has more results than fit in initial batch, but can't " << "create cursor since collection " - << ns + << nsForCursor << " doesn't exist"); } @@ -159,7 +161,7 @@ bool handleCursorCommand(OperationContext* txn, } const long long cursorId = cursor ? cursor->cursorid() : 0LL; - appendCursorResponseObject(cursorId, ns, resultsArray.arr(), &result); + appendCursorResponseObject(cursorId, nsForCursor, resultsArray.arr(), &result); return static_cast<bool>(cursor); } @@ -303,31 +305,21 @@ public: return Pipeline::checkAuthForCommand(client, dbname, cmdObj); } - virtual bool run(OperationContext* txn, - const string& db, - BSONObj& cmdObj, - int options, - string& errmsg, - BSONObjBuilder& result) { - const std::string ns = parseNs(db, cmdObj); - if (nsToCollectionSubstring(ns).empty()) { - errmsg = "missing collection name"; - return false; - } - NamespaceString nss(ns); - - // Parse the options for this request. - auto request = AggregationRequest::parseFromBSON(nss, cmdObj); - if (!request.isOK()) { - return appendCommandStatus(result, request.getStatus()); - } + bool runParsed(OperationContext* txn, + const NamespaceString& origNss, + const AggregationRequest& request, + BSONObj& cmdObj, + string& errmsg, + BSONObjBuilder& result) { + // For operations on views, this will be the underlying namespace. + const NamespaceString& nss = request.getNamespaceString(); // Set up the ExpressionContext. - intrusive_ptr<ExpressionContext> expCtx = new ExpressionContext(txn, request.getValue()); + intrusive_ptr<ExpressionContext> expCtx = new ExpressionContext(txn, request); expCtx->tempDir = storageGlobalParams.dbpath + "/_tmp"; // Parse the pipeline. - auto statusWithPipeline = Pipeline::parse(request.getValue().getPipeline(), expCtx); + auto statusWithPipeline = Pipeline::parse(request.getPipeline(), expCtx); if (!statusWithPipeline.isOK()) { return appendCommandStatus(result, statusWithPipeline.getStatus()); } @@ -388,13 +380,18 @@ public: ctx.releaseLocksForView(); // Parse the resolved view into a new aggregation request. - auto viewCmd = - resolvedView.getValue().asExpandedViewAggregation(request.getValue()); - if (!viewCmd.isOK()) { - return appendCommandStatus(result, viewCmd.getStatus()); + auto newCmd = resolvedView.getValue().asExpandedViewAggregation(request); + if (!newCmd.isOK()) { + return appendCommandStatus(result, newCmd.getStatus()); + } + auto newNss = resolvedView.getValue().getNamespace(); + auto newRequest = AggregationRequest::parseFromBSON(newNss, newCmd.getValue()); + if (!newRequest.isOK()) { + return appendCommandStatus(result, newRequest.getStatus()); } - bool status = this->run(txn, db, viewCmd.getValue(), options, errmsg, result); + bool status = runParsed( + txn, origNss, newRequest.getValue(), newCmd.getValue(), errmsg, result); { // Set the namespace of the curop back to the view namespace so ctx records // stats on this view namespace on destruction. @@ -406,7 +403,7 @@ public: // If the pipeline does not have a user-specified collation, set it from the collection // default. - if (request.getValue().getCollation().isEmpty() && collection && + if (request.getCollation().isEmpty() && collection && collection->getDefaultCollator()) { invariant(!expCtx->getCollator()); expCtx->setCollator(collection->getDefaultCollator()->clone()); @@ -427,7 +424,7 @@ public: // re-parsing every command in debug builds. This is important because sharded // aggregations rely on this ability. Skipping when inShard because this has // already been through the transformation (and this un-sets expCtx->inShard). - pipeline = reparsePipeline(pipeline, request.getValue(), expCtx); + pipeline = reparsePipeline(pipeline, request, expCtx); } // This does mongod-specific stuff like creating the input PlanExecutor and adding @@ -492,7 +489,7 @@ public: // BSONObj will also fit inside a single batch. // // We occasionally log a deprecation warning. - if (!request.getValue().isCursorCommand()) { + if (!request.isCursorCommand()) { RARELY { warning() << "Use of the aggregate command without the 'cursor' " @@ -504,12 +501,12 @@ public: // If both explain and cursor are specified, explain wins. if (expCtx->isExplain) { result << "stages" << Value(pipeline->writeExplainOps()); - } else if (request.getValue().isCursorCommand()) { + } else if (request.isCursorCommand()) { keepCursor = handleCursorCommand(txn, - nss.ns(), + origNss.ns(), pin.get(), pin ? pin->c()->getExecutor() : exec.get(), - request.getValue(), + request, result); } else { pipeline->run(result); @@ -547,9 +544,30 @@ public: throw; } // Any code that needs the cursor pinned must be inside the try block, above. - return appendCommandStatus(result, Status::OK()); } + + virtual bool run(OperationContext* txn, + const string& db, + BSONObj& cmdObj, + int options, + string& errmsg, + BSONObjBuilder& result) { + const std::string ns = parseNs(db, cmdObj); + if (nsToCollectionSubstring(ns).empty()) { + errmsg = "missing collection name"; + return false; + } + NamespaceString nss(ns); + + // Parse the options for this request. + auto request = AggregationRequest::parseFromBSON(nss, cmdObj); + if (!request.isOK()) { + return appendCommandStatus(result, request.getStatus()); + } + + return runParsed(txn, nss, request.getValue(), cmdObj, errmsg, result); + } }; MONGO_INITIALIZER(PipelineCommand)(InitializerContext* context) { diff --git a/src/mongo/db/s/operation_sharding_state.cpp b/src/mongo/db/s/operation_sharding_state.cpp index b1ba9e0e16f..a940ffc01df 100644 --- a/src/mongo/db/s/operation_sharding_state.cpp +++ b/src/mongo/db/s/operation_sharding_state.cpp @@ -90,6 +90,11 @@ void OperationShardingState::setShardVersion(NamespaceString nss, ChunkVersion n _hasVersion = true; } +void OperationShardingState::unsetShardVersion(NamespaceString nss) { + invariant(!_hasVersion || _ns == nss); + _clear(); +} + bool OperationShardingState::waitForMigrationCriticalSectionSignal(OperationContext* txn) { // Must not block while holding a lock invariant(!txn->lockState()->isLocked()); diff --git a/src/mongo/db/s/operation_sharding_state.h b/src/mongo/db/s/operation_sharding_state.h index d83822d5696..aa03834da6a 100644 --- a/src/mongo/db/s/operation_sharding_state.h +++ b/src/mongo/db/s/operation_sharding_state.h @@ -93,6 +93,11 @@ public: void setShardVersion(NamespaceString nss, ChunkVersion newVersion); /** + * Undoes setting the shard version for the given namespace. Needed for views. + */ + void unsetShardVersion(NamespaceString nss); + + /** * This call is a no op if there isn't a currently active migration critical section. Otherwise * it will wait for the critical section to complete up to the remaining operation time. * |