Skip to content
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.

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

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

Closed
bakerkretzmar opened this issue Apr 24, 2022 · 2 comments · Fixed by #308

Comments

@bakerkretzmar
Copy link
Contributor

  • Laravel version: 9.8.1
  • Algolia Scout Extended version: 2.0.2
  • Algolia Client Version: 3.2.0
  • Language Version: 8.1.4

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
Copy link
Contributor

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants