Deprecate negative slice indexes

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.

Fixes: #5606
Change-Id: I5f5dcf984a8f41ab3d0e233ef7553e77fd99a771

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[:],
             [
                 (