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

AlgoliaEngine::map does not respect the order of the Engine results #341

Closed
nunomaduro opened this issue Jan 3, 2019 · 10 comments · Fixed by #369
Closed

AlgoliaEngine::map does not respect the order of the Engine results #341

nunomaduro opened this issue Jan 3, 2019 · 10 comments · Fixed by #369
Labels

Comments

@nunomaduro
Copy link
Member

Context: Engines, like Algolia, allow to define the ranking - order of the results.

An example could be: I want products to be sorted by the quantity, so when I type "iphone" i want to get the ones that I have more in stock.

Problem: The method AlgoliaEngine::map does not return the models in the same order of the returned results by Algolia. Mainly because a whereIn is applied behind-the-scenes.

Solution: Sort the models by the same order of the given results just before returning the result of AlgoliaEngine::map.

@nunomaduro nunomaduro added the bug label Jan 3, 2019
@nunomaduro nunomaduro changed the title AlgoliaEngine::map does not respect the order of the Engine results AlgoliaEngine::map does not respect the order of the Engine results Jan 3, 2019
@driesvints
Copy link
Member

@nunomaduro I tried looking into this but I don't see how whereIn influences the order?

@nunomaduro
Copy link
Member Author

The AlgoliaEngine::map will call the method getScoutModelsByIds from the trait Searchable.php.

The method getScoutModelsByIds will perform a Model::whereIn. The problem here: When you perform a regular $users = User::whereIn('id', [2,3,1])->get();, and assuming that you are using Mylsql, Eloquent will just tranform that in SELECT * FROM users WHERE id IN (2,3,1);. And mysql, be default, will return the rows in ID ascending order (1,2,3).

In this process you loose the order returned by the search engine.

# currently
$models = User::search('iphone')->get(); // search engine returns (2,3,1)
echo $models[0]->id // 1
echo $models[1]->id // 2
echo $models[2]->id // 3

# my proposal 
$models = User::search('iphone')->get(); // search engine returns (2,3,1)
echo $models[0]->id // 2
echo $models[1]->id // 3
echo $models[2]->id // 1

@obby-xiang
Copy link

obby-xiang commented Feb 6, 2019

AlgoliaEngine::map

        if (count($results['hits']) === 0) {
            return $model->newCollection();
        }

        $objectIds = collect($results['hits'])->pluck('objectID')->values()->all();

        return $model->getScoutModelsByIds(
            $builder, $objectIds
        )->filter(function ($model) use ($objectIds) {
            return in_array($model->getScoutKey(), $objectIds);
        })->sort(function ($modelA, $modelB) use ($objectIds) {
            //
            // sort
            //
            $indexA = array_search($modelA->getScoutKey(), $objectIds);
            $indexB = array_search($modelB->getScoutKey(), $objectIds);
            return $indexA == $indexB ? 0 : ($indexA < $indexB ? -1 : 1);
        });
        

@devinfd
Copy link

devinfd commented Mar 6, 2019

I can second this bug and I hope that you consider it critical. My search results are now in a seemingly random order. So basically the search results are useless. I can't see where this bug was introduced so I can't recommend a change. Sorry

@devinfd
Copy link

devinfd commented Mar 6, 2019

fyi: I had to implement an immediate fix so I overrode getScoutModelsByIds in my model with

return $query->whereIn(
            $this->getScoutKeyName(), $ids
        )->orderByRaw('FIELD('.$this->getScoutKeyName().', '.implode(', ', $ids).')')->get();

It's sloppy but it works

@nunomaduro
Copy link
Member Author

@devinfd On Scout Extended this problem do not exists. Can you try see this source code https://github.com/algolia/scout-extended/blob/7935885de233a1d651e46f621b2c7ccce64da3fc/src/Searchable/ModelsResolver.php#L87 and perform a pull request to Laravel Scout?

@devinfd
Copy link

devinfd commented Mar 7, 2019

I can try that at some point. I'm away at conference and cant dedicate the time for at least a week.

@ChrisThompsonTLDR
Copy link
Contributor

ChrisThompsonTLDR commented Mar 15, 2019

This should fix the issue.

    public function map(Builder $builder, $results, $model)
    {
        if (count($results['hits']) === 0) {
            return $model->newCollection();
        }

        $objectIds = collect($results['hits'])->pluck('objectID')->values()->all();

        return $model->getScoutModelsByIds(
                $builder, $objectIds
            )->filter(function ($model) use ($objectIds) {
                return in_array($model->getScoutKey(), $objectIds);
            })->sortBy(function($model) use ($objectIds) {
                return array_search($model->getScoutKey(), $objectIds);
            });
    }

@driesvints
Copy link
Member

PR Made: #369

@devinfd
Copy link

devinfd commented Apr 1, 2019

thank you @driesvints !

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

Successfully merging a pull request may close this issue.

5 participants