From 277a6fc7ea3ddb9c2851d8944bc6d50ae18d7996 Mon Sep 17 00:00:00 2001 From: Dariusz Olszewski Date: Sun, 25 Feb 2024 21:11:27 +0100 Subject: [PATCH 1/3] test: Additional tests for issue #35776 Signed-off-by: Dariusz Olszewski --- .../Search/QueryOptimizer/CombinedTests.php | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) diff --git a/tests/lib/Files/Search/QueryOptimizer/CombinedTests.php b/tests/lib/Files/Search/QueryOptimizer/CombinedTests.php index c1d0428176d..c5abd5603da 100644 --- a/tests/lib/Files/Search/QueryOptimizer/CombinedTests.php +++ b/tests/lib/Files/Search/QueryOptimizer/CombinedTests.php @@ -42,4 +42,150 @@ class CombinedTests extends TestCase { $this->assertEquals('(storage eq 1 and path in ["foo","bar","asd"])', $operator->__toString()); } + + public function testComplexSearchPattern1() { + $operator = new SearchBinaryOperator( + ISearchBinaryOperator::OPERATOR_OR, + [ + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 1), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 2), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "201"), + new SearchComparison(ISearchComparison::COMPARE_LIKE, "path", "201/%"), + ]), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "301"), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 4), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "401"), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "302"), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 4), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "402"), + ]), + ] + ); + $this->assertEquals('((storage eq 1) or (storage eq 2 and (path eq "201" or path like "201\/%")) or (storage eq 3 and path eq "301") or (storage eq 4 and path eq "401") or (storage eq 3 and path eq "302") or (storage eq 4 and path eq "402"))', $operator->__toString()); + + $this->optimizer->processOperator($operator); + + $this->assertEquals('(storage eq 1 or (storage eq 2 and (path eq "201" or path like "201\/%")) or (storage eq 3 and path in ["301","302"]) or (storage eq 4 and path in ["401","402"]))', $operator->__toString()); + } + + public function testComplexSearchPattern2() { + $operator = new SearchBinaryOperator( + ISearchBinaryOperator::OPERATOR_OR, + [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 1), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 2), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "201"), + new SearchComparison(ISearchComparison::COMPARE_LIKE, "path", "201/%"), + ]), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "301"), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 4), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "401"), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "302"), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 4), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "402"), + ]), + ] + ); + $this->assertEquals('(storage eq 1 or (storage eq 2 and (path eq "201" or path like "201\/%")) or (storage eq 3 and path eq "301") or (storage eq 4 and path eq "401") or (storage eq 3 and path eq "302") or (storage eq 4 and path eq "402"))', $operator->__toString()); + + $this->optimizer->processOperator($operator); + + $this->assertEquals('(storage eq 1 or (storage eq 2 and (path eq "201" or path like "201\/%")) or (storage eq 3 and path in ["301","302"]) or (storage eq 4 and path in ["401","402"]))', $operator->__toString()); + } + + public function testApplySearchConstraints1() { + $operator = new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "mimetype", "image/png"), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "mimetype", "image/jpeg"), + ]), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [ + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 1), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files"), + new SearchComparison(ISearchComparison::COMPARE_LIKE, "path", "files/%"), + ]), + ]), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 2), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files/301"), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files/302"), + ]), + ]), + ]); + $this->assertEquals('(((mimetype eq "image\/png" or mimetype eq "image\/jpeg")) and ((storage eq 1 and (path eq "files" or path like "files\/%")) or storage eq 2 or (storage eq 3 and path eq "files\/301") or (storage eq 3 and path eq "files\/302")))', $operator->__toString()); + + $this->optimizer->processOperator($operator); + + $this->assertEquals('(mimetype in ["image\/png","image\/jpeg"] and ((storage eq 1 and (path eq "files" or path like "files\/%")) or storage eq 2 or (storage eq 3 and path in ["files\/301","files\/302"])))', $operator->__toString()); + } + + public function testApplySearchConstraints2() { + $operator = new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "mimetype", "image/png"), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "mimetype", "image/jpeg"), + ]), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [ + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 1), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files"), + new SearchComparison(ISearchComparison::COMPARE_LIKE, "path", "files/%"), + ]), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 2), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files/301"), + ]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "storage", 3), + new SearchComparison(ISearchComparison::COMPARE_EQUAL, "path", "files/302"), + ]), + ]), + ]); + $this->assertEquals('(((mimetype eq "image\/png" or mimetype eq "image\/jpeg")) and ((storage eq 1 and (path eq "files" or path like "files\/%")) or (storage eq 2) or (storage eq 3 and path eq "files\/301") or (storage eq 3 and path eq "files\/302")))', $operator->__toString()); + + $this->optimizer->processOperator($operator); + + $this->assertEquals('(mimetype in ["image\/png","image\/jpeg"] and ((storage eq 1 and (path eq "files" or path like "files\/%")) or storage eq 2 or (storage eq 3 and path in ["files\/301","files\/302"])))', $operator->__toString()); + } } From 3971313d4c40468a1d8811e828f8e3e714c6802c Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 4 Mar 2024 11:39:14 +0100 Subject: [PATCH 2/3] fix: don't short circuit query optimizer Signed-off-by: Robin Appelman --- .../Files/Search/QueryOptimizer/ReplacingOptimizerStep.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/private/Files/Search/QueryOptimizer/ReplacingOptimizerStep.php b/lib/private/Files/Search/QueryOptimizer/ReplacingOptimizerStep.php index 546061522bc..473f8a87151 100644 --- a/lib/private/Files/Search/QueryOptimizer/ReplacingOptimizerStep.php +++ b/lib/private/Files/Search/QueryOptimizer/ReplacingOptimizerStep.php @@ -20,7 +20,9 @@ class ReplacingOptimizerStep extends QueryOptimizerStep { $modified = false; $arguments = $operator->getArguments(); foreach ($arguments as &$argument) { - $modified = $modified || $this->processOperator($argument); + if ($this->processOperator($argument)) { + $modified = true; + } } if ($modified) { $operator->setArguments($arguments); From 6fb447676ec04f5e0406ae5587fc23bbb21c3e1a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 4 Mar 2024 14:25:10 +0100 Subject: [PATCH 3/3] fix: handle cases where non-binary operators are mixed in for optimizing search queries Signed-off-by: Robin Appelman --- .../MergeDistributiveOperations.php | 53 ++++++------------- 1 file changed, 15 insertions(+), 38 deletions(-) diff --git a/lib/private/Files/Search/QueryOptimizer/MergeDistributiveOperations.php b/lib/private/Files/Search/QueryOptimizer/MergeDistributiveOperations.php index 922b0ccb17c..7986df82adc 100644 --- a/lib/private/Files/Search/QueryOptimizer/MergeDistributiveOperations.php +++ b/lib/private/Files/Search/QueryOptimizer/MergeDistributiveOperations.php @@ -18,21 +18,19 @@ use OCP\Files\Search\ISearchOperator; */ class MergeDistributiveOperations extends ReplacingOptimizerStep { public function processOperator(ISearchOperator &$operator): bool { - if ( - $operator instanceof SearchBinaryOperator && - $this->isAllSameBinaryOperation($operator->getArguments()) - ) { + if ($operator instanceof SearchBinaryOperator) { // either 'AND' or 'OR' $topLevelType = $operator->getType(); // split the arguments into groups that share a first argument - // (we already know that all arguments are binary operators with at least 1 child) $groups = $this->groupBinaryOperatorsByChild($operator->getArguments(), 0); $outerOperations = array_map(function (array $operators) use ($topLevelType) { // no common operations, no need to change anything if (count($operators) === 1) { return $operators[0]; } + + // for groups with size >1 we know they are binary operators with at least 1 child /** @var ISearchBinaryOperator $firstArgument */ $firstArgument = $operators[0]; @@ -72,46 +70,25 @@ class MergeDistributiveOperations extends ReplacingOptimizerStep { return parent::processOperator($operator); } - /** - * Check that a list of operators is all the same type of (non-empty) binary operators - * - * @param ISearchOperator[] $operators - * @return bool - * @psalm-assert-if-true SearchBinaryOperator[] $operators - */ - private function isAllSameBinaryOperation(array $operators): bool { - $operation = null; - foreach ($operators as $operator) { - if (!$operator instanceof SearchBinaryOperator) { - return false; - } - if (!$operator->getArguments()) { - return false; - } - if ($operation === null) { - $operation = $operator->getType(); - } else { - if ($operation !== $operator->getType()) { - return false; - } - } - } - return true; - } - /** * Group a list of binary search operators that have a common argument * - * @param SearchBinaryOperator[] $operators - * @return SearchBinaryOperator[][] + * Non-binary operators, or empty binary operators will each get their own 1-sized group + * + * @param ISearchOperator[] $operators + * @return ISearchOperator[][] */ private function groupBinaryOperatorsByChild(array $operators, int $index = 0): array { $result = []; foreach ($operators as $operator) { - /** @var SearchBinaryOperator|SearchComparison $child */ - $child = $operator->getArguments()[$index]; - $childKey = (string) $child; - $result[$childKey][] = $operator; + if ($operator instanceof ISearchBinaryOperator && count($operator->getArguments()) > 0) { + /** @var SearchBinaryOperator|SearchComparison $child */ + $child = $operator->getArguments()[$index]; + $childKey = (string)$child; + $result[$childKey][] = $operator; + } else { + $result[] = [$operator]; + } } return array_values($result); }