Skip to content

refactor: Query Builder WHERE/HAVING condition handling #10217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
memleakd wants to merge 1 commit into
base: 4.8
Choose a base branch
from
+159 −107
Open

refactor: Query Builder WHERE/HAVING condition handling #10217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
227 changes: 121 additions & 106 deletions system/Database/BaseBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,16 +855,14 @@ protected function whereColumnHaving(string $qbKey, string $first, string $secon

$escape ??= $this->db->protectIdentifiers;

$prefix = $this->{$qbKey} === [] ? $this->groupGetType('') : $this->groupGetType($type);

$this->{$qbKey}[] = [
$this->addWhereHavingCondition($qbKey, [
'columnComparison' => true,
'condition' => $prefix,
'condition' => '',
'escape' => $escape,
'first' => $first,
'operator' => $operator,
'second' => $second,
];
], $type);

return $this;
}
Expand Down Expand Up @@ -977,17 +975,15 @@ private function whereBetweenHaving(string $qbKey, ?string $key = null, $values
$lowerBind = $this->setBind($key, $values[0], $escape);
$upperBind = $this->setBind($key, $values[1], $escape);
$not = $not ? ' NOT' : '';
$prefix = $this->{$qbKey} === [] ? $this->groupGetType('') : $this->groupGetType($type);

$this->{$qbKey}[] = [
$this->addWhereHavingCondition($qbKey, [
'betweenComparison' => true,
'condition' => $prefix,
'condition' => '',
'escape' => $escape,
'key' => $key,
'lowerBind' => $lowerBind,
'not' => $not,
'upperBind' => $upperBind,
];
], $type);

return $this;
}
Expand All @@ -1010,13 +1006,12 @@ protected function whereExistsSubquery($subquery, bool $not = false, string $typ
throw new InvalidArgumentException(sprintf('%s() expects $subquery to be of type BaseBuilder or closure', debug_backtrace(0, 2)[1]['function']));
}

$prefix = $this->QBWhere === [] ? $this->groupGetType('') : $this->groupGetType($type);
$operator = $not ? 'NOT EXISTS' : 'EXISTS';

$this->QBWhere[] = [
'condition' => "{$prefix}{$operator} {$this->buildSubquery($subquery, true)}",
$this->addWhereHavingCondition('QBWhere', [
'condition' => "{$operator} {$this->buildSubquery($subquery, true)}",
'escape' => false,
];
], $type);

return $this;
}
Expand Down Expand Up @@ -1055,8 +1050,6 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type
}

foreach ($keyValue as $k => $v) {
$prefix = empty($this->{$qbKey}) ? $this->groupGetType('') : $this->groupGetType($type);

if ($rawSqlOnly) {
$k = '';
$op = '';
Expand Down Expand Up @@ -1104,22 +1097,42 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type
$op = '';
}

$condition = $k . $op . $v;

if ($v instanceof RawSql) {
$this->{$qbKey}[] = [
'condition' => $v->with($prefix . $k . $op . $v),
'escape' => $escape,
];
} else {
$this->{$qbKey}[] = [
'condition' => $prefix . $k . $op . $v,
'escape' => $escape,
];
$condition = $v->with($condition);
}

$this->addWhereHavingCondition($qbKey, [
'condition' => $condition,
'escape' => $escape,
], $type);
}

return $this;
}

/**
* @param array<string, mixed> $condition
*/
private function addWhereHavingCondition(string $clause, array $condition, string $type): void
{
$prefix = $this->getWhereHavingPrefix($clause, $type);

if ($condition['condition'] instanceof RawSql) {
$condition['condition'] = $condition['condition']->with($prefix . $condition['condition']);
} else {
$condition['condition'] = $prefix . $condition['condition'];
}

$this->{$clause}[] = $condition;
}

private function getWhereHavingPrefix(string $clause, string $type): string
{
return $this->{$clause} === [] ? $this->groupGetType('') : $this->groupGetType($type);
}

/**
* Generates a WHERE field IN('item', 'item') SQL query,
* joined with 'AND' if appropriate.
Expand Down Expand Up @@ -1268,14 +1281,10 @@ protected function _whereIn(?string $key = null, $values = null, bool $not = fal

$ok = $this->setBind($ok, $whereIn, $escape);

$prefix = empty($this->{$clause}) ? $this->groupGetType('') : $this->groupGetType($type);

$whereIn = [
'condition' => "{$prefix}{$key}{$not} IN :{$ok}:",
$this->addWhereHavingCondition($clause, [
'condition' => "{$key}{$not} IN :{$ok}:",
'escape' => false,
];

$this->{$clause}[] = $whereIn;
], $type);

return $this;
}
Expand Down Expand Up @@ -1408,7 +1417,7 @@ protected function _like($field, string $match = '', string $type = 'AND ', stri
$v = $match;
$insensitiveSearch = false;

$prefix = empty($this->{$clause}) ? $this->groupGetType('') : $this->groupGetType($type);
$prefix = $this->getWhereHavingPrefix($clause, $type);

if ($side === 'none') {
$bind = $this->setBind($field->getBindingKey(), $v, $escape);
Expand Down Expand Up @@ -1442,7 +1451,7 @@ protected function _like($field, string $match = '', string $type = 'AND ', stri
$v = mb_strtolower($v, 'UTF-8');
}

$prefix = empty($this->{$clause}) ? $this->groupGetType('') : $this->groupGetType($type);
$prefix = $this->getWhereHavingPrefix($clause, $type);

if ($side === 'none') {
$bind = $this->setBind($k, $v, $escape);
Expand Down Expand Up @@ -3479,98 +3488,104 @@ protected function compileWhereHaving(string $qbKey): string
{
if (! empty($this->{$qbKey})) {
foreach ($this->{$qbKey} as &$qbkey) {
// Is this condition already compiled?
if (is_string($qbkey)) {
continue;
}
$qbkey = $this->compileWhereHavingCondition($qbkey);
}

if ($qbkey instanceof RawSql) {
continue;
}
return ($qbKey === 'QBHaving' ? "\nHAVING " : "\nWHERE ")
. implode("\n", $this->{$qbKey});
}

return '';
}

if ($qbkey['condition'] instanceof RawSql) {
$qbkey = $qbkey['condition'];
/**
* @used-by compileWhereHaving()
*
* @param array<string, mixed>|RawSql|string $condition
*/
private function compileWhereHavingCondition(array|RawSql|string $condition): RawSql|string
{
// Is this condition already compiled?
if (is_string($condition) || $condition instanceof RawSql) {
return $condition;
}

continue;
}
if ($condition['condition'] instanceof RawSql) {
return $condition['condition'];
}

if (($qbkey['columnComparison'] ?? false) === true) {
$qbkey = $this->compileColumnComparison($qbkey);
if (($condition['columnComparison'] ?? false) === true) {
return $this->compileColumnComparison($condition);
}

continue;
}
if (($condition['betweenComparison'] ?? false) === true) {
return $this->compileBetweenComparison($condition);
}

if (($qbkey['betweenComparison'] ?? false) === true) {
$qbkey = $this->compileBetweenComparison($qbkey);
if ($condition['escape'] === false) {
return $condition['condition'];
}

continue;
}
return $this->compileEscapedCondition($condition['condition']);
}

if ($qbkey['escape'] === false) {
$qbkey = $qbkey['condition'];
/**
* @used-by compileWhereHavingCondition()
*/
private function compileEscapedCondition(string $condition): string
{
// Split multiple conditions
$conditions = preg_split(
'/((?:^|\s+)AND\s+|(?:^|\s+)OR\s+)/i',
$condition,
-1,
PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY,
);

continue;
}
foreach ($conditions as &$condition) {
$op = $this->getOperator($condition);
if (
$op === false
|| preg_match(
'/^(\(?)(.*)(' . preg_quote($op, '/') . ')\s*(.*(?<!\)))?(\)?)$/i',
$condition,
$matches,
) !== 1
) {
continue;
}

// Split multiple conditions
$conditions = preg_split(
'/((?:^|\s+)AND\s+|(?:^|\s+)OR\s+)/i',
$qbkey['condition'],
-1,
PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY,
);

foreach ($conditions as &$condition) {
$op = $this->getOperator($condition);
if (
$op === false
|| preg_match(
'/^(\(?)(.*)(' . preg_quote($op, '/') . ')\s*(.*(?<!\)))?(\)?)$/i',
$condition,
$matches,
) !== 1
) {
continue;
}
// $matches = [
// 0 => '(test <= foo)', /* the whole thing */
// 1 => '(', /* optional */
// 2 => 'test', /* the field name */
// 3 => ' <= ', /* $op */
// 4 => 'foo', /* optional, if $op is e.g. 'IS NULL' */
// 5 => ')' /* optional */
// ];

// $matches = [
// 0 => '(test <= foo)', /* the whole thing */
// 1 => '(', /* optional */
// 2 => 'test', /* the field name */
// 3 => ' <= ', /* $op */
// 4 => 'foo', /* optional, if $op is e.g. 'IS NULL' */
// 5 => ')' /* optional */
// ];

if ($matches[4] !== '') {
$protectIdentifiers = false;
if (str_contains($matches[4], '.')) {
$protectIdentifiers = true;
}

if (! str_contains($matches[4], ':')) {
$matches[4] = $this->db->protectIdentifiers(trim($matches[4]), false, $protectIdentifiers);
}

$matches[4] = ' ' . $matches[4];
}
if ($matches[4] !== '') {
$protectIdentifiers = false;
if (str_contains($matches[4], '.')) {
$protectIdentifiers = true;
}

$condition = $matches[1] . $this->db->protectIdentifiers(trim($matches[2]))
. ' ' . trim($matches[3]) . $matches[4] . $matches[5];
if (! str_contains($matches[4], ':')) {
$matches[4] = $this->db->protectIdentifiers(trim($matches[4]), false, $protectIdentifiers);
}

$qbkey = implode('', $conditions);
$matches[4] = ' ' . $matches[4];
}

return ($qbKey === 'QBHaving' ? "\nHAVING " : "\nWHERE ")
. implode("\n", $this->{$qbKey});
$condition = $matches[1] . $this->db->protectIdentifiers(trim($matches[2]))
. ' ' . trim($matches[3]) . $matches[4] . $matches[5];
}

return '';
return implode('', $conditions);
}

/**
* @used-by compileWhereHaving()
* @used-by compileWhereHavingCondition()
*
* @param array{columnComparison: true, condition: string, escape: bool, first: string, operator: string, second: string} $condition
*/
Expand All @@ -3585,7 +3600,7 @@ private function compileColumnComparison(array $condition): string
}

/**
* @used-by compileWhereHaving()
* @used-by compileWhereHavingCondition()
*
* @param array{betweenComparison: true, condition: string, escape: bool, key: string, lowerBind: string, not: string, upperBind: string} $condition
*/
Expand Down
37 changes: 37 additions & 0 deletions tests/system/Database/Builder/WhereTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,43 @@ public function testWhereLikeInAssociateArray(): void
$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect()));
}

/**
* @param mixed $value
*/
#[DataProvider('provideWhereOperatorRegressionCases')]
public function testWhereOperatorRegressionCases(string $key, $value, string $expectedSQL): void
{
$builder = $this->db->table('jobs job');

$builder->where($key, $value);

$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect()));
}

/**
* @return iterable<string, array{string, mixed, string}>
*/
public static function provideWhereOperatorRegressionCases(): iterable
{
return [
'like operator with value' => [
'job.status LIKE',
'p%',
'SELECT * FROM "jobs" "job" WHERE "job"."status" LIKE \'p%\'',
],
'equals operator with null' => [
'job.deleted_at =',
null,
'SELECT * FROM "jobs" "job" WHERE "job"."deleted_at" IS NULL',
],
'not equals operator with null' => [
'job.deleted_at !=',
null,
'SELECT * FROM "jobs" "job" WHERE "job"."deleted_at" IS NOT NULL',
],
];
}

public function testWhereCustomString(): void
{
$builder = $this->db->table('jobs');
Expand Down
2 changes: 1 addition & 1 deletion utils/phpstan-baseline/empty.notAllowed.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ parameters:

-
message: '#^Construct empty\(\) is not allowed\. Use more strict comparison\.$#'
count: 28
count: 24
path: ../../system/Database/BaseBuilder.php

-
Expand Down
Loading