Fix for using pagination with custom query in Scout Builder.

[Malezha]

Problem

When calling the paginate method on Laravel\Scout\Builder, the getTotalCount method is triggered.

    public function paginate($perPage = null, $pageName = 'page', $page = null)
    {
        // method implementation

        return Container::getInstance()->makeWith(LengthAwarePaginator::class, [
            'items' => $results,
            'total' => $this->getTotalCount($results), // Calculates the total count of results
            'perPage' => $perPage,
            'currentPage' => $page,
            'options' => [
                'path' => Paginator::resolveCurrentPath(),
                'pageName' => $pageName,
            ],
        ])->appends('query', $this->query);
    }

The getTotalCount method retrieves the total count of results from the search engine. It includes a check for a query callback, which may trigger an additional database query if specified.

    protected function getTotalCount($results)
    {
        $engine = $this->engine();

        $totalCount = $engine->getTotalCount($results);

        // Checks if a query callback is defined, which could result in an additional database query.
        if (is_null($this->queryCallback)) {
            return $totalCount;
        }

        $ids = $engine->mapIdsFrom($results, $this->model->getScoutKeyName())->all();

        if (count($ids) < $totalCount) {
            $ids = $engine->keys(tap(clone $this, function ($builder) use ($totalCount) {
                // Problem: Setting `Builder::$limit` here does not affect the actual query.
                // Attempts to set a limit for the query based on the total count or the existing limit.
                $builder->take(
                    is_null($this->limit) ? $totalCount : min($this->limit, $totalCount)
                );
            }))->all();
        }

        return $this->model->queryScoutModelsByIds(
            $this, $ids
        )->toBase()->getCountForPagination();
    }

Additionally, the default take method on Laravel\Scout\Builder does not allow setting a response size limit in this implementation.

Proposed Fix

In the SearchFactory::create method, add a check for the presence of the $builder->limit parameter before examining the $options parameter. This adjustment preserves compatibility with the pagination implementation by ensuring that any explicitly set limit in $builder is prioritized.

This change will allow the pagination functionality to respect a custom limit if defined, while maintaining backward compatibility with existing configurations.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.21%. Comparing base (5c0392e) to head (418e5c3).
Report is 2 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #290      +/-   ##
============================================
+ Coverage     96.06%   96.21%   +0.14%     
- Complexity      201      204       +3     
============================================
  Files            36       36              
  Lines           636      661      +25     
============================================
+ Hits            611      636      +25     
  Misses           25       25              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Malezha]

@matchish also I can implement #252 in this request for make it compatible in future.

[matchish]

Great work on this pull request! The addition to set the size based on the builder's limit is a valuable enhancement.

However, I've noticed that setSize is now being called in two places which might override each other:

When $builder->limit is set
When the size option is passed
This could lead to unintended behavior. We might want to consider:

Restricting the usage of both to avoid conflicts
Warning the client about this potential issue
Additionally, to avoid introducing breaking changes, it might be better to move the new changes below the size option handling.

What do you think?

[matchish]

Thanks for suggesting that, @Malezha! 👍 It'd be great if you could implement #252 . Feel free to implement it as part of this request

@matchish also I can implement #252 in this request for make it compatible in future.

[Malezha]

@matchish this update maintains backward compatibility, as it only applies limit handling for pagination, following the same approach as Eloquent's paginator, which ignores limit and offset when paginating.

[matchish]

Yes, it follows the same approach as Eloquent's paginator, but I'm not sure about backward compatibility. For example, if someone is using -take(10)->options(['size' => 20]), before the update, it would return 20 results, but after the update, it might return 10.

I understand that no reason to write code this way, but someone could forget to remove after experiments. I just prefer to not bump to new major version if it is not necessary.

Would it cause any issues if we keep the size in options as the dominant setting? Or should we disregard this risk as minor and proceed with Eloquent's paginator approach without a major version bump?

[Malezha]

We will have to abandon support for the size parameter in the options() method, because then we will have two sources that are not synchronized with each other.
Unfortunately, $options is public and anyone can use the direct assignment of the size value.
I think should leave only $builder->limit as the main response size setting, which is overridden when using pagination, because exactly this option is used in Scout.
As for the option with options(['size' => 10]), it should not affect the code at all, just as it does not affect it now.

[Malezha]

I made suggested changes and added some behavioral tests.
If the proposed is good in your opinion, then I would add a description in documentation of how it works.

[matchish]

I made suggested changes and added some behavioral tests. If the proposed is good in your opinion, then I would add a description in documentation of how it works.

Don't forget to update CHANGELOG.md as well

[Malezha]

@matchish which version I should use?

[matchish]

Not sure what is happening but looks like not only we have failed upload to codecov codecov/codecov-action#1580 (comment)

[matchish]

I'm thinking about 7.9.0 as new feature introduced but not breaking changes

@matchish which version I should use?

[Malezha]

I updated version in the changelog.
Try run the workflow again, maybe it was already fixed by coverage service.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[matchish]

replaced codecov with sonarcloud. could you pull master to your branch?

[Malezha]

Branch merged

[matchish]

Released! Thank you for your valuable contribution! Great Job!