Django

Code

Ticket #6997 (closed: fixed)

Opened 8 months ago

Last modified 4 months ago

Paginator broken for one element and allow_empty_first_page=False

Reported by: framos Assigned to: simeon
Milestone: 1.0 Component: Core framework
Version: SVN Keywords:
Cc: Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

I'm using the object_list generic view like this: 

def galleryitem_list(request, shop, page):
    queryset = GalleryItem.objects.filter(shop__slug=shop)
    kwargs = {
        'paginate_by': 10,
        'page': page,
        'template_object_name': 'galleryitem',
        'allow_empty': False,
    }
    return list_detail.object_list(request, queryset, **kwargs)

I want to get an Http404 when the queryset is empty, but I'm getting it also when the queryset has exactly one object.

This fails in all my object_list views in the project, not just this one.

I've tracked down the problem to the line 46 of paginator.py, where hits is 0 instead of 1.

Attachments

paginator.diff (0.5 kB) - added by siudesign on 04/15/08 06:03:17.
paginator2.diff (1.4 kB) - added by simeon on 07/18/08 16:23:05.
Updated patch to hande orphans param, match svn head and add tests

Change History

04/10/08 07:17:12 changed by framos

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

I made an error when linking to the paginator.py file. I intended to link to line 46 inside the file, but ended up linking to revision 46, so the link is broken. This is the right link.

04/15/08 04:05:34 changed by siudesign

  • owner changed from nobody to siudesign.
  • summary changed from Paginator broken for one element and allow_empty_first_page=True to Paginator broken for one element and allow_empty_first_page=False.

04/15/08 06:03:17 changed by siudesign

  • attachment paginator.diff added.

04/15/08 06:19:07 changed by siudesign

  • has_patch set to 1.

The problem is that the _get_num_pages method in Paginator currently returns 0 if your query returns 1 result and has allow_empty=False set. This returns the number to the validate_number method, which then throws an "Invalid Page" exception resulting in the 404 that you see, regardless of whether 0 or 1 results have been returned.

This is caused by the following line of code, which takes a number so in this case '1' and sets hits to '0' 

 hits = self.count - 1 - self.orphans

The offending code however is this, which sees that hits are equal to 0 (even when there is 1 result) and that allow_empty=False and then goes ahead and sets num_pages to 0, which is fine if there truly is no matching query, but not if the query returns one result. 

if hits == 0 and not self.allow_empty_first_page:
    self._num_pages = 0

You would not get the problem if you had allow_empty set to true, as there is code in the validate_number method which takes that variation of the setting into account.

My solution, in order to have minimal impact is to set self.num_pages to self.count, which by the point it enters the code above, can only be 1 or 0.

It works on my machine for all cases (0 results, 1 result, more than 1 result, allow_empty=True, allow_empty=False) etc.

04/15/08 10:24:58 changed by framos

siudesign, the patch works for me too, at least in the tests I've done. Good work!

I think more unit tests for the Paginator may be needed if a bug like this has been undetected until now.

06/14/08 08:23:44 changed by telenieko

  • needs_tests set to 1.
  • stage changed from Unreviewed to Accepted.

Could you add a TestCase? on the patch that reproduces the bug (and demostrates it's fixed)? If not, please unassign the ticket.

07/18/08 16:22:06 changed by simeon

  • owner changed from siudesign to simeon.
  • status changed from new to assigned.
  • needs_tests deleted.

Added tests which fail against svn head but suceed with paginator2.diff applied. The patch also needed to handle the orphan parameter so I rewrote the method... In the first patch, consider the case where orphans = 2 and self.count = 1. self.num_pages would be set to 1 even though based on the orphans setting we should have an empty page...

07/18/08 16:23:05 changed by simeon

  • attachment paginator2.diff added.

Updated patch to hande orphans param, match svn head and add tests

07/18/08 17:49:50 changed by mtredinnick

I might be missing something, but isn't this replacing one problem with another one? If self.count is 1 and self.orphans is 2, it looks like the page count will be 0, when it should be 1.

07/28/08 00:30:35 changed by gwilson

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [8129]) Fixed #6997 -- Corrected num_pages calculation when one item is in the object list and allow_empty_first_page=False, thanks to framos for the report. Also, made Page's start_index() return 0 if there are no items in the object list (previously it was returning 1, but now it is consistent with end_index()). As an added bonus, I threw in quite a few pagination tests.

07/28/08 00:33:21 changed by gwilson

  • keywords deleted.
  • milestone set to 1.0.

Add/Change #6997 (Paginator broken for one element and allow_empty_first_page=False)




Change Properties
Action