Use ternary when running conditional with Query._offset

Fixed bug where using :meth:`.Query.first` or a slice expression in
conjunction with a query that has an expression based "offset" applied
would raise TypeError, due to an "or" conditional against "offset" that did
not expect it to be a SQL expression as opposed to an integer or None.

Fixes: #4803
Change-Id: I56b97a5d23cb45427a27a90ab557fa1ac5c6739e

diff --git a/doc/build/changelog/unreleased_13/4803.rst b/doc/build/changelog/unreleased_13/4803.rst
new file mode 100644 (file)
index 0000000..317816e
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/4803.rst
@@ -0,0 +1,9 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4803
+
+    Fixed bug where using :meth:`.Query.first` or a slice expression in
+    conjunction with a query that has an expression based "offset" applied
+    would raise TypeError, due to an "or" conditional against "offset" that did
+    not expect it to be a SQL expression as opposed to an integer or None.
+

diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py
index 4b176cb55891a8e3b3d0f386484f234529ba6be1..936929703303516515deda696bc3026472ff3ada 100644 (file)
--- a/lib/sqlalchemy/orm/query.py
+++ b/lib/sqlalchemy/orm/query.py
@@ -3071,14 +3071,18 @@ class Query(object):
 
         """
         if start is not None and stop is not None:
-            self._offset = (self._offset or 0) + start
+            self._offset = self._offset if self._offset is not None else 0
+            if start != 0:
+                self._offset += start
             self._limit = stop - start
         elif start is None and stop is not None:
             self._limit = stop
         elif start is not None and stop is None:
-            self._offset = (self._offset or 0) + start
+            self._offset = self._offset if self._offset is not None else 0
+            if start != 0:
+                self._offset += start
 
-        if self._offset == 0:
+        if isinstance(self._offset, int) and self._offset == 0:
             self._offset = None
 
     @_generative(_no_statement_condition)

diff --git a/lib/sqlalchemy/testing/requirements.py b/lib/sqlalchemy/testing/requirements.py
index 04537905b298ba3851a3088163849fbf3fdc6f5e..62e600eaa60a630a4f5b9e05a852b28514625544 100644 (file)
--- a/lib/sqlalchemy/testing/requirements.py
+++ b/lib/sqlalchemy/testing/requirements.py
@@ -111,6 +111,15 @@ class SuiteRequirements(Requirements):
 
         return exclusions.open()
 
+    @property
+    def sql_expression_limit_offset(self):
+        """target database can render LIMIT and/or OFFSET with a complete
+        SQL expression, such as one that uses the addition operator.
+        parameter
+        """
+
+        return exclusions.open()
+
     @property
     def parens_in_union_contained_select_w_limit_offset(self):
         """Target database must support parenthesized SELECT in UNION

diff --git a/test/orm/test_query.py b/test/orm/test_query.py
index cc1f79413eba9684eb49ad10aba95a737fe3391a..b9098bac13f59454a217852e8d907700e8e9772f 100644 (file)
--- a/test/orm/test_query.py
+++ b/test/orm/test_query.py
@@ -2417,6 +2417,8 @@ class ComparatorTest(QueryTest):
 
 # more slice tests are available in test/orm/generative.py
 class SliceTest(QueryTest):
+    __dialect__ = "default"
+
     def test_first(self):
         User = self.classes.User
 
@@ -2519,6 +2521,84 @@ class SliceTest(QueryTest):
             ],
         )
 
+    def test_first_against_expression_offset(self):
+        User = self.classes.User
+
+        sess = create_session()
+        q = sess.query(User).order_by(User.id).offset(literal_column("2"))
+
+        self.assert_sql(
+            testing.db,
+            q.first,
+            [
+                (
+                    "SELECT users.id AS users_id, users.name AS users_name "
+                    "FROM users ORDER BY users.id "
+                    "LIMIT :param_1 OFFSET 2",
+                    [{"param_1": 1}],
+                )
+            ],
+        )
+
+    @testing.requires.sql_expression_limit_offset
+    def test_full_slice_against_expression_offset(self):
+        User = self.classes.User
+
+        sess = create_session()
+        q = sess.query(User).order_by(User.id).offset(literal_column("2"))
+
+        self.assert_sql(
+            testing.db,
+            lambda: q[2:5],
+            [
+                (
+                    "SELECT users.id AS users_id, users.name AS users_name "
+                    "FROM users ORDER BY users.id "
+                    "LIMIT :param_1 OFFSET 2 + :2_1",
+                    [{"param_1": 3, "2_1": 2}],
+                )
+            ],
+        )
+
+    def test_full_slice_against_integer_offset(self):
+        User = self.classes.User
+
+        sess = create_session()
+        q = sess.query(User).order_by(User.id).offset(2)
+
+        self.assert_sql(
+            testing.db,
+            lambda: q[2:5],
+            [
+                (
+                    "SELECT users.id AS users_id, users.name AS users_name "
+                    "FROM users ORDER BY users.id "
+                    "LIMIT :param_1 OFFSET :param_2",
+                    [{"param_1": 3, "param_2": 4}],
+                )
+            ],
+        )
+
+    @testing.requires.sql_expression_limit_offset
+    def test_start_slice_against_expression_offset(self):
+        User = self.classes.User
+
+        sess = create_session()
+        q = sess.query(User).order_by(User.id).offset(literal_column("2"))
+
+        self.assert_sql(
+            testing.db,
+            lambda: q[2:],
+            [
+                (
+                    "SELECT users.id AS users_id, users.name AS users_name "
+                    "FROM users ORDER BY users.id "
+                    "LIMIT -1 OFFSET 2 + :2_1",
+                    [{"2_1": 2}],
+                )
+            ],
+        )
+
 
 class FilterTest(QueryTest, AssertsCompiledSQL):
     __dialect__ = "default"

diff --git a/test/requirements.py b/test/requirements.py
index fefe388688cbce77f46b2fc8de74550c96278cd4..49d29a949bbb38a48ac37ebfa06b20325e032e84 100644 (file)
--- a/test/requirements.py
+++ b/test/requirements.py
@@ -617,6 +617,16 @@ class DefaultRequirements(SuiteRequirements):
         equivalent to a result set."""
         return fails_if(["sybase"], "no support for OFFSET or equivalent")
 
+    @property
+    def sql_expression_limit_offset(self):
+        return (
+            fails_if(
+                ["mysql"],
+                "MySQL can't accommodate full expressions in OFFSET or LIMIT",
+            )
+            + self.offset
+        )
+
     @property
     def window_functions(self):
         return only_if(