Skip to content

Commit 31d43a6

Browse files
mneudertmneudert
committed
Prevent errors with unselected columns in window ranking queries
1 parent 53f6f7a commit 31d43a6

File tree

2 files changed

+216
-15
lines changed

2 files changed

+216
-15
lines changed

‎core/RankingQuery.php‎

Lines changed: 95 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -317,8 +317,10 @@ public function generateRankingQuery(string $innerQuery, bool $withRollup = fals
317317
$additionalColumnsExpressions = $this->generateAdditionalColumnsExpressions();
318318

319319
if (Schema::getInstance()->supportsWindowFunctions()) {
320-
$counterExpressions = $this->generateWindowCounterExpressions($innerQuery, $withRollup);
321-
$counterRollupExpressions = $this->generateWindowCounterRollupExpressions($innerQuery, $withRollup);
320+
$windowOrderBy = $this->generateWindowOrderByExpression($innerQuery, $withRollup);
321+
322+
$counterExpressions = $this->generateWindowCounterExpressions($windowOrderBy, $withRollup);
323+
$counterRollupExpressions = $this->generateWindowCounterRollupExpressions($withRollup, $windowOrderBy);
322324
$groupByExpression = $this->generateWindowGroupByExpression($limit, $withRollup);
323325
} else {
324326
$counterExpressions = $this->generateVariableCounterExpressions($limit, $withRollup);
@@ -552,18 +554,14 @@ private function generateVariableGroupByExpression(bool $withRollup): string
552554
*
553555
* @return array{counter: string, init: string}
554556
*/
555-
private function generateWindowCounterExpressions(string $innerQuery, bool $withRollup): array
557+
private function generateWindowCounterExpressions(string $windowOrderBy, bool $withRollup): array
556558
{
557559
$partitionBy = '';
558560

559561
if ($this->partitionColumn !== false) {
560562
$partitionBy = "PARTITION BY `{$this->partitionColumn}`";
561563
}
562564

563-
$orderByColumns = DbHelper::extractOrderByFromQuery($innerQuery, true)
564-
?? DbHelper::extractGroupByFromQuery($innerQuery, true)
565-
?? $this->generateLabelColumnsString();
566-
567565
$excludeWhens = [];
568566
$orderByWhens = [];
569567

@@ -580,15 +578,15 @@ private function generateWindowCounterExpressions(string $innerQuery, bool $with
580578
}
581579

582580
if ([] === $orderByWhens) {
583-
$orderBy = $orderByColumns;
581+
$orderBy = $windowOrderBy;
584582
} else {
585583
$orderBy = "
586584
CASE
587585
" . implode("
588586
", $orderByWhens) . "
589587
ELSE 0
590588
END,
591-
$orderByColumns
589+
$windowOrderBy
592590
";
593591
}
594592

@@ -619,24 +617,20 @@ private function generateWindowCounterExpressions(string $innerQuery, bool $with
619617
*
620618
* @return array{counter: string, init: string}
621619
*/
622-
private function generateWindowCounterRollupExpressions(string $innerQuery, bool $withRollup): array
620+
private function generateWindowCounterRollupExpressions(bool $withRollup, string $withRollupOrderBy): array
623621
{
624622
$counter = '';
625623

626624
if ($withRollup) {
627625
$rollupColumns = array_keys($this->labelColumns);
628626

629-
$orderByColumns = DbHelper::extractOrderByFromQuery($innerQuery, true)
630-
?? DbHelper::extractGroupByFromQuery($innerQuery, true)
631-
?? $this->generateLabelColumnsString();
632-
633627
$orderBy = "
634628
CASE
635629
WHEN `" . implode('` IS NULL AND `', $rollupColumns) . "` IS NULL THEN 1
636630
WHEN `" . implode('` IS NULL OR `', $rollupColumns) . "` IS NULL THEN 0
637631
ELSE 1
638632
END,
639-
$orderByColumns
633+
$withRollupOrderBy
640634
";
641635

642636
$counter = "
@@ -679,6 +673,92 @@ private function generateWindowGroupByExpression(int $limit, bool $withRollup):
679673
return $groupBy;
680674
}
681675

676+
private function generateWindowOrderByExpression(string $innerQuery, bool $withRollup): string
677+
{
678+
$selectColumns = DbHelper::extractSelectFromQuery($innerQuery);
679+
680+
if ($withRollup && '*' === $selectColumns) {
681+
// special case for wrapped ROLLUP query
682+
// we find the real columns one level deeper
683+
$outerSelectPos = stripos($innerQuery, 'SELECT');
684+
$innerSelectPos = stripos($innerQuery, 'SELECT', $outerSelectPos + strlen('SELECT'));
685+
$realInnerQuery = substr($innerQuery, $innerSelectPos);
686+
$selectColumns = DbHelper::extractSelectFromQuery($realInnerQuery);
687+
}
688+
689+
if (null === $selectColumns || '*' === $selectColumns) {
690+
// order by label columns if we can not find
691+
// the named SELECT columns to order by
692+
return $this->generateLabelColumnsString();
693+
}
694+
695+
$columns = DbHelper::extractOrderByFromQuery($innerQuery, true);
696+
$columns = $this->prepareColumnsForWindowOrderBy($columns, $selectColumns);
697+
698+
if (null === $columns) {
699+
$columns = DbHelper::extractGroupByFromQuery($innerQuery, true);
700+
701+
if (null === $columns && $withRollup) {
702+
// a rollup has the GROUP BY inside the wrapper
703+
$outerSelectPos = stripos($innerQuery, 'SELECT');
704+
$innerSelectPos = stripos($innerQuery, 'SELECT', $outerSelectPos + strlen('SELECT'));
705+
$lastClosingParenthesis = strrpos($innerQuery, ')');
706+
707+
$realInnerQuery = substr($innerQuery, $innerSelectPos, $lastClosingParenthesis - $innerSelectPos);
708+
$columns = DbHelper::extractGroupByFromQuery($realInnerQuery, true);
709+
}
710+
711+
$columns = $this->prepareColumnsForWindowOrderBy($columns, $selectColumns);
712+
}
713+
714+
if (null === $columns) {
715+
$columns = $this->generateLabelColumnsString();
716+
}
717+
718+
return $columns;
719+
}
720+
721+
private function prepareColumnsForWindowOrderBy(?string $expr, string $selectColumns): ?string
722+
{
723+
if (null === $expr) {
724+
return null;
725+
}
726+
727+
$exprColumns = explode(',', $expr);
728+
729+
foreach ($exprColumns as $i => &$exprColumn) {
730+
$columnParts = explode(' ', trim($exprColumn));
731+
732+
if (count($columnParts) > 2) {
733+
// the column contains more than just "column ASC"
734+
// remove the column for safety, we don't know what we are really dealing with
735+
unset($exprColumns[$i]);
736+
continue;
737+
}
738+
739+
$column = str_replace('`', '', trim($columnParts[0]));
740+
741+
if (preg_match('/`?' . $column . '`? AS `?(\w+)`?(?:,|$)/is', $selectColumns, $matches)) {
742+
// unalias the column to allow usage in window
743+
$column = trim($matches[1]);
744+
$columnParts[0] = '`' . $column . '`';
745+
$exprColumn = implode(' ', $columnParts);
746+
}
747+
748+
if (!preg_match('/`?' . $column . '`?(?:,|$)/is', $selectColumns)) {
749+
// the column was not found as "column," or "column<end of select>" in the SELECT part
750+
// we remove it from the window because it otherwise break the query
751+
unset($exprColumns[$i]);
752+
}
753+
}
754+
755+
if ([] === $exprColumns) {
756+
return null;
757+
}
758+
759+
return implode(', ', $exprColumns);
760+
}
761+
682762
/**
683763
* Prepare the inner query for usage in the ranking query.
684764
*/

‎tests/PHPUnit/Unit/RankingQueryTest.php‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -643,4 +643,125 @@ public function getPartitionResultTestData(): iterable
643643
$expectedQuery,
644644
];
645645
}
646+
647+
/**
648+
* @dataProvider getGenerateWindowOrderByStringTestData
649+
*/
650+
public function testGenerateWindowOrderByString(
651+
RankingQuery $rankingQuery,
652+
string $innerQuery,
653+
bool $withRollup,
654+
string $expectedOrderBy
655+
): void {
656+
$reflection = new \ReflectionClass($rankingQuery);
657+
$method = $reflection->getMethod('generateWindowOrderByExpression');
658+
$method->setAccessible(true);
659+
660+
$windowOrderBy = $method->invokeArgs($rankingQuery, [$innerQuery, $withRollup]);
661+
662+
$this->assertEquals($expectedOrderBy, $windowOrderBy);
663+
}
664+
665+
public function getGenerateWindowOrderByStringTestData(): iterable
666+
{
667+
$rankingQuery = new RankingQuery(1);
668+
$rankingQuery->addLabelColumn('label_1');
669+
$rankingQuery->addLabelColumn('label_2');
670+
671+
$reflection = new \ReflectionClass($rankingQuery);
672+
$method = $reflection->getMethod('generateLabelColumnsString');
673+
$method->setAccessible(true);
674+
675+
$labelColumnsString = $method->invokeArgs($rankingQuery, []);
676+
677+
yield 'SELECT extraction fails' => [
678+
$rankingQuery,
679+
'SET @counter = 1',
680+
false,
681+
$labelColumnsString,
682+
];
683+
684+
yield 'SELECT extraction fails - with rollup' => [
685+
$rankingQuery,
686+
'SELECT * FROM (SET @counter = 1)',
687+
true,
688+
$labelColumnsString,
689+
];
690+
691+
yield 'window matches ORDER BY' => [
692+
$rankingQuery,
693+
'SELECT column_one, column_two FROM my_table GROUP BY column_two ORDER BY column_one ASC',
694+
false,
695+
'column_one ASC',
696+
];
697+
698+
yield 'window matches ORDER BY - rollup' => [
699+
$rankingQuery,
700+
'
701+
SELECT *
702+
FROM (
703+
SELECT column_one, column_two
704+
FROM my_table
705+
GROUP BY column_two WITH ROLLUP
706+
) AS rollupQuery
707+
ORDER BY column_one ASC
708+
',
709+
true,
710+
'column_one ASC',
711+
];
712+
713+
yield 'window matches GROUP BY' => [
714+
$rankingQuery,
715+
'
716+
SELECT column_one, column_two
717+
FROM my_table
718+
GROUP BY column_one
719+
',
720+
false,
721+
'column_one',
722+
];
723+
724+
yield 'window matches GROUP BY - rollup' => [
725+
$rankingQuery,
726+
'
727+
SELECT *
728+
FROM (
729+
SELECT column_one, column_two
730+
FROM my_table
731+
GROUP BY column_one WITH ROLLUP
732+
) AS rollupQuery
733+
',
734+
true,
735+
'column_one',
736+
];
737+
738+
yield 'unselected column is removed' => [
739+
$rankingQuery,
740+
'SELECT column_one FROM my_table ORDER BY column_one, column_two',
741+
false,
742+
'column_one',
743+
];
744+
745+
yield 'column aliases are resolved' => [
746+
$rankingQuery,
747+
'SELECT column_one AS column_new FROM my_table ORDER BY column_one',
748+
false,
749+
'`column_new`',
750+
];
751+
752+
yield 'column aliases are resolved - rollup' => [
753+
$rankingQuery,
754+
'
755+
SELECT *
756+
FROM (
757+
SELECT column_one AS column_new
758+
FROM my_table
759+
GROUP BY column_two WITH ROLLUP
760+
) AS rollupQuery
761+
ORDER BY column_new
762+
',
763+
true,
764+
'column_new',
765+
];
766+
}
646767
}

0 commit comments

Comments
 (0)