Opened 11 years ago

Closed 11 years ago

Last modified 7 years ago

#6997 closed (fixed)

Paginator broken for one element and allow_empty_first_page=False

Reported by: Fidel Ramos Owned by: simeon
Component: Core (Other) Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


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 [source:django/core/ line 46 of], where hits is 0 instead of 1.

Attachments (2)

paginator.diff (517 bytes) - added by siudesign 11 years ago.
paginator2.diff (1.4 KB) - added by simeon 11 years ago.
Updated patch to hande orphans param, match svn head and add tests

Download all attachments as: .zip

Change History (12)

comment:1 Changed 11 years ago by Fidel Ramos

I made an error when linking to the 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.

comment:2 Changed 11 years ago by siudesign

Owner: changed from nobody to siudesign
Summary: Paginator broken for one element and allow_empty_first_page=TruePaginator broken for one element and allow_empty_first_page=False

Changed 11 years ago by siudesign

Attachment: paginator.diff added

comment:3 Changed 11 years ago by siudesign

Has patch: set

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.

comment:4 Changed 11 years ago by Fidel Ramos

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.

comment:5 Changed 11 years ago by Marc Fargas

Needs tests: set
Triage Stage: UnreviewedAccepted

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

comment:6 Changed 11 years ago by simeon

Needs tests: unset
Owner: changed from siudesign to simeon
Status: newassigned

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...

Changed 11 years ago by simeon

Attachment: paginator2.diff added

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

comment:7 Changed 11 years ago by Malcolm Tredinnick

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.

comment:8 Changed 11 years ago by Gary Wilson

Resolution: fixed
Status: assignedclosed

(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.

comment:9 Changed 11 years ago by Gary Wilson

Keywords: paginator removed
milestone: 1.0

comment:10 Changed 7 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

Note: See TracTickets for help on using tickets.
Back to Top