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

[8.x] fix #37483 (aggregates with having) #37487

Merged
merged 1 commit into from
May 26, 2021
Merged

[8.x] fix #37483 (aggregates with having) #37487

merged 1 commit into from
May 26, 2021

Conversation

Tofandel
Copy link
Contributor

@Tofandel Tofandel commented May 26, 2021

The problem

Executing the following statement results in an sql error because the subquery is dismissed but the havings are kept:

User::query()->withCount('roles')->having('roles_count', '>', '2')->count()

Query:

select count(*) as aggregate from `users` having `roles_count` > 2

Error:

Illuminate\Database\QueryException with message 'SQLSTATE[42S22]: Column not found: 1054 Unknown column 'roles_count' in 'having clause' (SQL: select count(*) as aggregate from `users` having `roles_count` > 2)'

But executing the following runs without error

User::query()->withCount('roles')->having('roles_count', '>', '2')->get()

Query:

select `users`.*, (select count(*) from `roles` where `users`.`id` = `roles`.`user_id`) as `roles_count` from `users` having `roles_count` > 2

When comparing the queries of the previous statements, it is clear the original query should instead be wrapped within a subquery to be valid when dealing with havings to get a correct count (because having only applies to grouped data), exactly how it's currently done with unions, eg:

select count(*) as aggregate from (select `users`.*, (select count(*) from `roles` where `users`.`id` = `roles`.`user_id`) as `roles_count` from `users` having `roles_count` > 2) as `temp_table`

This PR

It applies the methodology of aggregates with union to aggregates with havings and does indeed fix the aforementioned issue #37483

It is not a breaking change because the previous behavior of this specific case wasn't previously working

TLDR

Queries with havings should behave exactly the same way as unions when executing aggregates functions

@driesvints
Copy link
Member

@Tofandel thanks for the PR. It's best to add a much more thorough description to the PR so Taylor knows exactly why this is needed (see the PR description when submitting PR's).

@Tofandel

This comment has been minimized.

@taylorotwell taylorotwell merged commit 5a92f04 into laravel:8.x May 26, 2021
@Tofandel Tofandel deleted the fix-37483 branch May 26, 2021 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants