-
Notifications
You must be signed in to change notification settings - Fork 340
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
Conversation
- 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.
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. |
@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. |
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:
paginate
function.Changes
Code Changes:
In
src/Engines/TypesenseEngine.php
:search()
method to properly handle theper_page
parameter set by the builder when using query callbacks, specifically on thegetTotalCount
method.performPaginatedSearch()
to respect the explicitly set pagination limit.In
src/EngineManager.php
:createTypesenseDriver()
to ensure compatibility with the updated TypesenseEngine.Added Features:
tests/Integration/TypesenseSearchableTest.php
:test_it_can_paginate_with_query_callback_and_custom_limit()
: Verifies that pagination works correctly with query callbacks and custom limits.Documentation Updates:
config/scout.php
: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:
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 thepaginate
function.