Skip to content

Commit 026bcb7

Browse files
memleakdmemleakd
committed
fix(database): validate lockForUpdate query restrictions
- add driver validation for Postgre and OCI8 unsupported query shapes - reject lockForUpdate with union queries across supported builders - track Query Builder aggregate helper selections for lock validation - document row-locking restrictions and add focused builder coverage Signed-off-by: memleakd <121398829+memleakd@users.noreply.github.com>
1 parent 3f76f4e commit 026bcb7

6 files changed

Lines changed: 149 additions & 20 deletions

File tree

‎system/Database/BaseBuilder.php‎

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,11 @@ class BaseBuilder
115115
*/
116116
protected bool $QBLockForUpdate = false;
117117

118+
/**
119+
* QB SELECT aggregate helper flag
120+
*/
121+
protected bool $QBSelectUsesAggregate = false;
122+
118123
/**
119124
* QB ORDER BY data
120125
*
@@ -547,8 +552,9 @@ protected function maxMinAvgSum(string $select = '', string $alias = '', string
547552

548553
$sql = $type . '(' . $this->db->protectIdentifiers(trim($select)) . ') AS ' . $this->db->escapeIdentifiers(trim($alias));
549554

550-
$this->QBSelect[] = $sql;
551-
$this->QBNoEscape[] = null;
555+
$this->QBSelect[] = $sql;
556+
$this->QBNoEscape[] = null;
557+
$this->QBSelectUsesAggregate = true;
552558

553559
return $this;
554560
}
@@ -3254,6 +3260,10 @@ protected function compileSelect($selectOverride = false): string
32543260
*/
32553261
protected function compileLockForUpdate(): string
32563262
{
3263+
if ($this->QBLockForUpdate && $this->QBUnion !== []) {
3264+
throw new DatabaseException('Query Builder does not support lockForUpdate() with union() or unionAll().');
3265+
}
3266+
32573267
return $this->QBLockForUpdate ? "\nFOR UPDATE" : '';
32583268
}
32593269

@@ -3564,18 +3574,19 @@ protected function resetRun(array $qbResetItems)
35643574
protected function resetSelect()
35653575
{
35663576
$this->resetRun([
3567-
'QBSelect' => [],
3568-
'QBJoin' => [],
3569-
'QBWhere' => [],
3570-
'QBGroupBy' => [],
3571-
'QBHaving' => [],
3572-
'QBOrderBy' => [],
3573-
'QBNoEscape' => [],
3574-
'QBDistinct' => false,
3575-
'QBLimit' => false,
3576-
'QBOffset' => false,
3577-
'QBLockForUpdate' => false,
3578-
'QBUnion' => [],
3577+
'QBSelect' => [],
3578+
'QBJoin' => [],
3579+
'QBWhere' => [],
3580+
'QBGroupBy' => [],
3581+
'QBHaving' => [],
3582+
'QBOrderBy' => [],
3583+
'QBNoEscape' => [],
3584+
'QBDistinct' => false,
3585+
'QBLimit' => false,
3586+
'QBOffset' => false,
3587+
'QBLockForUpdate' => false,
3588+
'QBSelectUsesAggregate' => false,
3589+
'QBUnion' => [],
35793590
]);
35803591

35813592
if ($this->db instanceof BaseConnection) {

‎system/Database/OCI8/Builder.php‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,10 +218,18 @@ protected function _limit(string $sql, bool $offsetIgnore = false): string
218218
*/
219219
protected function compileLockForUpdate(): string
220220
{
221-
if ($this->QBLockForUpdate && ($this->QBLimit !== false || $this->QBOffset)) {
221+
if (! $this->QBLockForUpdate) {
222+
return '';
223+
}
224+
225+
if ($this->QBLimit !== false || $this->QBOffset) {
222226
throw new DatabaseException('OCI8 does not support lockForUpdate() with limit() or offset().');
223227
}
224228

229+
if ($this->QBDistinct || $this->QBGroupBy !== [] || $this->QBHaving !== [] || $this->QBSelectUsesAggregate) {
230+
throw new DatabaseException('OCI8 does not support lockForUpdate() with distinct(), groupBy(), having(), or aggregate helper selections.');
231+
}
232+
225233
return parent::compileLockForUpdate();
226234
}
227235

‎system/Database/Postgre/Builder.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,22 @@ protected function compileIgnore(string $statement)
6262
return $sql;
6363
}
6464

65+
/**
66+
* Compile the SELECT lock clause.
67+
*/
68+
protected function compileLockForUpdate(): string
69+
{
70+
if (! $this->QBLockForUpdate) {
71+
return '';
72+
}
73+
74+
if ($this->QBDistinct || $this->QBGroupBy !== [] || $this->QBHaving !== [] || $this->QBSelectUsesAggregate) {
75+
throw new DatabaseException('Postgre does not support lockForUpdate() with distinct(), groupBy(), having(), or aggregate helper selections.');
76+
}
77+
78+
return parent::compileLockForUpdate();
79+
}
80+
6581
/**
6682
* ORDER BY
6783
*

‎system/Database/SQLSRV/Builder.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -703,6 +703,10 @@ protected function compileLockForUpdate(): string
703703
throw new DatabaseException('SQLSRV does not support lockForUpdate() without a FROM table.');
704704
}
705705

706+
if ($this->QBUnion !== []) {
707+
throw new DatabaseException('Query Builder does not support lockForUpdate() with union() or unionAll().');
708+
}
709+
706710
foreach ($this->QBFrom as $value) {
707711
if (str_starts_with($value, '(SELECT')) {
708712
throw new DatabaseException('SQLSRV does not support lockForUpdate() on subqueries.');

‎tests/system/Database/Builder/SelectTest.php‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use CodeIgniter\Database\Exceptions\DatabaseException;
1818
use CodeIgniter\Database\Exceptions\DataException;
1919
use CodeIgniter\Database\OCI8\Builder as OCI8Builder;
20+
use CodeIgniter\Database\Postgre\Builder as PostgreBuilder;
2021
use CodeIgniter\Database\RawSql;
2122
use CodeIgniter\Database\SQLite3\Builder as SQLite3Builder;
2223
use CodeIgniter\Database\SQLSRV\Builder as SQLSRVBuilder;
@@ -417,6 +418,28 @@ public function testLockForUpdateResetsWithSelect(): void
417418
$this->assertSame('SELECT * FROM "users"', str_replace("\n", ' ', $builder->getCompiledSelect()));
418419
}
419420

421+
public function testLockForUpdateThrowsExceptionWithUnion(): void
422+
{
423+
$builder = new BaseBuilder('users', $this->db);
424+
425+
$this->expectException(DatabaseException::class);
426+
$this->expectExceptionMessage('Query Builder does not support lockForUpdate() with union() or unionAll().');
427+
428+
$builder->union(new BaseBuilder('jobs', $this->db))->lockForUpdate()->getCompiledSelect();
429+
}
430+
431+
public function testLockForUpdateThrowsExceptionWithSQLSRVUnion(): void
432+
{
433+
$this->db = new MockConnection(['DBDriver' => 'SQLSRV', 'database' => 'test', 'schema' => 'dbo']);
434+
435+
$builder = new SQLSRVBuilder('users', $this->db);
436+
437+
$this->expectException(DatabaseException::class);
438+
$this->expectExceptionMessage('Query Builder does not support lockForUpdate() with union() or unionAll().');
439+
440+
$builder->union(new SQLSRVBuilder('jobs', $this->db))->lockForUpdate()->getCompiledSelect();
441+
}
442+
420443
public function testLockForUpdateWithOCI8(): void
421444
{
422445
$builder = new OCI8Builder('users', $this->db);
@@ -436,6 +459,66 @@ public function testLockForUpdateThrowsExceptionWithOCI8Limit(): void
436459
$builder->limit(1)->lockForUpdate()->getCompiledSelect();
437460
}
438461

462+
#[DataProvider('provideLockForUpdateUnsupportedSelectClauses')]
463+
public function testLockForUpdateThrowsExceptionWithOCI8SelectClause(string $clause): void
464+
{
465+
$builder = new OCI8Builder('users', $this->db);
466+
467+
$this->expectException(DatabaseException::class);
468+
$this->expectExceptionMessage('OCI8 does not support lockForUpdate() with distinct(), groupBy(), having(), or aggregate helper selections.');
469+
470+
$this->applyLockForUpdateUnsupportedClause($builder, $clause)
471+
->lockForUpdate()
472+
->getCompiledSelect();
473+
}
474+
475+
public function testLockForUpdateWithPostgre(): void
476+
{
477+
$builder = new PostgreBuilder('users', $this->db);
478+
479+
$expected = 'SELECT * FROM "users" FOR UPDATE';
480+
481+
$this->assertSame($expected, str_replace("\n", ' ', $builder->lockForUpdate()->getCompiledSelect()));
482+
}
483+
484+
#[DataProvider('provideLockForUpdateUnsupportedSelectClauses')]
485+
public function testLockForUpdateThrowsExceptionWithPostgreSelectClause(string $clause): void
486+
{
487+
$builder = new PostgreBuilder('users', $this->db);
488+
489+
$this->expectException(DatabaseException::class);
490+
$this->expectExceptionMessage('Postgre does not support lockForUpdate() with distinct(), groupBy(), having(), or aggregate helper selections.');
491+
492+
$this->applyLockForUpdateUnsupportedClause($builder, $clause)
493+
->lockForUpdate()
494+
->getCompiledSelect();
495+
}
496+
497+
/**
498+
* @return iterable<string, list<string>>
499+
*/
500+
public static function provideLockForUpdateUnsupportedSelectClauses(): iterable
501+
{
502+
yield 'distinct' => ['distinct'];
503+
504+
yield 'groupBy' => ['groupBy'];
505+
506+
yield 'having' => ['having'];
507+
508+
yield 'aggregate selection' => ['aggregate'];
509+
}
510+
511+
private function applyLockForUpdateUnsupportedClause(BaseBuilder $builder, string $clause): BaseBuilder
512+
{
513+
return match ($clause) {
514+
'distinct' => $builder->distinct(),
515+
'groupBy' => $builder->groupBy('role'),
516+
'having' => $builder->having('COUNT(id) >', 1, false),
517+
'aggregate' => $builder->selectCount('id'),
518+
default => throw new DatabaseException('Unsupported clause: ' . $clause),
519+
};
520+
}
521+
439522
public function testLockForUpdateThrowsExceptionOnSQLite3(): void
440523
{
441524
$builder = new SQLite3Builder('users', $this->db);

‎user_guide_src/source/database/query_builder.rst‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -777,18 +777,25 @@ Use this method inside a database transaction. The exact locking behavior is
777777
determined by the database server and transaction isolation level.
778778

779779
This method is supported by the **MySQLi**, **Postgre**, **OCI8**, and
780-
**SQLSRV** drivers. Unsupported drivers throw a ``DatabaseException``. See the
781-
following notes for OCI8 and SQLSRV driver-specific behavior.
780+
**SQLSRV** drivers. Unsupported drivers throw a ``DatabaseException``.
781+
``lockForUpdate()`` is not supported with ``union()`` or ``unionAll()``.
782+
Some databases restrict which query shapes can be used with row locking. When
783+
CodeIgniter can detect an unsupported combination, it throws a
784+
``DatabaseException``. See the following warnings for driver-specific behavior.
785+
786+
.. warning:: Postgre does not support ``lockForUpdate()`` with ``distinct()``,
787+
``groupBy()``, ``having()``, or aggregate helper selections such as
788+
``selectCount()``.
782789

783790
.. warning:: SQLSRV uses SQL Server table hints instead of a trailing ``FOR UPDATE``
784791
clause. The hint is applied to table references in the ``FROM`` clause;
785792
joined tables are not hinted. Its exact lock granularity depends on SQL
786793
Server's execution plan and transaction isolation level. SQLSRV does not
787794
support ``lockForUpdate()`` without a ``FROM`` table or on subqueries.
788795

789-
.. warning:: OCI8 does not support ``lockForUpdate()`` together with ``limit()`` or
790-
``offset()`` because Oracle does not allow ``FOR UPDATE`` with row
791-
limiting.
796+
.. warning:: OCI8 does not support ``lockForUpdate()`` together with
797+
``limit()``, ``offset()``, ``distinct()``, ``groupBy()``, ``having()``, or
798+
aggregate helper selections such as ``selectCount()``.
792799

793800
.. _query-builder-union:
794801

0 commit comments

Comments
 (0)