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

Incorrect Total Count in Laravel Scout Pagination #824

Closed
Evenerik opened this issue Apr 25, 2024 · 23 comments
Closed

Incorrect Total Count in Laravel Scout Pagination #824

Evenerik opened this issue Apr 25, 2024 · 23 comments

Comments

@Evenerik
Copy link

Scout Version

10.8.6

Scout Driver

Typesense

Laravel Version

10.48.3

PHP Version

8.3.0

Database Driver & Version

MySQL 8.0.36

SDK Version

No response

Meilisearch CLI Version

No response

Description

This is pretty much a reopening of @razvaniacob's issue, who has encountered the same problem I am currently facing. I won't go into too much detail since he described the bug perfectly well in his issue, but Scout's LengthAwarePaginator shows wrong total when using Typesense's engine

Steps To Reproduce

See steps in #819

@driesvints
Copy link
Member

@karakhanyans do you have any thoughts here?

@karakhanyans
Copy link
Contributor

@driesvints will check this and get back with more info 👌

@razvaniacob
Copy link

I will be happy to see this solved guys. Good luck!

@driesvints
Copy link
Member

@karakhanyans any news here?

@AbdullahFaqeir
Copy link
Contributor

I'll be working on this

@tamakoma1129
Copy link

Hello,

I encountered the same issue reported here. In my case, I was using a model with a ULID as the primary key but initially forgot to set protected $keyType = 'string'; in my model. After updating this setting, I can confirm that the pagination now works correctly.

Here are the specifics of my development environment:

  • Laravel Version: 10.48.4
  • Scout Version: 10.8.6
  • PHP Version: 8.1.27
  • Scout Driver: Meilisearch
  • Database: MySQL 8.0.36

Apologies if this solution has already been mentioned. I hope this helps someone else experiencing the same problem.

Thank you.

@driesvints
Copy link
Member

Ping @jasonbosco

@jasonbosco
Copy link
Contributor

@AbdullahFaqeir will be working on this going forward.

@AbdullahFaqeir
Copy link
Contributor

Hi @Evenerik!

Can you please dump the collection schema, and sample record in typesense and the same record in database.

Below you can see a dump of my env showing a working fresh sample, I've seeded 10K users.

User model settings.

screenshot 7

Search performed.

screenshot 8

Paginated search results.

screenshot 6

@driesvints
Copy link
Member

Closing this issue because it's inactive, already solved, old or not relevant anymore. Feel to open up a new issue if you're still experiencing this.

@razvaniacob
Copy link

razvaniacob commented Jun 13, 2024

Is it possible to reopen this because the problem persists. I tracked it down to the method getTotalCount on Builder class
https://github.com/laravel/scout/blob/10.x/src/Builder.php#L486

the $totalCount is correct but because of $this->queryCallback not being null, the total returned is the perpage number.

I don't really understand what is happening after that if statment
`if (is_null($this->queryCallback)) {
return $totalCount;
}
@driesvints Do I need to open a new issue about this?

@jasonbosco
Copy link
Contributor

Could you share the information that @AbdullahFaqeir asked for here to debug this further: #824 (comment)

@razvaniacob
Copy link

I'm happy to share more information. I've notice that things stop working as they should when using a callback query indicated in the documentation here: https://laravel.com/docs/11.x/scout#customizing-the-eloquent-results-query

The ecpected number of results should be 512 but they are limited to the number per page.

**City model settings **
Screenshot 2024-06-13 at 6 49 13 PM

Search performed.

Route::get('test', function () {
    return City::search('buzau')
        ->query(fn (Builder $query) => $query->with(['county']))
        ->paginate(10)->onEachSide(1)->withQueryString()
        ->through(fn ($obj) => [
            'name' => $obj->name,
            'city' => $obj->county?->name ?? '',
        ]);
});

Paginated search results.

{
  "current_page": 1,
  "data": [
    {
      "name": "Buzau",
      "city": "Judetul Buzau"
    },
    {
      "name": "Comuna Munteni Buzau",
      "city": "Judetul Ialomita"
    },
    {
      "name": "Dealul Viei",
      "city": "Judetul Buzau"
    },
    {
      "name": "Dealul Viei",
      "city": "Judetul Buzau"
    },
    {
      "name": "Valcele",
      "city": "Judetul Buzau"
    },
    {
      "name": "Trestioara",
      "city": "Judetul Buzau"
    },
    {
      "name": "Suditi",
      "city": "Judetul Buzau"
    },
    {
      "name": "Satu Nou",
      "city": "Judetul Buzau"
    },
    {
      "name": "Recea",
      "city": "Judetul Buzau"
    },
    {
      "name": "Posta",
      "city": "Judetul Buzau"
    }
  ],
  "first_page_url": "https://bliss-spa.test/test?query=buzau&page=1",
  "from": 1,
  "last_page": 1,
  "last_page_url": "https://bliss-spa.test/test?query=buzau&page=1",
  "links": [
    {
      "url": null,
      "label": "« Previous",
      "active": false
    },
    {
      "url": "https://bliss-spa.test/test?query=buzau&page=1",
      "label": "1",
      "active": true
    },
    {
      "url": null,
      "label": "Next »",
      "active": false
    }
  ],
  "next_page_url": null,
  "path": "https://bliss-spa.test/test",
  "per_page": 10,
  "prev_page_url": null,
  "to": 10,
  "total": 10
}

I verified the total number in the Typesense Dashboard

Screenshot 2024-06-13 at 6 56 59 PM

So I think the problem lies somewhere in the getTotalCount method in the Builder class
https://github.com/laravel/scout/blob/10.x/src/Builder.php#L486

Please let me know how else I can help with this.

Thank you!

Raz

@AbdullahFaqeir
Copy link
Contributor

Hey @jasonbosco @razvaniacob

I don't know how I've seen the whole execution within my head to figure this out 😅 I literally didn't even go through the code.

What's actually causing the issue is what happens when you apply a customisation to the query (db) which happens after the (search) query.

When @razvaniacob indexed his data, he didn't index the (id) of the city, that's first.

After the search results is returned normally with the correct total number count, while not just typesense, even other engines, generate a list of the plucked id's from the (search) query, using this list of id's the fetch the actual entities inside the database on order to apply the (db) query customisation on them.

In this case, there was no id's being returned from the (search) query, thus, when trying to execute the (db) query customisation, it returns null because, you know! 😅.

So I believe the only way for this to be fixed is to force index the models id's.

@razvaniacob
Copy link

razvaniacob commented Jun 14, 2024

Good observation. But actually the id is indexed to typesense and you can see the id in the typesense dashboard screenshot, but even with that I get the same result.

This is what I have in my City model:
Screenshot 2024-06-14 at 8 52 11 AM

@AbdullahFaqeir
Copy link
Contributor

Good observation. But actually the id is indexed to typesense and you can see the id in the typesense dashboard screenshot, but even with that I get the same result.

This is what I have in my City model:

Screenshot 2024-06-14 at 8 52 11 AM

Tho is it defined in the typesense scheme?, check your previous comment

@razvaniacob
Copy link

razvaniacob commented Jun 14, 2024

I added the id to the typesense scheme and reindexed and still no change in the results. I get 10 results (the per page total) instead of 512
Screenshot 2024-06-14 at 9 40 34 AM

@razvaniacob
Copy link

@AbdullahFaqeir do you need anything else from me in order to finally find a fix for this problem?

@jasonbosco
Copy link
Contributor

Mostly for @AbdullahFaqeir: Quick note that the id field is automatically generated by Typesense if a document sent to Typesense does not have a field called id defined in the document (even if the id field is not defined in the collection schema).

But if an id field is explicitly set in the document, then the specified value is used as the id of the document.

Unless exclude_fields or include_fields search parameters are used to exclude the id field from the Typesense search API response, this field will always be returned by Typesense.

@jackkitley
Copy link

Have the same issue when using ->query()

Issue is over 2 years old now.

        return $this->model->search($search)
            ->query(fn(Builder $query) => $query->with(['mainListingImage']))
            ->options([
                'query_by' => 'vehicle_description, transmission, fuel_type, colour, year'
            ])
            ->paginate()
            ->withQueryString();

@tharropoulos
Copy link
Contributor

After spending some time debugging, I've come to the following findings:

When using the paginate function in Laravel Scout with the Typesense engine, the number of records returned is always limited to the perPage value, regardless of the actual total number of matches on the Typesense server. This behavior stems from the interaction between various methods within the Builder and TypesenseEngine classes.

Details

  1. Hits on the Current Page:
    The line here

    $ids = $engine->mapIdsFrom($results, $this->model->getScoutKeyName())->all();
    returns the number of hits on the current page, which will always be capped by the limit (default is Eloquent's perPage 15, I go into more detail about this below). This is evident here
    public function mapIds($results)
    {
    return collect($results['hits'])
    ->pluck('document.id')
    ->values();
    }

    As mapIdsFrom just returns the mapIds function:
    public function mapIdsFrom($results, $key)
    {
    return $this->mapIds($results);
    }

  2. Mutation of the ids Variable:
    The ids variable, as a result, is always less than the total matches, and it is then mutated by the keys method, which uses the tap function to return a cloned builder object:

    public function keys(Builder $builder)
    {
    return $this->mapIds($this->search($builder));
    }

    This, in turn, will just return the same number as the previous call to the mapIds function.

  3. Limit Setting Logic:
    The logic here:

    scout/src/Builder.php

    Lines 500 to 502 in 9910562

    $builder->take(
    is_null($this->limit) ? $totalCount : min($this->limit, $totalCount)
    );

    attempts to set the limit, but since the limit is initialized and always less than the totalAmount variable, this condition never triggers. Thus, the number of total records in TypesenseEngine defaults to the perPage value.

  4. Why:
    The paginate function on TypesenseEngine

    public function paginate(Builder $builder, $perPage, $page)
    {
    $builder->take($builder->limit ?? $perPage);
    return $this->performSearch(
    $builder,
    $this->buildSearchParameters($builder, $page, $perPage)
    );
    }

    Accesses the builder's take function, which sets the limit attribute to the perPage parameter, which is never null. This way, the aforementioned check will always result in what perPage is equal to, and in turn, will result in the behavior you encountered.

Fix

Remove the assignment of perPage's value to the builder's limit attribute. I'll create a relevant PR shortly, so you can check it out yourselves.

@razvaniacob
Copy link

Wow.. good job @tharropoulos I was able to track down the issue you found but didn't have time to come up with a fix. I wasn't sure if removing the assignment of perPage to the builder's limit attribute won't affect other search engines, because I found it odd that it was there to begin with.

I'm happy you're on this problem!

@tharropoulos
Copy link
Contributor

tharropoulos commented Aug 26, 2024

Wow.. good job @tharropoulos I was able to track down the issue you found but didn't have time to come up with a fix. I wasn't sure if removing the assignment of perPage to the builder's limit attribute won't affect other search engines, because I found it odd that it was there to begin with.

I'm happy you're on this problem!

I've manually tested it but haven't created any new tests to verify. If need be, I'll be sure to include some! Rest of the tests seem to pass as expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants