matchish / laravel-scout-elasticsearch

Search among multiple models with ElasticSearch and Laravel Scout
MIT License
705 stars 113 forks source link

Implement lazyMap #202

Closed matchish closed 2 years ago

matchish commented 2 years ago

Is your feature request related to a problem? Please describe. LazyMap required for Laravel Nova. Here if a possible implementation that will fail with MixedSearch https://github.com/matchish/laravel-scout-elasticsearch/pull/172

Describe the solution you'd like Implement lazyMap that can work with any Builder

Describe alternatives you've considered We can use the solution https://github.com/matchish/laravel-scout-elasticsearch/pull/172 but meaningfull error have to be thrown when cursor used with MixedSearch that prevent debugging

hkulekci commented 2 years ago

I read the messages on #172. I did some tests on my local but am not finished yet. I did not use this lazyMap feature before for this reason; I need to work on it a little bit to learn what it is exactly. Please ping me also if anybody wants to work on this.

matchish commented 2 years ago

Sure. I'll let you know if anyone interested in the feature as well

hkulekci commented 2 years ago

I want to just write my opinions to share. It can be a little bit hacky, but I need to share it to understand the current situation.

For the builder, we don't have a solution to say this is a MixedSearch. For this reason, we are using anonymous classes while creating MixedSearch. Because according to the nature of MixedSearch, there is no 1 model. In fact, we can find a solution by sending a callback function for every document of the LazyCollection if we can, but I am not super familiar with how the LazyCollection works. Anyhow, we need to understand lazyMap function, which is coming as a normal search or MixedSearch. So, we can check basically whether the class is anonymous or not with ReflectionClass. But, we need to be aware that it will be slow. I just put an if at the beginning of the @derpierre65 solution.

        if ((new \ReflectionClass($model))->isAnonymous()) {
            throw new \Error('Not implemented for MixedSearch');
        }

Another solution is extending the trait of the Searchable for MixedSearch to be able to change the function logic of Searchable::queryScoutModelsByIds(). But the problem, we could not extend the traits. We can create a MixedModel with a queryScoutModelsByIds() function in this library. Even with this solution, we will have problems with how we understand which model we need to create and find the record by id.

Also, if we don't want to use ReflectionClass, we can create a MixedModel inside the library and we can use it as MixedSearch model as below :

final class MixedSearch
{
    public static function search(string $query = '', $callback = null): Builder
    {
        return new Builder(new MixedModel(), $query, $callback);
    }
}

Then we can check inside the lazyMap method as below :

        if ($model instanceof MixedModel) {
            throw new \Error('Not implemented for MixedSearch');
        }

what do you think? @matchish

matchish commented 2 years ago

@hkulekci I like the idea with reflection. We call it once for a search. And usually there is one search for a request. So should be ok. It's good first step to support cursor. Later when we have a lazymap that works for mixedsearch as well, we need to remove reflection and that's it. Thanks for your research)

Actually I don't remember why a came to a solution with the anonymous class. Maybe it was just easiest way to make things work)

About MixedModel I think it's ok to have it in the library but we introduce it only for type checking and it's kind of smell. It have to be removed anyway later when we have lazymap that works for mixedsearch as well. If we will find a case where MixedModel will be useful for better reason I think we can to introduce it. Thoughts?

matchish commented 2 years ago

I see that you implemented MixedModel approach so we can go this way. Anyway it's implementation details so actually not so important will we go with reflection or MixedModel. @hkulekci

hkulekci commented 2 years ago

I decide to use ReflectionClass again to remove the extra class. Because we need to put extra namespace on lots of files. After finding a proper solution, we will remove it, anyway.

matchish commented 2 years ago

In master. Could you please update change log?

hkulekci commented 2 years ago

Nice. Thanks for reviewing. I will do changes to the changelog for sure. 👍

hkulekci commented 2 years ago

I think I forgot to do changes on the changelog, right? Let me do it in a couple of days.

matchish commented 2 years ago

Yeah I will be appreciated if you do it) Then I can do a release

hkulekci commented 2 years ago

Sure, I need a reminder for the issues on GitHub :D

hkulekci commented 2 years ago

I think we can close this issue, right?

matchish commented 2 years ago

You're right. Thanks for your investigation and great job)