diff --git a/CHANGELOG b/CHANGELOG index 913219c41f03..7e6e6d3d79e3 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,8 @@ v3.5.7 (XXXX-XX-XX) ------------------- +* Fix AR-113. Disallow non-values in the AQL geo-index-optimizer rule. + * Updated ArangoDB Starter to 0.14.15-1. * Updated arangosync to 0.7.13. diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index a93e35ae5692..660282b14230 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -6213,10 +6213,6 @@ void arangodb::aql::inlineSubqueriesRule(Optimizer* opt, std::unique_ptraddPlan(std::move(plan), rule, modified); } -static bool isValueOrReference(AstNode const* node) { - return node->type == NODE_TYPE_VALUE || node->type == NODE_TYPE_REFERENCE; -} - /// Essentially mirrors the geo::QueryParams struct, but with /// abstracts AstNode value objects struct GeoIndexInfo { @@ -6522,7 +6518,7 @@ bool checkGeoFilterExpression(ExecutionPlan* plan, AstNode const* node, GeoIndex // checks @first `smaller` @second // note: this only modifies "info" if the function returns true auto eval = [&](AstNode const* first, AstNode const* second, bool lessequal) -> bool { - if (isValueOrReference(second) && // no attribute access + if (second->type == NODE_TYPE_VALUE && // only constants allowed info.maxDistanceExpr == nullptr && // max distance is not yet set checkDistanceFunc(plan, first, /*legacy*/ true, info)) { TRI_ASSERT(info.index); @@ -6530,7 +6526,7 @@ bool checkGeoFilterExpression(ExecutionPlan* plan, AstNode const* node, GeoIndex info.maxInclusive = info.maxInclusive && lessequal; info.nodesToRemove.insert(node); return true; - } else if (isValueOrReference(first) && // no attribute access + } else if (first->type == NODE_TYPE_VALUE && // only constants allowed info.minDistanceExpr == nullptr && // min distance is not yet set checkDistanceFunc(plan, second, /*legacy*/ true, info)) { info.minDistanceExpr = first; diff --git a/arangod/GeoIndex/Index.cpp b/arangod/GeoIndex/Index.cpp index f51701e2ecf2..0b980b0fef5f 100644 --- a/arangod/GeoIndex/Index.cpp +++ b/arangod/GeoIndex/Index.cpp @@ -246,6 +246,7 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref, case aql::NODE_TYPE_OPERATOR_BINARY_LE: qp.maxInclusive = true; // intentional fallthrough + [[fallthrough]]; case aql::NODE_TYPE_OPERATOR_BINARY_LT: { TRI_ASSERT(node->numMembers() == 2); qp.origin = Index::parseDistFCall(node->getMemberUnchecked(0), ref); @@ -255,6 +256,9 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref, aql::AstNode const* max = node->getMemberUnchecked(1); TRI_ASSERT(max->type == aql::NODE_TYPE_VALUE); + if (max->type != aql::NODE_TYPE_VALUE) { + THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE); + } if (!max->isValueType(aql::VALUE_TYPE_STRING)) { qp.maxDistance = max->getDoubleValue(); } // else assert(max->getStringValue() == "unlimited") @@ -263,22 +267,25 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref, case aql::NODE_TYPE_OPERATOR_BINARY_GE: qp.minInclusive = true; // intentional fallthrough + [[fallthrough]]; case aql::NODE_TYPE_OPERATOR_BINARY_GT: { TRI_ASSERT(node->numMembers() == 2); qp.origin = Index::parseDistFCall(node->getMemberUnchecked(0), ref); if (!qp.origin.is_valid()) { THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE); } - // LOG_TOPIC("a9633", ERR, Logger::FIXME) << "Found center: " << c.toString(); aql::AstNode const* min = node->getMemberUnchecked(1); TRI_ASSERT(min->type == aql::NODE_TYPE_VALUE); + if (min->type != aql::NODE_TYPE_VALUE) { + THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE); + } qp.minDistance = min->getDoubleValue(); break; } default: TRI_ASSERT(false); - break; + THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE); } } diff --git a/tests/js/server/aql/aql-optimizer-geoindex.js b/tests/js/server/aql/aql-optimizer-geoindex.js index 36fcaa15f16e..08e629bc095c 100644 --- a/tests/js/server/aql/aql-optimizer-geoindex.js +++ b/tests/js/server/aql/aql-optimizer-geoindex.js @@ -1039,7 +1039,86 @@ function optimizerRuleTestSuite() { var result = AQL_EXPLAIN(query.string, query.bindVars); hasIndexNode(result, query); hasNoFilterNode(result, query); - } + }, + + // Regression test (https://arangodb.atlassian.net/browse/AR-113): + // Previously, the filter was moved into the index, but one access of `dist` in `dist < dist` was left, while the + // variable obviously can't be available yet at the index node. + // When failing, throws an error like + // missing variable #1 (dist) for node #8 (IndexNode) while planning registers + testSelfReference1: function () { + const query = { + string: ` + FOR doc IN @@col + LET dist = GEO_DISTANCE(doc.geo, [16, 53]) + FILTER dist < dist + RETURN {doc, dist} + `, + bindVars: {'@col': colName}, + }; + + const result = AQL_EXPLAIN(query.string, query.bindVars); + hasNoIndexNode(result, query); + hasFilterNode(result, query); + }, + + // See testSelfReference1 + testSelfReference2: function () { + const query = { + string: ` + FOR doc IN @@col + LET dist = GEO_DISTANCE(doc.geo, [16, 53]) + LET dist2 = GEO_DISTANCE(doc.geo, [3, 7]) + FILTER dist < dist2 + RETURN {doc, dist, dist2} + `, + bindVars: {'@col': colName}, + }; + + const result = AQL_EXPLAIN(query.string, query.bindVars); + hasNoIndexNode(result, query); + hasFilterNode(result, query); + }, + + // See testSelfReference1 + testSelfReference3: function () { + const query = { + string: ` + FOR doc IN @@col + LET dist = GEO_DISTANCE(doc.geo, [16, 53]) + LET dist2 = doc.maxDist + FILTER dist < dist2 + RETURN {doc, dist, dist2} + `, + bindVars: {'@col': colName}, + }; + + const result = AQL_EXPLAIN(query.string, query.bindVars); + hasNoIndexNode(result, query); + hasFilterNode(result, query); + }, + + // See testSelfReference1 + // Note that currently, the optimizer rule does not work with `dist2` being + // not a value (but a variable reference) in the filter expression. When + // this changes in the future, the expectations in this test can simply be + // changed. + testInaccessibleReference: function () { + const query = { + string: ` + FOR doc IN @@col + LET dist = GEO_DISTANCE(doc.geo, [16, 53]) + LET dist2 = NOEVAL(7) + FILTER dist < dist2 + RETURN {doc, dist, dist2} + `, + bindVars: {'@col': colName}, + }; + + const result = AQL_EXPLAIN(query.string, query.bindVars); + hasNoIndexNode(result, query); + hasFilterNode(result, query); + }, }; // test dictionary (return) } // optimizerRuleTestSuite