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

withCount not working as expected #35059

Closed
nnerijuss opened this issue Nov 2, 2020 · 15 comments
Closed

withCount not working as expected #35059

nnerijuss opened this issue Nov 2, 2020 · 15 comments

Comments

@nnerijuss
Copy link

nnerijuss commented Nov 2, 2020

  • Laravel Version: 8.12.3
  • PHP Version: 7.4
  • Database Driver & Version: Mysql 8

Description:

After upgrading Laravel from 7 to 8 $model->withCount('relation') returns unexpected value because of the limit 1 in Illuminate/Database/Eloquent/Concerns/QueriesRelationships.php file 417 row.

Steps To Reproduce:

  1. Create few models(for example products) with ids(1,2,3,4) with morphMany relation, for example comments
  2. Create comments table with column(id, comment, confirmed_at)
  3. Create 2 comments for all products
  4. For all products except with id 3, add confirmed_at date now date, or some other date
  5. For product with id 3, make all comments confirmed_at column null
  6. Call Products::withCount(['comments'=> fn($q) => $q->whereNotNull('confirmed_at')])

Expected result:

id comments_count
1 2
2 2
3 0
4 2

Result returned:

id comments_count
1 2
2 2
3 0
4 0

After removing ->limit(1) in Illuminate/Database/Eloquent/Concerns/QueriesRelationships.php file 417 row, everything starts working as expected.

@paras-malhotra
Copy link
Contributor

paras-malhotra commented Nov 2, 2020

Yes, this PR introduces a breaking change I think: #34965. Quick way to verify is that all the tests have been changed to output a different query now. Ping @khalilst

@khalilst
Copy link
Contributor

khalilst commented Nov 2, 2020

@paras-malhotra The test changed to support new query including limit 1 only and not anything else.

@nnerijuss I'm gonna create a project as your steps, but if you can share a gist from your source.

@paras-malhotra
Copy link
Contributor

I understand @khalilst, I'm not criticising the PR at all - great work on the PR! Just saying that the limit 1 change may be the issue here.

@nnerijuss
Copy link
Author

nnerijuss commented Nov 2, 2020

@khalilst I really do not understand what more can i share with you to help you solve this issue.

app/Product.php

class Product extends Model
{
    public function comments()
    {
        return $this->morphMany(Comment::class, 'commentable');
    }

    public function scopeWithCommentsCount(Builder $builder): Builder
    {
        return $builder->withCount([
            'comments' => function ($query) {
                $query->whereNotNull('confirmed_at');
            }
        ]);
    }
}

app/Comment.php

class Comment extends Model
{

}

Call withCount scope:

Products::withCommentsCount()->whereIn('id', [1,2,3,4])->get();

If you convert this eloquent query to Mysql query you will get something like this:

SELECT 
    `id`,
    (SELECT 
           COUNT(*)
        FROM
            `comments`
        WHERE
            `products`.`id` = `comments`.`commentable_id`
                AND `comments`.`commentable_type` = 'App\\Product'
                AND `comments`.`confirmed_at` IS NOT NULL
        LIMIT 1) AS `comments_count`
FROM
    `products`
WHERE
    `id` IN (1 , 2, 3, 4)

I write this code on the fly, but idea is similar to my production code(Sorry, repo is private and i cant share full codebase with you).

As you can see, there is LIMIT 1 is subselect, and there is the problem. Actually it is logical error, you should not format such sql query when counting results.

@khalilst
Copy link
Contributor

khalilst commented Nov 2, 2020

@nnerijuss I have created a project with your steps and everything works fine.
the limit 1 in the above sub-select has no problem. Please check your data.
If it was not working, it should not work for all items not only one item!

Psy Shell v0.10.4 (PHP 7.4.11 — cli) by Justin Hileman
>>> Product::withCount(['comments'=> fn($q) => $q->whereNotNull('confirmed_at')])->get()
[!] Aliasing 'Product' to 'App\Models\Product' for this Tinker session.
=> Illuminate\Database\Eloquent\Collection {#3964
     all: [
       App\Models\Product {#3926
         id: 1,
         name: "fSbKRoIpBl",
         created_at: "2020-11-02 09:48:39",
         updated_at: "2020-11-02 09:48:39",
         comments_count: 2,
       },
       App\Models\Product {#4173
         id: 2,
         name: "TBpEYufkJI",
         created_at: "2020-11-02 09:48:39",
         updated_at: "2020-11-02 09:48:39",
         comments_count: 2,
       },
       App\Models\Product {#3965
         id: 3,
         name: "iBlkWmyfU3",
         created_at: "2020-11-02 09:48:39",
         updated_at: "2020-11-02 09:48:39",
         comments_count: 0,
       },
       App\Models\Product {#3558
         id: 4,
         name: "c0o1InKesx",
         created_at: "2020-11-02 09:48:47",
         updated_at: "2020-11-02 09:48:47",
         comments_count: 2,
       },
     ],
   }

@nnerijuss
Copy link
Author

nnerijuss commented Nov 2, 2020

@khalilst what version of Mysql do you use ? Both of my enviroments(local and production) works on same mysql 8.0.18 version, and i have this problem in both environments. Until upgrading to Laravel 8 i do not faced this issue.

@khalilst
Copy link
Contributor

khalilst commented Nov 2, 2020

@nnerijuss I think you have problem with your data. As I told you limit 1 has no problem even for older versions of MySQL.
This is MySQL version:

Server version: 8.0.20 MySQL Community Server - GPL

And this is test result with your new code. Everything is fine again:

>>> Product::withCommentsCount()->whereIn('id', [1,2,3,4])->get();                                                                                                                                    
[!] Aliasing 'Product' to 'App\Models\Product' for this Tinker session.                                                                                                                               
=> Illuminate\Database\Eloquent\Collection {#4175                                                                                                                                                     
     all: [                                                                                                                                                                                           
       App\Models\Product {#3925                                                                                                                                                                      
         id: 1,                                                                                                                                                                                       
         name: "fSbKRoIpBl",                                                                                                                                                                          
         created_at: "2020-11-02 09:48:39",                                                                                                                                                           
         updated_at: "2020-11-02 09:48:39",                                                                                                                                                           
         comments_count: 2,                                                                                                                                                                           
       },                                                                                                                                                                                             
       App\Models\Product {#4173                                                                                                                                                                      
         id: 2,                                                                                                                                                                                       
         name: "TBpEYufkJI",                                                                                                                                                                          
         created_at: "2020-11-02 09:48:39",
         updated_at: "2020-11-02 09:48:39",
         comments_count: 2,
       },
       App\Models\Product {#4174
         id: 3,
         name: "iBlkWmyfU3",
         created_at: "2020-11-02 09:48:39",
         updated_at: "2020-11-02 09:48:39",
         comments_count: 0,
       },
       App\Models\Product {#3557
         id: 4,
         name: "c0o1InKesx",
         created_at: "2020-11-02 09:48:47",
         updated_at: "2020-11-02 09:48:47",
         comments_count: 2,
       },
     ],
   }

You can see the limit 1 in the SQL query:

>>> Product::withCommentsCount()->whereIn('id', [1,2,3,4])->toSql()
=> "select `products`.*, (select count(*) from `comments` where `products`.`id` = `comments`.`commentable_id` and `comments`.`commentable_type` = ? and `confirmed_at` is not null limit 1) as `comments_count` from `products` where `id` in (?, ?, ?, ?)"

@nnerijuss
Copy link
Author

nnerijuss commented Nov 2, 2020

@khalilst thanks, but still very strange. I have seeded absolutely fresh data, and still have facing same issue. Without LIMIT 1 everything works just fine, with LIMIT 1 i get result without counted relations.

I try to dig in little bit more, because i don't understand why same query works in your machine and do not works in mine(even when local and production machines ar absolutely different).

@khalilst
Copy link
Contributor

khalilst commented Nov 2, 2020

@nnerijuss
Create a project and put your data via seeder.
Then share your code here.

@nnerijuss
Copy link
Author

nnerijuss commented Nov 2, 2020

@khalilst there is full repo with my test models and data. You can see in Readme.md file the result i get in executed command. I hope this will help you.
https://github.com/nnerijuss/relations-count

@dbakan
Copy link
Contributor

dbakan commented Nov 2, 2020

@khalilst @nnerijuss Cannot confirm. I get the correct counts:

array:4 [
  0 => array:5 [
    "id" => 1
    "comments_count" => 2
  ]
  1 => array:5 [
    "id" => 2
    "comments_count" => 2
  ]
  2 => array:5 [
    "id" => 3
    "comments_count" => 0
  ]
  3 => array:5 [
    "id" => 4
    "comments_count" => 2
  ]
]

I'm still curious why this limit(1) was added in the first place though. If I remove it in the QueriesRelationships.php and in the tests, everything still passes locally.

@khalilst
Copy link
Contributor

khalilst commented Nov 2, 2020

@dbakan Because the withAggregate was not for Aggregation only.
I named it withColumn, but in other PR renamed to withAggregate.
It could return a simple column without aggregation. e.g.

  Post::withAggregate('comments', 'confirmed_at'); //without third argument

And it returns the first comment confirmed_at.

@khalilst
Copy link
Contributor

khalilst commented Nov 2, 2020

@nnerijuss I confirm the problem exists in your provided application.
I'm trying to find the problem and I will create a PR for this problem.

@dbakan
Copy link
Contributor

dbakan commented Nov 2, 2020

@dbakan Because the withAggregate was not for Aggregation only.
I named it withColumn, but in other PR renamed to withAggregate.
It could return a simple column without aggregation. e.g.

  Post::withAggregate('comments', 'confirmed_at'); //without third argument

And it returns the first comment confirmed_at.

@khalilst Thank you for clarifying!

@driesvints
Copy link
Member

This will be fixed in tomorrow's patch release. Thanks all.

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

5 participants