Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Oct 4, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
1959292
Add getUnqualifiedScoutKeyName method
stevebauman Sep 30, 2022
6146bb7
Add RemoveableScoutCollection tests
stevebauman Sep 30, 2022
071c9de
Change order of update indexing so a custom key cannot be overridden …
stevebauman Sep 30, 2022
d996a04
Return a RemoveableScoutCollection instance and fix key type when sco…
stevebauman Sep 30, 2022
73227bb
Add `RemoveFromSearch` meilisearch tests
stevebauman Sep 30, 2022
aba8564
Flush the container on tear down
stevebauman Sep 30, 2022
7bea1cc
Remove unneeded tearDown
stevebauman Sep 30, 2022
625ba1b
Update algolia engine to delete using custom keys
stevebauman Sep 30, 2022
745ce0b
Add Algolia deletion tests with custom keys
stevebauman Sep 30, 2022
ce381f9
Rename "$values" to "$keys" for clarity
stevebauman Sep 30, 2022
8b9270f
Spacing
stevebauman Sep 30, 2022
e137aa4
Remove unused imports
stevebauman Sep 30, 2022
d3557d2
CS fixes
stevebauman Sep 30, 2022
1934e76
Comment clarification
stevebauman Sep 30, 2022
3fdb281
Move scout key array merge priority
stevebauman Oct 4, 2022
1d6a576
Update RemoveFromSearch.php
taylorotwell Oct 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
use Illuminate\Container\Container;
use Illuminate\Pagination\LengthAwarePaginator;
use Illuminate\Pagination\Paginator;
use Illuminate\Support\Str;
use Illuminate\Support\Traits\Macroable;
use Laravel\Scout\Contracts\PaginatesEloquentModels;

Expand Down Expand Up @@ -444,7 +443,7 @@ protected function getTotalCount($results)

$ids = $engine->mapIdsFrom(
$results,
Str::afterLast($this->model->getScoutKeyName(), '.')
$this->model->getUnqualifiedScoutKeyName()
)->all();

if (count($ids) < $totalCount) {
Expand Down
19 changes: 12 additions & 7 deletions src/Engines/AlgoliaEngine.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Illuminate\Database\Eloquent\SoftDeletes;
use Illuminate\Support\LazyCollection;
use Laravel\Scout\Builder;
use Laravel\Scout\Jobs\RemoveableScoutCollection;

class AlgoliaEngine extends Engine
{
Expand Down Expand Up @@ -63,9 +64,9 @@ public function update($models)
}

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

Expand All @@ -82,13 +83,17 @@ public function update($models)
*/
public function delete($models)
{
if ($models->isEmpty()) {
return;
}

$index = $this->algolia->initIndex($models->first()->searchableAs());

$index->deleteObjects(
$models->map(function ($model) {
return $model->getScoutKey();
})->values()->all()
);
$keys = $models instanceof RemoveableScoutCollection
? $models->pluck($models->first()->getUnqualifiedScoutKeyName())
: $models->map->getScoutKey();

$index->deleteObjects($keys->all());
}

/**
Expand Down
22 changes: 13 additions & 9 deletions src/Engines/MeiliSearchEngine.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
namespace Laravel\Scout\Engines;

use Illuminate\Support\LazyCollection;
use Illuminate\Support\Str;
use Laravel\Scout\Builder;
use Laravel\Scout\Jobs\RemoveableScoutCollection;
use MeiliSearch\Client as MeiliSearchClient;
use MeiliSearch\MeiliSearch;
use MeiliSearch\Search\SearchResult;
Expand Down Expand Up @@ -64,9 +64,9 @@ public function update($models)
}

return array_merge(
[$model->getKeyName() => $model->getScoutKey()],
$searchableData,
$model->scoutMetadata()
$model->scoutMetadata(),
[$model->getKeyName() => $model->getScoutKey()],
Copy link

@DarronEngelbrechtEdge DarronEngelbrechtEdge Oct 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@mmachatschek mmachatschek Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@driesvints driesvints Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevebauman can you maybe answer this question?

Copy link
Contributor Author

@stevebauman stevebauman Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@stevebauman stevebauman Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link

@DarronEngelbrechtEdge DarronEngelbrechtEdge Oct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@stevebauman stevebauman Oct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👍

);
})->filter()->values()->all();

Expand All @@ -83,13 +83,17 @@ public function update($models)
*/
public function delete($models)
{
if ($models->isEmpty()) {
return;
}

$index = $this->meilisearch->index($models->first()->searchableAs());

$index->deleteDocuments(
$models->map->getScoutKey()
->values()
->all()
);
$keys = $models instanceof RemoveableScoutCollection
? $models->pluck($models->first()->getUnqualifiedScoutKeyName())
: $models->map->getScoutKey();

$index->deleteDocuments($keys->all());
}

/**
Expand Down Expand Up @@ -244,7 +248,7 @@ public function mapIdsFrom($results, $key)
*/
public function keys(Builder $builder)
{
$scoutKey = Str::afterLast($builder->model->getScoutKeyName(), '.');
$scoutKey = $builder->model->getUnqualifiedScoutKeyName();

return $this->mapIdsFrom($this->search($builder), $scoutKey);
}
Expand Down
31 changes: 9 additions & 22 deletions src/Jobs/RemoveFromSearch.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@

use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Database\Eloquent\Collection as EloquentCollection;
use Illuminate\Queue\SerializesModels;
use Illuminate\Support\Str;

class RemoveFromSearch implements ShouldQueue
{
Expand All @@ -15,7 +13,7 @@ class RemoveFromSearch implements ShouldQueue
/**
* The models to be removed from the search index.
*
* @var \Illuminate\Database\Eloquent\Collection
* @var \Laravel\Scout\Jobs\RemoveableScoutCollection
*/
public $models;

Expand Down Expand Up @@ -46,35 +44,24 @@ public function handle()
* Restore a queueable collection instance.
*
* @param \Illuminate\Contracts\Database\ModelIdentifier $value
* @return \Illuminate\Database\Eloquent\Collection
* @return \Laravel\Scout\Jobs\RemoveableScoutCollection
*/
protected function restoreCollection($value)
{
if (! $value->class || count($value->id) === 0) {
return new EloquentCollection;
return new RemoveableScoutCollection;
}

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

$model->forceFill([$keyName => $id]);
$model->setKeyType(
is_string($id) ? 'string' : 'int'
)->forceFill([
$model->getUnqualifiedScoutKeyName() => $id,
]);
});
})
);
}

/**
* Get the unqualified Scout key name.
*
* @param string $keyName
* @return string
*/
protected function getUnqualifiedScoutKeyName($keyName)
{
return Str::afterLast($keyName, '.');
}
}
11 changes: 11 additions & 0 deletions src/Searchable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Illuminate\Database\Eloquent\SoftDeletes;
use Illuminate\Support\Collection as BaseCollection;
use Illuminate\Support\Str;

trait Searchable
{
Expand Down Expand Up @@ -389,6 +390,16 @@ public function getScoutKeyName()
return $this->getQualifiedKeyName();
}

/**
* Get the unqualified Scout key name.
*
* @return string
*/
public function getUnqualifiedScoutKeyName()
{
return Str::afterLast($this->getScoutKeyName(), '.');
}

/**
* Determine if the current class should use soft deletes with searching.
*
Expand Down
57 changes: 57 additions & 0 deletions tests/Unit/AlgoliaEngineTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
namespace Laravel\Scout\Tests\Unit;

use Algolia\AlgoliaSearch\SearchClient;
use Illuminate\Container\Container;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Support\Facades\Config;
use Illuminate\Support\LazyCollection;
use Laravel\Scout\Builder;
use Laravel\Scout\EngineManager;
use Laravel\Scout\Engines\AlgoliaEngine;
use Laravel\Scout\Jobs\RemoveFromSearch;
use Laravel\Scout\Tests\Fixtures\EmptySearchableModel;
use Laravel\Scout\Tests\Fixtures\SearchableModel;
use Laravel\Scout\Tests\Fixtures\SoftDeletedEmptySearchableModel;
Expand All @@ -25,6 +28,7 @@ protected function setUp(): void

protected function tearDown(): void
{
Container::getInstance()->flush();
m::close();
}

Expand All @@ -51,6 +55,59 @@ public function test_delete_removes_objects_to_index()
$engine->delete(Collection::make([new SearchableModel(['id' => 1])]));
}

public function test_delete_removes_objects_to_index_with_a_custom_search_key()
{
$client = m::mock(SearchClient::class);
$client->shouldReceive('initIndex')->with('table')->andReturn($index = m::mock(Indexes::class));
$index->shouldReceive('deleteObjects')->once()->with(['my-algolia-key.5']);

$engine = new AlgoliaEngine($client);
$engine->delete(Collection::make([new AlgoliaCustomKeySearchableModel(['id' => 5])]));
}

public function test_delete_with_removeable_scout_collection_using_custom_search_key()
{
$job = new RemoveFromSearch(Collection::make([
new AlgoliaCustomKeySearchableModel(['id' => 5]),
]));

$job = unserialize(serialize($job));

$client = m::mock(SearchClient::class);
$client->shouldReceive('initIndex')->with('table')->andReturn($index = m::mock(stdClass::class));
$index->shouldReceive('deleteObjects')->once()->with(['my-algolia-key.5']);

$engine = new AlgoliaEngine($client);
$engine->delete($job->models);
}

public function test_remove_from_search_job_uses_custom_search_key()
{
$job = new RemoveFromSearch(Collection::make([
new AlgoliaCustomKeySearchableModel(['id' => 5]),
]));

$job = unserialize(serialize($job));

Container::getInstance()->bind(EngineManager::class, function () {
$engine = m::mock(AlgoliaEngine::class);

$engine->shouldReceive('delete')->once()->with(m::on(function ($collection) {
$keyName = ($model = $collection->first())->getUnqualifiedScoutKeyName();

return $model->getAttributes()[$keyName] === 'my-algolia-key.5';
}));

$manager = m::mock(EngineManager::class);

$manager->shouldReceive('engine')->andReturn($engine);

return $manager;
});

$job->handle();
}

public function test_search_sends_correct_parameters_to_algolia()
{
$client = m::mock(SearchClient::class);
Expand Down
73 changes: 71 additions & 2 deletions tests/Unit/MeiliSearchEngineTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@

namespace Laravel\Scout\Tests\Unit;

use Illuminate\Container\Container;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Support\Facades\Config;
use Illuminate\Support\LazyCollection;
use Laravel\Scout\Builder;
use Laravel\Scout\EngineManager;
use Laravel\Scout\Engines\MeiliSearchEngine;
use Laravel\Scout\Jobs\RemoveFromSearch;
use Laravel\Scout\Tests\Fixtures\EmptySearchableModel;
use Laravel\Scout\Tests\Fixtures\SearchableModel;
use Laravel\Scout\Tests\Fixtures\SoftDeletedEmptySearchableModel;
Expand All @@ -18,6 +22,18 @@

class MeiliSearchEngineTest extends TestCase
{
protected function setUp(): void
{
Config::shouldReceive('get')->with('scout.after_commit', m::any())->andReturn(false);
Config::shouldReceive('get')->with('scout.soft_delete', m::any())->andReturn(false);
}

protected function tearDown(): void
{
Container::getInstance()->flush();
m::close();
}

public function test_update_adds_objects_to_index()
{
$client = m::mock(Client::class);
Expand All @@ -43,6 +59,59 @@ public function test_delete_removes_objects_to_index()
$engine->delete(Collection::make([new SearchableModel(['id' => 1])]));
}

public function test_delete_removes_objects_to_index_with_a_custom_search_key()
{
$client = m::mock(Client::class);
$client->shouldReceive('index')->with('table')->andReturn($index = m::mock(Indexes::class));
$index->shouldReceive('deleteDocuments')->once()->with(['my-meilisearch-key.5']);

$engine = new MeiliSearchEngine($client);
$engine->delete(Collection::make([new MeiliSearchCustomKeySearchableModel(['id' => 5])]));
}

public function test_delete_with_removeable_scout_collection_using_custom_search_key()
{
$job = new RemoveFromSearch(Collection::make([
new MeiliSearchCustomKeySearchableModel(['id' => 5]),
]));

$job = unserialize(serialize($job));

$client = m::mock(Client::class);
$client->shouldReceive('index')->with('table')->andReturn($index = m::mock(Indexes::class));
$index->shouldReceive('deleteDocuments')->once()->with(['my-meilisearch-key.5']);

$engine = new MeiliSearchEngine($client);
$engine->delete($job->models);
}

public function test_remove_from_search_job_uses_custom_search_key()
{
$job = new RemoveFromSearch(Collection::make([
new MeiliSearchCustomKeySearchableModel(['id' => 5]),
]));

$job = unserialize(serialize($job));

Container::getInstance()->bind(EngineManager::class, function () {
$engine = m::mock(MeiliSearchEngine::class);

$engine->shouldReceive('delete')->once()->with(m::on(function ($collection) {
$keyName = ($model = $collection->first())->getUnqualifiedScoutKeyName();

return $model->getAttributes()[$keyName] === 'my-meilisearch-key.5';
}));

$manager = m::mock(EngineManager::class);

$manager->shouldReceive('engine')->andReturn($engine);

return $manager;
});

$job->handle();
}

public function test_search_sends_correct_parameters_to_meilisearch()
{
$client = m::mock(Client::class);
Expand Down Expand Up @@ -171,7 +240,7 @@ public function test_returns_primary_keys_when_custom_array_order_present()
$builder = m::mock(Builder::class);

$model = m::mock(stdClass::class);
$model->shouldReceive(['getScoutKeyName' => 'table.custom_key']);
$model->shouldReceive(['getUnqualifiedScoutKeyName' => 'custom_key']);
$builder->model = $model;

$engine->shouldReceive('keys')->passthru();
Expand Down Expand Up @@ -300,7 +369,7 @@ public function test_a_model_is_indexed_with_a_custom_meilisearch_key()
$client = m::mock(Client::class);
$client->shouldReceive('index')->with('table')->andReturn($index = m::mock(Indexes::class));
$index->shouldReceive('addDocuments')->once()->with([[
'id' => 5,
'id' => 'my-meilisearch-key.5',
]], 'id');

$engine = new MeiliSearchEngine($client);
Expand Down
Loading