Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

whereHas, withWhereHas doesn't work properly with "withAvg.... " methods. #50286

Closed
muratgorken opened this issue Feb 27, 2024 · 4 comments
Closed

Comments

@muratgorken
Copy link

muratgorken commented Feb 27, 2024

Laravel Version

Laravel v10.45.1 (PHP v8.1.0)

PHP Version

Laravel v10.45.1 (PHP v8.1.0)

Database Driver & Version

MySQL 5.7.36

Description

Normally, I expect the withAggregate function to work by including the "where" clauses within it. However, when I use one of the whereHas and withWhereHas methods and then use one of the withAggregate functions afterward, it doesn't include the closure part of the whereHas method.

This leads to the following problem:

For example, consider a relationship between an author and books.

$books = Author::whereHas('books', function ($query) {
    $query->where('type', 'drama');
})->withAvg('books as page_count_avg', 'page_count')->get();

In this query, we would expect the average page count to be calculated only for books of type "drama". However, the result we encounter is as follows:

SELECT `author`.*,
  (
    SELECT avg(`books`.`page_count`)
    FROM `books`
    WHERE `author`.`id` = `books`.`author_id`
  ) AS `page_count_avg`
FROM `author`
WHERE EXISTS (
    SELECT *
    FROM `books`
    WHERE `author`.`id` = `books`.`author_id`
      and `TYPE` = 'drama'
  )

To solve this, when examining the code, I noticed that the withAggregate method in QueriesRelationships.php normally parses the where clauses in the query. However, it doesn't parse the whereHas queries. To address this, I temporarily added a parameter called conditions:

public function withAggregate($relations, $column, $function = null, $conditions = null)

Then within the code:

if ($conditions) {
    $query->callScope($conditions);
}

$query = $query->mergeConstraintsFrom($relation->getQuery())->toBase();

I used it like this:

$books = Author::whereHas('books', function ($query) {
    $query->where('type', 'drama');
})->withAggregate('books as page_count_avg', 'page_count', 'avg', function($query) {
    $query->where('type', 'drama');
})->get();

The SQL output is now:

SELECT `author`.*,
  (
    SELECT avg(`books`.`page_count`)
    FROM `books`
    WHERE `author`.`id` = `books`.`author_id`
      and `TYPE` = 'drama'
  ) AS `page_count_avg`
FROM `author`
WHERE EXISTS (
    SELECT *
    FROM `books`
    WHERE `author`.`id` = `books`.`author_id`
      and `TYPE` = 'drama'
  )

I needed this because I had to sort the main table based on data from the related table. However, I noticed that when sorting the table filtered by withWhereHas, it was sorted based on the unfiltered entries.

I'm not sure if there's a better way to handle this, but there's definitely an issue here. I'm undecided about whether to open a pull request or not.

Steps To Reproduce

Create new laravel project and make two model called author and books.

Steps To Reproduce

Create new laravel project and make two model called author and books.

@driesvints
Copy link
Member

Heya, thanks for reporting.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Please do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

@muratgorken
Copy link
Author

https://github.com/muratgorken/bug-report

@staudenmeir
Copy link
Contributor

staudenmeir commented Feb 28, 2024

Hi @muratgorken,
Laravel's behavior here is correct: whereHas() and withAvg() are and should be completely separate from each other. You don't always want to have the same constraint on both subqueries.

You can define the constraint once and then reuse it:

$constraint = fn ($query) => $query->where('type', 'drama');

$books = Author::whereHas('books', $constraint)
    ->withAvg(['books as page_count_avg' => $constraint], 'page_count')
    ->get();

@muratgorken
Copy link
Author

muratgorken commented Feb 28, 2024

Why should they be separate?
For example, if I want to calculate how many posts "Admin" users have shared in a month, why should I expect all users to get the sum of their posts? On the one hand, restricting with whereHas and on the other hand, making an unconstrained query withAvg. If the relation in whereHas and the relation in withAvg are the same, I think the same constraint should be applied in both.

On the other hand, when I want to sort by the number of likes of posts after applying filters on a listing page with various filtering and sorting functions, it seemed a bit unusual to write the same conditions over and over again in the aggregate method.

I would prefer that it only has the same constraint as the default behavior, with a prefix to remove the constraint if necessary. Ex: allWithAvg(('books as page_count, 'page_count')

My suggestion would be to add withWhereHasAggregate and whereHasAggregate methods in addition to whereHas and withWhereHas methods. Because there is no easier way in my use case.

I didn't know the usage in your example (['books as page_count_avg' => $constraint], 'page_count'), but I expected the other one as the default behavior.

If you can give me an example where I wouldn't want to have the same constraint, you might convince me.

@laravel laravel locked and limited conversation to collaborators Feb 29, 2024
@crynobone crynobone converted this issue into discussion #50309 Feb 29, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

3 participants