search

laravel / scout

Laravel Scout provides a driver based solution to searching your Eloquent models.
https://laravel.com/docs/scout
MIT License
1.54k stars 327 forks source link

refactor(typesense): remove unused exists checks #847

Closed saibotk closed 1 month ago

saibotk commented 1 month ago

In https://github.com/laravel/scout/pull/820 we introduced an exists check method, which then lead to static state issues as described in https://github.com/laravel/scout/issues/845

Those where addressed in https://github.com/laravel/scout/pull/846

But now that the function always double-checks the existence, we can remove the exists check entirely and only rely on the Typesense server response for this state.

This "fixes" issues where the server already has a collection and the client would try to recreate it. E.g. when it has flushed the index and another process or worker then creates the collection.

saibotk commented 1 month ago

@tharropoulos could you review this, just to be sure?

saibotk commented 1 month ago

We could even remove the $collection->setExists(true) after creation, or i would also need to add $collection->setExists(true) right before returning the existing collection, to be sure that this field is set.

What are the plans regarding the exists field in the typesense sdk?

EDIT: Updated my code to also set the exists field when the server has the collection to be 100% correct.

tharropoulos commented 1 month ago

Thanks for iterating upon the PR! I'll have to check it first thing on Monday, as I'm away from my laptop for this weekend. I'll get back to you as soon as I can

tharropoulos commented 1 month ago

Looking back on it, the verbosity used on the previous PR might be overkill, and after spending some time messing with it, it seems to work just fine.

CC: @jasonbosco could you also take a look at the code yourself? Looks just fine to me

saibotk commented 1 month ago

I feel like this is pretty much ready otherwise, so i will mark this as ready :)