From: Mike Bayer Date: Wed, 1 Apr 2015 23:29:09 +0000 (-0400) Subject: - :class:`.Query` doesn't support joins, subselects, or special X-Git-Tag: rel_0_9_10~53 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=01a1c3256aeef7994dca5652e62d37af2c3055b6;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - :class:`.Query` doesn't support joins, subselects, or special FROM clauses when using the :meth:`.Query.update` or :meth:`.Query.delete` methods; instead of silently ignoring these fields if methods like :meth:`.Query.join` or :meth:`.Query.select_from` has been called, a warning is emitted. In 1.0.0b5 this will raise an error. Partial cherry pick from 5302bcebb8e18fdad7448ffb60d2a2017eab26c8. fixes #3349 - don't realy need _no_select_modifiers anymore --- diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst index 53c4eeaaab..32339a1544 100644 --- a/doc/build/changelog/changelog_09.rst +++ b/doc/build/changelog/changelog_09.rst @@ -14,6 +14,17 @@ .. changelog:: :version: 0.9.10 + .. change:: + :tags: bug, orm + :tickets: 3349 + + :class:`.Query` doesn't support joins, subselects, or special + FROM clauses when using the :meth:`.Query.update` or + :meth:`.Query.delete` methods; instead of silently ignoring these + fields if methods like :meth:`.Query.join` or + :meth:`.Query.select_from` has been called, a warning is emitted. + As of 1.0.0b5 this will raise an error. + .. change:: :tags: bug, mysql, pymysql :tickets: 3337 diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py index d6de31f95a..6f2c278948 100644 --- a/lib/sqlalchemy/orm/persistence.py +++ b/lib/sqlalchemy/orm/persistence.py @@ -20,6 +20,7 @@ from .. import sql, util, exc as sa_exc, schema from . import attributes, sync, exc as orm_exc, evaluator from .base import _state_mapper, state_str, _attr_as_key from ..sql import expression +from ..sql.base import _from_objects from . import loading @@ -874,6 +875,34 @@ class BulkUD(object): def __init__(self, query): self.query = query.enable_eagerloads(False) + self._validate_query_state() + + def _validate_query_state(self): + for attr, methname, notset, err in ( + ('_limit', 'limit()', None, True), + ('_offset', 'offset()', None, True), + ('_order_by', 'order_by()', False, True), + ('_group_by', 'group_by()', False, True), + ('_distinct', 'distinct()', False, True), + ( + '_from_obj', + 'join(), outerjoin(), select_from(), or from_self()', + (), False) + ): + if getattr(self.query, attr) is not notset: + if err: + raise sa_exc.InvalidRequestError( + "Can't call Query.update() or Query.delete() " + "when %s has been called" % + (methname, ) + ) + else: + util.warn( + "Can't call Query.update() or Query.delete() " + "when %s has been called. This will be an " + "exception in 1.0" % + (methname, ) + ) @property def session(self): @@ -900,6 +929,10 @@ class BulkUD(object): def _do_pre(self): query = self.query + + # the completion of the full Query statement here is wasteful; + # in 1.0 we will directly extract the target table without + # this step. self.context = context = query._compile_context() if len(context.statement.froms) != 1 or \ not isinstance(context.statement.froms[0], schema.Table): @@ -976,7 +1009,6 @@ class BulkUpdate(BulkUD): def __init__(self, query, values): super(BulkUpdate, self).__init__(query) - self.query._no_select_modifiers("update") self.values = values @classmethod @@ -1005,7 +1037,6 @@ class BulkDelete(BulkUD): def __init__(self, query): super(BulkDelete, self).__init__(query) - self.query._no_select_modifiers("delete") @classmethod def factory(cls, query, synchronize_session): diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py index b927db60ab..4e662f4c27 100644 --- a/lib/sqlalchemy/orm/query.py +++ b/lib/sqlalchemy/orm/query.py @@ -396,22 +396,6 @@ class Query(object): % (meth, meth) ) - def _no_select_modifiers(self, meth): - if not self._enable_assertions: - return - for attr, methname, notset in ( - ('_limit', 'limit()', None), - ('_offset', 'offset()', None), - ('_order_by', 'order_by()', False), - ('_group_by', 'group_by()', False), - ('_distinct', 'distinct()', False), - ): - if getattr(self, attr) is not notset: - raise sa_exc.InvalidRequestError( - "Can't call Query.%s() when %s has been called" % - (meth, methname) - ) - def _get_options(self, populate_existing=None, version_check=None, only_load_props=None, diff --git a/test/orm/test_query.py b/test/orm/test_query.py index 74b9c2956b..7a7584fb43 100644 --- a/test/orm/test_query.py +++ b/test/orm/test_query.py @@ -697,39 +697,6 @@ class InvalidGenerationsTest(QueryTest, AssertsCompiledSQL): text("select * from table")) assert_raises(sa_exc.InvalidRequestError, q.with_polymorphic, User) - def test_cancel_order_by(self): - User = self.classes.User - - s = create_session() - - q = s.query(User).order_by(User.id) - self.assert_compile( - q, - "SELECT users.id AS users_id, users.name AS users_name " - "FROM users ORDER BY users.id", - use_default_dialect=True) - - assert_raises( - sa_exc.InvalidRequestError, q._no_select_modifiers, "foo") - - q = q.order_by(None) - self.assert_compile( - q, - "SELECT users.id AS users_id, users.name AS users_name FROM users", - use_default_dialect=True) - - assert_raises( - sa_exc.InvalidRequestError, q._no_select_modifiers, "foo") - - q = q.order_by(False) - self.assert_compile( - q, - "SELECT users.id AS users_id, users.name AS users_name FROM users", - use_default_dialect=True) - - # after False was set, this should pass - q._no_select_modifiers("foo") - def test_mapper_zero(self): User, Address = self.classes.User, self.classes.Address diff --git a/test/orm/test_update_delete.py b/test/orm/test_update_delete.py index 36cea69d29..807b2407b4 100644 --- a/test/orm/test_update_delete.py +++ b/test/orm/test_update_delete.py @@ -19,12 +19,20 @@ class UpdateDeleteTest(fixtures.MappedTest): test_needs_autoincrement=True), Column('name', String(32)), Column('age', Integer)) + Table( + "addresses", metadata, + Column('id', Integer, primary_key=True), + Column('user_id', ForeignKey('users.id')) + ) @classmethod def setup_classes(cls): class User(cls.Comparable): pass + class Address(cls.Comparable): + pass + @classmethod def insert_data(cls): users = cls.tables.users @@ -41,7 +49,13 @@ class UpdateDeleteTest(fixtures.MappedTest): User = cls.classes.User users = cls.tables.users - mapper(User, users) + Address = cls.classes.Address + addresses = cls.tables.addresses + + mapper(User, users, properties={ + 'addresses': relationship(Address) + }) + mapper(Address, addresses) def test_illegal_eval(self): User = self.classes.User @@ -57,25 +71,45 @@ class UpdateDeleteTest(fixtures.MappedTest): def test_illegal_operations(self): User = self.classes.User + Address = self.classes.Address s = Session() - for q, mname in ( - (s.query(User).limit(2), "limit"), - (s.query(User).offset(2), "offset"), - (s.query(User).limit(2).offset(2), "limit"), - (s.query(User).order_by(User.id), "order_by"), - (s.query(User).group_by(User.id), "group_by"), - (s.query(User).distinct(), "distinct") + for q, mname, err in ( + (s.query(User).limit(2), r"limit\(\)", True), + (s.query(User).offset(2), r"offset\(\)", True), + (s.query(User).limit(2).offset(2), r"limit\(\)", True), + (s.query(User).order_by(User.id), r"order_by\(\)", True), + (s.query(User).group_by(User.id), r"group_by\(\)", True), + (s.query(User).distinct(), r"distinct\(\)", True), + (s.query(User).join(User.addresses), + r"join\(\), outerjoin\(\), select_from\(\), or from_self\(\)", + False), + (s.query(User).outerjoin(User.addresses), + r"join\(\), outerjoin\(\), select_from\(\), or from_self\(\)", + False), + (s.query(User).select_from(Address), + r"join\(\), outerjoin\(\), select_from\(\), or from_self\(\)", + False), + (s.query(User).from_self(), + r"join\(\), outerjoin\(\), select_from\(\), or from_self\(\)", + False), ): + if err: + exc_cls = exc.InvalidRequestError + else: + exc_cls = exc.SAWarning + assert_raises_message( - exc.InvalidRequestError, - r"Can't call Query.update\(\) when %s\(\) has been called" % mname, + exc_cls, + r"Can't call Query.update\(\) or Query.delete\(\) when " + "%s has been called" % mname, q.update, {'name': 'ed'}) assert_raises_message( - exc.InvalidRequestError, - r"Can't call Query.delete\(\) when %s\(\) has been called" % mname, + exc_cls, + r"Can't call Query.update\(\) or Query.delete\(\) when " + "%s has been called" % mname, q.delete) def test_delete(self):