-
Notifications
You must be signed in to change notification settings - Fork 88
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
Aggregators broken since laravel/scout was updated to v9.1.0
when deleting Models became queueable
#282
Comments
Thank you for reporting @GC-Mark, and sorry for the late reply. Do you perhaps already have some ideas on how we could fix this? I can dig into this next week, but if you already have some ideas that would help us a lot! Also, feel free to include the steps we need to take to reproduce this error 🙂 Thank you in advance! |
We did dig into this, but ultimately hit a brick wall coming up with a solution. I will try and put together a repo that recreates the error this week if I get a chance. 👍 |
Using the latest scout / scout extended, I get the same error message if I try to update a model instance which is excluded from the aggregate index by shouldBeSearchable(). The update works fine if shouldBeSearchable() returns true. @GC-Mark did you just go back to Scout pre v9.1.0? |
rolling back to laravel/scout:9.0.0 resolved the issues for me |
@goldmerc yes, we downgraded for the time being |
Just an update on this... The problem I think comes from the fact that deletions are now queued and when the job is run, the Models get reinitialised in the queued job ready for deletion. The problems start in this file https://github.com/laravel/scout/blob/9.x/src/Jobs/RemoveFromSearch.php The models are passed in and then serialized. When the I think that perhaps the |
Another option is to override the |
I think the cleanest way is if we override the I'll make some time early next week to play around with this and open a PR, I would love to get your thoughts and feedback on it by then. |
Yeah, that sounds like the way to go. I look forward to seeing and testing a PR 👍 It would be nice to get this to work in the future with Aggregators, but I guess it would require quite a major overhaul in how Aggregators work. |
This is now a bit of an issue as to avoid this problem we had to lock down Scout to This will be fixed once #287 is merged 🙏 |
@GC-Mark Depending on your use case, you can just disable queuing for scout. The PR disables queuing for delete anyway. Alternatively it should also work if you just copy and paste the method from the PR into your searchable classes if you prefer to keep queueing for updates.
I think there may be a better solution than the proposed PR. We could set the Scout::$removeFromSearchJob to a custom Job which can detect whether it's been handed a normal searchable or an aggregate and behave accordingly. Or continue with overriding the queueRemoveFromSearch method and dispatch to a removeAggregateFromSearchJob. I haven't looked in detail but the Laravel\Scout\Jobs\MakeSearchable seems to work with Aggregates just fine, so it shouldn't be too hard. |
This seems to get queued deletes working. In your app service provider... Scout::removeFromSearchUsing(RemoveFromSearch::class); And then create a simple job class extending the default scout RemoveFromSearch job...
|
@goldmerc thanks for the info, I will definitely test this out this week 👍 |
A constructor has a void return type. The solution above doesn't seem to work. Setting $models to an empty EloquentCollection seems to do the trick. Still feels a bit hacky. namespace App\Jobs;
use Algolia\ScoutExtended\Searchable\AggregatorCollection;
use Illuminate\Database\Eloquent\Collection;
use Laravel\Scout\Jobs\RemoveFromSearch as BaseRemoveFromSearch;
class RemoveFromSearch extends BaseRemoveFromSearch
{
public function __construct($models)
{
if ($models instanceof AggregatorCollection) {
$models = Collection::empty();
}
parent::__construct($models);
}
} |
@mishavantol That would just pass an empty collection to the job. So, it wouldn't fail but it wouldn't do anything. The return is simply to skip the parent::__construct, so you could just change it to the following...
|
ps. it is very hacky! Just a quick solution until there is a proper fix. As mentioned above, the easiest solution is just to disable queuing for scout if that works for your app |
After a few hours of investigation, we finally figured out that this PR over at
laravel/scout
, broke our production setup when trying to removeModels
from Algolia using anAggregator
.The following exception is thrown...
Here's the stack trace...
If you need any more information to figure this out, let me know.
Thanks
The text was updated successfully, but these errors were encountered: