8000 Fix string comparison for AstNodes (#10837) · waveray/arangodb@ec4a33a · GitHub
[go: up one dir, main page]
Skip to content

Commit ec4a33a

Browse files
markuspfmchacki
markuspf
authored and
mchacki
committed
Fix string comparison for AstNodes (arangodb#10837)
* Fix string comparison for AstNodes * Use StaticStrings for TraversalConditionFinder * AstNode stringEquals rewrite * Remove version which takes a boolean parameter to determine whether to compare case sensitively or not and replace it by its own function * Introduce string constants for uses of this function * Add CHANGELOG for this bugfix
1 parent d813a88 commit ec4a33a

File tree

7 files changed

+64
-36
lines changed

7 files changed

+64
-36
lines changed

‎CHANGELOG

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
devel
22
-----
33

4+
* Fix string comparison bug that lead to traversal queries accepting
5+
prefixes of "edges" and "vertices" to be used as object accessors
6+
for the path object
7+
48
* Add ability to choose logging to file in Windows installer of ArangoDB server
59
(enabled by default).
610

‎arangod/Aql/AstNode.cpp

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2838,14 +2838,20 @@ void AstNode::setStringValue(char const* v, size_t length) {
28382838
value.length = static_cast<uint32_t>(length);
28392839
}
28402840

2841-
bool AstNode::stringEquals(char const* other, bool caseInsensitive) const {
2842-
if (caseInsensitive) {
2843-
return (strncasecmp(getStringValue(), other, getStringLength()) == 0);
2844-
}
2845-
return (strncmp(getStringValue(), other, getStringLength()) == 0);
2841+
bool AstNode::stringEqualsCaseInsensitive(std::string const& other) const {
2842+
// Since we're not sure in how much trouble we are with unicode
2843+
// strings, we assert here that strings we use are 7-bit ASCII
2844+
TRI_ASSERT(std::none_of(other.begin(), other.end(),
2845+
[](const char c) { return c & 0x80; }));
2846+
return (other.size() == getStringLength() &&
2847+
strncasecmp(other.c_str(), getStringValue(), getStringLength()) == 0);
28462848
}
28472849

28482850
bool AstNode::stringEquals(std::string const& other) const {
2851+
// Since we're not sure in how much trouble we are with unicode
2852+
// strings, we assert here that strings we use are 7-bit ASCII
2853+
TRI_ASSERT(std::none_of(other.begin(), other.end(),
2854+
[](const char c) { return c & 0x80; }));
28492855
return (other.size() == getStringLength() &&
28502856
memcmp(other.c_str(), getStringValue(), getStringLength()) == 0);
28512857
}

‎arangod/Aql/AstNode.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -521,8 +521,9 @@ struct AstNode {
521521
/// @brief set the string value of a node
522522
void setStringValue(char const* v, size_t length);
523523

524-
/// @brief whether or not a string is equal to another
525-
bool stringEquals(char const* other, bool caseInsensitive) const;
524+
/// @brief whether the string value of this node is equal to other
525+
/// ignoring case
526+
bool stringEqualsCaseInsensitive(std::string const& other) const;
526527

527528
/// @brief whether or not a string is equal to another
528529
bool stringEquals(std::string const& other) const;

‎arangod/Aql/ExecutionPlan.cpp

Lines changed: 16 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -161,19 +161,19 @@ std::unique_ptr<graph::BaseOptions> createTraversalOptions(aql::Query* query,
161161
if (name == "bfs") {
162162
options->useBreadthFirst = value->isTrue();
163163
} else if (name == "uniqueVertices" && value->isStringValue()) {
164-
if (value->stringEquals("path", true)) {
164+
if (value->stringEqualsCaseInsensitive(StaticStrings::GraphQueryPath)) {
165165
options->uniqueVertices =
166166
arangodb::traverser::TraverserOptions::UniquenessLevel::PATH;
167-
} else if (value->stringEquals("global", true)) {
167+
} else if (value->stringEqualsCaseInsensitive(StaticStrings::GraphQueryGlobal)) {
168168
options->uniqueVertices =
169169
arangodb::traverser::TraverserOptions::UniquenessLevel::GLOBAL;
170170
}
171171
} else if (name == "uniqueEdges" && value->isStringValue()) {
172172
// path is the default
173-
if (value->stringEquals("none", true)) {
173+
if (value->stringEqualsCaseInsensitive(StaticStrings::GraphQueryNone)) {
174174
options->uniqueEdges =
175175
arangodb::traverser::TraverserOptions::UniquenessLevel::NONE;
176-
} else if (value->stringEquals("global", true)) {
176+
} else if (value->stringEqualsCaseInsensitive(StaticStrings::GraphQueryGlobal)) {
177177
THROW_ARANGO_EXCEPTION_MESSAGE(
178178
TRI_ERROR_BAD_PARAMETER,
179179
"uniqueEdges: 'global' is not supported, "
@@ -444,13 +444,9 @@ bool ExecutionPlan::hasAppliedRule(int level) const {
444444
[level](int l) { return l == level; });
445445
}
446446

447-
void ExecutionPlan::enableRule(int rule) {
448-
_disabledRules.erase(rule);
449-
}
447+
void ExecutionPlan::enableRule(int rule) { _disabledRules.erase(rule); }
450448

451-
void ExecutionPlan::disableRule(int rule) {
452-
_disabledRules.emplace(rule);
453-
}
449+
void ExecutionPlan::disableRule(int rule) { _disabledRules.emplace(rule); }
454450

455451
bool ExecutionPlan::isDisabledRule(int rule) const {
456452
return (_disabledRules.find(rule) != _disabledRules.end());
@@ -472,8 +468,7 @@ ExecutionNode* ExecutionPlan::getNodeById(size_t id) const {
472468
}
473469

474470
/// @brief creates a calculation node for an arbitrary expression
475-
ExecutionNode* ExecutionPlan::createCalculation(Variable* out,
476-
AstNode const* expression,
471+
ExecutionNode* ExecutionPlan::createCalculation(Variable* out, AstNode const* expression,
477472
ExecutionNode* previous) {
478473
TRI_ASSERT(out != nullptr);
479474

@@ -834,7 +829,8 @@ ExecutionNode* ExecutionPlan::registerNode(ExecutionNode* node) {
834829
try {
835830
auto [it, emplaced] = _ids.try_emplace(node->id(), node);
836831
if (!emplaced) {
837-
THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "unable to register node in plan");
832+
THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL,
833+
"unable to register node in plan");
838834
}
839835
TRI_ASSERT(it != _ids.end());
840836
} catch (...) {
@@ -1324,10 +1320,10 @@ ExecutionNode* ExecutionPlan::fromNodeSort(ExecutionNode* previous, AstNode cons
13241320
if (ascending->type == NODE_TYPE_VALUE) {
13251321
if (ascending->value.type == VALUE_TYPE_STRING) {
13261322
// special treatment for string values ASC/DESC
1327-
if (ascending->stringEquals("ASC", true)) {
1323+
if (ascending->stringEqualsCaseInsensitive(StaticStrings::QuerySortASC)) {
13281324
isAscending = true;
13291325
handled = true;
1330-
} else if (ascending->stringEquals("DESC", true)) {
1326+
} else if (ascending->stringEqualsCaseInsensitive(StaticStrings::QuerySortDESC)) {
13311327
isAscending = false;
13321328
handled = true;
13331329
}
@@ -2390,9 +2386,9 @@ bool ExecutionPlan::isDeadSimple() const {
23902386
auto const nodeType = current->getType();
23912387

23922388
if (nodeType == ExecutionNode::SUBQUERY || nodeType == ExecutionNode::ENUMERATE_COLLECTION ||
2393-
nodeType == ExecutionNode::ENUMERATE_LIST || nodeType == ExecutionNode::TRAVERSAL ||
2394-
nodeType == ExecutionNode::SHORTEST_PATH || nodeType == ExecutionNode::K_SHORTEST_PATHS ||
2395-
nodeType == ExecutionNode::INDEX) {
2389+
nodeType == ExecutionNode::ENUMERATE_LIST ||
2390+
nodeType == ExecutionNode::TRAVERSAL || nodeType == ExecutionNode::SHORTEST_PATH ||
2391+
nodeType == ExecutionNode::K_SHORTEST_PATHS || nodeType == ExecutionNode::INDEX) {
23962392
// these node types are not simple
23972393
return false;
23982394
}
@@ -2426,13 +2422,9 @@ struct Shower final : public WalkerWorker<ExecutionNode> {
24262422
return true;
24272423
}
24282424

2429-
void leaveSubquery(ExecutionNode*, ExecutionNode*) final {
2430-
indent--;
2431-
}
2425+
void leaveSubquery(ExecutionNode*, ExecutionNode*) final { indent--; }
24322426

2433-
bool before(ExecutionNode* en) final {
2434-
return false;
2435-
}
2427+
bool before(ExecutionNode* en) final { return false; }
24362428

24372429
void after(ExecutionNode* en) final {
24382430
if (en->getType() == ExecutionNode::SUBQUERY_END) {

‎arangod/Aql/TraversalConditionFinder.cpp

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@
3232
#include "Graph/TraverserOptions.h"
3333
#include "Logger/LogMacros.h"
3434

35+
#include "Basics/StaticStrings.h"
36+
37+
using namespace arangodb;
3538
using namespace arangodb::aql;
3639
using namespace arangodb::basics;
3740
using EN = arangodb::aql::ExecutionNode;
@@ -272,9 +275,9 @@ static bool checkPathVariableAccessFeasible(Ast* ast, AstNode* parent, size_t te
272275
notSupported = true;
273276
return node;
274277
}
275-
if (node->stringEquals("edges", false)) {
278+
if (node->stringEquals(StaticStrings::GraphQueryEdges)) {
276279
isEdge = true;
277-
} else if (node->stringEquals("vertices", false)) {
280+
} else if (node->stringEquals(StaticStrings::GraphQueryVertices)) {
278281
isEdge = false;
279282
} else {
280283
notSupported = true;
@@ -517,8 +520,7 @@ bool TraversalConditionFinder::before(ExecutionNode* en) {
517520
case EN::LIMIT:
518521
case EN::SHORTEST_PATH:
519522
case EN::K_SHORTEST_PATHS:
520-
case EN::ENUMERATE_IRESEARCH_VIEW:
521-
{
523+
case EN::ENUMERATE_IRESEARCH_VIEW: {
522524
// in these cases we simply ignore the intermediate nodes, note
523525
// that we have taken care of nodes that could throw exceptions
524526
// above.
@@ -544,7 +546,8 @@ bool TraversalConditionFinder::before(ExecutionNode* en) {
544546

545547
case EN::FILTER: {
546548
// register which variable is used in a FILTER
547-
_filterVariables.emplace(ExecutionNode::castTo<FilterNode const*>(en)->inVariable()->id);
549+
_filterVariables.emplace(
550+
ExecutionNode::castTo<FilterNode const*>(en)->inVariable()->id);
548551
break;
549552
}
550553

‎lib/Basics/StaticStrings.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,17 @@ std::string const StaticStrings::GraphInitial("initial");
252252
std::string const StaticStrings::GraphInitialCid("initialCid");
253253
std::string const StaticStrings::GraphName("name");
254254

255+
// Query Strings
256+
std::string const StaticStrings::QuerySortASC("ASC");
257+
std::string const StaticStrings::QuerySortDESC("DESC");
258+
259+
// Graph Query Strings
260+
std::string const StaticStrings::GraphQueryEdges("edges");
261+
std::string const StaticStrings::GraphQueryVertices("vertices");
262+
std::string const StaticStrings::GraphQueryPath("path");
263+
std::string const StaticStrings::GraphQueryGlobal("global");
264+
std::string const StaticStrings::GraphQueryNone("none");
265+
255266
// rest query parameter
256267
std::string const StaticStrings::GraphDropCollections("dropCollections");
257268
std::string const StaticStrings::GraphDropCollection("dropCollection");

‎lib/Basics/StaticStrings.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,17 @@ class StaticStrings {
238238
static std::string const GraphInitialCid;
239239
static std::string const GraphName;
240240

241+
// Query Strings
242+
static std::string const QuerySortASC;
243+
static std::string const QuerySortDESC;
244+
245+
// Graph Query Strings
246+
static std::string const GraphQueryEdges;
247+
static std::string const GraphQueryVertices;
248+
static std::string const GraphQueryPath;
249+
static std::string const GraphQueryGlobal;
250+
static std::string const GraphQueryNone;
251+
241252
// Replication
242253
static std::string const ReplicationSoftLockOnly;
243254
static std::string const FailoverCandidates;

0 commit comments

Comments
 (0)
0