Skip to content

fix: QueryBuilder limit(0) bug #8280

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.

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

Merged
merged 14 commits into from
Dec 16, 2023
8 changes: 8 additions & 0 deletions app/Config/Feature.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,12 @@ class Feature extends BaseConfig
* Use filter execution order in 4.4 or before.
*/
public bool $oldFilterOrder = false;

/**
* The behavior of `limit(0)` in Query Builder.
*
* If true, `limit(0)` returns all records. (the behavior of 4.4.x or before in version 4.x.)
* If false, `limit(0)` returns no records. (the behavior of 3.1.9 or later in version 3.x.)
*/
public bool $limitZeroAsAll = true;
}
2 changes: 1 addition & 1 deletion phpstan-baseline.php
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,7 @@
];
$ignoreErrors[] = [
'message' => '#^Construct empty\\(\\) is not allowed\\. Use more strict comparison\\.$#',
'count' => 40,
'count' => 37,
'path' => __DIR__ . '/system/Database/BaseBuilder.php',
];
$ignoreErrors[] = [
Expand Down
13 changes: 9 additions & 4 deletions system/BaseModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
use CodeIgniter\I18n\Time;
use CodeIgniter\Pager\Pager;
use CodeIgniter\Validation\ValidationInterface;
use Config\Feature;
use Config\Services;
use InvalidArgumentException;
use ReflectionClass;
Expand Down Expand Up @@ -379,12 +380,12 @@ abstract protected function doFindColumn(string $columnName);
* Fetches all results, while optionally limiting them.
* This method works only with dbCalls.
*
* @param int $limit Limit
* @param int $offset Offset
* @param int|null $limit Limit
* @param int $offset Offset
*
* @return array
*/
abstract protected function doFindAll(int $limit = 0, int $offset = 0);
abstract protected function doFindAll(?int $limit = null, int $offset = 0);

/**
* Returns the first row of the result set.
Expand Down Expand Up @@ -600,8 +601,12 @@ public function findColumn(string $columnName)
*
* @return array
*/
public function findAll(int $limit = 0, int $offset = 0)
public function findAll(?int $limit = null, int $offset = 0)
{
if (config(Feature::class)->limitZeroAsAll) {
$limit ??= 0;
}

if ($this->tempAllowCallbacks) {
// Call the before event and check for a return
$eventData = $this->trigger('beforeFind', [
Expand Down
42 changes: 37 additions & 5 deletions system/Database/BaseBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use CodeIgniter\Database\Exceptions\DatabaseException;
use CodeIgniter\Database\Exceptions\DataException;
use CodeIgniter\Traits\ConditionalTrait;
use Config\Feature;
use InvalidArgumentException;

/**
Expand Down Expand Up @@ -1488,6 +1489,10 @@ public function orderBy(string $orderBy, string $direction = '', ?bool $escape =
*/
public function limit(?int $value = null, ?int $offset = 0)
{
if (config(Feature::class)->limitZeroAsAll && $value === 0) {
$value = null;
}

if ($value !== null) {
$this->QBLimit = $value;
}
Expand Down Expand Up @@ -1605,6 +1610,10 @@ protected function compileFinalQuery(string $sql): string
*/
public function get(?int $limit = null, int $offset = 0, bool $reset = true)
{
if (config(Feature::class)->limitZeroAsAll && $limit === 0) {
$limit = null;
}

if ($limit !== null) {
$this->limit($limit, $offset);
}
Expand Down Expand Up @@ -1738,7 +1747,11 @@ public function getWhere($where = null, ?int $limit = null, ?int $offset = 0, bo
$this->where($where);
}

if (! empty($limit)) {
if (config(Feature::class)->limitZeroAsAll && $limit === 0) {
$limit = null;
}

if ($limit !== null) {
$this->limit($limit, $offset);
}

Expand Down Expand Up @@ -2454,7 +2467,11 @@ public function update($set = null, $where = null, ?int $limit = null): bool
$this->where($where);
}

if (! empty($limit)) {
if (config(Feature::class)->limitZeroAsAll && $limit === 0) {
$limit = null;
}

if ($limit !== null) {
if (! $this->canLimitWhereUpdates) {
throw new DatabaseException('This driver does not allow LIMITs on UPDATE queries using WHERE.');
}
Expand Down Expand Up @@ -2495,10 +2512,17 @@ protected function _update(string $table, array $values): string
$valStr[] = $key . ' = ' . $val;
}

if (config(Feature::class)->limitZeroAsAll) {
return 'UPDATE ' . $this->compileIgnore('update') . $table . ' SET ' . implode(', ', $valStr)
. $this->compileWhereHaving('QBWhere')
. $this->compileOrderBy()
. ($this->QBLimit ? $this->_limit(' ', true) : '');
}

return 'UPDATE ' . $this->compileIgnore('update') . $table . ' SET ' . implode(', ', $valStr)
. $this->compileWhereHaving('QBWhere')
. $this->compileOrderBy()
. ($this->QBLimit ? $this->_limit(' ', true) : '');
. ($this->QBLimit !== false ? $this->_limit(' ', true) : '');
}

/**
Expand Down Expand Up @@ -2764,7 +2788,11 @@ public function delete($where = '', ?int $limit = null, bool $resetData = true)

$sql = $this->_delete($this->removeAlias($table));

if (! empty($limit)) {
if (config(Feature::class)->limitZeroAsAll && $limit === 0) {
$limit = null;
}

if ($limit !== null) {
$this->QBLimit = $limit;
}

Expand Down Expand Up @@ -3030,7 +3058,11 @@ protected function compileSelect($selectOverride = false): string
. $this->compileWhereHaving('QBHaving')
. $this->compileOrderBy();

if ($this->QBLimit) {
if (config(Feature::class)->limitZeroAsAll) {
if ($this->QBLimit) {
$sql = $this->_limit($sql . "\n");
}
} elseif ($this->QBLimit !== false || $this->QBOffset) {
$sql = $this->_limit($sql . "\n");
}

Expand Down
19 changes: 18 additions & 1 deletion system/Database/SQLSRV/Builder.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use CodeIgniter\Database\Exceptions\DataException;
use CodeIgniter\Database\RawSql;
use CodeIgniter\Database\ResultInterface;
use Config\Feature;

/**
* Builder for SQLSRV
Expand Down Expand Up @@ -308,6 +309,14 @@ private function addIdentity(string $fullTable, string $insert): string
*/
protected function _limit(string $sql, bool $offsetIgnore = false): string
{
// SQL Server cannot handle `LIMIT 0`.
// DatabaseException:
// [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]The number of
// rows provided for a FETCH clause must be greater then zero.
if (! config(Feature::class)->limitZeroAsAll && $this->QBLimit === 0) {
return "SELECT * \nFROM " . $this->_fromTables() . ' WHERE 1=0 ';
}

if (empty($this->QBOrderBy)) {
$sql .= ' ORDER BY (SELECT NULL) ';
}
Expand Down Expand Up @@ -590,7 +599,11 @@ protected function compileSelect($selectOverride = false): string
. $this->compileOrderBy(); // ORDER BY

// LIMIT
if ($this->QBLimit) {
if (config(Feature::class)->limitZeroAsAll) {
if ($this->QBLimit) {
$sql = $this->_limit($sql . "\n");
}
} elseif ($this->QBLimit !== false || $this->QBOffset) {
$sql = $this->_limit($sql . "\n");
}

Expand All @@ -605,6 +618,10 @@ protected function compileSelect($selectOverride = false): string
*/
public function get(?int $limit = null, int $offset = 0, bool $reset = true)
{
if (config(Feature::class)->limitZeroAsAll && $limit === 0) {
$limit = null;
}

if ($limit !== null) {
$this->limit($limit, $offset);
}
Expand Down
11 changes: 8 additions & 3 deletions system/Model.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
use CodeIgniter\Exceptions\ModelException;
use CodeIgniter\Validation\ValidationInterface;
use Config\Database;
use Config\Feature;
use ReflectionException;

/**
Expand Down Expand Up @@ -223,14 +224,18 @@ protected function doFindColumn(string $columnName)
* all results, while optionally limiting them.
* This method works only with dbCalls.
*
* @param int $limit Limit
* @param int $offset Offset
* @param int|null $limit Limit
* @param int $offset Offset
*
* @return array
* @phpstan-return list<row_array|object>
*/
protected function doFindAll(int $limit = 0, int $offset = 0)
protected function doFindAll(?int $limit = null, int $offset = 0)
{
if (config(Feature::class)->limitZeroAsAll) {
$limit ??= 0;
}

$builder = $this->builder();

if ($this->tempUseSoftDeletes) {
Expand Down
20 changes: 20 additions & 0 deletions tests/system/Database/Builder/GetTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\Mock\MockConnection;
use Config\Feature;

/**
* @internal
Expand Down Expand Up @@ -46,6 +47,25 @@ public function testGet(): void
*/
public function testGetWithReset(): void
{
$config = config(Feature::class);
$config->limitZeroAsAll = false;

$builder = $this->db->table('users');
$builder->testMode()->where('username', 'bogus');

$expectedSQL = 'SELECT * FROM "users" WHERE "username" = \'bogus\' LIMIT 50, 0';
$expectedSQLafterreset = 'SELECT * FROM "users" LIMIT 50, 0';

$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->get(0, 50, false)));
$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->get(0, 50, true)));
$this->assertSame($expectedSQLafterreset, str_replace("\n", ' ', $builder->get(0, 50, true)));
}

public function testGetWithResetWithLimitZeroAsAll(): void
{
$config = config(Feature::class);
$config->limitZeroAsAll = true;

$builder = $this->db->table('users');
$builder->testMode()->where('username', 'bogus');

Expand Down
21 changes: 21 additions & 0 deletions tests/system/Database/Live/GetTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use CodeIgniter\Database\Exceptions\DatabaseException;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\DatabaseTestTrait;
use Config\Feature;
use Tests\Support\Database\Seeds\CITestSeeder;

/**
Expand Down Expand Up @@ -50,6 +51,26 @@ public function testGetWitLimit(): void
$this->assertSame('Musician', $jobs[1]->name);
}

public function testGetWithLimitZero(): void
{
$config = config(Feature::class);
$config->limitZeroAsAll = false;

$jobs = $this->db->table('job')->limit(0)->get()->getResult();

$this->assertCount(0, $jobs);
}

public function testGetWithLimitZeroAsAll(): void
{
$config = config(Feature::class);
$config->limitZeroAsAll = true;

$jobs = $this->db->table('job')->limit(0)->get()->getResult();

$this->assertCount(4, $jobs);
}

public function testGetWhereArray(): void
{
$jobs = $this->db->table('job')
Expand Down
26 changes: 26 additions & 0 deletions user_guide_src/source/changelogs/v4.5.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,21 @@ Database
Query Builder
-------------

.. _v450-query-builder-limit-0-behavior:

limit(0) Behavior
^^^^^^^^^^^^^^^^^

- Added a feature flag ``Feature::$limitZeroAsAll`` to fix the incorrect behavior
of ``limit(0)``.
- If ``LIMIT 0`` is specified in a SQL statement, 0 records are returned. However,
there is a bug in the Query Builder, and if ``limit(0)`` is specified, the
generated SQL statement will have no ``LIMIT`` clause and all records will be
returned.
- It is recommended that ``$limitZeroAsAll`` in **app/Config/Feature.php** be set
to ``false`` as this incorrect behavior will be fixed in a future version. See
also :ref:`v450-model-findall-limit-0-behavior`.

Forge
-----

Expand All @@ -260,6 +275,17 @@ Others
Model
=====

.. _v450-model-findall-limit-0-behavior:

findAll(0) Behavior
-------------------

- Added a feature flag ``Feature::$limitZeroAsAll`` to fix the incorrect behavior
of ``limit(0)`` for Query Builder. See :ref:`v450-query-builder-limit-0-behavior`
for details.
- If you disable this flag, you need to change code like ``findAll(0, $offset)``
to ``findAll(null, $offset)``.

Libraries
=========

Expand Down
8 changes: 8 additions & 0 deletions user_guide_src/source/database/query_builder.rst
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,14 @@ Lets you limit the number of rows you would like returned by the query:

.. literalinclude:: query_builder/070.php

.. note:: If ``LIMIT 0`` is specified in a SQL statement, 0 records are returned.
However, there is a bug in the Query Builder, and if ``limit(0)`` is specified,
the generated SQL statement will have no ``LIMIT`` clause and all records will
be returned. To fix the incorrect behavior, a setting was added in v4.5.0. See
:ref:`v450-query-builder-limit-0-behavior` for details. The incorrect behavior
will be fixed in a future version, so it is recommended that you change the
default setting.

The second parameter lets you set a result offset.

.. literalinclude:: query_builder/071.php
Expand Down
2 changes: 2 additions & 0 deletions user_guide_src/source/installation/upgrade_450.rst
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,8 @@ Others
- The default value of ``DBCollat`` in ``$default`` has been change to ``utf8mb4_general_ci``.
- The default value of ``DBCollat`` in ``$tests`` has been change to ``''``.
- app/Config/Feature.php
- ``Config\Feature::$limitZeroAsAll`` has been added. See
:ref:`v450-query-builder-limit-0-behavior`.
- ``Config\Feature::$multipleFilters`` has been removed, because now
:ref:`multiple-filters` are always enabled.
- app/Config/Kint.php
Expand Down
1 change: 1 addition & 0 deletions user_guide_src/source/installation/upgrade_database.rst
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ Upgrade Guide
- ``$this->db->having('user_id', 45);`` to ``$builder->having('user_id', 45);``
6. CI4 does not provide `Database Caching <https://www.codeigniter.com/userguide3/database/caching.html>`_
layer known from CI3, so if you need to cache the result, use :doc:`../libraries/caching` instead.
7. If you use ``limit(0)`` in Query Builder, CI4 returns all records in stead of no records due to a bug. But since v4.5.0, you can change the incorrect behavior with a setting. So change the setting. See :ref:`v450-query-builder-limit-0-behavior` for details.

Code Example
============
Expand Down