Custom Routing is broken in scout 9.1.0+ #104
Comments
|
Hey @spiritinlife, I need more details here. What In the current version, you only need to define public function shardRouting()
{
return $this->user->id;
}I don't see how it should break the routing. Even if you use a relation like in the example above, then it should be loaded if missing. Do you have a specific error message? |
|
Hey, sorry for not giving more details, will provide more soon. Deletes are broken with this scout pr and queues enabled. |
|
@babenkoivan i hope this gives more info. Scout version is the issue here, anything over 9.1.0 should fail due to this pr laravel/scout#471. Maybe we can override the RemoveFromSearch job to deserialise the shardRouting value along with the id. The error stacktrace when RemoveFromSearch job fails. |
|
Thanks for the details, @spiritinlife! The problem is that I'm not even sure if |
|
Hey guys! I'll do my best to help out here 👍
This won't be possible without querying the database unfortunately. When a queued job is serialized, Laravel will process each property on the job class (which may be the model or collection of models), and overwrite them with a https://github.com/laravel/framework/blob/8.x/src/Illuminate/Queue/SerializesModels.php Unless you're referring to querying the ElasticSearch instance with the scout keys and hydrating the model(s) with its data instead? Update: Actually this may be very tricky, especially in the OP's scenario above due to the routing key being dependent on a relationship. Maybe we could override the
This was actually resolved here: laravel/scout#480 shortly after laravel/scout#471. |
|
Wouldn't that problem be fixed if we didn't use Also another thing that we could do is separate the option of queueing deletes from queuing indexing. So add another config option |
To my knowledge, Laravel does this to ensure all models can be serialized, as they may contain closures inside of their property values which cannot be serialized without an external package (PHP cannot serialize closures by default). When Laravel unserializes the job, it uses the serialized queuable ID's to query for all the models that were in the job, and re-hydrates the jobs properties with the resulting models, bypassing this issue of unserializable values. Overriding this job and removing this trait will break environments if the above case is true.
Yea we could do that, though we'd have to PR Scout itself and see if that's something they'd like to be included. With Laravel's current implementation of serialization, we'd have to slap a heavy layer of overrides to get this working smoothly. |
Doesn't Laravel query DB by default? protected function restoreCollection($value)
{
if (! $value->class || count($value->id) === 0) {
return new EloquentCollection;
}
$collection = $this->getQueryForModelRestoration(
(new $value->class)->setConnection($value->connection), $value->id
)->useWritePdo()->get();
if (is_a($value->class, Pivot::class, true) ||
in_array(AsPivot::class, class_uses($value->class))) {
return $collection;
}
$collection = $collection->keyBy->getKey();
$collectionClass = get_class($collection);
return new $collectionClass(
collect($value->id)->map(function ($id) use ($collection) {
return $collection[$id] ?? null;
})->filter()
);
}It is just overwritten here. We can restore this code in Elastic Driver Plus, I think this would solve the issue or am I missing something? |
|
The problem with querying the database is that the model will most likely don't exist. This job runs in response to a model deletion. To me the only viable solution is to either remove |
|
@spiritinlife indeed! I didn't think it through 🙃 Yeah, we probably need a custom serializer then. I will try to work on it next week. |
|
@spiritinlife I've just released v3.2.2 with a fix that should eliminate the problem. Unfortunately, I was only able to test it with the |
|
@babenkoivan Thank you, will do a test and inform you. |
|
@babenkoivan I can confirm it works. Thank you 🙏 |
This pr laravel/scout#471 breaks custom routing since the deserialized model does not have the needed properties to define the routing property
The text was updated successfully, but these errors were encountered: