Skip to content
This repository has been archived by the owner on Feb 5, 2023. It is now read-only.

Return empty collection if models are not found. #36

Closed
sasin91 opened this issue Feb 2, 2017 · 4 comments
Closed

Return empty collection if models are not found. #36

sasin91 opened this issue Feb 2, 2017 · 4 comments
Labels

Comments

@sasin91
Copy link

sasin91 commented Feb 2, 2017

I'm not sure if this is a problem that's working as intended..

However, when searching and the map method on ElasticSearchEngine is called, it only checks the results from elasticsearch..

What if the model was manually deleted from the database?
That would make it unable to find the $hit matching the id and generate a error 500 :)

@ErickTamayo
Copy link
Owner

ErickTamayo commented Feb 2, 2017

If you delete manually your model from the DB you should get rid of the the document in Elasticsearch as well otherwise you will have an inconsistency. I don't know what's the behavior with the Algolia driver in the official package, have to check.

Also I don't know if "hidding" this exception is healthy.

@sasin91
Copy link
Author

sasin91 commented Feb 3, 2017

In Algolia, it checks if the key isset before returning, avoiding Undefined offset errors, which is the actual error here .. :)

to mimic that behaviour,
map method @ElasticsearchEngine could return a collection like this:
return Collection::make($results['hits']['hits'])->map(function ($hit) use ($model, $models) { return isset($models[$hit['_id']]) ? $models[$hit['_id']] : null; })->filter()->values();

@loranger
Copy link

This behaviour would also apply for soft deleted models which are actually imported, but cannot be retrieved and throws Undefined Offset exception 🤔

@ErickTamayo
Copy link
Owner

Yes it should not crash on soft deletes. I will have a look at it soon.

ErickTamayo added a commit that referenced this issue Mar 31, 2017
…arch)

`within` now is being taken into account when specified #31
Now if the record is not found on the db will not throw undefined offset #36
Implemented orderBy #46
@ErickTamayo ErickTamayo mentioned this issue Mar 31, 2017
ErickTamayo added a commit that referenced this issue Mar 31, 2017
bumping to php 5.6.6 (Earliest version that can support php elasticsearch) (#48)
`within` now is being taken into account when specified #31
Now if the record is not found on the db will not throw undefined offset #36
Implemented orderBy #46
sonnysantino pushed a commit to sonnysantino/laravel-scout-elastic that referenced this issue Jan 17, 2020
* Updating 0.0.1 (ErickTamayo#8)

* fixing config key + updating the Usage on README (ErickTamayo#6)

* Rename ElasticsearchEngineTets.php to ElasticsearchEngineTest.php (ErickTamayo#7)

* Updating 0.0.1 (ErickTamayo#11)

* fixing config key + updating the Usage on README (ErickTamayo#6)

* Rename ElasticsearchEngineTets.php to ElasticsearchEngineTest.php (ErickTamayo#7)

* Update ElasticsearchEngine.php (ErickTamayo#10)

* Updating 0.0.1 (ErickTamayo#14)

* fixing config key + updating the Usage on README (ErickTamayo#6)

* Rename ElasticsearchEngineTets.php to ElasticsearchEngineTest.php (ErickTamayo#7)

* Update ElasticsearchEngine.php (ErickTamayo#10)

* optimized creation of params array for better performance when dealing with huge amount of models (ErickTamayo#13)

* Make stable (ErickTamayo#18) (ErickTamayo#19)

* Bumped Scout version to v2.0.x
Bumped elasticsearch/elasticsearch to v5.0

* Bumped Scout version to 3.0

* Fix config name (ErickTamayo#35)

* v3.0.2

bumping to php 5.6.6 (Earliest version that can support php elasticsearch) (ErickTamayo#48)
`within` now is being taken into account when specified ErickTamayo#31
Now if the record is not found on the db will not throw undefined offset ErickTamayo#36
Implemented orderBy ErickTamayo#46

* Update ElasticsearchProvider.php (ErickTamayo#54)

use `app` instead of `resolve` to make lumen 5.4 compatible.

`app` and `resolve` are identical since 5.4, see https://github.com/laravel/framework/blob/5.4/src/Illuminate/Foundation/helpers.php#L690

* chain `values` method in `map` function (ErickTamayo#66)

In 
```php
    collect($results['hits']['hits'])->map(function ($hit) use ($model, $models) {
            return isset($models[$hit['_id']]) ? $models[$hit['_id']] : null;
        })->filter();
```
would generate some holds which would cause `paginate` results objects rather than list.

* Support for "WHERE IN" searches if array is passed to `where` (ErickTamayo#55)

Right now passing an array results in: "illegal_state_exception: Can't get text on a START_ARRAY"
Solution based on http://stackoverflow.com/a/40737488/7362396

* Execute the callback that can be passed to the Builder in performSearch (ErickTamayo#56)

Context: laravel/scout#111

Implementation analogical to the official AlgoliaEngine implementation: https://github.com/laravel/scout/blob/v3.0.3/src/Engines/AlgoliaEngine.php#L112-L119

* Update ElasticsearchEngine.php (ErickTamayo#51)

Predefine elastic variable.

* Enable Laravel5.5 package Auto-Discovery (ErickTamayo#81)

* Removed count function. Fixes ErickTamayo#80 (ErickTamayo#84)

Since php 7.2, count a number will generate a warning. Because
$results['hits']['total'] is a number we can just compare it to zero.

`if ($results['hits']['total'] === 0)`

* Allow installation with Scout 4 (ErickTamayo#99)

* Compatibility with Scout 5 (ErickTamayo#106)

* Compatibility with Scout 5

No "cross-installation" possible due to changed method signature

Pass around new $builder var

Also: Use getScoutModelsByIds/getScoutKey

Inspired by laravel/scout@4.0...5.0#diff-f2faa85f27580a9b67d73a8a273bfef1 / laravel/scout@9f06b9c

* Remove PHP5 as laravel/scout^5 / laravel5.6 also removed it

* Added support for Laravel Scout 6.0 and 7.0, Laravel 5.8 (ErickTamayo#119)

* Elasticsearch downgrade

Co-authored-by: Erick Tamayo <ErickTamayo@users.noreply.github.com>
Co-authored-by: Till Krüss <tillkruss@users.noreply.github.com>
Co-authored-by: Joey <joey.xf@gmail.com>
Co-authored-by: Tobias Kronthaler <kronthto@users.noreply.github.com>
Co-authored-by: Nirav <nirav.t.prajapati21@gmail.com>
Co-authored-by: Camilo <camilo.castro.cabrera@gmail.com>
Co-authored-by: Semyon Chetvertnyh <semyon.chetvertnyh@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants