brewster / elastictastic

Object-document mapper and lightweight API adapter for ElasticSearch
MIT License
88 stars 13 forks source link

Model.all does not return an array of objects #27

Closed mrcasals closed 11 years ago

mrcasals commented 11 years ago

Expected behavior:

Product.all
# => [#<Product id:1, ...>, #<Product id: 2, ...>, ...]

Current behavior:

Product.all
# => Product:my_index
mrcasals commented 11 years ago

I searched the source and found the implementation of the .all method here:

https://github.com/brewster/elastictastic/blob/master/lib/elastictastic/scope.rb#L109-L111

But I don't understand why it fails to load all the objects. Any thought on where to look at? I'll try to make a pull request for this :)

Thanks! :)

outoftime commented 11 years ago

Hi,

This is very much a feature. The reason is that ElasticSearch, if you wish to scan over all of the results matching a query, does so using a cursor. So Elastictastic makes multiple batch requests and exposes an Enumerable object to make this transparent to the client. Unlike in, say, ActiveRecord, where batch loading is a performance optimization, in Elastictastic it is a requirement.

The object returned by Model.all acts more or less like an Array, although it may be confusing that it does not look like one in the console. I'm ambivalent concerning the implementation of #inspect on the result object; I'd be willing to change it to conform to the principle of least surprise, if that is the primary cause of confusion here.

outoftime commented 11 years ago

More on the way ES implements result scrolling here: http://www.elasticsearch.org/guide/reference/api/search/search-type.html

Scroll down to the "Scan" type.

mrcasals commented 11 years ago

The problem I found here is that when using Product.all I expected to get an array of objects, just like in ActiveRecord. This way I can use Product.all.should include @product in RSpec tests. That's why I opened the issue, because I expected to see the array instead of the name of the class returned by #inspect.

I guess I can use the .entries method provided by Enumerable then, it's just that I'm too used to ActiveRecord :)

Thanks for the quick feedback!

outoftime commented 11 years ago

Does the RSpec use case not work? If you can't perform Product.all.should include(@product), that's a legit bug.

mrcasals commented 11 years ago

Oh wait, it is actually, but as #inspect returns Model:index I didn't see it in the .should include test failures... My bad, looks like elastictastic is working well, but it is difficult to debug...

mrcasals commented 11 years ago

Conclusion: can we change what #inspect returns to show the object entries please? :) I can open a PR tomorrow if you want :)

outoftime commented 11 years ago

Absolutely -- that's exactly the route I went with Cequel when approaching the same problem. Will await your pull request.