From: Mike Bayer Date: Thu, 18 Jul 2019 14:58:24 +0000 (-0400) Subject: Fixes for uselist=True with m2o relationships X-Git-Tag: rel_1_3_6~5^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=86f4ae32664ca8dedd5a8f82a7afb9291af94dec;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Fixes for uselist=True with m2o relationships Fixed bug where a many-to-one relationship that specified ``uselist=True`` would fail to update correctly during a primary key change where a related column needs to change. Fixed bug where the detection for many-to-one or one-to-one use with a "dynamic" relationship, which is an invalid configuration, would fail to raise if the relationship were configured with ``uselist=True``. The current fix is that it warns, instead of raises, as this would otherwise be backwards incompatible, however in a future release it will be a raise. Fixes: #4772 Change-Id: Ibd5d2f7329ff245c88118e2533fc8ef42a09fef3 (cherry picked from commit 5e8c7c88de2d9bac58e82bc1e5af7fcad5405855) --- diff --git a/doc/build/changelog/unreleased_13/4772.rst b/doc/build/changelog/unreleased_13/4772.rst new file mode 100644 index 0000000000..f3f29a07c6 --- /dev/null +++ b/doc/build/changelog/unreleased_13/4772.rst @@ -0,0 +1,19 @@ +.. change:: + :tags: bug, orm + :tickets: 4772 + + Fixed bug where a many-to-one relationship that specified ``uselist=True`` + would fail to update correctly during a primary key change where a related + column needs to change. + + +.. change:: + :tags: bug, orm + :tickets: 4772 + + Fixed bug where the detection for many-to-one or one-to-one use with a + "dynamic" relationship, which is an invalid configuration, would fail to + raise if the relationship were configured with ``uselist=True``. The + current fix is that it warns, instead of raises, as this would otherwise be + backwards incompatible, however in a future release it will be a raise. + diff --git a/lib/sqlalchemy/orm/dependency.py b/lib/sqlalchemy/orm/dependency.py index f0ba6051d8..e93ad9eb7c 100644 --- a/lib/sqlalchemy/orm/dependency.py +++ b/lib/sqlalchemy/orm/dependency.py @@ -938,7 +938,13 @@ class DetectKeySwitch(DependencyProcessor): related is not attributes.PASSIVE_NO_RESULT and related is not None ): - related_state = attributes.instance_state(dict_[self.key]) + if self.prop.uselist: + if not related: + continue + related_obj = related[0] + else: + related_obj = related + related_state = attributes.instance_state(related_obj) if related_state in switchers: uowcommit.register_object( state, False, self.passive_updates diff --git a/lib/sqlalchemy/orm/dynamic.py b/lib/sqlalchemy/orm/dynamic.py index 20544dbbef..3218b9fa3b 100644 --- a/lib/sqlalchemy/orm/dynamic.py +++ b/lib/sqlalchemy/orm/dynamic.py @@ -14,6 +14,7 @@ basic add/delete mutation. from . import attributes from . import exc as orm_exc +from . import interfaces from . import object_mapper from . import object_session from . import properties @@ -36,6 +37,17 @@ class DynaLoader(strategies.AbstractRelationshipLoader): "many-to-one/one-to-one relationships and/or " "uselist=False." % self.parent_property ) + elif self.parent_property.direction not in ( + interfaces.ONETOMANY, + interfaces.MANYTOMANY, + ): + util.warn( + "On relationship %s, 'dynamic' loaders cannot be used with " + "many-to-one/one-to-one relationships and/or " + "uselist=False. This warning will be an exception in a " + "future release." % self.parent_property + ) + strategies._register_attribute( self.parent_property, mapper, diff --git a/lib/sqlalchemy/testing/entities.py b/lib/sqlalchemy/testing/entities.py index b894979d00..cbc7a2fd54 100644 --- a/lib/sqlalchemy/testing/entities.py +++ b/lib/sqlalchemy/testing/entities.py @@ -7,7 +7,7 @@ import sqlalchemy as sa from .. import exc as sa_exc - +from ..util import compat _repr_stack = set() @@ -90,7 +90,9 @@ class ComparableEntity(BasicEntity): except (AttributeError, sa_exc.UnboundExecutionError): return False - if hasattr(value, "__iter__"): + if hasattr(value, "__iter__") and not isinstance( + value, compat.string_types + ): if hasattr(value, "__getitem__") and not hasattr( value, "keys" ): diff --git a/test/orm/test_dynamic.py b/test/orm/test_dynamic.py index b861a9fe57..94ecf4ee29 100644 --- a/test/orm/test_dynamic.py +++ b/test/orm/test_dynamic.py @@ -145,6 +145,29 @@ class DynamicTest(_DynamicFixture, _fixtures.FixtureTest, AssertsCompiledSQL): configure_mappers, ) + def test_no_m2o_w_uselist(self): + users, Address, addresses, User = ( + self.tables.users, + self.classes.Address, + self.tables.addresses, + self.classes.User, + ) + mapper( + Address, + addresses, + properties={ + "user": relationship(User, uselist=True, lazy="dynamic") + }, + ) + mapper(User, users) + assert_raises_message( + exc.SAWarning, + "On relationship Address.user, 'dynamic' loaders cannot be " + "used with many-to-one/one-to-one relationships and/or " + "uselist=False.", + configure_mappers, + ) + def test_order_by(self): User, Address = self._user_address_fixture() sess = create_session() diff --git a/test/orm/test_naturalpks.py b/test/orm/test_naturalpks.py index 3788eb3297..6108a28c4c 100644 --- a/test/orm/test_naturalpks.py +++ b/test/orm/test_naturalpks.py @@ -37,7 +37,7 @@ def _backend_specific_fk_args(): class NaturalPKTest(fixtures.MappedTest): # MySQL 5.5 on Windows crashes (the entire server, not the client) # if you screw around with ON UPDATE CASCADE type of stuff. - __requires__ = "skip_mysql_on_windows", "on_update_or_deferrable_fks" + __requires__ = ("skip_mysql_on_windows",) __backend__ = True @classmethod @@ -283,6 +283,13 @@ class NaturalPKTest(fixtures.MappedTest): def test_manytoone_nonpassive(self): self._test_manytoone(False) + @testing.requires.on_update_cascade + def test_manytoone_passive_uselist(self): + self._test_manytoone(True, True) + + def test_manytoone_nonpassive_uselist(self): + self._test_manytoone(False, True) + def test_manytoone_nonpassive_cold_mapping(self): """test that the mapper-level m2o dependency processor is set up even if the opposite side relationship @@ -318,7 +325,7 @@ class NaturalPKTest(fixtures.MappedTest): self.assert_sql_count(testing.db, go, 2) - def _test_manytoone(self, passive_updates): + def _test_manytoone(self, passive_updates, uselist=False, dynamic=False): users, Address, addresses, User = ( self.tables.users, self.classes.Address, @@ -331,19 +338,27 @@ class NaturalPKTest(fixtures.MappedTest): Address, addresses, properties={ - "user": relationship(User, passive_updates=passive_updates) + "user": relationship( + User, uselist=uselist, passive_updates=passive_updates + ) }, ) sess = create_session() a1 = Address(email="jack1") a2 = Address(email="jack2") + a3 = Address(email="fred") u1 = User(username="jack", fullname="jack") - a1.user = u1 - a2.user = u1 + if uselist: + a1.user = [u1] + a2.user = [u1] + else: + a1.user = u1 + a2.user = u1 sess.add(a1) sess.add(a2) + sess.add(a3) sess.flush() u1.username = "ed" @@ -363,10 +378,24 @@ class NaturalPKTest(fixtures.MappedTest): assert a1.username == a2.username == "ed" sess.expunge_all() - eq_( - [Address(username="ed"), Address(username="ed")], - sess.query(Address).all(), - ) + if uselist: + eq_( + [ + Address(email="fred", user=[]), + Address(username="ed"), + Address(username="ed"), + ], + sess.query(Address).order_by(Address.email).all(), + ) + else: + eq_( + [ + Address(email="fred", user=None), + Address(username="ed"), + Address(username="ed"), + ], + sess.query(Address).order_by(Address.email).all(), + ) @testing.requires.on_update_cascade def test_onetoone_passive(self): @@ -1245,7 +1274,15 @@ class CascadeToFKPKTest(fixtures.MappedTest, testing.AssertsCompiledSQL): def test_change_m2o_nonpassive(self): self._test_change_m2o(False) - def _test_change_m2o(self, passive_updates): + @testing.requires.on_update_cascade + def test_change_m2o_passive_uselist(self): + self._test_change_m2o(True, True) + + @testing.requires.non_updating_cascade + def test_change_m2o_nonpassive_uselist(self): + self._test_change_m2o(False, True) + + def _test_change_m2o(self, passive_updates, uselist=False): User, Address, users, addresses = ( self.classes.User, self.classes.Address, @@ -1258,13 +1295,18 @@ class CascadeToFKPKTest(fixtures.MappedTest, testing.AssertsCompiledSQL): Address, addresses, properties={ - "user": relationship(User, passive_updates=passive_updates) + "user": relationship( + User, uselist=uselist, passive_updates=passive_updates + ) }, ) sess = create_session() u1 = User(username="jack") - a1 = Address(user=u1, email="foo@bar") + if uselist: + a1 = Address(user=[u1], email="foo@bar") + else: + a1 = Address(user=u1, email="foo@bar") sess.add_all([u1, a1]) sess.flush()