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

Pagination not working properly when using query callback #30

Open
devaygun opened this issue Aug 9, 2022 · 5 comments
Open

Pagination not working properly when using query callback #30

devaygun opened this issue Aug 9, 2022 · 5 comments

Comments

@devaygun
Copy link

devaygun commented Aug 9, 2022

Description

It seems the TypesenseEngine doesn't correctly work with Scout's ->paginate, particularly when results exceeed 250 hits, as the total in the pagination gets set to the default perPage for some reason. It seems the MeiliSearch adapter also had this issue, see: laravel/scout#535

The workaround I have is to implement the pagination manually with:

$searchData = $query->query(fn ($search) => $search->with($relations))->get();
$paginatedSearchResults = new LengthAwarePaginator($searchData, $query->raw()['found'], $perPage);

Steps to reproduce

  1. Have a collection that exceeds 250 documents.
  2. Search against that collection utilising Scout and Typesense of course.
  3. Attempt to paginate that data with a query callback, for example with:
$query->query(fn ($search) => $search->with($relations))
                ->paginate($perPage)
                ->withQueryString();

Expected Behavior

Return a LengthAwarePaginator with 1278 total items.

Actual Behavior

Returns a LengthAwarePaginator with 15 total items.

Metadata

Typsense Version: v5.1.0-rc1

OS: MacOS 12.4 Monterey

Raw results from Typesense:

array:8 [▼
  "facet_counts" => []
  "found" => 1278
  "hits" => array:15 [▶]
  "out_of" => 171361
  "page" => 1
  "request_params" => array:3 [▶]
  "search_cutoff" => false
  "search_time_ms" => 1
]

Pagination without query callback works as expected:

Illuminate\Pagination\LengthAwarePaginator {#2096 ▼
  #items: Illuminate\Database\Eloquent\Collection {#2102 ▶}
  #perPage: 15
  #currentPage: 1
  #query: array:1 [▶]
  #fragment: null
  #pageName: "page"
  +onEachSide: 3
  #options: array:2 [▶]
  #total: 1278
  #lastPage: 86
}

Pagination with query callback does NOT work as expected:

Illuminate\Pagination\LengthAwarePaginator {#2431 ▼
  #items: Illuminate\Database\Eloquent\Collection {#2164 ▶}
  #perPage: 15
  #currentPage: 1
  #query: array:1 [▶]
  #fragment: null
  #pageName: "page"
  +onEachSide: 3
  #options: array:2 [▶]
  #total: 15
  #lastPage: 1
}
@nickrupert7
Copy link

We are also experiencing this issue on our team. The original issue explains the issue exactly so I don't feel a real need to add on details of our particular scenario.

However, I did dig into the source code of Laravel Scout and the Scout Typesense driver here and discovered the root of the problem (although I have not figured out an actual fix yet).

The base Laravel Scout Builder object has a function called paginate. This function actually issues two requests to the Typesense engine The first is to retrieve the paginated data, exactly the way you would expect. The second has to do with getting the total number of records, and herein lies the problem...

paginate calls a function $this->getTotalCount(). (https://github.com/laravel/scout/blob/9.x/src/Builder.php#L384)
getTotalCount calls a function $engine->keys(...) (https://github.com/laravel/scout/blob/9.x/src/Builder.php#L450) and passes in your $builder instance, but not without injecting one little change first.

$builder->take(
  is_null($this->limit) ? $totalCount : min($this->limit, $totalCount)
);

(https://github.com/laravel/scout/blob/9.x/src/Builder.php#L451-L453)

This code sets the limit of the query to the total count which was already found in the first query! So when the keys function calls $this->search(...). (https://github.com/laravel/scout/blob/9.x/src/Builder.php#L450), it cascades down into the Typesense driver, issues the second request to the Typesense instance, but because the limit is set to the total number of entries, this triggers the Typesense error that only a maximum of 250 records can be fetched per page.

I will likely be working on a fork/fix/PR in the next few days, but I wanted to go ahead and document my findings here in case any other poor souls out in the world run into this issue.

One last thing I'd like to point out is that it is unclear to me whether this is a flaw with this package or with the laravel/scout package. One would think that if this is the only driver experiencing issues, it must be this package, but it could also be that laravel/scout is not built flexibly enough to account for restrictions such as provider-enforced maximum page size. For example the DatabaseDriver only calls your database with a raw SQL query - it can (theoretically) query every record in the table without hitting an actual error. It is odd to me that a pagination query should issue two separate calls to search... However, I have not really looked into the source of the other drivers to see how they handle this issue, so I can't be sure.

@RightInTwo
Copy link

Same issue here! @nickrupert7 Hit me up if you can use any support/testing on this.

@nickrupert7
Copy link

@RightInTwo thanks, will do! I got pulled into other projects so my progress here is delayed, but I'll post an update when I finally get around to it.

@RightInTwo
Copy link

We've implemented a solution based on SQL Views that include the $relations with the joinRelations package. So this issue is not blocking us anymore...

@nibsirahsieu
Copy link

Hi @nickrupert7, do you have a workaround? i was hit by this issue.

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

No branches or pull requests

4 participants