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.

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

ActiveDataProvider::prepareModels: wrong queries when using union subqueries and pagination #20239

Open
santilin opened this issue Aug 1, 2024 · 7 comments
Milestone

Comments

@santilin
Copy link
Contributor

santilin commented Aug 1, 2024

What steps will reproduce the problem?

Create a query with a union subquery and use it with a paginated dataprovider:

$q1 = (new ActiveQuery())->from('my_table');
$q2 = (new ActiveQuery())->from('my_table');
$q1->andWhere(['id' => 2]);
$q2->andWhere(['user_id' => 4]);
$union = $this->union($q2);
// create a dataprovider with a pagination.
$d = new ActiveDataProvider( ['query' => $union, 'pagination' => .....] );
print_r ($d->getModels());

What is the expected result?

No errors should be reported.

What do you get instead?

SQLSTATE[HY000]: General error: 1 LIMIT clause should come after UNION not before
Failed to prepare SQL: SELECT "my_table".* FROM "my_table" WHERE ("my_table".id=:qp0) LIMIT 10 UNION SELECT "my_table".* FROM "my_table" WHERE ("my_table".user_id=:qp2)

The problem lies in ActiveDataProvider::prepareModels():

    if (($pagination = $this->getPagination()) !== false) {
        $pagination->totalCount = $this->getTotalCount();
        if ($pagination->totalCount === 0) {
            return [];
        }
        $query->limit($pagination->getLimit())->offset($pagination->getOffset());
    }

The $query->limit should take into account that if there is a union clause, that limit should be put in the last union query:

          if (count($query->union)>0) {
              $query->union[count($query->union)-1]['query']->limit($pagination->getLimit())->offset($pagination->getOffset());
         } else {
            $query->limit($pagination->getLimit())->offset($pagination->getOffset());
         }

There ara also lots of problems with orderBy clauses and unions, but all of them can be sorted out removing all the orderBy clauses and adding just one to the last union subquery, but it would be nice if Yii2 made it for us.

@santilin
Copy link
Contributor Author

santilin commented Aug 1, 2024

Working on this issue, I find impossible to use a union query for a gridview with an activecord, because when I click on the header of a cell to sort its contents, the orderby is added to the main query and I get the error:

SQLSTATE[HY000]: General error: 1 ORDER BY clause should come after UNION not before

Using Yii2.0.52-dev and php8.1 and mysql 8.0.39

@santilin
Copy link
Contributor Author

santilin commented Aug 1, 2024

The final function that fixes all these problems would be:

/**
 * {@inheritdoc}
 */
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;

    $has_union = $query->union && count($query->union);
    if (($pagination = $this->getPagination()) !== false) {
        $pagination->totalCount = $this->getTotalCount();
        if ($pagination->totalCount === 0) {
            return [];
        }
        if ($has_union) {
            $query->union[count($query->union)-1]['query']->limit($pagination->getLimit())->offset($pagination->getOffset());
        } else {
            $query->limit($pagination->getLimit())->offset($pagination->getOffset());
        }
    }
    if (($sort = $this->getSort()) !== false) {
        $query->addOrderBy($sort->getOrders());
    }
    if ($has_union) {
        if ($query->orderBy) {
            $query->union[count($query->union)-1]['query']->addOrderBy($query->orderBy);
            $query->orderBy = null;
        }
    }

    return $query->all($this->db);
}

@samdark samdark added this to the 2.0.52 milestone Aug 1, 2024
@samdark
Copy link
Member

samdark commented Aug 1, 2024

Do you have time for a pull request?

@santilin
Copy link
Contributor Author

santilin commented Aug 1, 2024

Sure, I'll do it ASAP.

@cepaim
Copy link

cepaim commented Aug 16, 2024

From which branch should I make the pr?
How can I add a tests for that pr?
Note: I'll do it under a different user: cepaim

cepaim added a commit to cepaim/yii2 that referenced this issue Aug 16, 2024
…ION queries

As for issue yiisoft#20239, move order by and limit clauses to the last UNION query to avoid SQL errors on UNION queries
@samdark
Copy link
Member

samdark commented Aug 16, 2024

master, just add test files.

@santilin
Copy link
Contributor Author

Have a look at this pr: #20246

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants