Merge pull request #558 from davidism/no_negative_offset

set page and per_page defaults when invalid

CHANGES.rst

@@ -16,11 +16,14 @@ In development
   flushed yet. (`#555`_)
 - Allow specifying a ``max_per_page`` limit for pagination, to avoid users
   specifying high values in the request args. (`#542`_)
+- For ``paginate`` with ``error_out=False``, the minimum value for ``page`` is
+  1 and ``per_page`` is 0. (`#558`_)
 
 .. _#542: https://github.com/mitsuhiko/flask-sqlalchemy/pull/542
 .. _#551: https://github.com/mitsuhiko/flask-sqlalchemy/pull/551
 .. _#555: https://github.com/mitsuhiko/flask-sqlalchemy/pull/555
 .. _#556: https://github.com/mitsuhiko/flask-sqlalchemy/pull/556
+.. _#558: https://github.com/mitsuhiko/flask-sqlalchemy/pull/558
 
 
 Version 2.3.0

flask_sqlalchemy/__init__.py

@@ -430,16 +430,20 @@ def first_or_404(self):
     def paginate(self, page=None, per_page=None, error_out=True, max_per_page=None):
         """Returns ``per_page`` items from page ``page``.
 
-        If no items are found and ``page`` is greater than 1, or if page is
-        less than 1, it aborts with 404.
-        This behavior can be disabled by passing ``error_out=False``.
-
         If ``page`` or ``per_page`` are ``None``, they will be retrieved from
-        the request query.
-        If the values are not ints and ``error_out`` is ``True``, it aborts
-        with 404.
-        If there is no request or they aren't in the query, they default to 1
-        and 20 respectively.
+        the request query. If ``max_per_page`` is specified, ``per_page`` will
+        be limited to that value. If there is no request or they aren't in the
+        query, they default to 1 and 20 respectively.
+
+        When ``error_out`` is ``True`` (default), the following rules will
+        cause a 404 response:
+
+        * No items are found and ``page`` is not 1.
+        * ``page`` is less than 1, or ``per_page`` is negative.
+        * ``page`` or ``per_page`` are not ints.
+
+        When ``error_out`` is ``False``, ``page`` and ``per_page`` default to
+        1 and 20 respectively.
 
         Returns a :class:`Pagination` object.
         """
@@ -472,8 +476,17 @@ def paginate(self, page=None, per_page=None, error_out=True, max_per_page=None):
         if max_per_page is not None:
             per_page = min(per_page, max_per_page)
 
-        if error_out and page < 1:
-            abort(404)
+        if page < 1:
+            if error_out:
+                abort(404)
+            else:
+                page = 1
+
+        if per_page < 0:
+            if error_out:
+                abort(404)
+            else:
+                per_page = 20
 
         items = self.limit(per_page).offset((page - 1) * per_page).all()
 

tests/test_pagination.py

@@ -1,3 +1,6 @@
+import pytest
+from werkzeug.exceptions import NotFound
+
 import flask_sqlalchemy as fsa
 
 
@@ -49,3 +52,19 @@ def test_query_paginate_more_than_20(app, db, Todo):
         db.session.commit()
 
     assert len(Todo.query.paginate(max_per_page=10).items) == 10
+
+
+def test_paginate_min(app, db, Todo):
+    with app.app_context():
+        db.session.add_all(Todo(str(x), '') for x in range(20))
+        db.session.commit()
+
+    assert Todo.query.paginate(error_out=False, page=-1).items[0].title == '0'
+    assert len(Todo.query.paginate(error_out=False, per_page=0).items) == 0
+    assert len(Todo.query.paginate(error_out=False, per_page=-1).items) == 20
+
+    with pytest.raises(NotFound):
+        Todo.query.paginate(page=0)
+
+    with pytest.raises(NotFound):
+        Todo.query.paginate(per_page=-1)