8000 Fix erroneous "access after data-modification" errors (#14646) · arangodb/arangodb@0bff56b · GitHub
[go: up one dir, main page]
Skip to content

Commit 0bff56b

Browse files
jsteemannjsteemann
authored
Fix erroneous "access after data-modification" errors (#14646)
1 parent 33e42b7 commit 0bff56b

File tree

7 files changed

+72
-38
lines changed

7 files changed

+72
-38
lines changed

‎CHANGELOG

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
devel
22
-----
33

4+
* Some AQL queries erroneously reported the "access after data-modification"
5+
error for queries in which there was a read attempt from a collection
6+
_before_ a data-modification operation. Such access is legal and should not
7+
trigger said error anymore. Accessing a collection _after_ in a query a
8+
data-modification in the same query is still disallowed.
9+
410
* Make AQL modification operations in a cluster asynchronous. This allows to
511
free the thread for other work until both the write and synchronous
612
replication are complete.
@@ -18,8 +24,8 @@ devel
1824
* Fixed: _api/transaction/begin called on edge collections of disjoint
1925
SmartGraphs falsely returned CollectionNotFound errors.
2026

21-
* Bug-Fix: In more complex queries there was a code-path where a (Disjoint-) Smart
22-
graph access was not properly optimized.
27+
* Bugfix: In more complex queries there was a code-path where a (Disjoint-)
28+
Smart graph access was not properly optimized.
2329

2430
* Improve log messages for Pregel runs by giving them more context.
2531

‎arangod/Aql/Ast.cpp

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2050,7 +2050,6 @@ void Ast::validateAndOptimize(transaction::Methods& trx) {
20502050
TraversalContext(transaction::Methods& t) : trx(t) {}
20512051

20522052
std::unordered_set<std::string> writeCollectionsSeen;
2053-
std::unordered_map<std::string, int64_t> collectionsFirstSeen;
20542053
std::unordered_map<Variable const*, AstNode const*> variableDefinitions;
20552054
AqlFunctionsInternalCache aqlFunctionsInternalCache;
20562055
transaction::Methods& trx;
@@ -2096,12 +2095,7 @@ void Ast::validateAndOptimize(transaction::Methods& trx) {
20962095
} else if (node->type == NODE_TYPE_COLLECTION_LIST) {
20972096
// a collection list is produced by WITH a, b, c
20982097
// or by traversal declarations
2099-
// we do not want to descend further, in order to not track
2100-
// the collections in collectionsFirstSeen
21012098
return false;
2102-
} else if (node->type == NODE_TYPE_COLLECTION) {
2103-
// note the level on which we first saw a collection
2104-
ctx->collectionsFirstSeen.try_emplace(node->getString(), ctx->nestingLevel);
21052099
} else if (node->type == NODE_TYPE_AGGREGATIONS) {
21062100
++ctx->stopOptimizationRequests;
21072101
} else if (node->type == NODE_TYPE_SUBQUERY) {
@@ -2157,17 +2151,6 @@ void Ast::validateAndOptimize(transaction::Methods& trx) {
21572151
auto collection = node->getMember(1);
21582152
std::string name = collection->getString();
21592153
ctx->writeCollectionsSeen.emplace(name);
2160-
2161-
auto it = ctx->collectionsFirstSeen.find(name);
2162-
2163-
if (it != ctx->collectionsFirstSeen.end()) {
2164-
if ((*it).second < ctx->nestingLevel) {
2165-
name = "collection '" + name;
2166-
name.push_back('\'');
2167-
THROW_ARANGO_EXCEPTION_PARAMS(TRI_ERROR_QUERY_ACCESS_AFTER_MODIFICATION,
2168-
name.c_str());
2169-
}
2170-
}
21712154
} else if (node->type == NODE_TYPE_FCALL) {
21722155
auto func = static_cast<Function*>(node->getData());
21732156
TRI_ASSERT(func != nullptr);

‎tests/Aql/ReplaceExecutorTest.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,6 @@ TEST_P(ReplaceExecutorIntegrationTest, replace_in_subquery_multi_access) {
340340
expected.add(VPackValue(i));
341341
}
342342
}
343-
AssertQueryFailsWith(vocbase, query, TRI_ERROR_QUERY_ACCESS_AFTER_MODIFICATION);
344343
AssertQueryHasResult(vocbase, GetAllDocs, expected.slice());
345344
}
346345

@@ -434,4 +433,4 @@ INSTANTIATE_TEST_CASE_P(ReplaceExecutorIntegration, ReplaceExecutorIntegrationTe
434433

435434
} // namespace aql
436435
} // namespace tests
437-
} // namespace arangodb
436+
} // namespace arangodb

‎tests/Aql/UpdateExecutorTest.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -394,7 +394,6 @@ TEST_P(UpdateExecutorIntegrationTest, update_in_subquery_multi_access) {
394394
expected.add(VPackValue(i));
395395
}
396396
}
397-
AssertQueryFailsWith(vocbase, query, TRI_ERROR_QUERY_ACCESS_AFTER_MODIFICATION);
398397
AssertQueryHasResult(vocbase, GetAllDocs, expected.slice());
399398
}
400399

@@ -488,4 +487,4 @@ INSTANTIATE_TEST_CASE_P(UpdateExecutorIntegration, UpdateExecutorIntegrationTest
488487

489488
} // namespace aql
490489
} // namespace tests
491-
} // namespace arangodb
490+
} // namespace arangodb

‎tests/Aql/UpsertExecutorTest.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -607,7 +607,6 @@ TEST_P(UpsertExecutorIntegrationTest, upsert_in_subquery_multi_access) {
607607
expected.add(VPackValue(i));
608608
}
609609
}
610-
AssertQueryFailsWith(vocbase, query, TRI_ERROR_QUERY_ACCESS_AFTER_MODIFICATION);
611610
AssertQueryHasResult(vocbase, GetAllDocs, expected.slice());
612611
}
613612

‎tests/js/server/aql/aql-functions-misc.js

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,32 @@ function ahuacatlMiscFunctionsTestSuite () {
469469

470470
internal.db._drop(cn);
471471
},
472+
473+
testDocumentUseAfterModification : function () {
474+
const cn = "UnitTestsAhuacatlFunctions";
475+
476+
let c = internal.db._create(cn);
477+
try {
478+
c.insert({ "title" : "123", "value" : 456 });
479+
c.insert({ "title" : "nada", "value" : 123 });
480+
481+
let res = AQL_EXECUTE("FOR doc IN " + cn + " SORT doc.value RETURN DOCUMENT(doc._id).title");
482+
assertEqual([ "nada", "123" ], res.json);
483+
484+
try {
485+
AQL_EXECUTE("FOR doc IN " + cn + " SORT doc.value REMOVE doc IN " + cn + " RETURN DOCUMENT(doc._id).title");
486+
fail();
487+
} catch (err) {
488+
assertEqual(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, err.errorNum);
489+
}
490+
491+
res = AQL_EXECUTE("FOR doc IN " + cn + " SORT doc.value LET title = DOCUMENT(doc._id).title INSERT { title } INTO " + cn + " RETURN NEW");
492+
assertEqual(2, res.json.length);
493+
assertEqual(4, c.count());
494+
} finally {
495+
internal.db._drop(cn);
496+
}
497+
},
472498

473499
////////////////////////////////////////////////////////////////////////////////
474500
/// @brief test document function
@@ -772,16 +798,21 @@ function ahuacatlCollectionCountTestSuite () {
772798
////////////////////////////////////////////////////////////////////////////////
773799
/// @brief test LENGTH(collection)
774800
////////////////////////////////////////////////////////////////////////////////
801+
802+
testLengthUseBeforeModification : function () {
803+
let res = AQL_EXECUTE("FOR doc IN " + cn + " LET l = LENGTH(" + cn + ") REMOVE doc IN " + cn + " RETURN l");
804+
assertEqual(Array(1000).fill(1000), res.json);
805+
},
775806

776-
testLengthUseInModification : function () {
807+
testLengthUseAfterModification : function () {
777808
try {
778809
AQL_EXECUTE("FOR doc IN " + cn + " REMOVE doc IN " + cn + " RETURN LENGTH(" + cn + ")");
779810
fail();
780811
} catch (err) {
781812
assertEqual(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, err.errorNum);
782813
}
783814
assertEqual(1000, c.count());
784-
},
815+
},
785816

786817
////////////////////////////////////////////////////////////////////////////////
787818
/// @brief test COLLECTIONS()

‎tests/js/server/aql/aql-multi-modify.js

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,8 @@ function ahuacatlMultiModifySuite () {
307307
testMultiInsertLoopSubquerySameCollection : function () {
308308
AQL_EXECUTE("FOR i IN 1..10 INSERT { value: i } INTO @@cn", { "@cn" : cn1 });
309309
var q = "FOR i IN @@cn LET sub = (FOR j IN 1..2 INSERT { value: j } INTO @@cn) RETURN 1";
310-
assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, { "@cn": cn1 });
310+
var actual = AQL_EXECUTE(q, { "@cn": cn1 });
311+
assertEqual([ 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 ], actual.json);
311312
},
312313

313314
testMultiInsertLoopSubqueryOtherCollection : function () {
@@ -411,7 +412,8 @@ function ahuacatlMultiModifySuite () {
411412
testMultiRemoveLoopSubquerySameCollection : function () {
412413
AQL_EXECUTE("FOR i IN 1..2010 INSERT { _key: CONCAT('test', i) } INTO @@cn", { "@cn" : cn1 });
413414
var q = "FOR i IN @@cn LET sub = (REMOVE { _key: i._key } INTO @@cn) RETURN 1";
414-
assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, { "@cn": cn1 });
415+
var actual = AQL_EXECUTE(q, { "@cn": cn1 });
416+
assertEqual(Array(2010).fill(1), actual.json);
415417
},
416418

417419
testMultiRemoveLoopSubqueryOtherCollection : function () {
@@ -451,25 +453,45 @@ function ahuacatlMultiModifySuite () {
451453
testRemoveInSubqueryNoResult : function () {
452454
AQL_EXECUTE("FOR i IN 1..2010 INSERT { value: i } INTO @@cn", { "@cn" : cn1 });
453455
var q = "FOR doc IN @@cn SORT doc.value LET f = (REMOVE doc IN @@cn) RETURN f";
454-
assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, {"@cn": cn1 });
456+
var actual = AQL_EXECUTE(q, { "@cn": cn1 });
457+
let result = [];
458+
for (let i = 1; i <= 2010; ++i) {
459+
result.push([]);
460+
}
461+
assertEqual(result, actual.json);
455462
},
456463

457464
testRemoveInSubqueryNoResultReturnInside : function () {
458-
AQL_EXECUTE("FOR i IN 1..2010 INSERT { value: i } INTO @@cn", { "@cn" : cn1 });
465+
AQL_EXECUTE("FOR i IN 1..2010 INSERT { _key: CONCAT('test', i), value: i } INTO @@cn", { "@cn" : cn1 });
459466
var q = "FOR doc IN @@cn SORT doc.value LET f = (REMOVE doc IN @@cn RETURN OLD._key) RETURN f";
460-
assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, {"@cn": cn1 });
467+
var actual = AQL_EXECUTE(q, { "@cn": cn1 });
468+
let result = [];
469+
for (let i = 1; i <= 2010; ++i) {
470+
result.push([ "test" + i ]);
471+
}
472+
assertEqual(result, actual.json);
461473
},
462474

463475
testRemoveInSubqueryReturnKeys : function () {
464476
AQL_EXECUTE("FOR i IN 1..2010 INSERT { value: i } INTO @@cn", { "@cn" : cn1 });
465477
var q = "FOR doc IN @@cn SORT doc.value LET f = (REMOVE doc IN @@cn RETURN OLD.value) RETURN f";
466-
assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, {"@cn": cn1 });
478+
var actual = AQL_EXECUTE(q, { "@cn": cn1 });
479+
let result = [];
480+
for (let i = 1; i <= 2010; ++i) {
481+
result.push([i]);
482+
}
483+
assertEqual(result, actual.json);
467484
},
468485

469486
testRemoveInSubqueryReturnKeysDoc : function () {
470487
AQL_EXECUTE("FOR i IN 1..2010 INSERT { value: i } INTO @@cn", { "@cn" : cn1 });
471488
var q = "FOR doc IN @@cn SORT doc.value LET f = (REMOVE doc IN @@cn RETURN OLD) RETURN f[0].value";
472-
assertQueryError(errors.ERROR_QUERY_ACCESS_AFTER_MODIFICATION.code, q, {"@cn": cn1 });
489+
var actual = AQL_EXECUTE(q, { "@cn": cn1 });
490+
let result = [];
491+
for (let i = 1; i <= 2010; ++i) {
492+
result.push(i);
493+
}
494+
assertEqual(result, actual.json);
473495
},
474496

475497
testInsertRemove : function () {
@@ -529,11 +551,6 @@ function ahuacatlMultiModifySuite () {
529551
};
530552
}
531553

532-
////////////////////////////////////////////////////////////////////////////////
533-
/// @brief executes the test suites
534-
////////////////////////////////////////////////////////////////////////////////
535-
536554
jsunity.run(ahuacatlMultiModifySuite);
537555

538556
return jsunity.done();
539-

0 commit comments

Comments
 (0)
0