arangodb · jsteemann · Aug 17, 2021 · Aug 16, 2021 · Aug 16, 2021 · Aug 16, 2021
diff --git 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
@@ -2050,7 +2050,6 @@ void Ast::validateAndOptimize(transaction::Methods& trx) {
     TraversalContext(transaction::Methods& t) : trx(t) {}
 
     std::unordered_set<std::string> writeCollectionsSeen;
-    std::unordered_map<std::string, int64_t> collectionsFirstSeen;
     std::unordered_map<Variable const*, AstNode const*> 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<Function*>(node->getData());
       TRI_ASSERT(func != nullptr);

diff --git 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
+}  // namespace arangodb
diff --git 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
+}  // namespace arangodb
diff --git 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
@@ -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,16 +798,21 @@ 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();
       } catch (err) {
         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
@@ -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();
-
-Original file line number
+Diff line change
@@ Expand Up @@
           expected.add(VPackValue(i));
         }
       }
-      AssertQueryFailsWith(vocbase, query, TRI_ERROR_QUERY_ACCESS_AFTER_MODIFICATION);
       AssertQueryHasResult(vocbase, GetAllDocs, expected.slice());
     }
@@ Expand Down @@