[9.x] Fix custom scout keys not being utilized when deleting from queue #656

stevebauman · 2022-09-30T13:48:30Z

Closes #655
Closes #653

This PR resolves an issue described in #653 where the MeiliSearchEngine (as well as the AlgoliaEngine, it turns out) does not utilize the custom scout key when removing models from the search index when deleting from the queued RemoveFromSearch job.

The issue, is that when the faux model instances (faux, because they don't actually exist in the database) are instantiated from the RemoveFromSearch job after unserialization, all of the model's attributes are (of course) not available. The models are instead force-filled with the restored from queue custom scout keys:

scout/src/Jobs/RemoveFromSearch.php

Lines 59 to 65 in 3352fbc

    
           return tap(new $value->class, function ($model) use ($id) { 
        
               $keyName = $this->getUnqualifiedScoutKeyName( 
        
                   $model->getScoutKeyName() 
        
               ); 
        
               $model->forceFill([$keyName => $id]); 
        
           });

This means, that the getScoutKey() method on these faux model instances will not return the appropriate keys inside of the delete() methods of their respective Engine when they are retrieved, as they will be overridden, and all other model attributes will be missing.

Another issue, was that using a custom scout key that returned as a string, would interfere with Eloquent's primary key type casting. If the primary key of the searchable model matches the unqualified scout key (which is likely always the case), then Eloquent would attempt to cast the string as an integer when retrieved from the model naturally. This would result in getScoutKey() returning 0 if the returned custom key is prefixed with a string.

To resolve this, I've implemented a check to determine if the Scout key is a string, and alter the primary key type of the faux model instance to suit, so this issue no longer occurs:

https://github.com/stevebauman/scout/blob/8b9270f812e8de28e6371ecd5ecb20e61f491116/src/Jobs/RemoveFromSearch.php#L59-L68

return tap(new $value->class, function ($model) use ($id) {
    // The scout key may not be an integer. In this case,
    // we will force a key type of string so it is not
    // cast when retrieving it from the model.
    $model->setKeyType(
        is_string($id) ? 'string' : 'int'
    )->forceFill([
        $model->getUnqualifiedScoutKeyName() => $id
    ]);
});

@DarronEngelbrechtEdge This should resolve your issue -- can you review this PR? I've included tests that cover your use case.

Let me know if you'd like anything adjusted, thanks for your time! ❤️

…and resolve deleting with custom key

…ut key is a string

driesvints · 2022-09-30T14:43:43Z

Thanks a lot @stevebauman! Great work.

Did you keep in mind the changes that are present on master? Do we need to adjust a lot when merging this into master later on?

I'd love a review by you @mmachatschek if you're willing.

stevebauman · 2022-09-30T15:55:10Z

Happy to help @driesvints! 🙏

Did you keep in mind the changes that are present on master?

I didn't -- let me checkout master and see what the differences are and I'll comment back when I'm done!

stevebauman · 2022-09-30T16:22:00Z

Ok I've submitted a draft PR for the master (10.x) branch with this PR's changes 👍

Let me know your thoughts!

DarronEngelbrechtEdge · 2022-10-01T08:05:39Z

@stevebauman - Many thanks this PR seems like it will sort all uses cases and both engines. I will have time again on Monday to test it in my project and confirm that it's working for me.

DarronEngelbrechtEdge

Thanks!

DarronEngelbrechtEdge · 2022-10-01T08:14:10Z

src/Engines/MeiliSearchEngine.php

                $searchableData,
-                $model->scoutMetadata()
+                $model->scoutMetadata(),
+                [$model->getKeyName() => $model->getScoutKey()],


@stevebauman - PS. If people are already adding the key in their toSearchableArray() with some value other than what's returned from getScoutKey(). Worth considering perhaps? Might also then want to make this change in the MeiliSearchEngine to ensure the objectID key is not overridden?

this is also something that jumped to my eye. The order here was definitly on purpose the way it was before. is there a specific reason why this was change?

@stevebauman can you maybe answer this question?

Sorry for the late reply on this one guys! I modified this due to conflicts with the searchable data array. The priority here was intended.

$searchableData will contain the model's attributes, including its primary key, alongside other data:

['id' => 1, '...']

This means, the custom Scout key will always be overridden (unless the model uses a different primary key).

Without this, the array_merge() structure would be:

array_merge( ['id' => 'custom-scout-key:1'], ['id' => 1], ['metadata'], )

Leading to the custom scout key always being overridden by the primary key.

@DarronEngelbrechtEdge PS. If people are already adding the key in their toSearchableArray() with some value other than what's returned from getScoutKey(). Worth considering perhaps?

I'm not really sure how we can account for that due to the above scenario... The primary key will likely always be present, so the custom Scout key will never be included in the indexed data. Correct me if I'm wrong?

Might also then want to make this change in the MeiliSearchEngine to ensure the objectID key is not overridden?

Don't you specify the primary key with the MeilisearchEngine? I can't find any reference to objectID inside of Meilisearch's documentation.

The implementations the above array_merge() priority for the MeiliSearchEngine and AlgoliaEngine differ, as Algolia requires a primary key of objectID. You cannot change it (to my knowledge). While MeiliSearch allows you to change it:

scout/src/Engines/AlgoliaEngine.php

Lines 65 to 69 in 3352fbc

return array_merge(

['objectID' => $model->getScoutKey()],

$searchableData,

$model->scoutMetadata()

);

Since objectID is unlikely to be a models primary key, collisions in the array_merge() are unlikely, so I haven't adjusted it. Maybe we should to ensure the same "priority" consistency across the included engines?

AlgoliaEngine:

scout/src/Engines/AlgoliaEngine.php

Line 194 in 3352fbc

$objectIds = collect($results['hits'])->pluck('objectID')->values()->all();

MeiliSearchEngine:

scout/src/Engines/MeiliSearchEngine.php

Line 266 in 3352fbc

$objectIds = collect($results['hits'])->pluck($model->getKeyName())->values()->all();

I'm not really sure how we can account for that due to the above scenario... The primary key will likely always be present, so the custom Scout key will never be included in the indexed data. Correct me if I'm wrong?

@stevebauman - You're correct, the array_merge() must decide if it wants to override the toSearchableArray() or use the value from there if the user has specified the key returned by getScoutKeyName().

I am in favor of your method change which will override toSearchableArray() as this will ensure the rest of the functionality will operate on the correct ID giving the user some protection against bad configurations? Either way it might be worth a mention somewhere in the docs if it's not there already?

Don't you specify the primary key with the MeilisearchEngine? I can't find any reference to objectID inside of Meilisearch's documentation.

@stevebauman - Sorry noticed a typo in my previous suggestion, I meant to make the change in the AlgoliaEngine to be sure that the user can't override the objectID key in the toSearchableArray() but can only modify the value with getScoutKey() as intended.

Sorry noticed a typo in my previous suggestion, I meant to make the change in the AlgoliaEngine to be sure that the user can't override the objectID key in the toSearchableArray() but can only modify the value with getScoutKey() as intended.

No worries! Understood, I agree with this change. I've moved the order of this in the AlgoliaEngine as well. 👍

DarronEngelbrechtEdge · 2022-10-03T07:23:32Z

@stevebauman - Thanks again I can confirm this resolves issue #653 in my project (using MeiliSearchEngine).

stevebauman · 2022-10-03T15:12:36Z

That's excellent to hear @DarronEngelbrechtEdge! Thank you for taking the time to review the PR and testing it out ❤️

DarronEngelbrechtEdge · 2022-10-04T05:29:36Z

That's excellent to hear @DarronEngelbrechtEdge! Thank you for taking the time to review the PR and testing it out heart

@stevebauman Only a pleasure, thanks to you lot for saving so many people so much time!

driesvints · 2022-10-04T09:16:57Z

So where are we with this? Is this ready for review?

stevebauman · 2022-10-04T12:20:22Z

Ready for review now @driesvints! I've made one last change in regards to @DarronEngelbrechtEdge comment. I've moved the array_merge() priority for the AlgoliaEngine as well for unification across the engines:

3fdb281

return array_merge(
    $searchableData,
    $model->scoutMetadata(),
    ['objectID' => $model->getScoutKey()], // Moved below so it cannot be overridden.
);

driesvints · 2022-10-04T12:53:51Z

Thanks @stevebauman this looks good. @mmachatschek does this also look good to you?

mmachatschek

@driesvints The changes look good to me after @stevebauman answered my concerns

driesvints

Thanks everyone for your work/feedback on this!

taylorotwell · 2022-10-04T14:10:56Z

Thanks 👍

stevebauman added 14 commits September 30, 2022 08:40

Add getUnqualifiedScoutKeyName method

1959292

Add RemoveableScoutCollection tests

6146bb7

Change order of update indexing so a custom key cannot be overridden …

071c9de

…and resolve deleting with custom key

Return a RemoveableScoutCollection instance and fix key type when sco…

d996a04

…ut key is a string

Add RemoveFromSearch meilisearch tests

73227bb

Flush the container on tear down

aba8564

Remove unneeded tearDown

7bea1cc

Update algolia engine to delete using custom keys

625ba1b

Add Algolia deletion tests with custom keys

745ce0b

Rename "$values" to "$keys" for clarity

ce381f9

Spacing

8b9270f

Remove unused imports

e137aa4

CS fixes

d3557d2

Comment clarification

1934e76

driesvints marked this pull request as draft September 30, 2022 14:43

stevebauman mentioned this pull request Sep 30, 2022

[10.x] Fix custom scout keys not being utilized when deleting from queue #657

Merged

DarronEngelbrechtEdge reviewed Oct 1, 2022

View reviewed changes

Move scout key array merge priority

3fdb281

stevebauman marked this pull request as ready for review October 4, 2022 12:15

mmachatschek reviewed Oct 4, 2022

View reviewed changes

driesvints approved these changes Oct 4, 2022

View reviewed changes

Update RemoveFromSearch.php

1d6a576

taylorotwell merged commit 19d77a4 into laravel:9.x Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[9.x] Fix custom scout keys not being utilized when deleting from queue #656

[9.x] Fix custom scout keys not being utilized when deleting from queue #656

stevebauman commented Sep 30, 2022 •

edited

Loading

driesvints commented Sep 30, 2022

stevebauman commented Sep 30, 2022

stevebauman commented Sep 30, 2022 •

edited

Loading

DarronEngelbrechtEdge commented Oct 1, 2022

DarronEngelbrechtEdge left a comment

DarronEngelbrechtEdge Oct 1, 2022

mmachatschek Oct 3, 2022

driesvints Oct 3, 2022

stevebauman Oct 3, 2022 •

edited

Loading

stevebauman Oct 3, 2022 •

edited

Loading

DarronEngelbrechtEdge Oct 4, 2022

stevebauman Oct 4, 2022

DarronEngelbrechtEdge commented Oct 3, 2022

stevebauman commented Oct 3, 2022

DarronEngelbrechtEdge commented Oct 4, 2022

driesvints commented Oct 4, 2022

stevebauman commented Oct 4, 2022

driesvints commented Oct 4, 2022

mmachatschek left a comment

driesvints left a comment

taylorotwell commented Oct 4, 2022

	return tap(new $value->class, function ($model) use ($id) {
	$keyName = $this->getUnqualifiedScoutKeyName(
	$model->getScoutKeyName()
	);

	$model->forceFill([$keyName => $id]);
	});

	return array_merge(
	['objectID' => $model->getScoutKey()],
	$searchableData,
	$model->scoutMetadata()
	);

[9.x] Fix custom scout keys not being utilized when deleting from queue #656

[9.x] Fix custom scout keys not being utilized when deleting from queue #656

Conversation

stevebauman commented Sep 30, 2022 • edited Loading

driesvints commented Sep 30, 2022

stevebauman commented Sep 30, 2022

stevebauman commented Sep 30, 2022 • edited Loading

DarronEngelbrechtEdge commented Oct 1, 2022

DarronEngelbrechtEdge left a comment

Choose a reason for hiding this comment

DarronEngelbrechtEdge Oct 1, 2022

Choose a reason for hiding this comment

mmachatschek Oct 3, 2022

Choose a reason for hiding this comment

driesvints Oct 3, 2022

Choose a reason for hiding this comment

stevebauman Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

stevebauman Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

DarronEngelbrechtEdge Oct 4, 2022

Choose a reason for hiding this comment

stevebauman Oct 4, 2022

Choose a reason for hiding this comment

DarronEngelbrechtEdge commented Oct 3, 2022

stevebauman commented Oct 3, 2022

DarronEngelbrechtEdge commented Oct 4, 2022

driesvints commented Oct 4, 2022

stevebauman commented Oct 4, 2022

driesvints commented Oct 4, 2022

mmachatschek left a comment

Choose a reason for hiding this comment

driesvints left a comment

Choose a reason for hiding this comment

taylorotwell commented Oct 4, 2022

stevebauman commented Sep 30, 2022 •

edited

Loading

stevebauman commented Sep 30, 2022 •

edited

Loading

stevebauman Oct 3, 2022 •

edited

Loading

stevebauman Oct 3, 2022 •

edited

Loading