Skip to content

Commit

Permalink
Merge pull request #9009 from kenjis/fix-qb-select-rawsql
Browse files Browse the repository at this point in the history 
fix: [QueryBuilder] select() with RawSql may cause TypeError
  • Loading branch information
@kenjis
kenjis authored Jul 1, 2024
2 parents bab8828 + c022dd3 commit e7cb689
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 23 deletions.
12 changes: 0 additions & 12 deletions phpstan-baseline.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1819,12 +1819,6 @@
'count' => 1,
'path' => __DIR__ . '/system/Database/BaseBuilder.php',
];
$ignoreErrors[] = [
// identifier: missingType.iterableValue
'message' => '#^Method CodeIgniter\\\\Database\\\\BaseBuilder\\:\\:select\\(\\) has parameter \\$select with no value type specified in iterable type array\\.$#',
'count' => 1,
'path' => __DIR__ . '/system/Database/BaseBuilder.php',
];
$ignoreErrors[] = [
// identifier: missingType.iterableValue
'message' => '#^Method CodeIgniter\\\\Database\\\\BaseBuilder\\:\\:set\\(\\) has parameter \\$key with no value type specified in iterable type array\\.$#',
Expand Down Expand Up @@ -1993,12 +1987,6 @@
'count' => 1,
'path' => __DIR__ . '/system/Database/BaseBuilder.php',
];
$ignoreErrors[] = [
// identifier: missingType.iterableValue
'message' => '#^Property CodeIgniter\\\\Database\\\\BaseBuilder\\:\\:\\$QBNoEscape type has no value type specified in iterable type array\\.$#',
'count' => 1,
'path' => __DIR__ . '/system/Database/BaseBuilder.php',
];
$ignoreErrors[] = [
// identifier: missingType.iterableValue
'message' => '#^Property CodeIgniter\\\\Database\\\\BaseBuilder\\:\\:\\$QBOptions type has no value type specified in iterable type array\\.$#',
Expand Down
30 changes: 19 additions & 11 deletions system/Database/BaseBuilder.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,9 @@ class BaseBuilder
protected array $QBUnion = [];

/**
* QB NO ESCAPE data
* Whether to protect identifiers in SELECT
*
* @var array
* @var list<bool|null> true=protect, false=not protect
*/
public $QBNoEscape = [];

Expand Down Expand Up @@ -390,7 +390,8 @@ public function ignore(bool $ignore = true)
/**
* Generates the SELECT portion of the query
*
* @param array|RawSql|string $select
* @param list<RawSql|string>|RawSql|string $select
* @param bool|null $escape Whether to protect identifiers
*
* @return $this
*/
Expand All @@ -402,16 +403,21 @@ public function select($select = '*', ?bool $escape = null)
}

if ($select instanceof RawSql) {
$this->QBSelect[] = $select;

return $this;
$select = [$select];
}

if (is_string($select)) {
$select = $escape === false ? [$select] : explode(',', $select);
$select = ($escape === false) ? [$select] : explode(',', $select);
}

foreach ($select as $val) {
if ($val instanceof RawSql) {
$this->QBSelect[] = $val;
$this->QBNoEscape[] = false;

continue;
}

$val = trim($val);

if ($val !== '') {
Expand Down Expand Up @@ -3054,15 +3060,17 @@ protected function compileSelect($selectOverride = false): string

if (empty($this->QBSelect)) {
$sql .= '*';
} elseif ($this->QBSelect[0] instanceof RawSql) {
$sql .= (string) $this->QBSelect[0];
} else {
// Cycle through the "select" portion of the query and prep each column name.
// The reason we protect identifiers here rather than in the select() function
// is because until the user calls the from() function we don't know if there are aliases
foreach ($this->QBSelect as $key => $val) {
$noEscape = $this->QBNoEscape[$key] ?? null;
$this->QBSelect[$key] = $this->db->protectIdentifiers($val, false, $noEscape);
if ($val instanceof RawSql) {
$this->QBSelect[$key] = (string) $val;
} else {
$protect = $this->QBNoEscape[$key] ?? null;
$this->QBSelect[$key] = $this->db->protectIdentifiers($val, false, $protect);
}
}

$sql .= implode(', ', $this->QBSelect);
Expand Down
60 changes: 60 additions & 0 deletions tests/system/Database/Builder/SelectTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use CodeIgniter\Database\SQLSRV\Builder as SQLSRVBuilder;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\Mock\MockConnection;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\Group;

/**
Expand Down Expand Up @@ -67,6 +68,65 @@ public function testSelectAcceptsArray(): void
$this->assertSame($expected, str_replace("\n", ' ', $builder->getCompiledSelect()));
}

/**
* @param list<RawSql|string> $select
*/
#[DataProvider('provideSelectAcceptsArrayWithRawSql')]
public function testSelectAcceptsArrayWithRawSql(array $select, string $expected): void
{
$builder = new BaseBuilder('employees', $this->db);

$builder->select($select);

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

/**
* @return list<list<RawSql|string>|string>
*/
public static function provideSelectAcceptsArrayWithRawSql(): iterable
{
yield from [
[
[
new RawSql("IF(salary > 5000, 'High', 'Low') AS salary_level"),
'employee_id',
],
<<<'SQL'
SELECT IF(salary > 5000, 'High', 'Low') AS salary_level, "employee_id" FROM "employees"
SQL,
],
[
[
'employee_id',
new RawSql("IF(salary > 5000, 'High', 'Low') AS salary_level"),
],
<<<'SQL'
SELECT "employee_id", IF(salary > 5000, 'High', 'Low') AS salary_level FROM "employees"
SQL,
],
[
[
new RawSql("CONCAT(first_name, ' ', last_name) AS full_name"),
new RawSql("IF(salary > 5000, 'High', 'Low') AS salary_level"),
],
<<<'SQL'
SELECT CONCAT(first_name, ' ', last_name) AS full_name, IF(salary > 5000, 'High', 'Low') AS salary_level FROM "employees"
SQL,
],
[
[
new RawSql("CONCAT(first_name, ' ', last_name) AS full_name"),
'employee_id',
new RawSql("IF(salary > 5000, 'High', 'Low') AS salary_level"),
],
<<<'SQL'
SELECT CONCAT(first_name, ' ', last_name) AS full_name, "employee_id", IF(salary > 5000, 'High', 'Low') AS salary_level FROM "employees"
SQL,
],
];
}

public function testSelectAcceptsMultipleColumns(): void
{
$builder = new BaseBuilder('users', $this->db);
Expand Down

0 comments on commit e7cb689

Please sign in to comment.