Include active_history when propagating attribute listeners

Fixed issue where the :paramref:`.AttributeEvents.active_history` flag
would not be set for an event listener that propgated to a subclass via the
:paramref:`.AttributeEvents.propagate` flag.   This bug has been present
for the full span of the :class:`.AttributeEvents` system.

Fixes: #4695
Change-Id: Ie384f4847f37c267d94b6d56e7538438efc1a54c

diff --git a/doc/build/changelog/unreleased_13/4695.rst b/doc/build/changelog/unreleased_13/4695.rst
new file mode 100644 (file)
index 0000000..6a372cf
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/4695.rst
@@ -0,0 +1,9 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4695
+
+    Fixed issue where the :paramref:`.AttributeEvents.active_history` flag
+    would not be set for an event listener that propgated to a subclass via the
+    :paramref:`.AttributeEvents.propagate` flag.   This bug has been present
+    for the full span of the :class:`.AttributeEvents` system.
+

diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index 674c57feac719a89b93d817cf654dd683f60aa45..01f19d9917b4761e78486545d390e760df35201f 100644 (file)
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -99,6 +99,8 @@ class QueryableAttribute(
             for base in manager._bases:
                 if key in base:
                     self.dispatch._update(base[key].dispatch)
+                    if base[key].dispatch._active_history:
+                        self.dispatch._active_history = True
 
     @util.memoized_property
     def _supports_population(self):

diff --git a/lib/sqlalchemy/orm/events.py b/lib/sqlalchemy/orm/events.py
index 8a310479cd24403ecd11f44f0ec2e0d864f31f89..d73a20e93a36d86fa64d2dd36ad10fc2c429cb90 100644 (file)
--- a/lib/sqlalchemy/orm/events.py
+++ b/lib/sqlalchemy/orm/events.py
@@ -2009,6 +2009,8 @@ class AttributeEvents(event.Events):
                 event_key.with_dispatch_target(mgr[target.key]).base_listen(
                     propagate=True
                 )
+                if active_history:
+                    mgr[target.key].dispatch._active_history = True
 
     def append(self, target, value, initiator):
         """Receive a collection append event.

diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py
index d99fcc77bba39aade5775cafc88dead14c9fc2aa..17488f2365a1545ef50489a43b0e228fa918bf35 100644 (file)
--- a/test/orm/test_attributes.py
+++ b/test/orm/test_attributes.py
@@ -3462,8 +3462,88 @@ class ListenerTest(fixtures.ORMTest):
         # reversal of approach in #3061
         eq_(canary.mock_calls, [])
 
+
+class EventPropagateTest(fixtures.TestBase):
+    # tests that were expanded as of #4695
+    # in particular these reveal the inconsistency we have in returning the
+    # "old" value between object and non-object.
+    # this inconsistency might not be a bug, since the nature of scalar
+    # SQL attributes and ORM related objects is fundamentally different.
+
+    # the inconsistency is:
+
+    # with active_history=False if old value is not present, for scalar we
+    # return NO VALUE, for object we return NEVER SET
+    # with active_history=True if old value is not present, for scalar we
+    # return NEVER SET, for object we return None
+    # so it is basically fully inconsistent across both directions.
+
+    def test_propagate_active_history(self):
+        for (A, B, C, D), canary in self._test_propagate_fixtures(True, False):
+            b = B()
+            b.attrib = "foo"
+            eq_(b.attrib, "foo")
+            eq_(canary, [("foo", attributes.NEVER_SET)])
+
+            c = C()
+            c.attrib = "bar"
+            eq_(c.attrib, "bar")
+            eq_(
+                canary,
+                [("foo", attributes.NEVER_SET), ("bar", attributes.NEVER_SET)],
+            )
+
     def test_propagate(self):
-        classes = [None, None, None]
+        for (A, B, C, D), canary in self._test_propagate_fixtures(
+            False, False
+        ):
+            b = B()
+            b.attrib = "foo"
+            eq_(b.attrib, "foo")
+
+            eq_(canary, [("foo", attributes.NO_VALUE)])
+
+            c = C()
+            c.attrib = "bar"
+            eq_(c.attrib, "bar")
+            eq_(
+                canary,
+                [("foo", attributes.NO_VALUE), ("bar", attributes.NO_VALUE)],
+            )
+
+    def test_propagate_useobject_active_history(self):
+        for (A, B, C, D), canary in self._test_propagate_fixtures(True, True):
+            b = B()
+            d1 = D()
+            b.attrib = d1
+            is_(b.attrib, d1)
+            eq_(canary, [(d1, None)])
+
+            c = C()
+            d2 = D()
+            c.attrib = d2
+            is_(c.attrib, d2)
+            eq_(canary, [(d1, None), (d2, None)])
+
+    def test_propagate_useobject(self):
+        for (A, B, C, D), canary in self._test_propagate_fixtures(False, True):
+            b = B()
+            d1 = D()
+            b.attrib = d1
+            is_(b.attrib, d1)
+            eq_(canary, [(d1, attributes.NEVER_SET)])
+
+            c = C()
+            d2 = D()
+            c.attrib = d2
+            is_(c.attrib, d2)
+            eq_(
+                canary,
+                [(d1, attributes.NEVER_SET), (d2, attributes.NEVER_SET)],
+            )
+
+    def _test_propagate_fixtures(self, active_history, useobject):
+        classes = [None, None, None, None]
         canary = []
 
         def make_a():
@@ -3484,6 +3564,13 @@ class ListenerTest(fixtures.ORMTest):
 
             classes[2] = C
 
+        def make_d():
+            class D(object):
+                pass
+
+            classes[3] = D
+            return D
+
         def instrument_a():
             instrumentation.register_class(classes[0])
 
@@ -3493,30 +3580,35 @@ class ListenerTest(fixtures.ORMTest):
         def instrument_c():
             instrumentation.register_class(classes[2])
 
+        def instrument_d():
+            instrumentation.register_class(classes[3])
+
         def attr_a():
             attributes.register_attribute(
-                classes[0], "attrib", uselist=False, useobject=False
+                classes[0], "attrib", uselist=False, useobject=useobject
             )
 
         def attr_b():
             attributes.register_attribute(
-                classes[1], "attrib", uselist=False, useobject=False
+                classes[1], "attrib", uselist=False, useobject=useobject
             )
 
         def attr_c():
             attributes.register_attribute(
-                classes[2], "attrib", uselist=False, useobject=False
+                classes[2], "attrib", uselist=False, useobject=useobject
             )
 
         def set_(state, value, oldvalue, initiator):
-            canary.append(value)
+            canary.append((value, oldvalue))
 
         def events_a():
-            event.listen(classes[0].attrib, "set", set_, propagate=True)
-
-        def teardown():
-            classes[:] = [None, None, None]
-            canary[:] = []
+            event.listen(
+                classes[0].attrib,
+                "set",
+                set_,
+                propagate=True,
+                active_history=active_history,
+            )
 
         ordering = [
             (instrument_a, instrument_b),
@@ -3550,17 +3642,14 @@ class ListenerTest(fixtures.ORMTest):
             for fn in series:
                 fn()
 
-            b = classes[1]()
-            b.attrib = "foo"
-            eq_(b.attrib, "foo")
-            eq_(canary, ["foo"])
+            if useobject:
+                D = make_d()
+                instrument_d()
 
-            c = classes[2]()
-            c.attrib = "bar"
-            eq_(c.attrib, "bar")
-            eq_(canary, ["foo", "bar"])
+            yield classes, canary
 
-            teardown()
+            classes[:] = [None, None, None, None]
+            canary[:] = []
 
 
 class TestUnlink(fixtures.TestBase):