diff --git a/CHANGELOG b/CHANGELOG index 6b251f2307ac..f047d96661ef 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,12 @@ devel ----- +* Some AQL queries erroneously reported the "access after data-modification" + error for queries in which there was a read attempt from a collection + _before_ a data-modification operation. Such access is legal and should not + trigger said error anymore. Accessing a collection _after_ in a query a + data-modification in the same query is still disallowed. + * Make AQL modification operations in a cluster asynchronous. This allows to free the thread for other work until both the write and synchronous replication are complete. @@ -18,8 +24,8 @@ devel * Fixed: _api/transaction/begin called on edge collections of disjoint SmartGraphs falsely returned CollectionNotFound errors. -* Bug-Fix: In more complex queries there was a code-path where a (Disjoint-) Smart - graph access was not properly optimized. +* Bugfix: In more complex queries there was a code-path where a (Disjoint-) + Smart graph access was not properly optimized. * Improve log messages for Pregel runs by giving them more context. diff --git a/arangod/Aql/Ast.cpp b/arangod/Aql/Ast.cpp index a93726884aa0..3cb144c55547 100644 --- a/arangod/Aql/Ast.cpp +++ b/arangod/Aql/Ast.cpp @@ -2050,7 +2050,6 @@ void Ast::validateAndOptimize(transaction::Methods& trx) { TraversalContext(transaction::Methods& t) : trx(t) {} std::unordered_set writeCollectionsSeen; - std::unordered_map collectionsFirstSeen; std::unordered_map variableDefinitions; AqlFunctionsInternalCache aqlFunctionsInternalCache; transaction::Methods& trx; @@ -2096,12 +2095,7 @@ void Ast::validateAndOptimize(transaction::Methods& trx) { } else if (node->type == NODE_TYPE_COLLECTION_LIST) { // a collection list is produced by WITH a, b, c // or by traversal declarations - // we do not want to descend further, in order to not track - // the collections in collectionsFirstSeen return false; - } else if (node->type == NODE_TYPE_COLLECTION) { - // note the level on which we first saw a collection - ctx->collectionsFirstSeen.try_emplace(node->getString(), ctx->nestingLevel); } else if (node->type == NODE_TYPE_AGGREGATIONS) { ++ctx->stopOptimizationRequests; } else if (node->type == NODE_TYPE_SUBQUERY) { @@ -2157,17 +2151,6 @@ void Ast::validateAndOptimize(transaction::Methods& trx) { auto collection = node->getMember(1); std::string name = collection->getString(); ctx->writeCollectionsSeen.emplace(name); - - auto it = ctx->collectionsFirstSeen.find(name); - - if (it != ctx->collectionsFirstSeen.end()) { - if ((*it).second < ctx->nestingLevel) { - name = "collection '" + name; - name.push_back('\''); - THROW_ARANGO_EXCEPTION_PARAMS(TRI_ERROR_QUERY_ACCESS_AFTER_MODIFICATION, - name.c_str()); - } - } } else if (node->type == NODE_TYPE_FCALL) { auto func = static_cast(node->getData()); TRI_ASSERT(func != nullptr); diff --git a/tests/Aql/ReplaceExecutorTest.cpp b/tests/Aql/ReplaceExecutorTest.cpp index 499b1ed75a31..638f86b62705 100644 --- a/tests/Aql/ReplaceExecutorTest.cpp +++ b/tests/Aql/ReplaceExecutorTest.cpp @@ -340,7 +340,6 @@ TEST_P(ReplaceExecutorIntegrationTest, replace_in_subquery_multi_access) { expected.add(VPackValue(i)); } } - AssertQueryFailsWith(vocbase, query, TRI_ERROR_QUERY_ACCESS_AFTER_MODIFICATION); AssertQueryHasResult(vocbase, GetAllDocs, expected.slice()); } @@ -434,4 +433,4 @@ INSTANTIATE_TEST_CASE_P(ReplaceExecutorIntegration, ReplaceExecutorIntegrationTe } // namespace aql } // namespace tests -} // namespace arangodb \ No newline at end of file +} // namespace arangodb diff --git a/tests/Aql/UpdateExecutorTest.cpp b/tests/Aql/UpdateExecutorTest.cpp index 1119c45dc82e..df6a66a2958a 100644 --- a/tests/Aql/UpdateExecutorTest.cpp +++ b/tests/Aql/UpdateExecutorTest.cpp @@ -394,7 +394,6 @@ TEST_P(UpdateExecutorIntegrationTest, update_in_subquery_multi_access) { expected.add(VPackValue(i)); } } - AssertQueryFailsWith(vocbase, query, TRI_ERROR_QUERY_ACCESS_AFTER_MODIFICATION); AssertQueryHasResult(vocbase, GetAllDocs, expected.slice()); } @@ -488,4 +487,4 @@ INSTANTIATE_TEST_CASE_P(UpdateExecutorIntegration, UpdateExecutorIntegrationTest } // namespace aql } // namespace tests -} // namespace arangodb \ No newline at end of file +} // namespace arangodb diff --git a/tests/Aql/UpsertExecutorTest.cpp b/tests/Aql/UpsertExecutorTest.cpp index fcfd7a51b98b..503c1a609acc 100644 --- a/tests/Aql/UpsertExecutorTest.cpp +++ b/tests/Aql/UpsertExecutorTest.cpp @@ -607,7 +607,6 @@ TEST_P(UpsertExecutorIntegrationTest, upsert_in_subquery_multi_access) { expected.add(VPackValue(i)); } } - AssertQueryFailsWith(vocbase, query, TRI_ERROR_QUERY_ACCESS_AFTER_MODIFICATION); AssertQueryHasResult(vocbase, GetAllDocs, expected.slice()); } diff --git a/tests/js/server/aql/aql-functions-misc.js b/tests/js/server/aql/aql-functions-misc.js index 03c1a617bc3d..5fbbff09ac40 100644 --- a/tests/js/server/aql/aql-functions-misc.js +++ b/tests/js/server/aql/aql-functions-misc.js @@ -469,6 +469,32 @@ function ahuacatlMiscFunctionsTestSuite () { internal.db._drop(cn); }, + + testDocumentUseAfterModification : function () { + const cn = "UnitTestsAhuacatlFunctions"; + + let c = internal.db._create(cn); + try { + c.insert({ "title" : "123", "value" : 456 }); + c.insert({ "title" : "nada", "value" : 123 }); + + let res = AQL_EXECUTE("FOR doc IN " + cn + " SORT doc.value RETURN DOCUMENT(doc._id).title"); + assertEqual([ "nada", "123" ], res.json); + + try { + AQL_EXECUTE("FOR doc IN " + cn + " SORT doc.value REMOVE doc IN " + cn + " RETURN DOCUMENT(doc._id).title"); + fail(); + } catch (err) { + assertEqual(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, err.errorNum); + } + + res = AQL_EXECUTE("FOR doc IN " + cn + " SORT doc.value LET title = DOCUMENT(doc._id).title INSERT { title } INTO " + cn + " RETURN NEW"); + assertEqual(2, res.json.length); + assertEqual(4, c.count()); + } finally { + internal.db._drop(cn); + } + }, //////////////////////////////////////////////////////////////////////////////// /// @brief test document function @@ -772,8 +798,13 @@ function ahuacatlCollectionCountTestSuite () { //////////////////////////////////////////////////////////////////////////////// /// @brief test LENGTH(collection) //////////////////////////////////////////////////////////////////////////////// + + testLengthUseBeforeModification : function () { + let res = AQL_EXECUTE("FOR doc IN " + cn + " LET l = LENGTH(" + cn + ") REMOVE doc IN " + cn + " RETURN l"); + assertEqual(Array(1000).fill(1000), res.json); + }, - testLengthUseInModification : function () { + testLengthUseAfterModification : function () { try { AQL_EXECUTE("FOR doc IN " + cn + " REMOVE doc IN " + cn + " RETURN LENGTH(" + cn + ")"); fail(); @@ -781,7 +812,7 @@ function ahuacatlCollectionCountTestSuite () { assertEqual(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, err.errorNum); } assertEqual(1000, c.count()); - }, + }, //////////////////////////////////////////////////////////////////////////////// /// @brief test COLLECTIONS() diff --git a/tests/js/server/aql/aql-multi-modify.js b/tests/js/server/aql/aql-multi-modify.js index 80bd780f0396..050f068f590a 100644 --- a/tests/js/server/aql/aql-multi-modify.js +++ b/tests/js/server/aql/aql-multi-modify.js @@ -307,7 +307,8 @@ function ahuacatlMultiModifySuite () { testMultiInsertLoopSubquerySameCollection : function () { AQL_EXECUTE("FOR i IN 1..10 INSERT { value: i } INTO @@cn", { "@cn" : cn1 }); var q = "FOR i IN @@cn LET sub = (FOR j IN 1..2 INSERT { value: j } INTO @@cn) RETURN 1"; - assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, { "@cn": cn1 }); + var actual = AQL_EXECUTE(q, { "@cn": cn1 }); + assertEqual([ 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 ], actual.json); }, testMultiInsertLoopSubqueryOtherCollection : function () { @@ -411,7 +412,8 @@ function ahuacatlMultiModifySuite () { testMultiRemoveLoopSubquerySameCollection : function () { AQL_EXECUTE("FOR i IN 1..2010 INSERT { _key: CONCAT('test', i) } INTO @@cn", { "@cn" : cn1 }); var q = "FOR i IN @@cn LET sub = (REMOVE { _key: i._key } INTO @@cn) RETURN 1"; - assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, { "@cn": cn1 }); + var actual = AQL_EXECUTE(q, { "@cn": cn1 }); + assertEqual(Array(2010).fill(1), actual.json); }, testMultiRemoveLoopSubqueryOtherCollection : function () { @@ -451,25 +453,45 @@ function ahuacatlMultiModifySuite () { testRemoveInSubqueryNoResult : function () { AQL_EXECUTE("FOR i IN 1..2010 INSERT { value: i } INTO @@cn", { "@cn" : cn1 }); var q = "FOR doc IN @@cn SORT doc.value LET f = (REMOVE doc IN @@cn) RETURN f"; - assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, {"@cn": cn1 }); + var actual = AQL_EXECUTE(q, { "@cn": cn1 }); + let result = []; + for (let i = 1; i <= 2010; ++i) { + result.push([]); + } + assertEqual(result, actual.json); }, testRemoveInSubqueryNoResultReturnInside : function () { - AQL_EXECUTE("FOR i IN 1..2010 INSERT { value: i } INTO @@cn", { "@cn" : cn1 }); + AQL_EXECUTE("FOR i IN 1..2010 INSERT { _key: CONCAT('test', i), value: i } INTO @@cn", { "@cn" : cn1 }); var q = "FOR doc IN @@cn SORT doc.value LET f = (REMOVE doc IN @@cn RETURN OLD._key) RETURN f"; - assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, {"@cn": cn1 }); + var actual = AQL_EXECUTE(q, { "@cn": cn1 }); + let result = []; + for (let i = 1; i <= 2010; ++i) { + result.push([ "test" + i ]); + } + assertEqual(result, actual.json); }, testRemoveInSubqueryReturnKeys : function () { AQL_EXECUTE("FOR i IN 1..2010 INSERT { value: i } INTO @@cn", { "@cn" : cn1 }); var q = "FOR doc IN @@cn SORT doc.value LET f = (REMOVE doc IN @@cn RETURN OLD.value) RETURN f"; - assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, {"@cn": cn1 }); + var actual = AQL_EXECUTE(q, { "@cn": cn1 }); + let result = []; + for (let i = 1; i <= 2010; ++i) { + result.push([i]); + } + assertEqual(result, actual.json); }, testRemoveInSubqueryReturnKeysDoc : function () { AQL_EXECUTE("FOR i IN 1..2010 INSERT { value: i } INTO @@cn", { "@cn" : cn1 }); var q = "FOR doc IN @@cn SORT doc.value LET f = (REMOVE doc IN @@cn RETURN OLD) RETURN f[0].value"; - assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, {"@cn": cn1 }); + var actual = AQL_EXECUTE(q, { "@cn": cn1 }); + let result = []; + for (let i = 1; i <= 2010; ++i) { + result.push(i); + } + assertEqual(result, actual.json); }, testInsertRemove : function () { @@ -529,11 +551,6 @@ function ahuacatlMultiModifySuite () { }; } -//////////////////////////////////////////////////////////////////////////////// -/// @brief executes the test suites -//////////////////////////////////////////////////////////////////////////////// - jsunity.run(ahuacatlMultiModifySuite); return jsunity.done(); -