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

diff --git a/doc/build/changelog/unreleased_13/4772.rst b/doc/build/changelog/unreleased_13/4772.rst
new file mode 100644 (file)
index 0000000..f3f29a0
--- /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 f0ba6051d8a47dad44ac420ebfb26cb4b8e52e5e..e93ad9eb7cc16285eddb5ccf37c98926da92a14d 100644 (file)
--- 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 20544dbbef54ee05730c453ae1cfa42f33840904..3218b9fa3b1afb2fc229a7a79a3f03e55d842647 100644 (file)
--- 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 b894979d00a13da55acc6ef8f25405c2b884baa5..cbc7a2fd54a996733a69063c95559399bcd022e1 100644 (file)
--- 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 b861a9fe5728bb9e08874fe446d0aeb987ed38bb..94ecf4ee2983351df81c20d0a72a319d9cce93c2 100644 (file)
--- 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 3788eb3297826a996eedfdfb50b626bfa38aa9c2..6108a28c4cab26e0685a4da1bf03dd964939ecd9 100644 (file)
--- 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()