Refacto and fix search method #57
Conversation
curquiza
force-pushed
the
fix-objectId-error
branch
from
f468a47
to
bbba666
Compare
|
Hello @althaus |
src/Services/MeiliSearchService.php
Outdated
| @@ -209,24 +208,18 @@ public function search( | |||
| ObjectManager $objectManager, | |||
| string $className, | |||
| string $query = '', | |||
| array $requestOptions = [] | |||
| array $searchParams = [] | |||
There was a problem hiding this comment.
This change is not reflected in the SearchService interface.
There was a problem hiding this comment.
We should indeed do this change everywhere instead of just here in this PR.
Related to #58
| * | ||
| * @return array<string, int|string|array> | ||
| */ | ||
| public function searchIds(string $query, string $indexName, array $searchParams) |
There was a problem hiding this comment.
| public function searchIds(string $query, string $indexName, array $searchParams) | |
| public function searchIds(string $query, string $indexName, array $searchParams): array |
| public function searchIds(string $query, string $indexName, array $searchParams) | ||
| { | ||
| $primaryKey = $this->client->index($indexName)->fetchPrimaryKey(); | ||
| $result = $this->search($query, $indexName, $searchParams); |
There was a problem hiding this comment.
As I'm a fresh MS user: Would it improve performance or something to limit the search to only fetch the $primaryKey attribute?
https://docs.meilisearch.com/reference/features/search_parameters.html#attributes-to-retrieve
Something like this:
if (!isset($searchParams['attributesToRetrieve'])) {
$searchParams['attributesToRetrieve'] = [$primaryKey];
}There was a problem hiding this comment.
This is indeed not optimized! Unfortunately, the primary key is not accessible in the search response so we cannot use your suggestion, except if I missed something. I need to find another way.
There was a problem hiding this comment.
I've realized it's more complicated than I thought. The search() method was badly adapted for MeiliSearch (first coming from Algolia search bundle).
We indeed don't use objectID with MeiliSearch but your primary key (so the unique field of your documents) is inferred because containing id, so often, the primary key is the simple id field.
But, according to the way of handling the unique field, it means we are not able to know the primary key (= the unique field) during the search. And we clearly need to know it according to our code base!
Two possibilities:
- we are doing 2 HTTP calls during the
search()as I've already done in my PR: this is a quick fix and your PR (Added raw search (fixes #17) #60) is a temporary workaround for users having performance issues. I don't really like this solution if the second point can be applied. - removing
objectIDfrom our code base, I mean, in terms of usage. Related to Renaming some variables #58. It looks like being a big task to do and I don't have the time to do it, unfortunately, but this would be a good step in this package. I'm opening an issue.
| * @param string $indexName | ||
| * @param array<string, int|string|array> $searchParams | ||
| * | ||
| * @return array<string, int|string|array> |
There was a problem hiding this comment.
Can this method return an array<string, array> ? This would mean the ID itself would be an array.
|
Hello Matthias! I saw all your involvement in the meilisearch-symfony repository (issues, PR review and PR). I don't have the time to work on it at the moment, but I don't forget all of those and I will come back as soon as possible 🙂 |
|
Closing this due to obsolete code (a refactor was done) |
No description provided.