typesense / laravel-scout-typesense-driver

Laravel Scout Driver for Typesense
https://typesense.org
MIT License
131 stars 38 forks source link

Pagination not working properly when using query callback #30

Open devaygun opened 2 years ago

devaygun commented 2 years ago

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: https://github.com/laravel/scout/issues/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 commented 1 year ago

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 commented 1 year ago

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

nickrupert7 commented 1 year ago

@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 commented 1 year ago

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 commented 11 months ago

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

jackkitley commented 1 month ago

This is an old issue and has not been fixed. I have just been hit by the same issue when adding query() so i am now going to consider using a other working driver...

This issue is reported everywhere with no resolution

@nickrupert7 @RightInTwo @devaygun