Merge "Deprecate negative slice indexes"

8 files changed, 198 insertions, 36 deletions

diff --git a/doc/build/changelog/unreleased_14/5606.rst b/doc/build/changelog/unreleased_14/5606.rst
new file mode 100644
index 000000000..0ce9ebd5f
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/5606.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: change, orm
+    :tickets: 5606
+
+    The "slice index" feature used by :class:`_orm.Query` as well as by the
+    dynamic relationship loader will no longer accept negative indexes in
+    SQLAlchemy 2.0.  These operations do not work efficiently and load the
+    entire collection in, which is both surprising and undesirable.   These
+    will warn in 1.4 unless the :paramref:`_orm.Session.future` flag is set in
+    which case they will raise IndexError.
+
diff --git a/lib/sqlalchemy/orm/dynamic.py b/lib/sqlalchemy/orm/dynamic.py
index af67b4772..48161a256 100644
--- a/lib/sqlalchemy/orm/dynamic.py
+++ b/lib/sqlalchemy/orm/dynamic.py
@@ -510,7 +510,9 @@ class AppenderQuery(Generative):
                 attributes.PASSIVE_NO_INITIALIZE,
             ).indexed(index)
         else:
-            return orm_util._getitem(self, index)
+            return orm_util._getitem(
+                self, index, allow_negative=not self.session.future
+            )
 
     @_generative
     def limit(self, limit):
diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py
index c6e6f6466..42987932d 100644
--- a/lib/sqlalchemy/orm/query.py
+++ b/lib/sqlalchemy/orm/query.py
@@ -2495,7 +2495,11 @@ class Query(
         self._compile_options += {"_enable_single_crit": False}
 
     def __getitem__(self, item):
-        return orm_util._getitem(self, item)
+        return orm_util._getitem(
+            self,
+            item,
+            allow_negative=not self.session or not self.session.future,
+        )
 
     @_generative
     @_assertions(_no_statement_condition)
diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py
index 27b14d95b..c8d639e8c 100644
--- a/lib/sqlalchemy/orm/util.py
+++ b/lib/sqlalchemy/orm/util.py
@@ -1811,12 +1811,26 @@ def randomize_unitofwork():
     ) = session.set = mapper.set = dependency.set = RandomSet
 
 
-def _getitem(iterable_query, item):
+def _getitem(iterable_query, item, allow_negative):
     """calculate __getitem__ in terms of an iterable query object
     that also has a slice() method.
 
     """
 
+    def _no_negative_indexes():
+        if not allow_negative:
+            raise IndexError(
+                "negative indexes are not accepted by SQL "
+                "index / slice operators"
+            )
+        else:
+            util.warn_deprecated_20(
+                "Support for negative indexes for SQL index / slice operators "
+                "will be "
+                "removed in 2.0; these operators fetch the complete result "
+                "and do not work efficiently."
+            )
+
     if isinstance(item, slice):
         start, stop, step = util.decode_slice(item)
 
@@ -1827,11 +1841,10 @@ def _getitem(iterable_query, item):
         ):
             return []
 
-        # perhaps we should execute a count() here so that we
-        # can still use LIMIT/OFFSET ?
         elif (isinstance(start, int) and start < 0) or (
             isinstance(stop, int) and stop < 0
         ):
+            _no_negative_indexes()
             return list(iterable_query)[item]
 
         res = iterable_query.slice(start, stop)
@@ -1841,6 +1854,7 @@ def _getitem(iterable_query, item):
             return list(res)
     else:
         if item == -1:
+            _no_negative_indexes()
             return list(iterable_query)[-1]
         else:
             return list(iterable_query[item : item + 1])[0]
diff --git a/test/orm/test_deprecations.py b/test/orm/test_deprecations.py
index 532d0aa10..bcba1f031 100644
--- a/test/orm/test_deprecations.py
+++ b/test/orm/test_deprecations.py
@@ -62,6 +62,7 @@ from sqlalchemy.testing.schema import Column
 from sqlalchemy.testing.schema import Table
 from . import _fixtures
 from .inheritance import _poly_fixtures
+from .test_dynamic import _DynamicFixture
 from .test_events import _RemoveListeners
 from .test_options import PathTest as OptionsPathTest
 from .test_query import QueryTest
@@ -88,6 +89,73 @@ class DeprecatedQueryTest(_fixtures.FixtureTest, AssertsCompiledSQL):
             "subquery object."
         )
 
+    def test_deprecated_negative_slices(self):
+        User = self.classes.User
+
+        sess = create_session()
+        q = sess.query(User).order_by(User.id)
+
+        with testing.expect_deprecated(
+            "Support for negative indexes for SQL index / slice operators"
+        ):
+            eq_(q[-5:-2], [User(id=7), User(id=8)])
+
+        with testing.expect_deprecated(
+            "Support for negative indexes for SQL index / slice operators"
+        ):
+            eq_(q[-1], User(id=10))
+
+        with testing.expect_deprecated(
+            "Support for negative indexes for SQL index / slice operators"
+        ):
+            eq_(q[-2], User(id=9))
+
+        with testing.expect_deprecated(
+            "Support for negative indexes for SQL index / slice operators"
+        ):
+            eq_(q[:-2], [User(id=7), User(id=8)])
+
+        # this doesn't evaluate anything because it's a net-negative
+        eq_(q[-2:-5], [])
+
+    def test_deprecated_negative_slices_compile(self):
+        User = self.classes.User
+
+        sess = create_session()
+        q = sess.query(User).order_by(User.id)
+
+        with testing.expect_deprecated(
+            "Support for negative indexes for SQL index / slice operators"
+        ):
+            self.assert_sql(
+                testing.db,
+                lambda: q[-5:-2],
+                [
+                    (
+                        "SELECT users.id AS users_id, users.name "
+                        "AS users_name "
+                        "FROM users ORDER BY users.id",
+                        {},
+                    )
+                ],
+            )
+
+        with testing.expect_deprecated(
+            "Support for negative indexes for SQL index / slice operators"
+        ):
+            self.assert_sql(
+                testing.db,
+                lambda: q[-5:],
+                [
+                    (
+                        "SELECT users.id AS users_id, users.name "
+                        "AS users_name "
+                        "FROM users ORDER BY users.id",
+                        {},
+                    )
+                ],
+            )
+
     def test_invalid_column(self):
         User = self.classes.User
 
@@ -721,6 +789,33 @@ class SelfRefFromSelfTest(fixtures.MappedTest, AssertsCompiledSQL):
         )
 
 
+class DynamicTest(_DynamicFixture, _fixtures.FixtureTest):
+    def test_negative_slice_access_raises(self):
+        User, Address = self._user_address_fixture()
+        sess = create_session(testing.db)
+        u1 = sess.get(User, 8)
+
+        with testing.expect_deprecated_20(
+            "Support for negative indexes for SQL index / slice"
+        ):
+            eq_(u1.addresses[-1], Address(id=4))
+
+        with testing.expect_deprecated_20(
+            "Support for negative indexes for SQL index / slice"
+        ):
+            eq_(u1.addresses[-5:-2], [Address(id=2)])
+
+        with testing.expect_deprecated_20(
+            "Support for negative indexes for SQL index / slice"
+        ):
+            eq_(u1.addresses[-2], Address(id=3))
+
+        with testing.expect_deprecated_20(
+            "Support for negative indexes for SQL index / slice"
+        ):
+            eq_(u1.addresses[:-2], [Address(id=2)])
+
+
 class FromSelfTest(QueryTest, AssertsCompiledSQL):
     __dialect__ = "default"
 
diff --git a/test/orm/test_dynamic.py b/test/orm/test_dynamic.py
index 836d9f88c..5f18b9bce 100644
--- a/test/orm/test_dynamic.py
+++ b/test/orm/test_dynamic.py
@@ -18,6 +18,7 @@ from sqlalchemy.testing import assert_raises
 from sqlalchemy.testing import assert_raises_message
 from sqlalchemy.testing import AssertsCompiledSQL
 from sqlalchemy.testing import eq_
+from sqlalchemy.testing import expect_raises_message
 from sqlalchemy.testing import is_
 from sqlalchemy.testing.assertsql import CompiledSQL
 from test.orm import _fixtures
@@ -158,7 +159,35 @@ class DynamicTest(_DynamicFixture, _fixtures.FixtureTest, AssertsCompiledSQL):
 
         eq_(u1.addresses[0], Address(id=2))
         eq_(u1.addresses[0:2], [Address(id=2), Address(id=3)])
-        eq_(u1.addresses[-1], Address(id=4))
+
+    def test_negative_slice_access_raises(self):
+        User, Address = self._user_address_fixture()
+        sess = create_session(testing.db, future=True)
+        u1 = sess.get(User, 8)
+
+        with expect_raises_message(
+            IndexError,
+            "negative indexes are not accepted by SQL index / slice operators",
+        ):
+            u1.addresses[-1]
+
+        with expect_raises_message(
+            IndexError,
+            "negative indexes are not accepted by SQL index / slice operators",
+        ):
+            u1.addresses[-5:-2]
+
+        with expect_raises_message(
+            IndexError,
+            "negative indexes are not accepted by SQL index / slice operators",
+        ):
+            u1.addresses[-2]
+
+        with expect_raises_message(
+            IndexError,
+            "negative indexes are not accepted by SQL index / slice operators",
+        ):
+            u1.addresses[:-2]
 
     def test_statement(self):
         """test that the .statement accessor returns the actual statement that
diff --git a/test/orm/test_generative.py b/test/orm/test_generative.py
index c5cf4a2c0..0bca2f975 100644
--- a/test/orm/test_generative.py
+++ b/test/orm/test_generative.py
@@ -57,8 +57,6 @@ class GenerativeQueryTest(fixtures.MappedTest):
         orig = query.all()
 
         assert query[1] == orig[1]
-        assert query[-4] == orig[-4]
-        assert query[-1] == orig[-1]
 
         assert list(query[10:20]) == orig[10:20]
         assert list(query[10:]) == orig[10:]
@@ -66,10 +64,9 @@ class GenerativeQueryTest(fixtures.MappedTest):
         assert list(query[:10]) == orig[:10]
         assert list(query[5:5]) == orig[5:5]
         assert list(query[10:40:3]) == orig[10:40:3]
-        assert list(query[-5:]) == orig[-5:]
-        assert list(query[-2:-5]) == orig[-2:-5]
-        assert list(query[-5:-2]) == orig[-5:-2]
-        assert list(query[:-2]) == orig[:-2]
+
+        # negative slices and indexes are deprecated and are tested
+        # in test_query.py and test_deprecations.py
 
         assert query[10:20][5] == orig[10:20][5]
 
diff --git a/test/orm/test_query.py b/test/orm/test_query.py
index cee583ffb..31643e5ff 100644
--- a/test/orm/test_query.py
+++ b/test/orm/test_query.py
@@ -61,6 +61,7 @@ from sqlalchemy.sql import expression
 from sqlalchemy.sql import operators
 from sqlalchemy.sql.selectable import LABEL_STYLE_TABLENAME_PLUS_COL
 from sqlalchemy.testing import AssertsCompiledSQL
+from sqlalchemy.testing import expect_raises_message
 from sqlalchemy.testing import fixtures
 from sqlalchemy.testing import is_
 from sqlalchemy.testing import is_false
@@ -2414,6 +2415,39 @@ class SliceTest(QueryTest):
             create_session().query(User).filter(User.id == 27).first() is None
         )
 
+    def test_negative_indexes_raise(self):
+        User = self.classes.User
+
+        sess = create_session(future=True)
+        q = sess.query(User).order_by(User.id)
+
+        with expect_raises_message(
+            IndexError,
+            "negative indexes are not accepted by SQL index / slice operators",
+        ):
+            q[-5:-2]
+
+        with expect_raises_message(
+            IndexError,
+            "negative indexes are not accepted by SQL index / slice operators",
+        ):
+            q[-1]
+
+        with expect_raises_message(
+            IndexError,
+            "negative indexes are not accepted by SQL index / slice operators",
+        ):
+            q[-5]
+
+        with expect_raises_message(
+            IndexError,
+            "negative indexes are not accepted by SQL index / slice operators",
+        ):
+            q[:-2]
+
+        # this doesn't evaluate anything because it's a net-negative
+        eq_(q[-2:-5], [])
+
     def test_limit_offset_applies(self):
         """Test that the expected LIMIT/OFFSET is applied for slices.
 
@@ -2473,30 +2507,6 @@ class SliceTest(QueryTest):
 
         self.assert_sql(
             testing.db,
-            lambda: q[-5:-2],
-            [
-                (
-                    "SELECT users.id AS users_id, users.name AS users_name "
-                    "FROM users ORDER BY users.id",
-                    {},
-                )
-            ],
-        )
-
-        self.assert_sql(
-            testing.db,
-            lambda: q[-5:],
-            [
-                (
-                    "SELECT users.id AS users_id, users.name AS users_name "
-                    "FROM users ORDER BY users.id",
-                    {},
-                )
-            ],
-        )
-
-        self.assert_sql(
-            testing.db,
             lambda: q[:],
             [
                 (