algolia / algolia/scout-extended

Honor makeAllSearchableUsing() in Aggregator?

Open
#342 2 comments 1 reaction 0 assignees View on GitHub
Dominant language
PHP
Stars
420
Forks
94
PR merge metrics
No merged PRs in 30d

Description

- Laravel version: 10.48.10
- Algolia Scout Extended version: 3.1.0
- Algolia Client Version: 3.4.1
- Language Version:

### Description
First time experimenting with the Aggregator feature, and it's quite nice. However, I'm also a fan of the `makeAllSearchableUsing()` method in Scout which can implement a more specific query. (I'm aware that `$relations` can be used for eager loading and `shouldBeSearchable()` can be used to filter out results, but in my use case, the filtering logic relies on the result of a separate query, and this would create an n+1 query performance issue that would be resolved by putting the conditional logic in the main query to pull all searchable models.) The `makeAllSearchable()` method on the Aggregator class does not appear to honor the `makeAllSearchableUsing()` in the individual models the way the `makeAllSearchable()` method on the main `Laravel\Scout\Searchable` trait does.

I'm posing this as a question for discussion, because I realize that honoring any such `makeAllSearchableUsing()` methods in the individual models could be an undesirable behavior and/or breaking change for some folks (if they are using different logic in the individual index and the aggregator). Ideally there would be a way to optionally honor these methods, or perhaps to define them in the Aggregator class itself. I'm currently overriding the `makeAllSearchable()` method in the `Aggregator` class and chaining in that additional step along the lines of what the `Laravel\Scout\Searchable` trait does (note that this requires making the `makeAllSearchableUsing()` method public in the source model, otherwise it's not accessible to the Aggregator):

```php
getModels() as $model) {
$instance = new $model;

$softDeletes =
in_array(SoftDeletes::class, class_uses_recursive($model)) && config('scout.soft_delete', false);

// Added these two lines to determine if there's a public 'makeAllSearchableUsing' method in the model instance)
$makeAllSearchableUsingReflection = new \ReflectionMethod($instance, 'makeAllSearchableUsing');
$applyMakeAllSearchableUsing = $makeAllSearchableUsingReflection->isPublic();

$instance->newQuery()->when($softDeletes, function ($query) {
$query->withTrashed();
})->when($applyMakeAllSearchableUsing, function ($query) use ($instance) { // Added these two lines
$instance->makeAllSearchableUsing($query);
})->orderBy($instance->getKeyName())->chunk(config('scout.chunk.searchable', 500), function ($models) {
$models = $models->map(function ($model) {
return static::create($model);
})->filter->shouldBeSearchable();

$models->searchable();

event(new ModelsImported($models));
});
}
}
}
```

I don't love doing it this way, since it's possible the underlying `makeAllSearchable` logic will change in future updates and this code will need to be kept in sync. Perhaps there's a more elegant way to do this? Could we add an optionally extendable `makeAllSearchableUsing` method on the Aggregator class itself?

### Steps To Reproduce

See above

Contributor guide

Open the contributing guide

Research direction

Start with Aggregator::makeAllSearchable() and compare it with Laravel Scout's Searchable::makeAllSearchable() behavior, using the issue's override as the reference implementation. Determine whether the completed behavior should optionally honor model makeAllSearchableUsing() methods or expose an Aggregator-level extension point, then verify the chosen behavior against the documented compatibility concerns.

Written by the indexing model from the issue text.

Assessment

Tech stack
laravel, php
Domain
search
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.