- Fixed bug where :meth:`.Session.expunge` would not fully detach

the given object if the object had been subject to a delete
operation that was flushed, but not committed.  This would also
affect related operations like :func:`.make_transient`.
fixes #3139

5 files changed, 113 insertions, 6 deletions

diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst
index 4454dd98a..18742f81e 100644
--- a/doc/build/changelog/changelog_10.rst
+++ b/doc/build/changelog/changelog_10.rst
@@ -23,6 +23,19 @@
 
     .. change::
         :tags: bug, orm
+        :tickets: 3139
+
+        Fixed bug where :meth:`.Session.expunge` would not fully detach
+        the given object if the object had been subject to a delete
+        operation that was flushed, but not committed.  This would also
+        affect related operations like :func:`.make_transient`.
+
+        .. seealso::
+
+            :ref:`bug_3139`
+
+    .. change::
+        :tags: bug, orm
         :tickets: 3230
 
         A warning is emitted in the case of multiple relationships that
diff --git a/doc/build/changelog/migration_10.rst b/doc/build/changelog/migration_10.rst
index 3591ee0e2..c025390d2 100644
--- a/doc/build/changelog/migration_10.rst
+++ b/doc/build/changelog/migration_10.rst
@@ -927,6 +927,39 @@ symbol, and no change to the object's state occurs.
 
 :ticket:`3061`
 
+.. _bug_3139:
+
+session.expunge() will fully detach an object that's been deleted
+-----------------------------------------------------------------
+
+The behavior of :meth:`.Session.expunge` had a bug that caused an
+inconsistency in behavior regarding deleted objects.  The
+:func:`.object_session` function as well as the :attr:`.InstanceState.session`
+attribute would still report object as belonging to the :class:`.Session`
+subsequent to the expunge::
+
+    u1 = sess.query(User).first()
+    sess.delete(u1)
+
+    sess.flush()
+
+    assert u1 not in sess
+    assert inspect(u1).session is sess  # this is normal before commit
+
+    sess.expunge(u1)
+
+    assert u1 not in sess
+    assert inspect(u1).session is None  # would fail
+
+Note that it is normal for ``u1 not in sess`` to be True while
+``inspect(u1).session`` still refers to the session, while the transaction
+is ongoing subsequent to the delete operation and :meth:`.Session.expunge`
+has not been called; the full detachment normally completes once the
+transaction is committed.  This issue would also impact functions
+that rely on :meth:`.Session.expunge` such as :func:`.make_transient`.
+
+:ticket:`3139`
+
 .. _migration_yield_per_eager_loading:
 
 Joined/Subquery eager loading explicitly disallowed with yield_per
diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index db9d3a51d..f23983cbc 100644
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -292,7 +292,7 @@ class SessionTransaction(object):
             for s in self.session.identity_map.all_states():
                 s._expire(s.dict, self.session.identity_map._modified)
             for s in self._deleted:
-                s.session_id = None
+                s._detach()
             self._deleted.clear()
         elif self.nested:
             self._parent._new.update(self._new)
@@ -1409,6 +1409,7 @@ class Session(_SessionClassMethods):
             state._detach()
         elif self.transaction:
             self.transaction._deleted.pop(state, None)
+            state._detach()
 
     def _register_newly_persistent(self, states):
         for state in states:
@@ -2449,16 +2450,19 @@ def make_transient_to_detached(instance):
 
 
 def object_session(instance):
-    """Return the ``Session`` to which instance belongs.
+    """Return the :class:`.Session` to which the given instance belongs.
 
-    If the instance is not a mapped instance, an error is raised.
+    This is essentially the same as the :attr:`.InstanceState.session`
+    accessor.  See that attribute for details.
 
     """
 
     try:
-        return _state_session(attributes.instance_state(instance))
+        state = attributes.instance_state(instance)
     except exc.NO_STATE:
         raise exc.UnmappedInstanceError(instance)
+    else:
+        return _state_session(state)
 
 
 _new_sessionid = util.counter()
diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py
index 4756f1707..560149de5 100644
--- a/lib/sqlalchemy/orm/state.py
+++ b/lib/sqlalchemy/orm/state.py
@@ -145,7 +145,16 @@ class InstanceState(interfaces.InspectionAttr):
     @util.dependencies("sqlalchemy.orm.session")
     def session(self, sessionlib):
         """Return the owning :class:`.Session` for this instance,
-        or ``None`` if none available."""
+        or ``None`` if none available.
+
+        Note that the result here can in some cases be *different*
+        from that of ``obj in session``; an object that's been deleted
+        will report as not ``in session``, however if the transaction is
+        still in progress, this attribute will still refer to that session.
+        Only when the transaction is completed does the object become
+        fully detached under normal circumstances.
+
+        """
         return sessionlib._state_session(self)
 
     @property
diff --git a/test/orm/test_session.py b/test/orm/test_session.py
index b0b00d5ed..96728612d 100644
--- a/test/orm/test_session.py
+++ b/test/orm/test_session.py
@@ -204,6 +204,7 @@ class SessionUtilTest(_fixtures.FixtureTest):
         sess.flush()
         make_transient(u1)
         sess.rollback()
+        assert attributes.instance_state(u1).transient
 
     def test_make_transient_to_detached(self):
         users, User = self.tables.users, self.classes.User
@@ -661,7 +662,7 @@ class SessionStateTest(_fixtures.FixtureTest):
         go()
         eq_(canary, [False])
 
-    def test_deleted_expunged(self):
+    def test_deleted_auto_expunged(self):
         users, User = self.tables.users, self.classes.User
 
         mapper(User, users)
@@ -682,6 +683,53 @@ class SessionStateTest(_fixtures.FixtureTest):
 
         assert object_session(u1) is None
 
+    def test_explicit_expunge_pending(self):
+        users, User = self.tables.users, self.classes.User
+
+        mapper(User, users)
+        sess = Session()
+        u1 = User(name='x')
+        sess.add(u1)
+
+        sess.flush()
+        sess.expunge(u1)
+
+        assert u1 not in sess
+        assert object_session(u1) is None
+
+        sess.rollback()
+
+        assert u1 not in sess
+        assert object_session(u1) is None
+
+    def test_explicit_expunge_deleted(self):
+        users, User = self.tables.users, self.classes.User
+
+        mapper(User, users)
+        sess = Session()
+        sess.add(User(name='x'))
+        sess.commit()
+
+        u1 = sess.query(User).first()
+        sess.delete(u1)
+
+        sess.flush()
+
+        assert was_deleted(u1)
+        assert u1 not in sess
+        assert object_session(u1) is sess
+
+        sess.expunge(u1)
+        assert was_deleted(u1)
+        assert u1 not in sess
+        assert object_session(u1) is None
+
+        sess.rollback()
+        assert was_deleted(u1)
+        assert u1 not in sess
+        assert object_session(u1) is None
+
+
 class SessionStateWFixtureTest(_fixtures.FixtureTest):
     __backend__ = True