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.

Sign up for GitHub

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

Jump to bottom

[9.x] Return correct output of mapIds method for MeiliSearch #538

Merged
merged 3 commits into from
Oct 7, 2021
Merged

[9.x] Return correct output of mapIds method for MeiliSearch #538

merged 3 commits into from
Oct 7, 2021

Conversation

mmachatschek
Copy link
Contributor

This fixes issue #535

The problem with the meilisearch engine in this context is that when calling the mapIds() method on the engine, it tries to pluck all primary keys of the results. For MeiliSearch this key is customizable so we can not guess the primary key.

The inital implementation just returned the values of the first entry of the result array.

Algolia doesn't have this problem as it always uses objectID as the primary key.

This is a breaking change because we need to pass the model to the mapIds() method in order to correctly pluck the primary key from the results.

@mmachatschek mmachatschek marked this pull request as ready for review October 6, 2021 12:32
@mmachatschek mmachatschek changed the title [10.x] Return correct output of mapIds method [10.x] Return correct output of mapIds method for MeiliSearch Oct 6, 2021
@taylorotwell
Copy link
Member

Can you try to explain again why the current code is broken with examples? I'm not following.

@mmachatschek
Copy link
Contributor Author

@taylorotwell so if the result of the meilisearch payload is not ordered with the primary field first, then the ids are selected from the first element of each item. See this:

grafik

@taylorotwell
Copy link
Member

But that's not what the existing code is depending on - I don't see anywhere in the current code where we expect the id field or any other field to be "first" in the list.

@taylorotwell
Copy link
Member

Nevermind I see where you are talking about.

@mmachatschek
Copy link
Contributor Author

mmachatschek commented Oct 6, 2021 •
edited
Loading

When doing a search with Model::search('something')->query(function(){//dosomething})->paginate() the Builder instance on scout needs to get the primary ids from the underlying engine with mapIds() , to apply the query added to the builder.
The resulting pagination is broken because there are no entries in the database. because mapIds spits out the wrong primaryKeys to filter in the database query.

@taylorotwell
Copy link
Member

Honestly I suggest trying to fix this without a breaking change - even if you have to introduce a new method on the MeiliSearch engine like mapIdsFrom that accepts the results and a string column name and then use method_exists to know that you can call that on the engine or have the base Engine class simply proxy it to mapIds by default.

@mmachatschek
Copy link
Contributor Author

mmachatschek commented Oct 6, 2021 •
edited
Loading

@taylorotwell I updated the code

Rebase to 9.x?

@mmachatschek mmachatschek changed the base branch from master to 9.x October 6, 2021 20:44
@mmachatschek mmachatschek changed the base branch from 9.x to master October 6, 2021 20:44
mmachatschek
mmachatschek commented Oct 7, 2021
View reviewed changes
src/Engines/MeiliSearchEngine.php
/**
* Pluck and return the primary keys of the given results.
* This expects the first item of each search item array to be the primary key.
* Use mapIdsFrom() instead to get the correct results based on a given primary key.
*
* @param mixed $results
* @return \Illuminate\Support\Collection
Copy link
Contributor Author

@mmachatschek mmachatschek Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've let the mapIds() method untouched so others get the same result back if they were depending on it, even tough it doesnt return the results they actually would expect

@driesvints
Copy link
Member

@mmachatschek if there's no breaking changes left then yes we should target 9.x, thanks.

@mmachatschek mmachatschek changed the base branch from master to 9.x October 7, 2021 09:25
@mmachatschek mmachatschek changed the base branch from 9.x to master October 7, 2021 09:26
@mmachatschek mmachatschek changed the base branch from master to 9.x October 7, 2021 09:49
mmachatschek
mmachatschek commented Oct 7, 2021
View reviewed changes
Copy link
Contributor Author

@mmachatschek mmachatschek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driesvints I rebased the PR

src/Builder.php
@@ -5,6 +5,7 @@
use Illuminate\Container\Container;
use Illuminate\Pagination\LengthAwarePaginator;
use Illuminate\Pagination\Paginator;
use Illuminate\Support\Str;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be removed in 10.x

src/Builder.php Outdated
@@ -434,7 +435,7 @@ protected function getTotalCount($results)
return $totalCount;
}

$ids = $engine->mapIds($results)->all();
$ids = $engine->mapIdsFrom($results, Str::afterLast($this->model->getScoutKeyName(), '.'))->all();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driesvints this needs to be changed to this on the master branch where we don't need the getScoutKeyName fix

Suggested change
$ids = $engine->mapIdsFrom($results, Str::afterLast($this->model->getScoutKeyName(), '.'))->all();
$ids = $engine->mapIdsFrom($results, $this->model->getScoutKeyName())->all();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this is merged, I'll merge it into master. Can you send in a PR after that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

@driesvints driesvints changed the title [10.x] Return correct output of mapIds method for MeiliSearch [9.x] Return correct output of mapIds method for MeiliSearch Oct 7, 2021
@taylorotwell taylorotwell merged commit 8a7782c into laravel:9.x Oct 7, 2021
@mmachatschek mmachatschek deleted the meilisearch_mapids_issue branch October 7, 2021 14:24
@dvlpr91
Copy link

dvlpr91 commented Oct 26, 2021 •
edited
Loading

I had the same issue.

keys() method does not work correctly.

"laravel/framework": "^8.54",
"laravel/scout": "^9.3",
"meilisearch/meilisearch-php": "^0.19.2",

@mmachatschek mmachatschek mentioned this pull request Nov 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driesvints driesvints driesvints left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mmachatschek @taylorotwell @driesvints @dvlpr91