- [feature] A warning is emitted when a reference

    to an instrumented collection is no longer
    associated with the parent class due to
    expiration/attribute refresh/collection
    replacement, but an append
    or remove operation is received on the
    now-detached collection.  [ticket:2476]

diff --git a/CHANGES b/CHANGES
index b49c76d793f26800306f4f56e2501830c45ccbac..1180572e8c678ed3821a6aec0c6cea8dcde9f2c0 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -196,6 +196,14 @@ underneath "0.7.xx".
     strategies for producing queries with uniquely-
     named columns.  [ticket:1729].
 
+  - [feature] A warning is emitted when a reference
+    to an instrumented collection is no longer
+    associated with the parent class due to
+    expiration/attribute refresh/collection
+    replacement, but an append
+    or remove operation is received on the
+    now-detached collection.  [ticket:2476]
+
   - [removed] Deprecated identifiers removed:
 
     * allow_null_pks mapper() argument

diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index 045a9465dc47b02d7b9b66a57f217cfbff1b79bb..2e576b4d81640aaa96f673109140e0c96b9edc32 100644 (file)
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -569,6 +569,7 @@ class ScalarAttributeImpl(AttributeImpl):
     accepts_scalar_loader = True
     uses_objects = False
     supports_population = True
+    collection = False
 
     def delete(self, state, dict_):
 
@@ -630,6 +631,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl):
     accepts_scalar_loader = False
     uses_objects = True
     supports_population = True
+    collection = False
 
     def delete(self, state, dict_):
         old = self.get(state, dict_)
@@ -740,6 +742,7 @@ class CollectionAttributeImpl(AttributeImpl):
     accepts_scalar_loader = False
     uses_objects = True
     supports_population = True
+    collection = True
 
     def __init__(self, class_, key, callable_, dispatch,
                     typecallable=None, trackparent=False, extension=None,
@@ -930,6 +933,10 @@ class CollectionAttributeImpl(AttributeImpl):
         collections.bulk_replace(new_values, old_collection, new_collection)
         old_collection.unlink(old)
 
+    def _invalidate_collection(self, collection):
+        adapter = getattr(collection, '_sa_adapter')
+        adapter.invalidated = True
+
     def set_committed_value(self, state, dict_, value):
         """Set an attribute value on the given instance and 'commit' it."""
 

diff --git a/lib/sqlalchemy/orm/collections.py b/lib/sqlalchemy/orm/collections.py
index 9d99ce40a28fc8912cfcf9ec6a2dea93c574ed27..93b99d83c3026a5aeea3cfb497327b5e4fddf940 100644 (file)
--- a/lib/sqlalchemy/orm/collections.py
+++ b/lib/sqlalchemy/orm/collections.py
@@ -580,12 +580,17 @@ class CollectionAdapter(object):
     of custom methods, such as to unwrap Zope security proxies.
 
     """
+    invalidated = False
+
     def __init__(self, attr, owner_state, data):
         self._key = attr.key
         self._data = weakref.ref(data)
         self.owner_state = owner_state
         self.link_to_self(data)
 
+    def _warn_invalidated(self):
+        util.warn("This collection has been invalidated.")
+
     @property
     def data(self):
         "The entity collection being adapted."
@@ -712,6 +717,8 @@ class CollectionAdapter(object):
 
         """
         if initiator is not False and item is not None:
+            if self.invalidated:
+                self._warn_invalidated()
             return self.attr.fire_append_event(
                                     self.owner_state,
                                     self.owner_state.dict,
@@ -728,6 +735,8 @@ class CollectionAdapter(object):
 
         """
         if initiator is not False and item is not None:
+            if self.invalidated:
+                self._warn_invalidated()
             self.attr.fire_remove_event(
                                     self.owner_state,
                                     self.owner_state.dict,
@@ -740,6 +749,8 @@ class CollectionAdapter(object):
         fire_remove_event().
 
         """
+        if self.invalidated:
+            self._warn_invalidated()
         self.attr.fire_pre_remove_event(
                                     self.owner_state,
                                     self.owner_state.dict,

diff --git a/lib/sqlalchemy/orm/descriptor_props.py b/lib/sqlalchemy/orm/descriptor_props.py
index efbceb0fc2901861d956dd054875786631b63edf..91717974dad6132c26ca3b202b125901bcb727f4 100644 (file)
--- a/lib/sqlalchemy/orm/descriptor_props.py
+++ b/lib/sqlalchemy/orm/descriptor_props.py
@@ -29,6 +29,7 @@ class DescriptorProperty(MapperProperty):
         class _ProxyImpl(object):
             accepts_scalar_loader = False
             expire_missing = True
+            collection = False
 
             def __init__(self, key):
                 self.key = key

diff --git a/lib/sqlalchemy/orm/dynamic.py b/lib/sqlalchemy/orm/dynamic.py
index d2cb0ab05b082ae397c1134783f72355e16a746e..6d532e669b3568075afe79605a06fadf6ac608a1 100644 (file)
--- a/lib/sqlalchemy/orm/dynamic.py
+++ b/lib/sqlalchemy/orm/dynamic.py
@@ -38,6 +38,7 @@ class DynamicAttributeImpl(attributes.AttributeImpl):
     uses_objects = True
     accepts_scalar_loader = False
     supports_population = False
+    collection = False
 
     def __init__(self, class_, key, typecallable,
                      dispatch,

diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py
index 1c7fc2c41e22f1d7cd42981831c3800abebd3a23..9d17fa3443746f64f3b63d721a4264d11aea27cd 100644 (file)
--- a/lib/sqlalchemy/orm/state.py
+++ b/lib/sqlalchemy/orm/state.py
@@ -288,7 +288,9 @@ class InstanceState(interfaces._InspectionAttr):
         """Remove the given attribute and any
            callables associated with it."""
 
-        dict_.pop(key, None)
+        old = dict_.pop(key, None)
+        if old is not None and self.manager[key].impl.collection:
+            self.manager[key].impl._invalidate_collection(old)
         self.callables.pop(key, None)
 
     def _expire_attribute_pre_commit(self, dict_, key):
@@ -296,6 +298,8 @@ class InstanceState(interfaces._InspectionAttr):
 
         The additional bookkeeping is finished up in commit_all().
 
+        Should only be called for scalar attributes.
+
         This method is actually called a lot with joined-table
         loading, when the second table isn't present in the result.
 
@@ -307,7 +311,9 @@ class InstanceState(interfaces._InspectionAttr):
         """Remove the given attribute and set the given callable
            as a loader."""
 
-        dict_.pop(key, None)
+        old = dict_.pop(key, None)
+        if old is not None and self.manager[key].impl.collection:
+            self.manager[key].impl._invalidate_collection(old)
         self.callables[key] = callable_
 
     def _expire(self, dict_, modified_set):
@@ -331,7 +337,9 @@ class InstanceState(interfaces._InspectionAttr):
             if impl.accepts_scalar_loader and \
                 (impl.expire_missing or key in dict_):
                 self.callables[key] = self
-            dict_.pop(key, None)
+            old = dict_.pop(key, None)
+            if impl.collection and old is not None:
+                impl._invalidate_collection(old)
 
         self.manager.dispatch.expire(self, None)
 
@@ -342,7 +350,9 @@ class InstanceState(interfaces._InspectionAttr):
             impl = self.manager[key].impl
             if impl.accepts_scalar_loader:
                 self.callables[key] = self
-            dict_.pop(key, None)
+            old = dict_.pop(key, None)
+            if impl.collection and old is not None:
+                impl._invalidate_collection(old)
 
             self.committed_state.pop(key, None)
             if pending:

diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py
index bff29feb630b2fb7c40f246c1168df9fdf6b3f07..8a6e37419766dd9bca6853239a513fa38e6ab4cd 100644 (file)
--- a/test/orm/test_attributes.py
+++ b/test/orm/test_attributes.py
@@ -8,7 +8,7 @@ from test.lib.testing import eq_, ne_, assert_raises, \
     assert_raises_message
 from test.lib import fixtures
 from test.lib.util import gc_collect, all_partial_orderings
-from sqlalchemy.util import cmp, jython, topological
+from sqlalchemy.util import jython
 from sqlalchemy import event
 
 # global for pickling tests
@@ -2326,3 +2326,63 @@ class ListenerTest(fixtures.ORMTest):
 
             teardown()
 
+
+class TestUnlink(fixtures.TestBase):
+    def setUp(self):
+        class A(object):
+            pass
+        class B(object):
+            pass
+        self.A = A
+        self.B = B
+        instrumentation.register_class(A)
+        instrumentation.register_class(B)
+        attributes.register_attribute(A, 'bs', uselist=True,
+                useobject=True)
+
+    def test_expired(self):
+        A, B = self.A, self.B
+        a1 = A()
+        coll = a1.bs
+        a1.bs.append(B())
+        state = attributes.instance_state(a1)
+        state._expire(state.dict, set())
+        assert_raises(
+            Warning,
+            coll.append, B()
+        )
+
+    def test_replaced(self):
+        A, B = self.A, self.B
+        a1 = A()
+        coll = a1.bs
+        a1.bs.append(B())
+        a1.bs = []
+        # a bulk replace empties the old collection
+        assert len(coll) == 0
+        coll.append(B())
+        assert len(coll) == 1
+
+    def test_pop_existing(self):
+        A, B = self.A, self.B
+        a1 = A()
+        coll = a1.bs
+        a1.bs.append(B())
+        state = attributes.instance_state(a1)
+        state._reset(state.dict, "bs")
+        assert_raises(
+            Warning,
+            coll.append, B()
+        )
+
+    def test_ad_hoc_lazy(self):
+        A, B = self.A, self.B
+        a1 = A()
+        coll = a1.bs
+        a1.bs.append(B())
+        state = attributes.instance_state(a1)
+        state._set_callable(state.dict, "bs", lambda: B())
+        assert_raises(
+            Warning,
+            coll.append, B()
+        )