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

Fix Typesense pagination issue when using query callback #867

Merged
merged 8 commits into from
Sep 25, 2024

Conversation

tharropoulos
Copy link
Contributor

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:

  • Restore the ability to use query callbacks with Typesense without encountering pagination limitations.
  • Optimize the handling of large result sets to prevent performance degradation.
  • Maintain consistency with Typesense's limitation of 250 results per page while allowing for efficient pagination of larger datasets.
  • Maintain consistency with other scout engines defaulting to a maximum of 1000 results by default, when using the paginate function.

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.

- Implement performPaginatedSearch for large result sets
- Add maxTotalResults property to limit total fetched results
- Adjust search method to use maxPerPage as default limit
- Ensure search respects both maxPerPage and maxTotalResults limits

This ensures that when the `getTotalCount` builder function is called
with a query callback, the resulting request to the server will be
broken down batches of 250 results per page, Typesense's maximum.
- Introduce a new 'max_total_results' configuration option for Typesense
in scout.php.
- Update TypesenseEngine constructor and EngineManager to
utilize this new parameter, providing better control over the maximum
number of searchable results.

This ensures that the resulting requests to the server when broken up to
batches are handled by the user, not defaulting to Typesense's total
records, as that can introduce perfomance issues for larger datasets.
- Implement new test method in TypesenseSearchableTest to verify
pagination behavior with an empty query callback.

This ensures the search functionality works correctly when
no additional query constraints are applied.
@taylorotwell
Copy link
Member

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
Copy link
Contributor

jasonbosco commented Sep 23, 2024

@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.

@taylorotwell taylorotwell merged commit 5fae355 into laravel:10.x Sep 25, 2024
14 checks passed
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

Successfully merging this pull request may close these issues.

3 participants