Prevent Aggregates that use the same Models from registering multiple observers

[JayBizzle]

Take the following aggregates as an example

<?php

namespace App\Search;

use Algolia\ScoutExtended\Searchable\Aggregator;
use App\Models\Base\OpportunityType;

class KeywordSearch extends Aggregator
{
    /**
     * The names of the models that should be aggregated.
     *
     * @var string[]
     */
    protected $models = [
        \App\Models\Base\Employer::class,
        \App\Models\Base\Opportunity::class,
        \App\Models\Base\University::class,
    ];
}

<?php

namespace App\Search;

use Algolia\ScoutExtended\Searchable\Aggregator;
use App\Models\Base\OpportunityType;

class InternalSearch extends Aggregator
{
    /**
     * The names of the models that should be aggregated.
     *
     * @var string[]
     */
    protected $models = [
        \App\Models\Base\Employer::class,
        \App\Models\Base\Opportunity::class,
    ];
}

As you can see, both these aggregators use the Employer Model and the Opportunity Model (I have removed some custom toSearchableArray code for brevity).

Now, in the service provider, you would obviously boot these aggregators as follows...

\App\Search\KeywordSearch::bootSearchable();
\App\Search\InternalSearch::bootSearchable();

Now, this seems to work as expected, but the problem is, that when we boot the aggregators, what happens is that the Employer Model and the Opportunity Model each get 2 observers registered on them which results in twice as many jobs being sent to the queue when either of those Models is updated.

This PR introduces a new way of registering multiple aggregators with one method call

Aggregators::bootSearchables([
     \App\Search\KeywordSearch::class,
     \App\Search\InternalSearch::class,
]);

The new Aggregator class exposes a new bootSearchables() method which ensures that for all Models that are referenced across the specified aggregators, we only register one observer for each Model, and this single observer deals with triggering all the aggregators related to that specific Model.

This will need tests but wanted some feedback first as the test suite looks a bit tricky to get your head into.

[nunomaduro]

twice as many jobs being sent to the queue when either of those Models is updated.

Sorry for the my newbie question, but, what is your concern about having multiple jobs?

[JayBizzle]

twice as many jobs being sent to the queue when either of those Models is updated.

Sorry for the my newbie question, but, what is your concern about having multiple jobs?

They are duplicate jobs that are performing the EXACT same task. Seems a little wasteful.

[nunomaduro]

They are duplicate jobs that are performing the EXACT same task. Seems a little wasteful.

You mean that when you save a new model that exists in two aggregators:

1. 2 jobs are added to your queue
2. Each job updates both aggregators

Correct?

[JayBizzle]

I'll try and explain visually.

If we put dump($this->aggregators) on line 60 here...

scout-extended/src/Searchable/AggregatorObserver.php

Lines 53 to 64 in b6d0094

	public function saved($model): void
	{
	$class = get_class($model);
	if (! array_key_exists($class, $this->aggregators)) {
	return;
	}
	foreach ($this->aggregators[$class] as $aggregator) {
	parent::saved($aggregator::create($model));
	}
	}

...then trigger an update on the Employer Model, the output we will get is as follows (aggregator names have changed here as this is from our production code)...

The first dump() block is what is output from the first observer. As you can see, when the Employer Model gets updated, the observer is going to trigger the KeywordSearch and Console aggregator.

The second dump() block is the second observer. Again, as you can see, when the Employer Model is updated, the second observer is going to run the KeywordSearch and Console aggregators for a second time.

This PR solves this problem by figuring out which aggregators should run on which Models, and then attaches just one observer per Model which in turn prevents the duplication.

Hope that makes it clearer. Happy to explain further if not.

Thanks 👍

[DevinCodes]

I just have one remark that should fix the static analysis tool, other than that it looks good to me! Thank you for the contribution, this is a nice improvement!

[DevinCodes]

Awesome, this looks great! Would you mind writing tests for this, please?

[JayBizzle]

Ill try and look into writing tests soon, need to get my head back into it as it is a while since I have done any work around this.

[DevinCodes]

No worries, I fully understand 😄 Please let me know if I can be of any help!

[JayBizzle]

I'm really at a loss on getting any tests written for this unfortunately. If you would like to work together on moving this forward, just let me know @DevinCodes

[DevinCodes]

I'll try to schedule some time after my vacations to see if we can come up with some tests for this.

[DevinCodes]

@JayBizzle I've added some tests, they would fail if we boot the aggregators separately. WDYT?

[JayBizzle]

I'll take a look when I get back off holiday next week 👍

[DevinCodes]

Enjoy! 🌴

[JayBizzle]

This looks good to me 👍

Nice work on the tests, don't think I would have ever got there!

[damcou]

Hello @DevinCodes , LGTM
Just one question, I ran the tests and there are still remaining indices in my Algolia app afterwards, since I'm not familiar with this repo, I was just wondering if it was expected. If yes, then everything seems good :)

[DevinCodes]

@damcou I don't believe there's any cleanup in place: only for the app we use in the CI, but that's not handled by Scout Extended itself.

[DiOps]

@DevinCodes has this been added to the scout extended documentation as well?

[DevinCodes]

It's not in the documentation, no. I completely forgot to do that, thanks for pointing it out. I've put it on my todo list and will try to add it in the coming days.