Opened 7 years ago

Closed 7 years ago

Last modified 3 years ago

#6997 closed (fixed)

Paginator broken for one element and allow_empty_first_page=False

Reported by: framos 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: UI/UX:

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

Attachments (2)

paginator.diff (517 bytes) - added by siudesign 7 years ago.
paginator2.diff (1.4 KB) - added by simeon 7 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 7 years ago by framos

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

comment:2 Changed 7 years ago 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

Changed 7 years ago by siudesign

comment:3 Changed 7 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 7 years ago 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.

comment:5 Changed 7 years ago by telenieko

  • Needs tests set
  • Triage 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.

comment:6 Changed 7 years ago by simeon

  • Needs tests unset
  • Owner changed from siudesign to simeon
  • Status changed from new to assigned

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 7 years ago by simeon

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

comment:7 Changed 7 years ago 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.

comment:8 Changed 7 years ago by gwilson

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 7 years ago by gwilson

  • Keywords paginator removed
  • milestone set to 1.0

comment:10 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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