Skip to content
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

Fix #20239: fix yii\data\ActiveDataProvider to avoid unexpected pagination results with UNION queries #20311

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Fix #20239: fix yii\data\ActiveDataProvider to avoid unexpected pagination results with UNION queries #20311

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
360bd90
Fix #20239: fix `yii\data\ActiveDataProvider` to avoid unexpected pag…
Izumi-kun Jan 15, 2025
0e6358d
Revert composer.lock
Izumi-kun Jan 16, 2025
e9a919c
Revert unnecessary ActiveDataProviderTest changes
Izumi-kun Jan 16, 2025
96ff732
Improve testPaginationWithUnionQuery to cover more cases
Izumi-kun Jan 16, 2025
dcf8674
Replace resetting with real wrapping
Izumi-kun Jan 16, 2025
442761d
Wrap `createCommand` for respecting `\yii\db\ActiveRecord::getDb`
Izumi-kun Jan 16, 2025
2bedcf4
Merge remote-tracking branch 'origin/master' into bug20239
Izumi-kun Jan 17, 2025
30bc262
Merge branch 'master' into bug20239
samdark Jan 17, 2025
efb8b5e
Merge branch 'master' into bug20239
samdark Feb 14, 2025
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
130 changes: 65 additions & 65 deletions composer.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions framework/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Yii Framework 2 Change Log
- Bug #20300: Clear stat cache in `FileCache::setValue()` (rob006)
- Enh #20306: Add new `yii\helpers\ArrayHelper::flatten()` method (xcopy)
- Bug #20308: Allow CompositeAuth auth methods to use their own user if defined (mtangoo)
- Bug #20239: Fix `yii\data\ActiveDataProvider` to avoid unexpected pagination results with UNION queries (Izumi-kun)

2.0.51 July 18, 2024
--------------------
Expand Down
39 changes: 31 additions & 8 deletions framework/data/ActiveDataProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use yii\base\Model;
use yii\db\ActiveQueryInterface;
use yii\db\Connection;
use yii\db\Query;
use yii\db\QueryInterface;
use yii\di\Instance;

Expand Down Expand Up @@ -93,14 +94,40 @@ public function init()
}

/**
* {@inheritdoc}
* Creates a wrapper of [[query]] that allows adding limit and order.
* @return QueryInterface
* @throws InvalidConfigException
*/
protected function prepareModels()
protected function createQueryWrapper(): QueryInterface
{
if (!$this->query instanceof QueryInterface) {
throw new InvalidConfigException('The "query" property must be an instance of a class that implements the QueryInterface e.g. yii\db\Query or its subclasses.');
}
$query = clone $this->query;
$wrapper = clone $this->query;
if ($wrapper instanceof Query && !empty($wrapper->union)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we return $this->query if the condition isn't met right away?

if (!$wrapper instanceof Query || empty($wrapper->union)) {
    return $this->query;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather return $wrapper;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to keep the original logic (not reusing the original query):

yii2/framework/data/ActiveDataProvider.php

Lines 98 to 104 in b0b7832

protected function prepareModels()
{
if (!$this->query instanceof QueryInterface) {
throw new InvalidConfigException('The "query" property must be an instance of a class that implements the QueryInterface e.g. yii\db\Query or its subclasses.');
}
$query = clone $this->query;
if (($pagination = $this->getPagination()) !== false) {

yii2/framework/data/ActiveDataProvider.php

Lines 162 to 168 in b0b7832

protected function prepareTotalCount()
{
if (!$this->query instanceof QueryInterface) {
throw new InvalidConfigException('The "query" property must be an instance of a class that implements the QueryInterface e.g. yii\db\Query or its subclasses.');
}
$query = clone $this->query;
return (int) $query->limit(-1)->offset(-1)->orderBy([])->count('*', $this->db);

Should I change it?

$wrapper->where = [];
$wrapper->limit = null;
$wrapper->offset = null;
$wrapper->orderBy = [];
$wrapper->selectOption = null;
$wrapper->distinct = false;
$wrapper->groupBy = [];
$wrapper->join = [];
$wrapper->having = [];
$wrapper->union = [];
$wrapper->params = [];
$wrapper->withQueries = [];
$wrapper->select('*')->from(['q' => $this->query]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be more properties that needs to be reset. For example ActiveQuery has on and joinWith (and sql?) properties, that may leak to wrapper. Developer may extend these classes and create own properties with the same problem. Query needs some kind of reset() method, that would be responsible for resting query state.

Or we cold use new Query(), similar to ActiveQuery::queryScalar():

yii2/framework/db/ActiveQuery.php

Line 355 in b0b7832

$command = (new Query())->select([$selectExpression])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a new Query() will break prepareModels() itself if query is an ActiveQuery and the expected return value of ActiveQuery::all() is array of objects rather then an array of arrays. I'll think about the solution a little more.

}
return $wrapper;
}

/**
* {@inheritdoc}
*/
protected function prepareModels()
{
$query = $this->createQueryWrapper();
if (($pagination = $this->getPagination()) !== false) {
$pagination->totalCount = $this->getTotalCount();
if ($pagination->totalCount === 0) {
Expand Down Expand Up @@ -161,11 +188,7 @@ protected function prepareKeys($models)
*/
protected function prepareTotalCount()
{
if (!$this->query instanceof QueryInterface) {
throw new InvalidConfigException('The "query" property must be an instance of a class that implements the QueryInterface e.g. yii\db\Query or its subclasses.');
}
$query = clone $this->query;
return (int) $query->limit(-1)->offset(-1)->orderBy([])->count('*', $this->db);
return (int) $this->createQueryWrapper()->limit(-1)->offset(-1)->orderBy([])->count('*', $this->db);
}

/**
Expand Down
Loading
Loading