merge tip

10 files changed, 176 insertions, 45 deletions

diff --git a/CHANGES b/CHANGES
index 6e41063a5..6ac7acd74 100644
--- a/CHANGES
+++ b/CHANGES
@@ -15,6 +15,17 @@ CHANGES
     ConcurrentModificationError in an "except:"
     clause.
     
+  - An object that's been deleted now gets a flag
+    'deleted', which prohibits the object from
+    being re-add()ed to the session, as previously
+    the object would live in the identity map
+    silently until its attributes were accessed.
+    The make_transient() function now resets this
+    flag along with the "key" flag.
+
+  - make_transient() can be safely called on an
+    already transient instance.
+    
   - a warning is emitted in mapper() if the polymorphic_on
     column is not present either in direct or derived
     form in the mapped selectable or in the 
@@ -135,21 +146,6 @@ CHANGES
     of 64 for index names, separate from their
     overall max length of 255.  [ticket:1412]
 
-  - Calling fetchone() or similar on a result that
-    has already been exhausted, has been closed,
-    or is not a result-returning result now 
-    raises ResourceClosedError, a subclass of
-    InvalidRequestError, in all cases, regardless
-    of backend.  Previously, some DBAPIs would
-    raise ProgrammingError (i.e. pysqlite), others
-    would return None leading to downstream breakages
-    (i.e. MySQL-python).
-
-  - Connection, ResultProxy, as well as Session use
-    ResourceClosedError for all "this
-    connection/transaction/result is closed" types of
-    errors.
-
   - the text() construct, if placed in a column
     oriented situation, will at least return NULLTYPE
     for its type instead of None, allowing it to 
@@ -175,7 +171,33 @@ CHANGES
     operations which depend on the identity of the 
     _Labels themselves to return the correct result
     - fixes ORM bug [ticket:1852].
-    
+
+- engine
+
+  - Calling fetchone() or similar on a result that
+    has already been exhausted, has been closed,
+    or is not a result-returning result now 
+    raises ResourceClosedError, a subclass of
+    InvalidRequestError, in all cases, regardless
+    of backend.  Previously, some DBAPIs would
+    raise ProgrammingError (i.e. pysqlite), others
+    would return None leading to downstream breakages
+    (i.e. MySQL-python).
+
+  - Fixed bug in Connection whereby if a "disconnect"
+    event occurred in the "initialize" phase of the
+    first connection pool connect, an AttributeError
+    would be raised when the Connection would attempt
+    to invalidate the DBAPI connection.  [ticket:1894]
+
+  - Connection, ResultProxy, as well as Session use
+    ResourceClosedError for all "this
+    connection/transaction/result is closed" types of
+    errors.
+
+  - Connection.invalidate() can be called more than
+    once and subsequent calls do nothing.
+
 - declarative
   - if @classproperty is used with a regular class-bound
     mapper property attribute, it will be called to get the
diff --git a/examples/beaker_caching/caching_query.py b/examples/beaker_caching/caching_query.py
index 0b2238282..a94eea6ac 100644
--- a/examples/beaker_caching/caching_query.py
+++ b/examples/beaker_caching/caching_query.py
@@ -62,6 +62,14 @@ class CachingQuery(Query):
         """override __iter__ to pull results from Beaker
            if particular attributes have been configured.
            
+           Note that this approach does *not* detach the loaded objects from
+           the current session. If the cache backend is an in-process cache
+           (like "memory") and lives beyond the scope of the current session's
+           transaction, those objects may be expired. The method here can be
+           modified to first expunge() each loaded item from the current
+           session before returning the list of items, so that the items
+           in the cache are not the same ones in the current Session.
+           
         """
         if hasattr(self, '_cache_parameters'):
             return self.get_value(createfunc=lambda: list(Query.__iter__(self)))
diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py
index b481261d5..14ebf916b 100644
--- a/lib/sqlalchemy/engine/base.py
+++ b/lib/sqlalchemy/engine/base.py
@@ -911,6 +911,14 @@ class Connection(Connectable):
             raise exc.ResourceClosedError("This Connection is closed")
 
     @property
+    def _connection_is_valid(self):
+        # use getattr() for is_valid to support exceptions raised in
+        # dialect initializer, where the connection is not wrapped in
+        # _ConnectionFairy
+        
+        return getattr(self.__connection, 'is_valid', False)
+
+    @property
     def info(self):
         """A collection of per-DB-API connection instance properties."""
 
@@ -954,15 +962,18 @@ class Connection(Connectable):
         operations in a non-transactional state.
 
         """
-
+        if self.invalidated:
+            return
+            
         if self.closed:
             raise exc.ResourceClosedError("This Connection is closed")
 
-        if self.__connection.is_valid:
+        if self._connection_is_valid:
             self.__connection.invalidate(exception)
         del self.__connection
         self.__invalid = True
-
+    
+        
     def detach(self):
         """Detach the underlying DB-API connection from its connection pool.
 
@@ -1055,11 +1066,8 @@ class Connection(Connectable):
             raise
 
     def _rollback_impl(self):
-        # use getattr() for is_valid to support exceptions raised in 
-        # dialect initializer, 
-        # where we do not yet have the pool wrappers plugged in
         if not self.closed and not self.invalidated and \
-                        getattr(self.__connection, 'is_valid', False):
+                        self._connection_is_valid:
             if self._echo:
                 self.engine.logger.info("ROLLBACK")
             try:
@@ -1085,37 +1093,37 @@ class Connection(Connectable):
         if name is None:
             self.__savepoint_seq += 1
             name = 'sa_savepoint_%s' % self.__savepoint_seq
-        if self.__connection.is_valid:
+        if self._connection_is_valid:
             self.engine.dialect.do_savepoint(self, name)
             return name
 
     def _rollback_to_savepoint_impl(self, name, context):
-        if self.__connection.is_valid:
+        if self._connection_is_valid:
             self.engine.dialect.do_rollback_to_savepoint(self, name)
         self.__transaction = context
 
     def _release_savepoint_impl(self, name, context):
-        if self.__connection.is_valid:
+        if self._connection_is_valid:
             self.engine.dialect.do_release_savepoint(self, name)
         self.__transaction = context
 
     def _begin_twophase_impl(self, xid):
-        if self.__connection.is_valid:
+        if self._connection_is_valid:
             self.engine.dialect.do_begin_twophase(self, xid)
 
     def _prepare_twophase_impl(self, xid):
-        if self.__connection.is_valid:
+        if self._connection_is_valid:
             assert isinstance(self.__transaction, TwoPhaseTransaction)
             self.engine.dialect.do_prepare_twophase(self, xid)
 
     def _rollback_twophase_impl(self, xid, is_prepared):
-        if self.__connection.is_valid:
+        if self._connection_is_valid:
             assert isinstance(self.__transaction, TwoPhaseTransaction)
             self.engine.dialect.do_rollback_twophase(self, xid, is_prepared)
         self.__transaction = None
 
     def _commit_twophase_impl(self, xid, is_prepared):
-        if self.__connection.is_valid:
+        if self._connection_is_valid:
             assert isinstance(self.__transaction, TwoPhaseTransaction)
             self.engine.dialect.do_commit_twophase(self, xid, is_prepared)
         self.__transaction = None
diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index 86d5dd773..06d5b89a1 100644
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -278,8 +278,11 @@ class SessionTransaction(object):
 
         for s in set(self._new).union(self.session._new):
             self.session._expunge_state(s)
-
+            
         for s in set(self._deleted).union(self.session._deleted):
+            if s.deleted:
+                # assert s in self._deleted
+                del s.deleted
             self.session._update_impl(s)
 
         assert not self.session._deleted
@@ -1102,6 +1105,7 @@ class Session(object):
 
         self.identity_map.discard(state)
         self._deleted.pop(state, None)
+        state.deleted = True
 
     def _save_without_cascade(self, instance):
         """Used by scoping.py to save on init without cascade."""
@@ -1309,7 +1313,13 @@ class Session(object):
             raise sa_exc.InvalidRequestError(
                 "Instance '%s' is not persisted" %
                 mapperutil.state_str(state))
-
+        
+        if state.deleted:
+            raise sa_exc.InvalidRequestError(
+                "Instance '%s' has been deleted.  Use the make_transient() "
+                "function to send this object back to the transient state." %
+                mapperutil.state_str(state)
+            )
         self._attach(state)
         self._deleted.pop(state, None)
         self.identity_map.add(state)
@@ -1655,7 +1665,9 @@ def make_transient(instance):
     This will remove its association with any 
     session and additionally will remove its "identity key",
     such that it's as though the object were newly constructed,
-    except retaining its values.
+    except retaining its values.   It also resets the
+    "deleted" flag on the state if this object
+    had been explicitly deleted by its session.
     
     Attributes which were "expired" or deferred at the
     instance level are reverted to undefined, and 
@@ -1670,8 +1682,10 @@ def make_transient(instance):
     # remove expired state and 
     # deferred callables
     state.callables.clear()
-    del state.key
-    
+    if state.key:
+        del state.key
+    if state.deleted:
+        del state.deleted
     
 def object_session(instance):
     """Return the ``Session`` to which instance belongs.
diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py
index 82e7e9130..f6828f5a9 100644
--- a/lib/sqlalchemy/orm/state.py
+++ b/lib/sqlalchemy/orm/state.py
@@ -22,6 +22,7 @@ class InstanceState(object):
     _strong_obj = None
     modified = False
     expired = False
+    deleted = False
     
     def __init__(self, obj, manager):
         self.class_ = obj.__class__
diff --git a/test/engine/test_pool.py b/test/engine/test_pool.py
index 9db65d2ab..94d75a9a8 100644
--- a/test/engine/test_pool.py
+++ b/test/engine/test_pool.py
@@ -668,7 +668,7 @@ class QueuePoolTest(PoolTestBase):
         c1 = None
         c1 = p.connect()
         assert c1.connection.id != c_id
-
+        
     def test_recreate(self):
         dbapi = MockDBAPI()
         p = pool.QueuePool(creator=lambda : dbapi.connect('foo.db'),
diff --git a/test/engine/test_reconnect.py b/test/engine/test_reconnect.py
index b92c88066..8ef851358 100644
--- a/test/engine/test_reconnect.py
+++ b/test/engine/test_reconnect.py
@@ -1,13 +1,12 @@
-from sqlalchemy.test.testing import eq_
+from sqlalchemy.test.testing import eq_, assert_raises
 import time
 import weakref
 from sqlalchemy import select, MetaData, Integer, String, pool
-from sqlalchemy.test.schema import Table
-from sqlalchemy.test.schema import Column
+from sqlalchemy.test.schema import Table, Column
 import sqlalchemy as tsa
 from sqlalchemy.test import TestBase, testing, engines
 from sqlalchemy.test.util import gc_collect
-
+from sqlalchemy import exc
 
 class MockDisconnect(Exception):
     pass
@@ -257,7 +256,39 @@ class RealReconnectTest(TestBase):
         assert not conn.invalidated
 
         conn.close()
+    
+    def test_invalidate_twice(self):
+        conn = engine.connect()
+        conn.invalidate()
+        conn.invalidate()
+    
+    def test_explode_in_initializer(self):
+        engine = engines.testing_engine()
+        def broken_initialize(connection):
+            connection.execute("select fake_stuff from _fake_table")
+            
+        engine.dialect.initialize = broken_initialize
+        
+        # raises a DBAPIError, not an AttributeError
+        assert_raises(exc.DBAPIError, engine.connect)
 
+        # dispose connections so we get a new one on
+        # next go
+        engine.dispose()
+
+        p1 = engine.pool
+        
+        def is_disconnect(e):
+            return True
+            
+        engine.dialect.is_disconnect = is_disconnect
+
+        # invalidate() also doesn't screw up
+        assert_raises(exc.DBAPIError, engine.connect)
+        
+        # pool was recreated
+        assert engine.pool is not p1
+        
     def test_null_pool(self):
         engine = \
             engines.reconnecting_engine(options=dict(poolclass=pool.NullPool))
diff --git a/test/orm/test_collection.py b/test/orm/test_collection.py
index a33d2d6d1..730605a76 100644
--- a/test/orm/test_collection.py
+++ b/test/orm/test_collection.py
@@ -1566,14 +1566,11 @@ class DictHelpersTest(_base.MappedTest):
     def test_column_mapped_assertions(self):
         assert_raises_message(sa_exc.ArgumentError,
                               "Column-based expression object expected "
-                              "for argument 'mapping_spec'; got: 'a', "
-                              "type <type 'str'>",
+                              "for argument 'mapping_spec'; got: 'a'",
                               collections.column_mapped_collection, 'a')
         assert_raises_message(sa_exc.ArgumentError,
                               "Column-based expression object expected "
-                              "for argument 'mapping_spec'; got: 'a', "
-                              "type <class 'sqlalchemy.sql.expression._"
-                              "TextClause'>",
+                              "for argument 'mapping_spec'; got: 'a'",
                               collections.column_mapped_collection,
                               text('a'))
         
diff --git a/test/orm/test_expire.py b/test/orm/test_expire.py
index c0c96f873..c8bdf1719 100644
--- a/test/orm/test_expire.py
+++ b/test/orm/test_expire.py
@@ -89,6 +89,10 @@ class ExpireTest(_fixtures.FixtureTest):
         assert s.query(User).get(10) is None
         assert u not in s # and expunges
 
+        # trick the "deleted" flag so we can re-add for the sake
+        # of this test
+        del attributes.instance_state(u).deleted
+        
         # add it back
         s.add(u)
         # nope, raises ObjectDeletedError
diff --git a/test/orm/test_session.py b/test/orm/test_session.py
index 779db304e..4976db131 100644
--- a/test/orm/test_session.py
+++ b/test/orm/test_session.py
@@ -280,6 +280,52 @@ class SessionTest(_fixtures.FixtureTest):
         assert u1.id is None
         assert u1.name is None
 
+        # works twice
+        make_transient(u1)
+        
+        sess.close()
+        
+        u1.name = 'test2'
+        sess.add(u1)
+        sess.flush()
+        assert u1 in sess
+        sess.delete(u1)
+        sess.flush()
+        assert u1 not in sess
+        
+        assert_raises(sa.exc.InvalidRequestError, sess.add, u1)
+        make_transient(u1)
+        sess.add(u1)
+        sess.flush()
+        assert u1 in sess
+
+    @testing.resolve_artifact_names
+    def test_deleted_flag(self):
+        mapper(User, users)
+        
+        sess = sessionmaker()()
+        
+        u1 = User(name='u1')
+        sess.add(u1)
+        sess.commit()
+        
+        sess.delete(u1)
+        sess.flush()
+        assert u1 not in sess
+        assert_raises(sa.exc.InvalidRequestError, sess.add, u1)
+        sess.rollback()
+        assert u1 in sess
+        
+        sess.delete(u1)
+        sess.commit()
+        assert u1 not in sess
+        assert_raises(sa.exc.InvalidRequestError, sess.add, u1)
+        
+        make_transient(u1)
+        sess.add(u1)
+        sess.commit()
+        
+        eq_(sess.query(User).count(), 1)
         
     @testing.resolve_artifact_names
     def test_autoflush_expressions(self):