AggregatorObserver delete method shouldn't check individual model syncing status

[goldmerc]

• Laravel version: 8.49.2
• Algolia Scout Extended version: 1.20.0
• Algolia Client Version: 3.0.2

Description

AggregatorObserver's deleted + forcedDelete methods check if syncing is disabled for the model...

    public function deleted($model): void
    {
        if (static::syncingDisabledFor($model)) {
            return;
        }

So, if the model class has syncing disabled, the aggregated index will keep the record for the deleted model.

The recommended way to avoid indexing the individual models that make up an aggregated index is...

Laravel\Scout\ModelObserver::disableSyncingFor(Article::class);
Laravel\Scout\ModelObserver::disableSyncingFor(Event::class);

Furthermore the saved() method of AggregatorObserver doesn't make the same check.

Shouldn't the saved and deleted methods be consistent in the logic used to decide whether a model is synced to the index?

And in both cases, if a check is to be made to see if syncing is disabled, shouldn't that be on the aggregator class and not the model class?

Anyway, I can't see why disabling sync for an individual model should effect the behaviour of an aggregated index? But maybe there is a reason I'm missing?

Lastly, if you do call disableSearchSyncing() on an Aggregator class, it will add the class to the list for disabled sync but because the AggregatorObserver is using the model class to check, it will ignore the Aggregator and follow the model class.

[goldmerc]

Just following up, I think the code below would provide the intended behaviour and allows aggregated indexes to delete records while syncing to the individual model index is disabled.

    public function deleted($model): void
    {
        // The code commented out checks to see if the model class has syncing disabled
        // Surely we need to check the aggregator class? (see below)
        // if (static::syncingDisabledFor($model)) {
        //     return;
        // }

        if ($this->usesSoftDelete($model) && config('scout.soft_delete', false)) {
            $this->saved($model);
        } else {
            $class = get_class($model);

            if (! array_key_exists($class, $this->aggregators)) {
                return;
            }

            foreach ($this->aggregators[$class] as $aggregator) {
                // Here we check the Aggregator Class to see if syncing is disabled
                if (static::syncingDisabledFor($aggregator)) {
                    continue;
                }
                $aggregator::create($model)->unsearchable();
            }
        }
    }

Apologies if there is some reason for checking the model class (as opposed to the aggregator class) that I'm missing

[DevinCodes]

Hi @goldmerc ,

Thank you for reporting this issue, and sorry for the late reply.

I think you're right: we currently don't handle this well, and behavior should be more consistent than it currently is.

I also tend to agree that an aggregator should be unaware on whether a model has sync enabled or disabled: this is what the default ModelObserver will check anyway.

Do you think you would be able to open a PR with your proposed changes, please? Thank you in advance!

[goldmerc]

Hi @DevinCodes. My turn to say sorry for the late reply! I can open a PR but I'm afraid I wouldn't know how to do the testing. Coding is just a hobby for me and I'm still getting my head around testing. Perhaps, it would be sensible for someone more experienced to do it? but happy to try if you want - just let me know.

[DevinCodes]

If you're up to trying out testing, feel free to do so! 😄 If not that's fine as well, if you could open a PR that would already be of great help, and we can try to figure out a way to test it afterwards.

Thank you in advance, please let me know if you need anything.

[goldmerc]

Hi @DevinCodes, took me a while to get round to this but I just posted a PR #301

As discussed, I haven't added or changed any tests. I'm not sure if any are needed or how it would be done - Sorry! But hopefully, the PR will show the issue and a possible solution. It's my first PR on a public repo, so apologies if I've done anything wrong!