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

[9.x] Handle non-consecutive key collection on MeiliSearch document deletion #688

Merged
merged 1 commit into from
Jan 6, 2023

Conversation

pyrou
Copy link
Contributor

@pyrou pyrou commented Jan 5, 2023

Methods MeiliSearchEngine::update() and MeiliSearchEngine::delete() both expect a Illuminate\Database\Eloquent\Collection of models.

For my use case I used to fine-tune this collection using ->filter() on it. Using ->filter() on collection might result in a collection that doesn't wrap a list anymore, but an array with non-consecutive keys. While MeiliSearchEngine::update() implementation handle this well, calling the MeiliSearchEngine::delete() method results in following error from Meilisearch:

Meilisearch ApiException: Http Status: 400 - Message: Json deserialize error: invalid type: map, expected a sequence at line 1 column 0 - Code: bad_request - Type: invalid_request - Link: https://docs.meilisearch.com/errors#bad_request

This is because delete() method uses ->all() directly on the provided Collection.

You might argue I should use ->values() in my user-land code, and this is what I did for the moment as a workaround. But IMHO, update() and delete() should behave the same with array that are not list.

This PR propose to change ->all() to ->values()->all().

@mmachatschek
Copy link
Contributor

This should be updated on the Algolia driver as well I guess as that driver shares the same logic

@taylorotwell taylorotwell merged commit 12ff8b6 into laravel:9.x Jan 6, 2023
@driesvints
Copy link
Member

driesvints commented Jan 6, 2023

@pyrou can you PR to AlgoliaEngine as well?

@pyrou
Copy link
Contributor Author

pyrou commented Jan 7, 2023

@driesvints AlgoliaEngine doesn't suffer from same "issue".
If we follow the AlgoliaEngine code (deleteObjects, splitIntoBatches), model collection passed is used in a foreach that doesn't use keys.

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

Successfully merging this pull request may close these issues.

4 participants