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] Remove custom unserialize from RemoveFromSearch #479

Closed
wants to merge 1 commit into from
Closed

[9.x] Remove custom unserialize from RemoveFromSearch #479

wants to merge 1 commit into from

Conversation

Magnesium38
Copy link
Contributor

The custom unserialization process in the RemoveFromSearch job does not take a customized scout key into account correctly.

Given a model with an overriden key like below that relies on other data on the model, the engines won't be able to process the models since they only have their primary key to work with.

public function getScoutKey()
{
    return $this->custom_id;
}

In the case of the MeiliSearch engine, it will queue a request to delete a model with the key of null. This request continues on to MeiliSearch proper and it processes the request just fine, resulting in nothing being deleted. I'm not sure what the result would be on any other engine, but I would expect it to be similar.

The original PR that added this functionality added a test test_models_are_deserialized_without_the_database. In the process of creating this PR, I tried to think of ways to preserve that functionality by possibly customizing the serialization as well, or altering what the constructor does. I failed to come up with a clean and simple way to preserve that functionality. Every attempt I tried felt brittle.

Instead, I feel that this job being able to deserialize a model without the database is beyond the scope of what this package is aiming to provide, especially since the same feature is not available on the MakeSearchable job. If this avoiding the database for deserialization is required by an application, a custom version of the job could be used instead thanks to #476.

@driesvints
Copy link
Member

This will be a breaking change. This class was introduced in #471. @stevebauman did you test this with both meilisearch and algolia when you sent this in?

@driesvints driesvints changed the title Remove custom unserialize from RemoveFromSearch [9.x] Remove custom unserialize from RemoveFromSearch Jun 10, 2021
@stevebauman
Copy link
Contributor

Hi @driesvints! I tested this feature on Algolia --- however, I did not test using a custom scout key. My apologies!

I'm going to see if I can add a test and resolve this in some way (cleanly). Give me a couple hours. I'll reply back whether or not I've come up with anything 👍

stevebauman added a commit to stevebauman/scout that referenced this pull request Jun 10, 2021
@stevebauman
Copy link
Contributor

Hi @Magnesium38 -- removing the custom unserialization from the job will break the job, as the models will already be deleted by the time the job executes (if the model isn't using soft deletes), effectively leaving your index with data that doesn't actually exist in your database.

See: #331 (comment)

@Magnesium38
Copy link
Contributor Author

Oooo, that's a solid reasoning behind needing to avoid the database. My typical use case doesn't delete data from the database, it's toggling keys that'll get used by shouldBeSearchable, which triggers the job that way.

Closing this since it is superseded by #480

taylorotwell pushed a commit that referenced this pull request Jun 10, 2021
* Re-push remove from Scout queue fix

#479

* Fix test

* StyleCI fixes

* Update src/Jobs/RemoveFromSearch.php

Co-authored-by: Dries Vints <dries@vints.io>

* Update RemoveFromSearch.php

Co-authored-by: Dries Vints <dries@vints.io>
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.

3 participants