laravel / scout

Laravel Scout provides a driver based solution to searching your Eloquent models.
https://laravel.com/docs/scout
MIT License
1.55k stars 329 forks source link

[9.x] Pagination total messed up with query callback while eager loading #481

Closed edgarsn closed 3 years ago

edgarsn commented 3 years ago

Description:

After update from 8.x to 9.x, looks like #469 broke pagination total for me when using code like this:

$qb = Article::search('*')->query(function ($query) {
    $query->with(['authors']);
})->paginate(10, 'page', 1);

It always returns total number same as per_page I passed as argument to paginate function.

I understand other users reported (#450 ) invalid total numbers when using query callback to filter results with where. Any suggestions how can I force to use total number from engine while still using eager loading in query callback?

EDIT:

Since for us it's asap and we don't use where`s in query callback, I created my own scout builder to fix this issue for now.

<?php

namespace App\Support\Scout;

class Builder extends \Laravel\Scout\Builder
{
    /**
     * Get the total number of results from the Scout engine, or fallback to query builder.
     *
     * @param  mixed  $results
     * @return int
     */
    protected function getTotalCount($results)
    {
        $engine = $this->engine();

        return $engine->getTotalCount($results);
    }
}

and in AppServiceProvider.php

$this->app->bind(\Laravel\Scout\Builder::class, \App\Support\Scout\Builder::class);
driesvints commented 3 years ago

Heya, thanks for reporting.

I'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 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"

After you've posted the repository, I'll try to reproduce the issue.

Thanks!

edgarsn commented 3 years ago

Here is a repo - https://github.com/edgarsn/laravel-scout-issue-481

routes/web.php contains routes and queries.

php artisan migrate, php artisan db:seed, php artisan scout:import "App\Models\Article" to setup.

crynobone commented 3 years ago

In your use case I would suggest skipping query() since you can use load() later on, e.g:

 $articles = Article::search('*')->paginate(10, 'page', 1);

 $articles->load(['authors']);

Even with the PR, having $queryCallback would mean that we need to fetch all the ids again from Scout Engine and that's not useful when you are not using where.

edgarsn commented 3 years ago

In your use case I would suggest skipping query() since you can use load() later on, e.g:

 $articles = Article::search('*')->paginate(10, 'page', 1);

 $articles->loads(['authors']);

Even with the PR, having $queryCallback would mean that we need to fetch all the ids again from Scout Engine and that's not useful when you are not using where.

Hmm. I forgot about ->load() option. Tested out, it works for me.

If this issue is getting closed and code unchanged related to this, I would suggest to mention in docs to NOT eager load in query callback, but do it like @crynobone said.

driesvints commented 3 years ago

Hey @edgarsn. Can you send in a PR to the docs maybe? Would appreciate that.

edgarsn commented 3 years ago

Hey @edgarsn. Can you send in a PR to the docs maybe? Would appreciate that.

Sure. So, at the end we're only updating docs and leaving code as is, right?

driesvints commented 3 years ago

@edgarsn @crynobone sent in a PR: https://github.com/laravel/scout/pull/483

judgej commented 1 year ago

I'm using Scout 9.8, and hitting this same problem, I believe. This works fine:

Title::search($search)->paginate($pageSize);

That returns a full page of "titles" with a working pager. Then introduce a query callback (even if it does nothing) and everything changes:

$titles = Title::search($search)->query(fn ($query) => $query)->paginate($pageSize);

This time the pager returns just one page - the first page - and does not indicate there are any further pages available.

I too wanted to do this to add some eager loading. Instead I need to do it after the paged collection is created (and not in the paged collection retrieval query, as the documentation suggests):

$titles = tap(
   Title::search($search)->paginate($pageSize),
   fn ($titles) => $titles->load('owners'),
);

That works, but if you need to do anything more than a simple loading of a relationship, such as a sum of a child model, it starts to get complicated quickly. A withSum() in the eloquent query callback is almost trivial to add, but it breaks the pager.

I'm sorry I don't have any further insights to add, but thought it useful to at least confirm this is still here, and the documentation suggestion seemed to amount to, "query callback and pagers are broken; don't use them together". Hopefully this will help anyone landing here scratching their heads after encountering the same problem. I'll do a bit more digging and see if I can trace the root cause. Not found it yet though.


I feel the issue lies somewhere around here: https://github.com/laravel/scout/blob/9.x/src/Builder.php#L460

If no query callback has been supplied, then the total count for the pager is $engine->getTotalCount() and that's the one that works. If a callback has been supplied, then it does a bunch of stuff instead. That's where the pager breaks. I have no idea what that bunch of stuff is, but I'm wondering if it is legacy? Was the Builder::$getTotalCount once used for something else? If the query callback is used, and that callback does not affect the filtering at all (the docs say not to do that anyway) then there is no way the total count should change. So what is it doing?