laravel / scout

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

Fix Typesense pagination issue when using query callback #867

Closed tharropoulos closed 2 months ago

tharropoulos commented 2 months ago

Rationale

This pull request addresses a regression introduced in PR #858 that affects Typesense pagination when using query callbacks. The current implementation causes an error (#865) when the resulting query matches more than 250 results (Typesense's limit).

The motivation for these changes is to:

Changes

Code Changes:

  1. In src/Engines/TypesenseEngine.php:

    • Modified search() method to properly handle the per_page parameter set by the builder when using query callbacks, specifically on the getTotalCount method.
    • Implemented performPaginatedSearch() to respect the explicitly set pagination limit.
  2. In src/EngineManager.php:

    • Adjusted createTypesenseDriver() to ensure compatibility with the updated TypesenseEngine.

Added Features:

  1. New Test in tests/Integration/TypesenseSearchableTest.php:
    • Added test_it_can_paginate_with_query_callback_and_custom_limit(): Verifies that pagination works correctly with query callbacks and custom limits.

Documentation Updates:

  1. In config/scout.php:
    • Updated comments for Typesense configuration to clarify pagination behavior.

These changes restore the functionality that was working prior to MR #858 and ensure that Typesense pagination works correctly when using query callbacks, particularly for eager loading relations.

The fix allows developers to use code like this without encountering the "Only up to 250 hits can be fetched per page" error:

$results = Names::search($keyword)
    ->options([
        'filter_by' => $filterString,
        'sort_by' => $sortBy,
    ])
    ->query(fn (Builder $query) => $query->with('someRelation'))
    ->paginate(48);

This removes the need to explicitly set a limit for the builder, while ensuring that the correct amount of results will be returned by the getTotalAmount function, as previously it would only get the limit set by the paginate function.

taylorotwell commented 2 months ago

I wonder if it would be best to move TypeSense support out to a driver? It feels like it's becoming a headache for us to maintain and would be better if it was maintained on your side? We don't know which PRs to merge with confidence.

jasonbosco commented 2 months ago

@taylorotwell - the issues we've seen so far only affect the Typesense engine inside of Scout, so the impact is localized to only Typesense users. We (the Typesense team) take full responsibility for making sure these get addressed asap, now and in the future.

In the past, we tried to get external help (folks outside of the core Typesense team) to maintain this code, but it was hard to coordinate their availability on a consistent basis.

But we now have @tharropoulos who is part of the core Typesense team and is assigned to be the DRI on our side for the Typesense Laravel Scout integration. So as some of these historical issues resurface in different forms, we now have a consistent set of eyes evaluating them, and also writing tests to make sure there are no regressions in the future.

I hear you that merging these PRs feels risky, but the scope of the risk is fully contained to Typesense users, and we take responsibility to minimize these risks quickly, especially as more users use the native integration and give us feedback. May be one tactical thing we could do is to use GitHub's CodeOwners feature along with branch protection rules, to explicitly tag the DRI on the Typesense side anytime code inside the Typesense engine is touched, and if that person approves it, it's marked as ready to be merged? This way it's less headache on your side to review any Typesense specific pieces.

I think GitHub might allow tagging the codeowners via an at mention tag. When users report issues, if we tag the typesense codeowners group, then users would know that the issue is on the Typesense team to address, so the responsibility is visibly on us from users' perspective as well.

I'd appreciate it if we can continue having the Typesense engine inside of Scout, since it helps improve discoverability for Laravel users, that Typesense exists as a free and open source search option.