Merge "Strong reference parent object in association proxy"

doc/build/changelog/migration_13.rst

@@ -243,6 +243,72 @@ The fix now includes that ``address.user_id`` is left unchanged as per
 
 :ticket:`3844`
 
+.. _change_4268:
+
+Association Proxy now Strong References the Parent Object
+=========================================================
+
+The long-standing behavior of the association proxy collection maintaining
+only a weak reference to the parent object is reverted; the proxy will now
+maintain a strong reference to the parent for as long as the proxy
+collection itself is also in memory, eliminating the "stale association
+proxy" error. This change is being made on an experimental basis to see if
+any use cases arise where it causes side effects.
+
+As an example, given a mapping with association proxy::
+
+    class A(Base):
+        __tablename__ = 'a'
+
+        id = Column(Integer, primary_key=True)
+        bs = relationship("B")
+        b_data = association_proxy('bs', 'data')
+
+
+    class B(Base):
+        __tablename__ = 'b'
+        id = Column(Integer, primary_key=True)
+        a_id = Column(ForeignKey("a.id"))
+        data = Column(String)
+
+
+    a1 = A(bs=[B(data='b1'), B(data='b2')])
+
+    b_data = a1.b_data
+
+Previously, if ``a1`` were deleted out of scope::
+
+    del a1
+
+Trying to iterate the ``b_data`` collection after ``a1`` is deleted from scope
+would raise the error ``"stale association proxy, parent object has gone out of
+scope"``.  This is because the association proxy needs to access the actual
+``a1.bs`` collection in order to produce a view, and prior to this change it
+maintained only a weak reference to ``a1``.   In particular, users would
+frequently encounter this error when performing an inline operation
+such as::
+
+    collection = session.query(A).filter_by(id=1).first().b_data
+
+Above, because the ``A`` object would be garbage collected before the
+``b_data`` collection were actually used.
+
+The change is that the ``b_data`` collection is now maintaining a strong
+reference to the ``a1`` object, so that it remains present::
+
+    assert b_data == ['b1', 'b2']
+
+This change introduces the side effect that if an application is passing around
+the collection as above, **the parent object won't be garbage collected** until
+the collection is also discarded.   As always, if ``a1`` is persistent inside a
+particular :class:`.Session`, it will remain part of that session's  state
+until it is garbage collected.
+
+Note that this change may be revised if it leads to problems.
+
+
+:ticket:`4268`
+
 New Features and Improvements - Core
 ====================================
 

doc/build/changelog/unreleased_13/4268.rst

@@ -0,0 +1,15 @@
+.. change::
+    :tags: bug, ext
+    :tickets: 4268
+
+    The long-standing behavior of the association proxy collection maintaining
+    only a weak reference to the parent object is reverted; the proxy will now
+    maintain a strong reference to the parent for as long as the proxy
+    collection itself is also in memory, eliminating the "stale association
+    proxy" error. This change is being made on an experimental basis to see if
+    any use cases arise where it causes side effects.
+
+    .. seealso::
+
+        :ref:`change_4268`
+

lib/sqlalchemy/ext/associationproxy.py

@@ -14,7 +14,6 @@
 
 """
 import operator
-import weakref
 from .. import exc, orm, util
 from ..orm import collections, interfaces
 from ..sql import or_
@@ -637,22 +636,17 @@ def __ne__(self, obj):
 
 class _lazy_collection(object):
     def __init__(self, obj, target):
-        self.ref = weakref.ref(obj)
+        self.parent = obj
         self.target = target
 
     def __call__(self):
-        obj = self.ref()
-        if obj is None:
-            raise exc.InvalidRequestError(
-                "stale association proxy, parent object has gone out of "
-                "scope")
-        return getattr(obj, self.target)
+        return getattr(self.parent, self.target)
 
     def __getstate__(self):
-        return {'obj': self.ref(), 'target': self.target}
+        return {'obj': self.parent, 'target': self.target}
 
     def __setstate__(self, state):
-        self.ref = weakref.ref(state['obj'])
+        self.parent = state['obj']
         self.target = state['target']
 
 

test/ext/test_associationproxy.py

@@ -2362,6 +2362,7 @@ class Foob(object):
 
         eq_(Foob.assoc.info, {'foo': 'bar'})
 
+
 class MultiOwnerTest(fixtures.DeclarativeMappedTest,
                                      testing.AssertsCompiledSQL):
     __dialect__ = 'default'
@@ -2431,3 +2432,165 @@ def test_any_has(self):
             "AND d.value = :value_1)"
         )
 
+class ScopeBehaviorTest(fixtures.DeclarativeMappedTest):
+    # test some GC scenarios, including issue #4268
+
+    @classmethod
+    def setup_classes(cls):
+        Base = cls.DeclarativeBasic
+
+        class A(Base):
+            __tablename__ = 'a'
+
+            id = Column(Integer, primary_key=True)
+            data = Column(String(50))
+            bs = relationship("B")
+
+            b_dyn = relationship("B", lazy="dynamic")
+
+            b_data = association_proxy("bs", "data")
+
+            b_dynamic_data = association_proxy("bs", "data")
+
+        class B(Base):
+            __tablename__ = 'b'
+
+            id = Column(Integer, primary_key=True)
+            aid = Column(ForeignKey('a.id'))
+            data = Column(String(50))
+
+    @classmethod
+    def insert_data(cls):
+        A, B = cls.classes("A", "B")
+
+        s = Session(testing.db)
+        s.add_all([
+            A(id=1, bs=[B(data='b1'), B(data='b2')]),
+            A(id=2, bs=[B(data='b3'), B(data='b4')])])
+        s.commit()
+        s.close()
+
+    def test_plain_collection_gc(self):
+        A, B = self.classes("A", "B")
+
+        s = Session(testing.db)
+        a1 = s.query(A).filter_by(id=1).one()
+
+        a1bs = a1.bs  # noqa
+
+        del a1
+
+        gc_collect()
+
+        assert (A, (1, ), None) not in s.identity_map
+
+    @testing.fails("dynamic relationship strong references parent")
+    def test_dynamic_collection_gc(self):
+        A, B = self.classes("A", "B")
+
+        s = Session(testing.db)
+
+        a1 = s.query(A).filter_by(id=1).one()
+
+        a1bs = a1.b_dyn  # noqa
+
+        del a1
+
+        gc_collect()
+
+        # also fails, AppenderQuery holds onto parent
+        assert (A, (1, ), None) not in s.identity_map
+
+    @testing.fails("association proxy strong references parent")
+    def test_associated_collection_gc(self):
+        A, B = self.classes("A", "B")
+
+        s = Session(testing.db)
+
+        a1 = s.query(A).filter_by(id=1).one()
+
+        a1bs = a1.b_data # noqa
+
+        del a1
+
+        gc_collect()
+
+        assert (A, (1, ), None) not in s.identity_map
+
+    @testing.fails("association proxy strong references parent")
+    def test_associated_dynamic_gc(self):
+        A, B = self.classes("A", "B")
+
+        s = Session(testing.db)
+
+        a1 = s.query(A).filter_by(id=1).one()
+
+        a1bs = a1.b_dynamic_data # noqa
+
+        del a1
+
+        gc_collect()
+
+        assert (A, (1, ), None) not in s.identity_map
+
+    def test_plain_collection_iterate(self):
+        A, B = self.classes("A", "B")
+
+        s = Session(testing.db)
+
+        a1 = s.query(A).filter_by(id=1).one()
+
+        a1bs = a1.bs
+
+        del a1
+
+        gc_collect()
+
+        assert len(a1bs) == 2
+
+    def test_dynamic_collection_iterate(self):
+        A, B = self.classes("A", "B")
+
+        s = Session(testing.db)
+
+        a1 = s.query(A).filter_by(id=1).one()
+
+        a1bs = a1.b_dyn  # noqa
+
+        del a1
+
+        gc_collect()
+
+        assert len(list(a1bs)) == 2
+
+    def test_associated_collection_iterate(self):
+        A, B = self.classes("A", "B")
+
+        s = Session(testing.db)
+
+        a1 = s.query(A).filter_by(id=1).one()
+
+        a1bs = a1.b_data
+
+        del a1
+
+        gc_collect()
+
+        assert len(a1bs) == 2
+
+    def test_associated_dynamic_iterate(self):
+        A, B = self.classes("A", "B")
+
+        s = Session(testing.db)
+
+        a1 = s.query(A).filter_by(id=1).one()
+
+        a1bs = a1.b_dynamic_data
+
+        del a1
+
+        gc_collect()
+
+        assert len(a1bs) == 2
+
+