search

algolia / scout-extended

Scout Extended: The Full Power of Algolia in Laravel
https://www.algolia.com/doc/framework-integration/laravel/getting-started/introduction-to-scout-extended/
MIT License
393 stars 85 forks source link

Scout's collection macros override Scout Extended's if regular models booted after Aggregator #305

Closed bakerkretzmar closed 2 years ago

bakerkretzmar commented 2 years ago

Description

I have a simple aggregator that indexes my Event and Hub models, and just today I started getting a ModelNotDefinedInAggregatorException thrown any time I tried to save a Hub. Digging around in the stack trace I realized that the queueRemoveFromSearch method was being called from inside my Page model, which is also searchable but not part of an aggregator.

I think what's happening is that Scout's Searchable trait, which registers new searchable and unsearchable methods on the base Illuminate Collection, basically proxies those methods through to corresponding methods on the particular class that registers the macros. Because many different models can use the Searchable trait and each will boot it independently, this effectively means that the Collection macros will always end up calling queueMakeSearchable and queueRemoveFromSearch on whichever model booted the Searchable trait last.

In most apps this wouldn't matter because every model is using the the same Searchable trait, so the methods all do the same thing, but Scout Extended's aggregators override the booting of the Searchable trait and re-register the macros (along with their own observers). In my app I'm registering my own observers in a couple places, and they get registered after I call bootSearchable on my aggregator, and somehow, Scout Extended's macros or observers are being overridden by Scout's own.

If I comment out or move around my own observers so that my aggregator is the last thing to call bootSearchable, everything seems to work fine. If I fiddle with the order, the ModelNotDefined... exception always reports that it's coming from the model that had its observers registered last.

I think this started when I upgraded to v2, but I can't be sure. The only PR I can find recently that could have tweaked this behaviour is #239, but I can't figure out how it could be causing this.

Steps To Reproduce

Register any model observer on a regular searchable model after calling bootSearchable on an aggregator, then save one of the aggregator's models and see the above error.

I will spin up a new app and work on putting together a minimal reproduction of this.

DevinCodes commented 2 years ago

Thank you for the report @bakerkretzmar ! Please let me know when you've been able to spin up the app with reproduction. Thank you advance!

AlexVanderbist commented 2 years ago

Hi, I'm running into the same issue. As @bakerkretzmar explained, the searchable and unsearchable macros are being registered every time a generic Searchable model boots. This means that, depending on the order of execution, these macros (and thus queueMakeSearchable and queueRemoveFromSearch) will be called on the generic Searchable model and not on the Aggregator instance.

The easiest (and also hackiest) solution I can think of is re-registering the macros in the ModelObserver, right before calling the searchable()/unsearchable() methods. This ensures that the macro will call the Aggregator::queueMakeSearchable() method and not the generic Searchable::queueMakeSearchable().

I'm happy to create a PR for this change but I fear that this isn't the cleanest solution to keep in the package longterm