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

[BUG]: Phalcon\Paginator\Adapter\Model generates an invalid request with order parameter #16471

Closed
mhz-tamb opened this issue Nov 29, 2023 · 3 comments · Fixed by #16485
Closed
Assignees
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium

Comments

@mhz-tamb
Copy link

Questions? Forum: https://phalcon.io/forum or Discord: https://phalcon.io/discord

Describe the bug
Phalcon\Paginator\Adapter\Model generates an invalid query with order parameter and postgres query execution fails.
The result query:

SELECT COUNT(*) AS "rowcount" FROM "mymodel" ORDER BY "mymodel"."published_at" DESC
PDOException: SQLSTATE[42803]: Grouping error: 7 ERROR: column "my_model.published_at" must appear in the GROUP BY clause or be used in an aggregate function...

To Reproduce

Steps to reproduce the behavior:

Provide minimal script to reproduce the issue

<?php

use Phalcon\Paginator\Adapter\Model;

$paginator = new Model([
    'model' => MyModel::class,
    'parameters' => [
        'order' => 'publishedAt desc'
    ],
    'limit' => static::LIMIT,
    'page' => $this->request->get('page', 'int', 1)
]);

Expected behavior

The resulting query should not contain order parameters and should look like:

SELECT COUNT(*) AS "rowcount" FROM "mymodel"

Apparently a similar problem is solved here:
https://github.com/phalcon/cphalcon/blob/5.0.x/phalcon/Paginator/Adapter/QueryBuilder.zep#L211

Details

  • Phalcon version: 5.4.0
  • PHP Version: 8.2.12
  • Operating System: osx
  • Installation type: Compiling from source
  • Zephir version (if any):
  • Server: Nginx
  • Postgresql: 15.4
@mhz-tamb mhz-tamb added bug A bug report status: unverified Unverified labels Nov 29, 2023
@s-ohnishi
Copy link

Apparently a similar problem is solved here:

There should have been a countermeasure, but I don't think it's working.

@s-ohnishi
Copy link

@mhz-tamb, I looked into it a little more.
For Phalcon\Paginator\Adapter\QueryBuilder, as you pointed out, here has been taken care of.
But for Phalcon\Paginator\Adapter\Model, it just calls the model's count method like this.

let rowCountResult = call_user_func([modelClass, "count"], parameters);

In the model's count method, COUNT(*) AS rowcount is set as a column, and other parameters are also set unconditionally.
In a paginator, it is common for a sort order to be specified, so there is an order section in the parameters.
Then, the ORDER BY clause is added to COUNT() even though it is unnecessary.

From this, it is considered an obvious mistake to set all the parameters given when Model::count() is called.
The order section should be removed.
This is a bug.

@niden niden mentioned this issue Dec 26, 2023
5 tasks
@niden niden added status: medium Medium 5.0 The issues we want to solve in the 5.0 release and removed status: unverified Unverified labels Dec 26, 2023
@niden niden self-assigned this Dec 26, 2023
@niden niden linked a pull request Dec 26, 2023 that will close this issue
5 tasks
@niden
Copy link
Sponsor Member

niden commented Dec 26, 2023

Resolved in #16485

Thank you @s-ohnishi and @mhz-tamb

@niden niden closed this as completed Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium
Projects
Status: Released
Development

Successfully merging a pull request may close this issue.

3 participants