ChristopherRabotin / bungiesearch

UNMAINTAINED CODE -- Elasticsearch-dsl-py django wrapper with mapping generator
BSD 3-Clause "New" or "Revised" License
68 stars 20 forks source link

Removed redundant slicing that caused incorrect results #70

Closed JoshStegmaier closed 9 years ago

JoshStegmaier commented 9 years ago

Currently slicing on a results set causes the results to be sliced twice: once by elasticsearch_dsl and once by bungiesearch itself. This results in incorrectly, unexpected results being returned for many slicing attempts.

For instance, results[10:15] would cause elasticsearch_dsl to grab results 10 through 15 (for five total results), which bundiesearch then slices from 10 to 15, resulting in zero results.

This pull request removes the redundant slicing, allowing the correct results to be returned.

ChristopherRabotin commented 9 years ago

Thanks! That's actually a pretty big issue which hasn't popped up yet.

Can you update the tests.core.test_bungiesearch test_concat_queries function to the following code, which will test and allow for concatenation of queries, but only mapped queries?

    def test_concat_queries(self):
        items = Article.objects.bsearch_title_search('title')[::False] + NoUpdatedField.objects.search.query('match', title='My title')[::False]
        for item in items:
            model = item._meta.proxy_for_model if item._meta.proxy_for_model else type(item)
            self.assertIn(model, [Article, NoUpdatedField], 'Got an unmapped item ({}), or an item with an unexpected mapping.'.format(type(item)))
JoshStegmaier commented 9 years ago

Done.

When running the tests, I did run into a problem: the requirements.txt file includes bungiesearch, but bungiesearch isn't available through pip, so the entire install (pip instal -r requirements.txt) fails. I simply commented out that line to allow the rest of the packages to install, then installed bungiesearch manually as the documentation says to (besides, in this case, I don't think you'd want to use the bungiesearch from pip even if it was available.)

I didn't commit that change, since bungiesearch is a requirement of the tests anyway, and I didn't know how you wanted to handle it, if it needs to be changed at all.

ChristopherRabotin commented 9 years ago

Thanks!

Yes, it's not yet on pip, and it probably should be. You have to install it locally using pip and then run the tests.