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

Paginator\Adapter\QueryBuilder Column not found: 1054 Unknown column in 'having clause' #13552

Closed
BenWil opened this Issue Oct 26, 2018 · 5 comments

Comments

Projects
3 participants
@BenWil
Copy link

BenWil commented Oct 26, 2018

When using Paginator\Adapter\QueryBuilder with multiple groups of fields with the same column name, the following behavior occurs

GROUP BY vc.id, se.id, vc.zip, pc.id

becomes

pc.id AS id, vc.zip AS zip

in the totalBuilder count query witch results in an unkonwn column exception.

I think the solution would be to define an alias for all fields when building the groupColumn

@CameronHall

This comment has been minimized.

Copy link
Member

CameronHall commented Dec 11, 2018

Hey @BenWil,

I've had a go at reproducing this but to no avail. Can you please provide an example of your Builder.

Here's a test that I've written (bearing in mind it's for Phalcon 4.0.x).

// tests/integration/Paginator/Adapter/QueryBuilderCest.php
public function testIssue13552(IntegrationTester $I)
{
    $this->setDiMysql();
    $modelsManager = $this->getService('modelsManager');

    $builder = $modelsManager->createBuilder()
        ->columns("COUNT(*) as robos_count")
        ->from(['Robots' => Robots::class])
        ->join(RobotsParts::class, "RobotsParts.robots_id = Robots.id", "RobotsParts", "LEFT")
        ->join(RobotsParts::class, "RobotsParts_2.robots_id = Robots.id", "RobotsParts_2", "LEFT")
        ->groupBy('Robots.id, RobotsParts.id, RobotsParts.parts_id, RobotsParts_2.id, RobotsParts_2.parts_id')
        ->having('MAX(Robots.year) > 1970')
    ;

    // var_dump($builder->getQuery()->getSql());

    $paginate = (new QueryBuilder(
        [
            "builder" => $builder,
            "limit"   => 1,
            "page"    => 2
        ]
    ))->paginate();

    $I->assertEquals(4, $paginate->last);
    $I->assertEquals(4, $paginate->total_items);
}

Thank you :)

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Dec 11, 2018

Fixed Paginator Query Builder tests
Created tentative test for Issue phalcon#13552
@BenWil

This comment has been minimized.

Copy link

BenWil commented Dec 11, 2018

The error occurs only with non-aggregate conditions in having clause

Something like that should generate the error at the totalBuilder count query

$builder = $modelsManager->createBuilder()
        ->columns("Robots.id")
        ->from(['Robots' => Robots::class])
        ->join(RobotsParts::class, "RobotsParts.robots_id = Robots.id", "RobotsParts", "LEFT")
        ->join(RobotsParts::class, "RobotsParts_2.robots_id = Robots.id", "RobotsParts_2", "LEFT")
        ->groupBy('Robots.id, RobotsParts.id, RobotsParts.parts_id, RobotsParts_2.id, RobotsParts_2.parts_id')
        ->having('Robots.id> 2')
    ;

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Dec 11, 2018

Fixed Paginator Query Builder tests
Created tentative test for Issue phalcon#13552
@CameronHall

This comment has been minimized.

Copy link
Member

CameronHall commented Dec 12, 2018

Thanks for getting back to me so promptly @BenWil. I can reproduce the issue now, I'll see what I can do :)

niden added a commit that referenced this issue Dec 12, 2018

Fixed Paginator Query Builder tests
Created tentative test for Issue #13552
@BenWil

This comment has been minimized.

Copy link

BenWil commented Dec 12, 2018

@CameronHall I think the problem is not directly on the Paginator, but the query build itself.

For exampel, when you do somthing like this

$builder = $modelsManager->createBuilder()
                ->columns("Robots.id, RobotsParts.id, RobotsParts.parts_id, RobotsParts_2.id, RobotsParts_2.parts_id")
                ->from(['Robots' => Robots::class])
                ->join(RobotsParts::class, "RobotsParts.robots_id = Robots.id", "RobotsParts", "LEFT")
                ->join(RobotsParts::class, "RobotsParts_2.robots_id = Robots.id", "RobotsParts_2", "LEFT")
                ->groupBy('Robots.id, RobotsParts.id, RobotsParts.parts_id, RobotsParts_2.id, RobotsParts_2.parts_id')
                ->having('Robots.id > 1')
            ;

which is done for the total count calculation, it becomes like this in sql

SELECT `RobotsParts_2`.`id` AS `id`, `RobotsParts_2`.`parts_id` AS `parts_id` FROM `phalcon_test`.`Robots` AS `Robots` ...

that results in Unknown column in 'having clause'

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Dec 13, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Dec 13, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Dec 17, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Dec 17, 2018

niden added a commit that referenced this issue Dec 22, 2018

Merge branch 'bugfix/issue-13552' of https://github.com/CameronHall/c…
…phalcon into CameronHall-bugfix/issue-13552

* 'bugfix/issue-13552' of https://github.com/CameronHall/cphalcon:
  Fixed #13552: Overwriting columns with the same alias

niden added a commit that referenced this issue Dec 22, 2018

Merge branch 'CameronHall-bugfix/issue-13552' into 4.0.x
* CameronHall-bugfix/issue-13552:
  Fixed tests
  Fixed #13552: Overwriting columns with the same alias

@niden niden added this to To do in 4.0 Release via automation Dec 22, 2018

@niden niden added the Bug - Medium label Dec 22, 2018

@niden niden moved this from To do to In progress in 4.0 Release Dec 22, 2018

@niden niden referenced this issue Dec 22, 2018

Merged

[4.0.x] - Another correction to the test #13678

3 of 3 tasks complete
@niden

This comment has been minimized.

Copy link
Member

niden commented Dec 22, 2018

This has been addressed in #13653

Thank you @CameronHall and @BenWil

@niden niden closed this Dec 22, 2018

4.0 Release automation moved this from In progress to Done Dec 22, 2018

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