1
0
Fork 0

Bug fix/gharial edge from and tos verification (#9177)

* prepared new functions for more precise edge verification

* added tests

* more testing

* validate function added to replace edge functionality

* changelog

* applied requested mchackis refactor wishes

* adjustments for update and replace edge validation

* ruby testing adjustments due more prices update and replace edge verification mechanisms

* used wrong flag for the replacement of an edge process

* adjusted last failing tests

* rm print

* added edge to edge tests
This commit is contained in:
Heiko 2019-06-12 15:12:50 +02:00 committed by Michael Hackstein
parent a6b49e3a7e
commit 56c969316f
5 changed files with 1646 additions and 897 deletions

View File

@ -1,6 +1,9 @@
devel
-----
* fixed internal issue #4040: gharial api is now checking existence of `_from` and `_to` vertices
during edge replacements and edge updates
* fixed `Buffer.alloc` method
* `Buffer` is now iterable and accepts `ArrayBuffer` values as input
@ -18,6 +21,7 @@ devel
* fixed agency bug with TTL object writes discovered in 3.4.6
v3.5.0-rc.3 (2019-05-31)
------------------------

View File

@ -22,6 +22,7 @@
#include "GraphOperations.h"
#include <arangod/Transaction/V8Context.h>
#include <velocypack/Buffer.h>
#include <velocypack/Collection.h>
#include <velocypack/Iterator.h>
@ -156,9 +157,9 @@ OperationResult GraphOperations::eraseEdgeDefinition(bool waitForSync, std::stri
for (auto const& collection : collectionsToBeRemoved) {
Result resIn;
Result found = methods::Collections::lookup(
_vocbase,// vocbase to search
collection, // collection to find
[&](std::shared_ptr<LogicalCollection> const& coll)->void { // callback if found
_vocbase, // vocbase to search
collection, // collection to find
[&](std::shared_ptr<LogicalCollection> const& coll) -> void { // callback if found
TRI_ASSERT(coll);
resIn = methods::Collections::drop(*coll, false, -1.0);
});
@ -407,9 +408,9 @@ OperationResult GraphOperations::eraseOrphanCollection(bool waitForSync, std::st
for (auto const& collection : collectionsToBeRemoved) {
Result resIn;
Result found = methods::Collections::lookup(
_vocbase, // vocbase to search
collection, // collection to find
[&](std::shared_ptr<LogicalCollection> const& coll)->void { // callback if found
_vocbase, // vocbase to search
collection, // collection to find
[&](std::shared_ptr<LogicalCollection> const& coll) -> void { // callback if found
TRI_ASSERT(coll);
resIn = methods::Collections::drop(*coll, false, -1.0);
});
@ -525,22 +526,10 @@ OperationResult GraphOperations::removeEdge(const std::string& definitionName,
return removeEdgeOrVertex(definitionName, key, rev, waitForSync, returnOld);
}
OperationResult GraphOperations::modifyDocument(std::string const& collectionName,
std::string const& key,
VPackSlice document, bool isPatch,
boost::optional<TRI_voc_rid_t> rev,
bool waitForSync, bool returnOld,
bool returnNew, bool keepNull) {
OperationOptions options;
options.ignoreRevs = !rev.is_initialized();
options.waitForSync = waitForSync;
options.returnNew = returnNew;
options.returnOld = returnOld;
if (isPatch) {
options.keepNull = keepNull;
// options.mergeObjects = true;
}
OperationResult GraphOperations::modifyDocument(
std::string const& collectionName, std::string const& key, VPackSlice document,
bool isPatch, boost::optional<TRI_voc_rid_t> rev, bool waitForSync,
bool returnOld, bool returnNew, bool keepNull, transaction::Methods& trx) {
// extract the revision, if single document variant and header given:
std::unique_ptr<VPackBuilder> builder;
@ -560,25 +549,21 @@ OperationResult GraphOperations::modifyDocument(std::string const& collectionNam
document = builder->slice();
}
// find and load collection given by name or identifier
SingleCollectionTransaction trx(ctx(), collectionName, AccessMode::Type::WRITE);
trx.addHint(transaction::Hints::Hint::SINGLE_OPERATION);
Result res = trx.begin();
if (!res.ok()) {
return OperationResult(res);
}
OperationOptions options;
options.ignoreRevs = !rev.is_initialized();
options.waitForSync = waitForSync;
options.returnNew = returnNew;
options.returnOld = returnOld;
OperationResult result;
if (isPatch) {
options.keepNull = keepNull;
result = trx.update(collectionName, document, options);
} else {
result = trx.replace(collectionName, document, options);
}
res = trx.finish(result.result);
Result res = trx.finish(result.result);
if (result.ok() && res.fail()) {
return OperationResult(res);
@ -605,8 +590,16 @@ OperationResult GraphOperations::updateEdge(const std::string& definitionName,
boost::optional<TRI_voc_rid_t> rev,
bool waitForSync, bool returnOld,
bool returnNew, bool keepNull) {
OperationResult res;
std::unique_ptr<transaction::Methods> trx;
std::tie(res, trx) = validateEdge(definitionName, document, waitForSync, true);
if (res.fail()) {
return res;
}
TRI_ASSERT(trx != nullptr);
return modifyDocument(definitionName, key, document, true, std::move(rev),
waitForSync, returnOld, returnNew, keepNull);
waitForSync, returnOld, returnNew, keepNull, *trx.get());
}
OperationResult GraphOperations::replaceEdge(const std::string& definitionName,
@ -614,56 +607,74 @@ OperationResult GraphOperations::replaceEdge(const std::string& definitionName,
boost::optional<TRI_voc_rid_t> rev,
bool waitForSync, bool returnOld,
bool returnNew, bool keepNull) {
OperationResult res;
std::unique_ptr<transaction::Methods> trx;
std::tie(res, trx) = validateEdge(definitionName, document, waitForSync, false);
if (res.fail()) {
return res;
}
TRI_ASSERT(trx != nullptr);
return modifyDocument(definitionName, key, document, false, std::move(rev),
waitForSync, returnOld, returnNew, keepNull);
waitForSync, returnOld, returnNew, keepNull, *trx.get());
}
OperationResult GraphOperations::createEdge(const std::string& definitionName,
VPackSlice document,
bool waitForSync, bool returnNew) {
VPackSlice fromStringSlice = document.get(StaticStrings::FromString);
VPackSlice toStringSlice = document.get(StaticStrings::ToString);
if (fromStringSlice.isNone() || toStringSlice.isNone()) {
return OperationResult(TRI_ERROR_ARANGO_INVALID_EDGE_ATTRIBUTE);
}
std::string fromString = fromStringSlice.copyString();
std::string toString = toStringSlice.copyString();
size_t pos = fromString.find('/');
std::pair<OperationResult, std::unique_ptr<transaction::Methods>> GraphOperations::validateEdge(
const std::string& definitionName, const VPackSlice& document,
bool waitForSync, bool isUpdate) {
std::string fromCollectionName;
std::string fromCollectionKey;
if (pos != std::string::npos) {
fromCollectionName = fromString.substr(0, pos);
fromCollectionKey = fromString.substr(pos + 1, fromString.length());
} else {
return OperationResult(TRI_ERROR_ARANGO_INVALID_EDGE_ATTRIBUTE);
}
pos = toString.find('/');
std::string toCollectionName;
std::string toCollectionKey;
if (pos != std::string::npos) {
toCollectionName = toString.substr(0, pos);
toCollectionKey = toString.substr(pos + 1, toString.length());
} else {
return OperationResult(TRI_ERROR_ARANGO_INVALID_EDGE_ATTRIBUTE);
OperationResult res;
bool foundEdgeDefinition;
std::tie(res, foundEdgeDefinition) =
validateEdgeContent(document, fromCollectionName, fromCollectionKey,
toCollectionName, toCollectionKey, isUpdate);
if (res.fail()) {
return std::make_pair(std::move(res), nullptr);
}
// check if vertex collections are part of the graph definition
auto it = _graph.vertexCollections().find(fromCollectionName);
if (it == _graph.vertexCollections().end()) {
// not found from vertex
return OperationResult(TRI_ERROR_ARANGO_DATA_SOURCE_NOT_FOUND);
std::vector<std::string> readCollections;
std::vector<std::string> writeCollections;
std::vector<std::string> exclusiveCollections;
if (foundEdgeDefinition) {
readCollections.emplace_back(fromCollectionName);
readCollections.emplace_back(toCollectionName);
}
it = _graph.vertexCollections().find(toCollectionName);
if (it == _graph.vertexCollections().end()) {
// not found to vertex
return OperationResult(TRI_ERROR_ARANGO_DATA_SOURCE_NOT_FOUND);
writeCollections.emplace_back(definitionName);
transaction::Options trxOptions;
trxOptions.waitForSync = waitForSync;
auto trx = std::make_unique<transaction::Methods>(ctx(), readCollections, writeCollections,
exclusiveCollections, trxOptions);
Result tRes = trx->begin();
if (!tRes.ok()) {
return std::make_pair(OperationResult(tRes), nullptr);
}
OperationOptions options;
if (foundEdgeDefinition) {
res = validateEdgeVertices(fromCollectionName, fromCollectionKey,
toCollectionName, toCollectionKey, *trx.get());
if (res.fail()) {
return std::make_pair(std::move(res), nullptr);
}
}
return std::make_pair(std::move(res), std::move(trx));
}
OperationResult GraphOperations::validateEdgeVertices(
const std::string& fromCollectionName, const std::string& fromCollectionKey,
const std::string& toCollectionName, const std::string& toCollectionKey,
transaction::Methods& trx) {
VPackBuilder bT;
{
VPackObjectBuilder guard(&bT);
@ -676,30 +687,79 @@ OperationResult GraphOperations::createEdge(const std::string& definitionName,
bF.add(StaticStrings::KeyString, VPackValue(fromCollectionKey));
}
std::vector<std::string> readCollections;
std::vector<std::string> writeCollections;
readCollections.emplace_back(fromCollectionName);
readCollections.emplace_back(toCollectionName);
writeCollections.emplace_back(definitionName);
transaction::Options trxOptions;
trxOptions.waitForSync = waitForSync;
transaction::Methods trx(ctx(), readCollections, writeCollections, {}, trxOptions);
Result res = trx.begin();
if (!res.ok()) {
return OperationResult(TRI_ERROR_ARANGO_DOCUMENT_NOT_FOUND);
}
OperationOptions options;
OperationResult resultFrom = trx.document(fromCollectionName, bF.slice(), options);
OperationResult resultTo = trx.document(toCollectionName, bT.slice(), options);
if (!resultFrom.ok() || !resultTo.ok()) {
// actual result doesn't matter here
trx.finish(Result());
// actual result doesn't matter here
if (!resultFrom.ok()) {
trx.finish(resultFrom.result);
return OperationResult(TRI_ERROR_ARANGO_DOCUMENT_NOT_FOUND);
} else if (!resultTo.ok()) {
trx.finish(resultTo.result);
return OperationResult(TRI_ERROR_ARANGO_DOCUMENT_NOT_FOUND);
}
return createDocument(&trx, definitionName, document, waitForSync, returnNew);
return OperationResult(TRI_ERROR_NO_ERROR);
}
std::pair<OperationResult, bool> GraphOperations::validateEdgeContent(
const VPackSlice& document, std::string& fromCollectionName, std::string& fromCollectionKey,
std::string& toCollectionName, std::string& toCollectionKey, bool isUpdate) {
VPackSlice fromStringSlice = document.get(StaticStrings::FromString);
VPackSlice toStringSlice = document.get(StaticStrings::ToString);
if (fromStringSlice.isNone() || toStringSlice.isNone()) {
if (isUpdate) {
return std::make_pair(OperationResult(TRI_ERROR_NO_ERROR), false);
}
return std::make_pair(OperationResult(TRI_ERROR_ARANGO_INVALID_EDGE_ATTRIBUTE), false);
}
std::string fromString = fromStringSlice.copyString();
std::string toString = toStringSlice.copyString();
size_t pos = fromString.find('/');
if (pos != std::string::npos) {
fromCollectionName = fromString.substr(0, pos);
fromCollectionKey = fromString.substr(pos + 1, fromString.length());
} else {
return std::make_pair(OperationResult(TRI_ERROR_ARANGO_INVALID_EDGE_ATTRIBUTE), true);
}
pos = toString.find('/');
if (pos != std::string::npos) {
toCollectionName = toString.substr(0, pos);
toCollectionKey = toString.substr(pos + 1, toString.length());
} else {
return std::make_pair(OperationResult(TRI_ERROR_ARANGO_INVALID_EDGE_ATTRIBUTE), true);
}
// check if vertex collections are part of the graph definition
auto it = _graph.vertexCollections().find(fromCollectionName);
if (it == _graph.vertexCollections().end()) {
// not found from vertex
return std::make_pair(OperationResult(TRI_ERROR_ARANGO_DATA_SOURCE_NOT_FOUND), true);
}
it = _graph.vertexCollections().find(toCollectionName);
if (it == _graph.vertexCollections().end()) {
// not found to vertex
return std::make_pair(OperationResult(TRI_ERROR_ARANGO_DATA_SOURCE_NOT_FOUND), true);
}
return std::make_pair(OperationResult(TRI_ERROR_NO_ERROR), true);
}
OperationResult GraphOperations::createEdge(const std::string& definitionName,
VPackSlice document,
bool waitForSync, bool returnNew) {
OperationResult res;
std::unique_ptr<transaction::Methods> trx;
std::tie(res, trx) = validateEdge(definitionName, document, waitForSync, false);
if (res.fail()) {
return res;
}
TRI_ASSERT(trx != nullptr);
return createDocument(&(*trx.get()), definitionName, document, waitForSync, returnNew);
}
OperationResult GraphOperations::updateVertex(const std::string& collectionName,
@ -707,8 +767,20 @@ OperationResult GraphOperations::updateVertex(const std::string& collectionName,
boost::optional<TRI_voc_rid_t> rev,
bool waitForSync, bool returnOld,
bool returnNew, bool keepNull) {
std::vector<std::string> writeCollections;
writeCollections.emplace_back(collectionName);
transaction::Options trxOptions;
trxOptions.waitForSync = waitForSync;
transaction::Methods trx(ctx(), {}, writeCollections, {}, trxOptions);
Result tRes = trx.begin();
if (!tRes.ok()) {
return OperationResult(tRes);
}
return modifyDocument(collectionName, key, document, true, std::move(rev),
waitForSync, returnOld, returnNew, keepNull);
waitForSync, returnOld, returnNew, keepNull, trx);
}
OperationResult GraphOperations::replaceVertex(const std::string& collectionName,
@ -716,8 +788,20 @@ OperationResult GraphOperations::replaceVertex(const std::string& collectionName
boost::optional<TRI_voc_rid_t> rev,
bool waitForSync, bool returnOld,
bool returnNew, bool keepNull) {
std::vector<std::string> writeCollections;
writeCollections.emplace_back(collectionName);
transaction::Options trxOptions;
trxOptions.waitForSync = waitForSync;
transaction::Methods trx(ctx(), {}, writeCollections, {}, trxOptions);
Result tRes = trx.begin();
if (!tRes.ok()) {
return OperationResult(tRes);
}
return modifyDocument(collectionName, key, document, false, std::move(rev),
waitForSync, returnOld, returnNew, keepNull);
waitForSync, returnOld, returnNew, keepNull, trx);
}
OperationResult GraphOperations::createVertex(const std::string& collectionName,
@ -769,7 +853,7 @@ OperationResult GraphOperations::removeEdgeOrVertex(const std::string& collectio
trxCollections.emplace_back(collectionName);
CollectionNameResolver resolver {_vocbase};
CollectionNameResolver resolver{_vocbase};
for (auto const& it : edgeCollections) {
trxCollections.emplace_back(it);
auto col = resolver.getCollection(it);
@ -783,7 +867,7 @@ OperationResult GraphOperations::removeEdgeOrVertex(const std::string& collectio
trxCollections.emplace_back(it); // add to trx collections
edgeCollections.emplace(it); // but also to edgeCollections for later iteration
}
auto ctx = std::make_shared<transaction::StandaloneSmartContext>(_vocbase);
transaction::Options trxOptions;
trxOptions.waitForSync = waitForSync;
@ -815,10 +899,10 @@ OperationResult GraphOperations::removeEdgeOrVertex(const std::string& collectio
arangodb::aql::Query query(false, _vocbase, queryString, bindVars,
nullptr, arangodb::aql::PART_DEPENDENT);
query.setTransactionContext(ctx); // hack to share the same transaction
query.setTransactionContext(ctx); // hack to share the same transaction
auto queryResult = query.executeSync(QueryRegistryFeature::registry());
if (queryResult.result.fail()) {
return OperationResult(std::move(queryResult.result));
}
@ -906,8 +990,9 @@ Result GraphOperations::checkEdgeDefinitionPermissions(EdgeDefinition const& edg
return TRI_ERROR_FORBIDDEN;
}
if (!collectionExists(col) && !canUseDatabaseRW) {
LOG_TOPIC("2bcf2", DEBUG, Logger::GRAPHS) << logprefix << "Creation of " << databaseName
<< "." << col << " is not allowed.";
LOG_TOPIC("2bcf2", DEBUG, Logger::GRAPHS)
<< logprefix << "Creation of " << databaseName << "." << col
<< " is not allowed.";
return TRI_ERROR_FORBIDDEN;
}
}

View File

@ -106,6 +106,27 @@ class GraphOperations {
OperationResult createEdge(const std::string& definitionName, VPackSlice document,
bool waitForSync, bool returnNew);
// @brief This function is a helper function which is setting up a transaction
// and calls validateEdgeVertices and validateEdgeContent methods.
std::pair<OperationResult, std::unique_ptr<transaction::Methods>> validateEdge(
const std::string& definitionName, const VPackSlice& document,
bool waitForSync, bool isUpdate);
// @brief This function is checking whether the given _from and _to vertex documents are available or not
OperationResult validateEdgeVertices(const std::string& fromCollectionName,
const std::string& fromCollectionKey,
const std::string& toCollectionName,
const std::string& toCollectionKey,
transaction::Methods& trx);
// @brief This function is checking whether the given document defines _from and _to attributes or not
// and checks if they are correct or invalid if they are available.
std::pair<OperationResult, bool> validateEdgeContent(const VPackSlice& document, std::string& fromCollectionName,
std::string& fromCollectionKey,
std::string& toCollectionName,
std::string& toCollectionKey,
bool isUpdate);
OperationResult updateVertex(const std::string& collectionName,
const std::string& key, VPackSlice document,
boost::optional<TRI_voc_rid_t> rev, bool waitForSync,
@ -172,9 +193,10 @@ class GraphOperations {
boost::optional<TRI_voc_rid_t>& rev) const;
OperationResult modifyDocument(const std::string& collectionName,
const std::string& key, VPackSlice document, bool isPatch,
boost::optional<TRI_voc_rid_t> rev, bool waitForSync,
bool returnOld, bool returnNew, bool keepNull);
const std::string& key, VPackSlice document,
bool isPatch, boost::optional<TRI_voc_rid_t> rev,
bool waitForSync, bool returnOld, bool returnNew,
bool keepNull, transaction::Methods& trx);
OperationResult createDocument(transaction::Methods* trx, const std::string& collectionName,
VPackSlice document, bool waitForSync, bool returnNew);

File diff suppressed because it is too large Load Diff

View File

@ -844,20 +844,29 @@ describe ArangoDB do
oldTag.should_not eq(doc.headers['etag'])
doc.parsed_response['edge']['_key'].should eq(key)
end
it "can not replace a non existing edge" do
key = "unknownKey"
# Added _from and _to, because otherwise a 400 might conceal the
# 404. Another test checking that missing _from or _to trigger
# errors was added to api-gharial-spec.js.
doc = replace_edge( sync, graph_name, friend_collection, key, {"type2" => "divorced", "_from" => "1", "_to" => "2"})
doc = replace_edge( sync, graph_name, friend_collection, key, {"type2" => "divorced", "_from" => "#{user_collection}/1", "_to" => "#{user_collection}/2"})
doc.code.should eq(404)
doc.parsed_response['error'].should eq(true)
doc.parsed_response['errorMessage'].should include("document not found")
doc.parsed_response['code'].should eq(404)
end
it "can not replace a non valid edge" do
key = "unknownKey"
doc = replace_edge( sync, graph_name, friend_collection, key, {"type2" => "divorced", "_from" => "1", "_to" => "2"})
doc.code.should eq(400)
doc.parsed_response['error'].should eq(true)
doc.parsed_response['errorMessage'].should include("edge attribute missing or invalid")
doc.parsed_response['code'].should eq(400)
end
it "can update an edge" do
v1 = create_vertex( sync, graph_name, user_collection, {})
v1.code.should eq(sync ? 201 : 202)
@ -1258,8 +1267,12 @@ describe ArangoDB do
check404CRUD(update_edge( sync, graph_name, unknown_name, unknown_name, {}))
end
it "replace edge" do
check404CRUD(replace_edge( sync, graph_name, unknown_name, unknown_name, {}))
it "replace edge (invalid key)" do
check400CRUD(replace_edge( sync, graph_name, unknown_name, unknown_name, {}))
end
it "replace edge (valid key, but not existing)" do
check404(replace_edge( sync, graph_name, user_collection + "/" + unknown_name, unknown_name, {}))
end
it "delete edge" do
@ -1278,6 +1291,22 @@ describe ArangoDB do
doc.parsed_response['errorMessage'].should include("document not found")
end
def check404Collection (doc)
doc.code.should eq(404)
doc.parsed_response['error'].should eq(true)
doc.parsed_response['code'].should eq(404)
doc.parsed_response['errorNum'].should eq(1203)
doc.parsed_response['errorMessage'].should include("collection or view not found")
end
def check400 (doc)
doc.code.should eq(400)
doc.parsed_response['error'].should eq(true)
doc.parsed_response['code'].should eq(400)
doc.parsed_response['errorNum'].should eq(1233)
doc.parsed_response['errorMessage'].should include("edge attribute missing or invalid")
end
it "get vertex" do
check404(get_vertex(graph_name, user_collection, unknown_name))
end
@ -1301,12 +1330,23 @@ describe ArangoDB do
it "update edge" do
check404(update_edge( sync, graph_name, friend_collection, unknown_name, {}))
end
it "replace edge invalid" do
check400(replace_edge( sync, graph_name, friend_collection, unknown_name, {"_from" => "1", "_to" => "2"}))
end
it "replace edge" do
it "replace edge (collection does not exist) not found" do
# Added _from and _to, because otherwise a 400 might conceal the
# 404. Another test checking that missing _from or _to trigger
# errors was added to api-gharial-spec.js.
check404(replace_edge( sync, graph_name, friend_collection, unknown_name, {"_from" => "1", "_to" => "2"}))
check404Collection(replace_edge( sync, graph_name, friend_collection, unknown_name, {"_from" => "xyz/1", "_to" => "abc/2"}))
end
it "replace edge (document does not exist) not found" do
# Added _from and _to, because otherwise a 400 might conceal the
# 404. Another test checking that missing _from or _to trigger
# errors was added to api-gharial-spec.js.
check404(replace_edge( sync, graph_name, friend_collection, unknown_name, {"_from" => "#{user_collection}/1", "_to" => "#{user_collection}/2"}))
end
it "delete edge" do