From 351ca4155329ffe1e1618dbb71a252a4fda2278f Mon Sep 17 00:00:00 2001 From: Jan Date: Fri, 13 Sep 2019 19:03:55 +0200 Subject: [PATCH] validate collection when leasing an existing transaction (#9990) * validate collection when leasing an existing transaction * use WITH statement * allow implicit read collections again * simplify collection name validation * re-implement it * remove dead code * remove unused method * apply review comments * whoops, deleted too many methods * fixit --- arangod/Aql/OptimizerRules.cpp | 2 +- arangod/RestHandler/RestCollectionHandler.cpp | 17 ++++- arangod/RestHandler/RestEdgesHandler.cpp | 24 ++++--- arangod/RestHandler/RestEdgesHandler.h | 11 +-- arangod/RestHandler/RestIndexHandler.cpp | 5 +- .../RestHandler/RestVocbaseBaseHandler.cpp | 14 +++- arangod/RestHandler/RestVocbaseBaseHandler.h | 8 ++- arangod/StorageEngine/TransactionState.cpp | 20 +++++- arangod/StorageEngine/TransactionState.h | 6 +- arangod/Transaction/Manager.cpp | 6 +- arangod/Transaction/Manager.h | 6 +- arangod/Transaction/Methods.cpp | 71 ++++++++++++++----- arangod/Transaction/Methods.h | 13 +++- arangod/Utils/SingleCollectionTransaction.cpp | 16 ++++- arangod/Utils/SingleCollectionTransaction.h | 5 +- js/server/modules/@arangodb/foxx/manager.js | 2 +- .../IResearchAnalyzerFeature-test.cpp | 2 +- .../client/shell/shell-graph-transaction.js | 2 +- tests/js/client/shell/shell-transaction.js | 2 - 19 files changed, 171 insertions(+), 61 deletions(-) diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index b8c15f8c6c..06d1c302b8 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -825,7 +825,7 @@ Collection* addCollectionToQuery(Query* query, std::string const& cname, bool as auto cptr = query->trx()->vocbase().lookupCollection(cname); coll->setCollection(cptr.get()); - query->trx()->addCollectionAtRuntime(cname); + query->trx()->addCollectionAtRuntime(cname, AccessMode::Type::READ); } } diff --git a/arangod/RestHandler/RestCollectionHandler.cpp b/arangod/RestHandler/RestCollectionHandler.cpp index ef1d739be2..a4a85ae78a 100644 --- a/arangod/RestHandler/RestCollectionHandler.cpp +++ b/arangod/RestHandler/RestCollectionHandler.cpp @@ -611,7 +611,22 @@ void RestCollectionHandler::collectionRepresentation( bool showProperties, bool showFigures, bool showCount, bool detailedCount) { if (showProperties || showCount) { // Here we need a transaction - auto trx = createTransaction(coll.name(), AccessMode::Type::READ); + std::unique_ptr trx; + try { + trx = createTransaction(coll.name(), AccessMode::Type::READ); + } catch (basics::Exception const& ex) { + if (ex.code() == TRI_ERROR_TRANSACTION_NOT_FOUND) { + // this will happen if the tid of a managed transaction is passed in, + // but the transaction hasn't yet started on the DB server. in + // this case, we create an ad-hoc transaction on the underlying + // collection + trx = std::make_unique(transaction::StandaloneContext::Create(_vocbase), coll.name(), AccessMode::Type::READ); + } else { + throw; + } + } + + TRI_ASSERT(trx != nullptr); Result res = trx->begin(); if (res.fail()) { diff --git a/arangod/RestHandler/RestEdgesHandler.cpp b/arangod/RestHandler/RestEdgesHandler.cpp index 213b19b3e7..a309f73253 100644 --- a/arangod/RestHandler/RestEdgesHandler.cpp +++ b/arangod/RestHandler/RestEdgesHandler.cpp @@ -27,10 +27,10 @@ #include "Aql/Variable.h" #include "Cluster/ClusterMethods.h" #include "Cluster/ServerState.h" +#include "Transaction/Methods.h" #include "Transaction/StandaloneContext.h" #include "Utils/CollectionNameResolver.h" #include "Utils/OperationCursor.h" -#include "Utils/SingleCollectionTransaction.h" #include #include @@ -64,7 +64,7 @@ RestStatus RestEdgesHandler::execute() { void RestEdgesHandler::readCursor(aql::AstNode* condition, aql::Variable const* var, std::string const& collectionName, - SingleCollectionTransaction& trx, + transaction::Methods& trx, std::function const& cb) { transaction::Methods::IndexHandle indexId; bool foundIdx = @@ -85,10 +85,10 @@ void RestEdgesHandler::readCursor(aql::AstNode* condition, aql::Variable const* } bool RestEdgesHandler::getEdgesForVertex( - std::string const& id, std::string const& collectionName, - TRI_edge_direction_e direction, SingleCollectionTransaction& trx, + std::string const& id, TRI_voc_cid_t cid, std::string const& collectionName, + TRI_edge_direction_e direction, transaction::Methods& trx, std::function const& cb) { - trx.pinData(trx.cid()); // will throw when it fails + trx.pinData(cid); // will throw when it fails // Create a conditionBuilder that manages the AstNodes for querying aql::EdgeConditionBuilderContainer condBuilder; @@ -234,7 +234,8 @@ bool RestEdgesHandler::readEdges() { resultBuilder.add(VPackValue("edges")); // only key resultBuilder.openArray(); - auto collection = trx->documentCollection(); + TRI_voc_cid_t cid = trx->addCollectionAtRuntime(collectionName, AccessMode::Type::READ); + auto collection = trx->documentCollection(cid); std::unordered_set foundTokens; auto cb = [&](LocalDocumentId const& token) { if (foundTokens.find(token) == foundTokens.end()) { @@ -249,7 +250,7 @@ bool RestEdgesHandler::readEdges() { }; // NOTE: collectionName is the shard-name in DBServer case - bool ok = getEdgesForVertex(startVertex, collectionName, direction, *(trx.get()), cb); + bool ok = getEdgesForVertex(startVertex, cid, collectionName, direction, *(trx.get()), cb); resultBuilder.close(); res = trx->finish(res); @@ -262,7 +263,7 @@ bool RestEdgesHandler::readEdges() { if (ServerState::instance()->isDBServer()) { // If we are a DBserver, we want to use the cluster-wide collection // name for error reporting: - collectionName = trx->resolver()->getCollectionNameCluster(trx->cid()); + collectionName = trx->resolver()->getCollectionNameCluster(cid); } generateTransactionError(collectionName, res, ""); return false; @@ -373,7 +374,8 @@ bool RestEdgesHandler::readEdgesForMultipleVertices() { resultBuilder.add(VPackValue("edges")); // only key resultBuilder.openArray(); - auto collection = trx->documentCollection(); + TRI_voc_cid_t cid = trx->addCollectionAtRuntime(collectionName, AccessMode::Type::READ); + auto collection = trx->documentCollection(cid); std::unordered_set foundTokens; auto cb = [&](LocalDocumentId const& token) { if (foundTokens.find(token) == foundTokens.end()) { @@ -392,7 +394,7 @@ bool RestEdgesHandler::readEdgesForMultipleVertices() { std::string startVertex = it.copyString(); // We ignore if this fails - getEdgesForVertex(startVertex, collectionName, direction, *(trx.get()), cb); + getEdgesForVertex(startVertex, cid, collectionName, direction, *(trx.get()), cb); } } resultBuilder.close(); @@ -402,7 +404,7 @@ bool RestEdgesHandler::readEdgesForMultipleVertices() { if (ServerState::instance()->isDBServer()) { // If we are a DBserver, we want to use the cluster-wide collection // name for error reporting: - collectionName = trx->resolver()->getCollectionNameCluster(trx->cid()); + collectionName = trx->resolver()->getCollectionNameCluster(cid); } generateTransactionError(collectionName, res, ""); return false; diff --git a/arangod/RestHandler/RestEdgesHandler.h b/arangod/RestHandler/RestEdgesHandler.h index 918f9542e4..20a880a4bd 100644 --- a/arangod/RestHandler/RestEdgesHandler.h +++ b/arangod/RestHandler/RestEdgesHandler.h @@ -31,7 +31,9 @@ namespace arangodb { class LocalDocumentId; -class SingleCollectionTransaction; +namespace transaction { +class Methods; +} namespace aql { struct AstNode; @@ -65,15 +67,16 @@ class RestEdgesHandler : public RestVocbaseBaseHandler { ////////////////////////////////////////////////////////////////////////////// void readCursor(aql::AstNode* condition, aql::Variable const* var, - std::string const& collectionName, SingleCollectionTransaction& trx, + std::string const& collectionName, transaction::Methods& trx, std::function const& cb); ////////////////////////////////////////////////////////////////////////////// /// @brief get all edges for a given vertex. Independent from the request ////////////////////////////////////////////////////////////////////////////// - bool getEdgesForVertex(std::string const& id, std::string const& collectionName, - TRI_edge_direction_e direction, SingleCollectionTransaction& trx, + bool getEdgesForVertex(std::string const& id, TRI_voc_cid_t cid, + std::string const& collectionName, + TRI_edge_direction_e direction, transaction::Methods& trx, std::function const& cb); ////////////////////////////////////////////////////////////////////////////// diff --git a/arangod/RestHandler/RestIndexHandler.cpp b/arangod/RestHandler/RestIndexHandler.cpp index dc7d475993..c583532aa4 100644 --- a/arangod/RestHandler/RestIndexHandler.cpp +++ b/arangod/RestHandler/RestIndexHandler.cpp @@ -26,6 +26,7 @@ #include "Cluster/ClusterInfo.h" #include "Cluster/ServerState.h" #include "Rest/HttpRequest.h" +#include "Transaction/Methods.h" #include "Transaction/StandaloneContext.h" #include "Utils/Events.h" #include "Utils/SingleCollectionTransaction.h" @@ -180,7 +181,7 @@ RestStatus RestIndexHandler::getSelectivityEstimates() { } // transaction protects access onto selectivity estimates - std::unique_ptr trx; + std::unique_ptr trx; try { trx = createTransaction(cName, AccessMode::Type::READ); @@ -204,7 +205,7 @@ RestStatus RestIndexHandler::getSelectivityEstimates() { return RestStatus::DONE; } - LogicalCollection* coll = trx->documentCollection(); + LogicalCollection* coll = trx->documentCollection(cName); auto idxs = coll->getIndexes(); VPackBuffer buffer; diff --git a/arangod/RestHandler/RestVocbaseBaseHandler.cpp b/arangod/RestHandler/RestVocbaseBaseHandler.cpp index 497fbbec27..bd79bc8ed8 100644 --- a/arangod/RestHandler/RestVocbaseBaseHandler.cpp +++ b/arangod/RestHandler/RestVocbaseBaseHandler.cpp @@ -36,6 +36,7 @@ #include "Transaction/Helpers.h" #include "Transaction/Manager.h" #include "Transaction/ManagerFeature.h" +#include "Transaction/Methods.h" #include "Transaction/SmartContext.h" #include "Transaction/StandaloneContext.h" #include "Utils/SingleCollectionTransaction.h" @@ -51,6 +52,15 @@ using namespace arangodb; using namespace arangodb::basics; using namespace arangodb::rest; +namespace { +class SimpleTransaction : public transaction::Methods { + public: + SimpleTransaction(std::shared_ptr&& transactionContext, + transaction::Options&& options = transaction::Options()) + : Methods(std::move(transactionContext), std::move(options)) {} +}; +} // namespace + //////////////////////////////////////////////////////////////////////////////// /// @brief agency public path //////////////////////////////////////////////////////////////////////////////// @@ -543,7 +553,7 @@ void RestVocbaseBaseHandler::extractStringParameter(std::string const& name, } } -std::unique_ptr RestVocbaseBaseHandler::createTransaction( +std::unique_ptr RestVocbaseBaseHandler::createTransaction( std::string const& collectionName, AccessMode::Type type) const { bool found = false; std::string value = _request->header(StaticStrings::TransactionId, found); @@ -581,7 +591,7 @@ std::unique_ptr RestVocbaseBaseHandler::createTrans LOG_TOPIC("e94ea", DEBUG, Logger::TRANSACTIONS) << "Transaction with id '" << tid << "' not found"; THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_TRANSACTION_NOT_FOUND, std::string("transaction '") + std::to_string(tid) + "' not found"); } - return std::make_unique(ctx, collectionName, type); + return std::make_unique(std::move(ctx)); } else { auto ctx = transaction::StandaloneContext::Create(_vocbase); return std::make_unique(ctx, collectionName, type); diff --git a/arangod/RestHandler/RestVocbaseBaseHandler.h b/arangod/RestHandler/RestVocbaseBaseHandler.h index 9541d93bad..783c13064a 100644 --- a/arangod/RestHandler/RestVocbaseBaseHandler.h +++ b/arangod/RestHandler/RestVocbaseBaseHandler.h @@ -35,8 +35,10 @@ struct TRI_vocbase_t; namespace arangodb { +namespace transaction { +class Methods; +} -class SingleCollectionTransaction; class VocbaseContext; /// @brief abstract base request handler @@ -228,8 +230,8 @@ class RestVocbaseBaseHandler : public RestBaseHandler { * @return A freshly created transaction for the given collection with proper * locking or a leased transaction. */ - std::unique_ptr createTransaction(std::string const& cname, - AccessMode::Type mode) const; + std::unique_ptr createTransaction(std::string const& cname, + AccessMode::Type mode) const; /// @brief create proper transaction context, including the proper IDs std::shared_ptr createTransactionContext() const; diff --git a/arangod/StorageEngine/TransactionState.cpp b/arangod/StorageEngine/TransactionState.cpp index 475908d5ee..d9a367c010 100644 --- a/arangod/StorageEngine/TransactionState.cpp +++ b/arangod/StorageEngine/TransactionState.cpp @@ -84,6 +84,24 @@ TransactionCollection* TransactionState::collection(TRI_voc_cid_t cid, return trxCollection; } +/// @brief return the collection from a transaction +TransactionCollection* TransactionState::collection(std::string const& name, + AccessMode::Type accessType) const { + TRI_ASSERT(_status == transaction::Status::CREATED || + _status == transaction::Status::RUNNING); + + auto it = std::find_if(_collections.begin(), _collections.end(), [&name](TransactionCollection const* trxColl) { + return trxColl->collectionName() == name; + }); + + if (it == _collections.end() || !(*it)->canAccess(accessType)) { + // not found or not accessible in the requested mode + return nullptr; + } + + return (*it); +} + TransactionState::Cookie* TransactionState::cookie(void const* key) noexcept { auto itr = _cookies.find(key); @@ -202,7 +220,7 @@ void TransactionState::allCollections( // iterate } } } - + /// @brief use all participating collections of a transaction Result TransactionState::useCollections(int nestingLevel) { Result res; diff --git a/arangod/StorageEngine/TransactionState.h b/arangod/StorageEngine/TransactionState.h index 668ab58756..93d23474f0 100644 --- a/arangod/StorageEngine/TransactionState.h +++ b/arangod/StorageEngine/TransactionState.h @@ -137,6 +137,10 @@ class TransactionState { /// @brief return the collection from a transaction TransactionCollection* collection(TRI_voc_cid_t cid, AccessMode::Type accessType) const; + + /// @brief return the collection from a transaction + TransactionCollection* collection(std::string const& name, + AccessMode::Type accessType) const; /// @brief add a collection to a transaction Result addCollection(TRI_voc_cid_t cid, std::string const& cname, @@ -150,7 +154,7 @@ class TransactionState { /// @brief run a callback on all collections of the transaction void allCollections(std::function const& cb); - + /// @brief return the number of collections in the transaction size_t numCollections() const { return _collections.size(); } diff --git a/arangod/Transaction/Manager.cpp b/arangod/Transaction/Manager.cpp index 87d7680f97..b61b7e23d6 100644 --- a/arangod/Transaction/Manager.cpp +++ b/arangod/Transaction/Manager.cpp @@ -199,10 +199,10 @@ uint64_t Manager::getActiveTransactionCount() { Manager::ManagedTrx::ManagedTrx(MetaType t, TransactionState* st) : type(t), + finalStatus(Status::UNDEFINED), usedTimeSecs(TRI_microtime()), state(st), user(::currentUser()), - finalStatus(Status::UNDEFINED), rwlock() {} bool Manager::ManagedTrx::expired() const { @@ -714,6 +714,8 @@ Result Manager::updateTransaction(TRI_voc_tid_t tid, transaction::Status status, "transaction was not running"); } + bool const isCoordinator = state->isCoordinator(); + auto ctx = std::make_shared(tid, state.get(), AccessMode::Type::NONE); state.release(); // now owned by ctx @@ -723,7 +725,7 @@ Result Manager::updateTransaction(TRI_voc_tid_t tid, transaction::Status status, TRI_ASSERT(trx.state()->nestingLevel() == 1); trx.state()->decreaseNesting(); TRI_ASSERT(trx.state()->isTopLevelTransaction()); - if (clearServers) { + if (clearServers && !isCoordinator) { trx.state()->clearKnownServers(); } if (status == transaction::Status::COMMITTED) { diff --git a/arangod/Transaction/Manager.h b/arangod/Transaction/Manager.h index ed03f17c43..8a1a7d0ff7 100644 --- a/arangod/Transaction/Manager.h +++ b/arangod/Transaction/Manager.h @@ -75,13 +75,13 @@ class Manager final { public: MetaType type; /// managed, AQL or tombstone - double usedTimeSecs; /// last time used - TransactionState* state; /// Transaction, may be nullptr - std::string user; /// user owning the transaction /// @brief final TRX state that is valid if this is a tombstone /// necessary to avoid getting error on a 'diamond' commit or accidantally /// repeated commit / abort messages transaction::Status finalStatus; + double usedTimeSecs; /// last time used + TransactionState* state; /// Transaction, may be nullptr + std::string user; /// user owning the transaction /// cheap usage lock for *state mutable basics::ReadWriteSpinLock rwlock; }; diff --git a/arangod/Transaction/Methods.cpp b/arangod/Transaction/Methods.cpp index 10ec272b83..42a0213a62 100644 --- a/arangod/Transaction/Methods.cpp +++ b/arangod/Transaction/Methods.cpp @@ -826,6 +826,15 @@ TransactionCollection* transaction::Methods::trxCollection(TRI_voc_cid_t cid, return _state->collection(cid, type); } +/// @brief return the transaction collection for a document collection +TransactionCollection* transaction::Methods::trxCollection(std::string const& name, + AccessMode::Type type) const { + TRI_ASSERT(_state != nullptr); + TRI_ASSERT(_state->status() == transaction::Status::RUNNING || + _state->status() == transaction::Status::CREATED); + return _state->collection(name, type); +} + /// @brief order a ditch for a collection void transaction::Methods::pinData(TRI_voc_cid_t cid) { TRI_ASSERT(_state != nullptr); @@ -1104,16 +1113,24 @@ TRI_voc_cid_t transaction::Methods::addCollectionAtRuntime(TRI_voc_cid_t cid, res = applyDataSourceRegistrationCallbacks(*dataSource, *this); + if (res.ok()) { + res = _state->ensureCollections(_state->nestingLevel()); + } if (res.fail()) { THROW_ARANGO_EXCEPTION(res); } - - _state->ensureCollections(_state->nestingLevel()); collection = trxCollection(cid); if (collection == nullptr) { throwCollectionNotFound(cname.c_str()); } + } else { + AccessMode::Type collectionAccessType = collection->accessType(); + if (AccessMode::isRead(collectionAccessType) && !AccessMode::isRead(type)) { + THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_TRANSACTION_UNREGISTERED_COLLECTION, + std::string(TRI_errno_string(TRI_ERROR_TRANSACTION_UNREGISTERED_COLLECTION)) + ": " + cname + + " [" + AccessMode::typeString(type) + "]"); + } } TRI_ASSERT(collection != nullptr); @@ -1121,7 +1138,8 @@ TRI_voc_cid_t transaction::Methods::addCollectionAtRuntime(TRI_voc_cid_t cid, } /// @brief add a collection to the transaction for read, at runtime -TRI_voc_cid_t transaction::Methods::addCollectionAtRuntime(std::string const& collectionName) { +TRI_voc_cid_t transaction::Methods::addCollectionAtRuntime(std::string const& collectionName, + AccessMode::Type type) { if (collectionName == _collectionCache.name && !collectionName.empty()) { return _collectionCache.cid; } @@ -1131,7 +1149,7 @@ TRI_voc_cid_t transaction::Methods::addCollectionAtRuntime(std::string const& co if (cid == 0) { throwCollectionNotFound(collectionName.c_str()); } - addCollectionAtRuntime(cid, collectionName); + addCollectionAtRuntime(cid, collectionName, type); _collectionCache.cid = cid; _collectionCache.name = collectionName; return cid; @@ -1164,8 +1182,9 @@ void transaction::Methods::invokeOnAllElements(std::string const& collectionName THROW_ARANGO_EXCEPTION(TRI_ERROR_NOT_IMPLEMENTED); } - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::READ); TransactionCollection* trxCol = trxCollection(cid, AccessMode::Type::READ); + TRI_ASSERT(trxCol != nullptr); LogicalCollection* collection = documentCollection(trxCol); TRI_ASSERT(collection != nullptr); _transactionContextPtr->pinData(collection); @@ -1218,7 +1237,7 @@ Result transaction::Methods::documentFastPath(std::string const& collectionName, return Result(); } - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::READ); LogicalCollection* collection = documentCollection(trxCollection(cid)); pinData(cid); // will throw when it fails @@ -1264,8 +1283,9 @@ Result transaction::Methods::documentFastPathLocal(std::string const& collection TRI_ASSERT(!ServerState::instance()->isCoordinator()); TRI_ASSERT(_state->status() == transaction::Status::RUNNING); - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::READ); TransactionCollection* trxColl = trxCollection(cid); + TRI_ASSERT(trxColl != nullptr); LogicalCollection* collection = documentCollection(trxColl); TRI_ASSERT(collection != nullptr); _transactionContextPtr->pinData(collection); // will throw when it fails @@ -1491,7 +1511,7 @@ OperationResult transaction::Methods::documentCoordinator(std::string const& col OperationResult transaction::Methods::documentLocal(std::string const& collectionName, VPackSlice const value, OperationOptions& options) { - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::READ); LogicalCollection* collection = documentCollection(trxCollection(cid)); if (!options.silent) { @@ -1647,7 +1667,7 @@ static double chooseTimeout(size_t count, size_t totalBytes) { OperationResult transaction::Methods::insertLocal(std::string const& collectionName, VPackSlice const value, OperationOptions& options) { - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::WRITE); LogicalCollection* collection = documentCollection(trxCollection(cid)); bool const needsLock = !isLocked(collection, AccessMode::Type::WRITE); @@ -1995,7 +2015,7 @@ OperationResult transaction::Methods::modifyLocal(std::string const& collectionN VPackSlice const newValue, OperationOptions& options, TRI_voc_document_operation_e operation) { - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::WRITE); LogicalCollection* collection = documentCollection(trxCollection(cid)); bool const needsLock = !isLocked(collection, AccessMode::Type::WRITE); @@ -2263,7 +2283,7 @@ OperationResult transaction::Methods::removeCoordinator(std::string const& colle OperationResult transaction::Methods::removeLocal(std::string const& collectionName, VPackSlice const value, OperationOptions& options) { - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::WRITE); LogicalCollection* collection = documentCollection(trxCollection(cid)); bool const needsLock = !isLocked(collection, AccessMode::Type::WRITE); @@ -2487,7 +2507,7 @@ OperationResult transaction::Methods::allCoordinator(std::string const& collecti OperationResult transaction::Methods::allLocal(std::string const& collectionName, uint64_t skip, uint64_t limit, OperationOptions& options) { - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::READ); pinData(cid); // will throw when it fails @@ -2550,7 +2570,7 @@ OperationResult transaction::Methods::truncateCoordinator(std::string const& col /// @brief remove all documents in a collection, local OperationResult transaction::Methods::truncateLocal(std::string const& collectionName, OperationOptions& options) { - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::WRITE); LogicalCollection* collection = documentCollection(trxCollection(cid)); @@ -2604,7 +2624,6 @@ OperationResult transaction::Methods::truncateLocal(std::string const& collectio TRI_ASSERT(isLocked(collection, AccessMode::Type::WRITE)); auto res = collection->truncate(*this, options); - ; if (res.fail()) { if (lockResult.is(TRI_ERROR_LOCKED)) { @@ -2765,7 +2784,7 @@ OperationResult transaction::Methods::countCoordinatorHelper( /// @brief count the number of documents in a collection OperationResult transaction::Methods::countLocal(std::string const& collectionName, transaction::CountType type) { - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::READ); LogicalCollection* collection = documentCollection(trxCollection(cid)); Result lockResult = lockRecursive(cid, AccessMode::Type::READ); @@ -3006,7 +3025,7 @@ std::unique_ptr transaction::Methods::indexScan(std::string const THROW_ARANGO_EXCEPTION(TRI_ERROR_CLUSTER_ONLY_ON_DBSERVER); } - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::READ); TransactionCollection* trxColl = trxCollection(cid); if (trxColl == nullptr) { THROW_ARANGO_EXCEPTION_MESSAGE( @@ -3062,6 +3081,22 @@ arangodb::LogicalCollection* transaction::Methods::documentCollection(TRI_voc_ci return trxColl->collection().get(); } +/// @brief return the collection +arangodb::LogicalCollection* transaction::Methods::documentCollection(std::string const& name) const { + TRI_ASSERT(_state != nullptr); + TRI_ASSERT(_state->status() == transaction::Status::RUNNING); + + auto trxColl = trxCollection(name, AccessMode::Type::READ); + if (trxColl == nullptr) { + THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, + "could not find collection"); + } + + TRI_ASSERT(trxColl != nullptr); + TRI_ASSERT(trxColl->collection() != nullptr); + return trxColl->collection().get(); +} + /// @brief add a collection by id, with the name supplied Result transaction::Methods::addCollection(TRI_voc_cid_t cid, std::string const& cname, AccessMode::Type type) { @@ -3183,7 +3218,7 @@ std::vector> transaction::Methods::indexesForCollection( } // For a DBserver we use the local case. - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::READ); LogicalCollection* document = documentCollection(trxCollection(cid)); std::vector> indexes = document->getIndexes(); if (!withHidden) { @@ -3253,7 +3288,7 @@ transaction::Methods::IndexHandle transaction::Methods::getIndexByIdentifier( return IndexHandle(idx); } - TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName); + TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::READ); LogicalCollection* document = documentCollection(trxCollection(cid)); if (indexHandle.empty()) { diff --git a/arangod/Transaction/Methods.h b/arangod/Transaction/Methods.h index f01aa76642..d188642ba7 100644 --- a/arangod/Transaction/Methods.h +++ b/arangod/Transaction/Methods.h @@ -248,10 +248,11 @@ class Methods { /// @brief add a collection to the transaction for read, at runtime ENTERPRISE_VIRT TRI_voc_cid_t addCollectionAtRuntime(TRI_voc_cid_t cid, std::string const& collectionName, - AccessMode::Type type = AccessMode::Type::READ); + AccessMode::Type type); /// @brief add a collection to the transaction for read, at runtime - virtual TRI_voc_cid_t addCollectionAtRuntime(std::string const& collectionName); + virtual TRI_voc_cid_t addCollectionAtRuntime(std::string const& collectionName, + AccessMode::Type type); /// @brief return the type of a collection bool isEdgeCollection(std::string const& collectionName) const; @@ -379,7 +380,10 @@ class Methods { ENTERPRISE_VIRT bool isLocked(arangodb::LogicalCollection*, AccessMode::Type) const; /// @brief fetch the LogicalCollection by CID - arangodb::LogicalCollection* documentCollection(TRI_voc_cid_t) const; + arangodb::LogicalCollection* documentCollection(TRI_voc_cid_t cid) const; + + /// @brief fetch the LogicalCollection by name + arangodb::LogicalCollection* documentCollection(std::string const& name) const; /// @brief get the index by its identifier. Will either throw or /// return a valid index. nullptr is impossible. @@ -478,6 +482,9 @@ class Methods { /// @brief return the transaction collection for a document collection ENTERPRISE_VIRT TransactionCollection* trxCollection( TRI_voc_cid_t cid, AccessMode::Type type = AccessMode::Type::READ) const; + + TransactionCollection* trxCollection( + std::string const& name, AccessMode::Type type = AccessMode::Type::READ) const; OperationResult countCoordinator(std::string const& collectionName, CountType type); diff --git a/arangod/Utils/SingleCollectionTransaction.cpp b/arangod/Utils/SingleCollectionTransaction.cpp index d95f4583f4..21267b04e5 100644 --- a/arangod/Utils/SingleCollectionTransaction.cpp +++ b/arangod/Utils/SingleCollectionTransaction.cpp @@ -22,6 +22,7 @@ //////////////////////////////////////////////////////////////////////////////// #include "SingleCollectionTransaction.h" +#include "Basics/StringUtils.h" #include "StorageEngine/TransactionCollection.h" #include "StorageEngine/TransactionState.h" #include "Transaction/Context.h" @@ -59,7 +60,7 @@ SingleCollectionTransaction::SingleCollectionTransaction( _accessType(accessType) { // add the (sole) collection _cid = resolver()->getCollectionId(name); - Result res = addCollection(_cid, name.c_str(), _accessType); + Result res = addCollection(_cid, name, _accessType); if (res.fail()) { THROW_ARANGO_EXCEPTION(res); } @@ -94,6 +95,19 @@ LogicalCollection* SingleCollectionTransaction::documentCollection() { TRI_ASSERT(_documentCollection != nullptr); return _documentCollection; } + +TRI_voc_cid_t SingleCollectionTransaction::addCollectionAtRuntime(std::string const& name, + AccessMode::Type type) { + // sanity check + TRI_ASSERT(!name.empty()); + if ((name[0] < '0' || name[0] > '9') && + name != resolveTrxCollection()->collectionName()) { + THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_TRANSACTION_UNREGISTERED_COLLECTION, + std::string(TRI_errno_string(TRI_ERROR_TRANSACTION_UNREGISTERED_COLLECTION)) + ": " + name); + } + + return _cid; +} /// @brief get the underlying collection's name std::string SingleCollectionTransaction::name() { diff --git a/arangod/Utils/SingleCollectionTransaction.h b/arangod/Utils/SingleCollectionTransaction.h index bf9f486919..5abf4746ba 100644 --- a/arangod/Utils/SingleCollectionTransaction.h +++ b/arangod/Utils/SingleCollectionTransaction.h @@ -25,6 +25,7 @@ #define ARANGOD_UTILS_SINGLE_COLLECTION_TRANSACTION_H 1 #include "Basics/Common.h" +#include "StorageEngine/TransactionCollection.h" #include "Transaction/Methods.h" #include "VocBase/AccessMode.h" #include "VocBase/voc-types.h" @@ -63,9 +64,7 @@ class SingleCollectionTransaction final : public transaction::Methods { #endif /// @brief add a collection to the transaction for read, at runtime /// note that this can only be ourselves - TRI_voc_cid_t addCollectionAtRuntime(std::string const&) override final { - return _cid; - } + TRI_voc_cid_t addCollectionAtRuntime(std::string const& name, AccessMode::Type type) override final; /// @brief get the underlying collection's name std::string name(); diff --git a/js/server/modules/@arangodb/foxx/manager.js b/js/server/modules/@arangodb/foxx/manager.js index 2fe3fcbb86..b729c8827f 100644 --- a/js/server/modules/@arangodb/foxx/manager.js +++ b/js/server/modules/@arangodb/foxx/manager.js @@ -167,7 +167,7 @@ function selfHeal () { const serviceCollection = utils.getStorage(); const bundleCollection = utils.getBundleStorage(); const serviceDefinitions = db._query(aql` - FOR doc IN ${serviceCollection} + WITH ${bundleCollection} FOR doc IN ${serviceCollection} FILTER LEFT(doc.mount, 2) != "/_" LET bundleExists = DOCUMENT(${bundleCollection}, doc.checksum) != null RETURN [doc.mount, doc.checksum, doc._rev, bundleExists] diff --git a/tests/IResearch/IResearchAnalyzerFeature-test.cpp b/tests/IResearch/IResearchAnalyzerFeature-test.cpp index 7c04728066..7fa14ad5aa 100644 --- a/tests/IResearch/IResearchAnalyzerFeature-test.cpp +++ b/tests/IResearch/IResearchAnalyzerFeature-test.cpp @@ -3268,7 +3268,7 @@ TEST_F(IResearchAnalyzerFeatureTest, test_upgrade_static_legacy) { LEGACY_ANALYZER_COLLECTION_NAME, arangodb::AccessMode::Type::WRITE); EXPECT_TRUE((true == trx.begin().ok())); EXPECT_TRUE( - (true == trx.insert(arangodb::tests::AnalyzerCollectionName, + (true == trx.insert(LEGACY_ANALYZER_COLLECTION_NAME, VPackParser::fromJson("{\"name\": \"legacy\"}")->slice(), options) .ok())); EXPECT_TRUE((trx.commit().ok())); diff --git a/tests/js/client/shell/shell-graph-transaction.js b/tests/js/client/shell/shell-graph-transaction.js index 34b6862656..34625fb0c3 100644 --- a/tests/js/client/shell/shell-graph-transaction.js +++ b/tests/js/client/shell/shell-graph-transaction.js @@ -136,7 +136,7 @@ function JSTransactionGraphSuite () { db[vertex].insert({ _key: "test", value: "test" }); db._executeTransaction({ - collections: { write: vertex }, + collections: { write: [vertex, edge] }, action: function(params) { let graph = require("@arangodb/general-graph")._graph(params.graph); let ERRORS = require("@arangodb").errors; diff --git a/tests/js/client/shell/shell-transaction.js b/tests/js/client/shell/shell-transaction.js index 06d436babc..24fb154797 100644 --- a/tests/js/client/shell/shell-transaction.js +++ b/tests/js/client/shell/shell-transaction.js @@ -1336,8 +1336,6 @@ function transactionCollectionsSuite () { } assertEqual(10, tc1.count()); - assertEqual(10, tc2.count()); - } catch(err) { fail(); } finally {