Closed GC-Mark closed 2 years ago
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 RemoveFromSearch
job runs, the Models are unserialized, but the Aggregators now don't have any meaningful reference back to the original Model because when being unserialized, the Aggregator classes are treated like normal Eloquent Models.
I think that perhaps the RemoveFromSearch
job needs to be overridden, and some new logic put in place to handle Aggregators.
Another option is to override the queueRemoveFromSearch()
method on the Searchable
trait that the Aggregator
uses and disable queuing for delete operations 🤷♂️
I think the cleanest way is if we override the RemoveFromSearch
job indeed, and register it using \Laravel\Scout\Scout::removeFromSearchUsing
in our provider. Here we would need to prevent queueing whenever we're handling deletions through an Aggregator.
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 v9.0.0
but that version requires illuminate/pagination[v8.0.0, ..., v8.11.2]
which requires a version of PHP less than 8.0
. We are now running PHP 8.1
and cant upgrade to Laravel 9 because of it.
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.
public function queueRemoveFromSearch($models)
{
if ($models->isEmpty()) {
return;
}
$models->first()->searchableUsing()->delete($models);
}
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...
namespace App\Jobs;
use Algolia\ScoutExtended\Searchable\AggregatorCollection;
use Laravel\Scout\Jobs\RemoveFromSearch as BaseRemoveFromSearch;
class RemoveFromSearch extends BaseRemoveFromSearch
{
public function __construct($models)
{
if ($models instanceof AggregatorCollection) {
return $this->models = $models;
}
parent::__construct($models);
}
}
@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...
public function __construct($models)
{
if ($models instanceof AggregatorCollection) {
$this->models = $models;
} else {
parent::__construct($models);
}
}
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