lugensa / scorched

Sunburnt offspring solr client
MIT License
27 stars 19 forks source link

Fixes issue #45 by using attributes of the reponse #46

Closed mlissner closed 7 years ago

mlissner commented 7 years ago

This fixes issue 45 so that the __len__ method uses attributes of the Solr response instead of counting the number of rows returned. In the old method, if rows=0, the SolrResponse would have a __len__ of zero. This made pagination and other things difficult. Even if rows=20, that'd be the highest length that the SolrResponse would have.

In this fix, the number of groups or the number of results is returned instead.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 95.37% when pulling fefbd7724bc863b734b8909736fb665c287afa04 on mlissner:issue-45 into 2c26c44a2f75de184c7d51fc9d20ef5fbedccece on lugensa:master.

delijati commented 7 years ago

Hmm is this breaking compatibility for you @lujh

lujh commented 7 years ago

@delijati you are right and actually I have some difficulties to understand the reasoning for this change. If one wants to simply iterate over the result, __len__ is used. @mlissner What would happen if result has only 30 docs, but numFound would be 200?

Also the information of numFound is easily found, how would one find the length of the returned batch? The batch-size is changeable in the Solr config.

mlissner commented 7 years ago

Shoot, yeah, this doesn't work like I wanted it to. On the second page of results, no items appear.

The reason I'm running into this is because I'm trying to use django paginators on a SolrResponse object.

Django paginator has this code for figuring out how long an iterable is:

@cached_property
def count(self):
    """
    Returns the total number of objects, across all pages.
    """
    try:
        return self.object_list.count()
    except (AttributeError, TypeError):
        # AttributeError if object_list has no count() method.
        # TypeError if object_list.count() requires arguments
        # (i.e. is of type list).
        return len(self.object_list)

So when we pass a SolrResponse to a Django paginator, there's no count() method, so we get the AttributeError. That takes the len of the SolrResponse, which returns 20, instead of the numFound value.

What's a good solution for using django pagination with scorched?

lujh commented 7 years ago

I would not mind a method count on the SolrResponse class. I think the django count comes from database result sets, so it's a bit slippery to start the RDB-response interface, but count() seems innocent enough.

mlissner commented 7 years ago

Great! I'll take a stab at this and see if I can get a coherent fix together. Thanks for the quick response and for catching my big error above!

mlissner commented 7 years ago

I looked at this some more. Adding a count method to the SolrResponse turns out to be a bad idea -- it works, but then the __getitem__ method doesn't work with pagination. So, my conclusion is that paginating the SolrResponse object is the wrong approach.

I think the best fix here, though, is to add count to the SolrSearch or BaseSearch object. That makes pagination work smoothly. Any thoughts on which object it should go on?

delijati commented 7 years ago

BaseSearch seams ok as MltSearch has also numFound

mlissner commented 7 years ago

Closing this out, so we can be more focused in #47. Thanks for the help working this one out.