From: Mike Bayer Date: Tue, 22 Sep 2020 14:26:35 +0000 (-0400) Subject: Deprecate negative slice indexes X-Git-Tag: rel_1_4_0b1~81^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=96888aee7611c15532af2ae47e567f68c28b2474;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 --- diff --git a/doc/build/changelog/unreleased_14/5606.rst b/doc/build/changelog/unreleased_14/5606.rst new file mode 100644 index 0000000000..0ce9ebd5fd --- /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 af67b47726..48161a256f 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 c6e6f64666..42987932d5 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 27b14d95bc..c8d639e8c9 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 532d0aa10d..bcba1f0315 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 836d9f88cc..5f18b9bce9 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 c5cf4a2c07..0bca2f975b 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 cee583ffbe..31643e5ff5 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. @@ -2471,30 +2505,6 @@ class SliceTest(QueryTest): self.assert_sql(testing.db, lambda: q[-2:-5], []) - 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[:],