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

Fix Aggregator Collection macros being overwritten when other models boot #308

Closed bakerkretzmar closed 2 years ago

bakerkretzmar commented 2 years ago
Q A
Bug fix? yes
New feature? no
BC breaks? no
Related Issue Fix #305
Need Doc update no

Describe your change

This PR adds an unsearchable method to Scout Extended's AggregatorCollection class. This ensures that any time an Aggregator is removed from search, it gets removed using the queueRemoveFromSearch method on itself or on the base Aggregator class it extends, never the method in Scout's base Searchable trait.

The issue is demonstrated with a failing test in the first commit, the fix is included in the second commit and that test should then pass.

I also removed the manual re-registration of the Collection macros in the aggregator, since it's now unnecessary—for searchable, Scout Extended's behaviour is the same as Scout's, and for unsearchable, Scout Extended now doesn't use a macro at all.

What problem is this fixing?

Any time any Searchable Eloquent model boots, it registers Scout's base searchable and unsearchable Collection macros. Scout Extended's Aggregators also register these same macros, overriding Scout's, specifically to avoid queuing the removal of deleted models. If any Searchable model boots after an Aggregator`, Scout's macros will be re-registered, overriding Scout Extended's. Fixes #305.

cc @DevinCodes

bakerkretzmar commented 2 years ago

Awesome, thanks!

JayBizzle commented 2 years ago

We have just updated our application the the latest version of this package (v2.0.2 -> v2.0.3) which includes this PR and it is causing all indexing operations to fail for us.

We get the follow exception whenever an indexing operation is performed...

BadMethodCallException: Method Illuminate\Database\Eloquent\Collection::searchable does not exist.

Surely this stems from the removal of this line?

https://github.com/algolia/scout-extended/blob/1534b7060f70f82ceade6b50db2e8a9ac8b5869f/src/Searchable/Aggregator.php#L65

/ping @bakerkretzmar @DevinCodes

JayBizzle commented 2 years ago

On further investigation, it seems that this stems from the fact that we don't use the Searchable trait on our models, as we don't want to create individual Model indexes. We only want the aggregated index.

bakerkretzmar commented 2 years ago

Interesting. Now that Scout Extended is not using the macros at all it probably doesn't make any difference to add that back. Thanks for the ping, looks like @DevinCodes is about to PR a fix but let me know if you need anything else 👍🏻 sorry about that!

JayBizzle commented 2 years ago

No worries. I guess we are all using this in various different ways and it is sometime tricky to think of everyone else's use cases.

PR #310 fixes the issue for us 👍

Fingers crossed for a fast merge. Maybe some tests would be good around this issue.

DevinCodes commented 2 years ago

@JayBizzle Do you have, by any chance, a short snippet we could use to reproduce this, please?

I'll make sure to merge and release this today so we can fix this asap, but I'd like to add a test if possible to prevent this from occurring again.

JayBizzle commented 2 years ago

I'll try and put together example this evening, but basically it would involve creating an Aggregator, of which the Models defined do not use the use the Searchable trait.

DevinCodes commented 2 years ago

Thanks, I've been able to reproduce it indeed, #310 will resolve the issue. I'll merge and release, and take time to write tests for this later in the week to prevent this from creeping in again.

Thank you both for your help on this!

DevinCodes commented 2 years ago

We just release v2.0.4 with a fix for this, sorry for the inconvenience!