From f14ec0f517eaf6956962d2d0a34dacb69bead46b Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sun, 14 Apr 2013 19:32:54 -0400 Subject: [PATCH 01/14] add cymysql... --- doc/build/dialects/mysql.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/build/dialects/mysql.rst b/doc/build/dialects/mysql.rst index b5119f23fb..1e2784554f 100644 --- a/doc/build/dialects/mysql.rst +++ b/doc/build/dialects/mysql.rst @@ -175,6 +175,11 @@ MySQL-Connector .. automodule:: sqlalchemy.dialects.mysql.mysqlconnector +cymysql +------------ + +.. automodule:: sqlalchemy.dialects.mysql.cymysql + Google App Engine ----------------------- From e5d0592180a554a1220985d28dab8533030281f0 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 18 Apr 2013 10:34:59 -0400 Subject: [PATCH 02/14] - additional test + correction for [ticket:2699] --- lib/sqlalchemy/orm/strategies.py | 9 ++--- test/orm/test_subquery_relations.py | 53 ++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index e08bb40cb7..0eed50ea41 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -778,11 +778,12 @@ class SubqueryLoader(AbstractRelationshipLoader): # to look only for significant columns q = orig_query._clone().correlate(None) - # TODO: why does polymporphic etc. require hardcoding - # into _adapt_col_list ? Does query.add_columns(...) work - # with polymorphic loading ? - if entity_mapper.isa(leftmost_mapper): + # set a real "from" if not present, as this is more + # accurate than just going off of the column expression + if not q._from_obj and entity_mapper.isa(leftmost_mapper): q._set_select_from(entity_mapper) + + # select from the identity columns of the outer q._set_entities(q._adapt_col_list(leftmost_attr)) if q._order_by is False: diff --git a/test/orm/test_subquery_relations.py b/test/orm/test_subquery_relations.py index 80dd73e983..3ee94cae9b 100644 --- a/test/orm/test_subquery_relations.py +++ b/test/orm/test_subquery_relations.py @@ -1036,7 +1036,7 @@ class BaseRelationFromJoinedSubclassTest(_Polymorphic): sess.add_all([e1, e2]) sess.flush() - def test_correct_subquery(self): + def test_correct_subquery_nofrom(self): sess = create_session() # use Person.paperwork here just to give the least # amount of context @@ -1083,6 +1083,57 @@ class BaseRelationFromJoinedSubclassTest(_Polymorphic): ) ) + def test_correct_subquery_existingfrom(self): + sess = create_session() + # use Person.paperwork here just to give the least + # amount of context + q = sess.query(Engineer).\ + filter(Engineer.primary_language == 'java').\ + join(Engineer.paperwork).\ + filter(Paperwork.description == "tps report #2").\ + options(subqueryload(Person.paperwork)) + def go(): + eq_(q.one().paperwork, + [Paperwork(description="tps report #1"), + Paperwork(description="tps report #2")], + + ) + self.assert_sql_execution( + testing.db, + go, + CompiledSQL( + "SELECT people.person_id AS people_person_id, " + "people.name AS people_name, people.type AS people_type, " + "engineers.engineer_id AS engineers_engineer_id, " + "engineers.primary_language AS engineers_primary_language " + "FROM people JOIN engineers " + "ON people.person_id = engineers.engineer_id " + "JOIN paperwork ON people.person_id = paperwork.person_id " + "WHERE engineers.primary_language = :primary_language_1 " + "AND paperwork.description = :description_1", + {"primary_language_1": "java", + "description_1": "tps report #2"} + ), + CompiledSQL( + "SELECT paperwork.paperwork_id AS paperwork_paperwork_id, " + "paperwork.description AS paperwork_description, " + "paperwork.person_id AS paperwork_person_id, " + "anon_1.people_person_id AS anon_1_people_person_id " + "FROM (SELECT people.person_id AS people_person_id " + "FROM people JOIN engineers ON people.person_id = " + "engineers.engineer_id JOIN paperwork " + "ON people.person_id = paperwork.person_id " + "WHERE engineers.primary_language = :primary_language_1 AND " + "paperwork.description = :description_1) AS anon_1 " + "JOIN paperwork ON anon_1.people_person_id = " + "paperwork.person_id " + "ORDER BY anon_1.people_person_id, paperwork.paperwork_id", + {"primary_language_1": "java", + "description_1": "tps report #2"} + ) + ) + + class SelfReferentialTest(fixtures.MappedTest): From 0bb05ffdf066ba108883a0a4165cb11894fb3d88 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 18 Apr 2013 11:00:12 -0400 Subject: [PATCH 03/14] Reworked internal exception raises that emit a rollback() before re-raising, so that the stack trace is preserved from sys.exc_info() before entering the rollback. This so that the traceback is preserved when using coroutine frameworks which may have switched contexts before the rollback function returns. [ticket:2703] --- doc/build/changelog/changelog_08.rst | 11 +++++ lib/sqlalchemy/engine/base.py | 64 +++++++++++----------------- lib/sqlalchemy/engine/default.py | 2 - lib/sqlalchemy/engine/result.py | 6 --- lib/sqlalchemy/orm/session.py | 12 +++--- lib/sqlalchemy/util/__init__.py | 5 ++- lib/sqlalchemy/util/compat.py | 26 +++++++++++ lib/sqlalchemy/util/langhelpers.py | 29 +++++++++++++ 8 files changed, 99 insertions(+), 56 deletions(-) diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index 60d61cb8da..a23d05fe79 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -6,6 +6,17 @@ .. changelog:: :version: 0.8.1 + .. change:: + :tags: bug, sql + :tickets: 2703 + + Reworked internal exception raises that emit + a rollback() before re-raising, so that the stack + trace is preserved from sys.exc_info() before entering + the rollback. This so that the traceback is preserved + when using coroutine frameworks which may have switched + contexts before the rollback function returns. + .. change:: :tags: bug, orm :tickets: 2697 diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py index e40af62199..b4c9b1e1c7 100644 --- a/lib/sqlalchemy/engine/base.py +++ b/lib/sqlalchemy/engine/base.py @@ -462,7 +462,6 @@ class Connection(Connectable): self.engine.dialect.do_begin(self.connection) except Exception, e: self._handle_dbapi_exception(e, None, None, None, None) - raise def _rollback_impl(self): if self._has_events: @@ -476,7 +475,6 @@ class Connection(Connectable): self.__transaction = None except Exception, e: self._handle_dbapi_exception(e, None, None, None, None) - raise else: self.__transaction = None @@ -491,7 +489,6 @@ class Connection(Connectable): self.__transaction = None except Exception, e: self._handle_dbapi_exception(e, None, None, None, None) - raise def _savepoint_impl(self, name=None): if self._has_events: @@ -693,7 +690,6 @@ class Connection(Connectable): dialect, self, conn) except Exception, e: self._handle_dbapi_exception(e, None, None, None, None) - raise ret = ctx._exec_default(default, None) if self.should_close_with_result: @@ -830,7 +826,6 @@ class Connection(Connectable): self._handle_dbapi_exception(e, str(statement), parameters, None, None) - raise if context.compiled: context.pre_exec() @@ -877,7 +872,6 @@ class Connection(Connectable): parameters, cursor, context) - raise if self._has_events: self.dispatch.after_cursor_execute(self, cursor, @@ -952,7 +946,6 @@ class Connection(Connectable): parameters, cursor, None) - raise def _safe_close_cursor(self, cursor): """Close the given cursor, catching exceptions @@ -983,22 +976,21 @@ class Connection(Connectable): cursor, context): + exc_info = sys.exc_info() + if not self._is_disconnect: self._is_disconnect = isinstance(e, self.dialect.dbapi.Error) and \ not self.closed and \ self.dialect.is_disconnect(e, self.__connection, cursor) if self._reentrant_error: - # Py3K - #raise exc.DBAPIError.instance(statement, parameters, e, - # self.dialect.dbapi.Error) from e - # Py2K - raise exc.DBAPIError.instance(statement, + util.raise_from_cause( + exc.DBAPIError.instance(statement, parameters, e, - self.dialect.dbapi.Error), \ - None, sys.exc_info()[2] - # end Py2K + self.dialect.dbapi.Error), + exc_info + ) self._reentrant_error = True try: # non-DBAPI error - if we already got a context, @@ -1021,26 +1013,18 @@ class Connection(Connectable): self._safe_close_cursor(cursor) self._autorollback() - if not should_wrap: - return + if should_wrap: + util.raise_from_cause( + exc.DBAPIError.instance( + statement, + parameters, + e, + self.dialect.dbapi.Error, + connection_invalidated=self._is_disconnect), + exc_info + ) - # Py3K - #raise exc.DBAPIError.instance( - # statement, - # parameters, - # e, - # self.dialect.dbapi.Error, - # connection_invalidated=self._is_disconnect) \ - # from e - # Py2K - raise exc.DBAPIError.instance( - statement, - parameters, - e, - self.dialect.dbapi.Error, - connection_invalidated=self._is_disconnect), \ - None, sys.exc_info()[2] - # end Py2K + util.reraise(*exc_info) finally: del self._reentrant_error @@ -1115,8 +1099,8 @@ class Connection(Connectable): trans.commit() return ret except: - trans.rollback() - raise + with util.safe_reraise(): + trans.rollback() def run_callable(self, callable_, *args, **kwargs): """Given a callable object or function, execute it, passing @@ -1222,8 +1206,8 @@ class Transaction(object): try: self.commit() except: - self.rollback() - raise + with util.safe_reraise(): + self.rollback() else: self.rollback() @@ -1548,8 +1532,8 @@ class Engine(Connectable, log.Identified): try: trans = conn.begin() except: - conn.close() - raise + with util.safe_reraise(): + conn.close() return Engine._trans_ctx(conn, trans, close_with_result) def transaction(self, callable_, *args, **kwargs): diff --git a/lib/sqlalchemy/engine/default.py b/lib/sqlalchemy/engine/default.py index abb9f0fc3d..daa9fe0855 100644 --- a/lib/sqlalchemy/engine/default.py +++ b/lib/sqlalchemy/engine/default.py @@ -737,7 +737,6 @@ class DefaultExecutionContext(interfaces.ExecutionContext): except Exception, e: self.root_connection._handle_dbapi_exception( e, None, None, None, self) - raise else: inputsizes = {} for key in self.compiled.bind_names.values(): @@ -756,7 +755,6 @@ class DefaultExecutionContext(interfaces.ExecutionContext): except Exception, e: self.root_connection._handle_dbapi_exception( e, None, None, None, self) - raise def _exec_default(self, default, type_): if default.is_sequence: diff --git a/lib/sqlalchemy/engine/result.py b/lib/sqlalchemy/engine/result.py index 1c148e1f0e..88930081e9 100644 --- a/lib/sqlalchemy/engine/result.py +++ b/lib/sqlalchemy/engine/result.py @@ -443,7 +443,6 @@ class ResultProxy(object): except Exception, e: self.connection._handle_dbapi_exception( e, None, None, self.cursor, self.context) - raise @property def lastrowid(self): @@ -467,7 +466,6 @@ class ResultProxy(object): self.connection._handle_dbapi_exception( e, None, None, self._saved_cursor, self.context) - raise @property def returns_rows(self): @@ -752,7 +750,6 @@ class ResultProxy(object): self.connection._handle_dbapi_exception( e, None, None, self.cursor, self.context) - raise def fetchmany(self, size=None): """Fetch many rows, just like DB-API @@ -772,7 +769,6 @@ class ResultProxy(object): self.connection._handle_dbapi_exception( e, None, None, self.cursor, self.context) - raise def fetchone(self): """Fetch one row, just like DB-API ``cursor.fetchone()``. @@ -792,7 +788,6 @@ class ResultProxy(object): self.connection._handle_dbapi_exception( e, None, None, self.cursor, self.context) - raise def first(self): """Fetch the first row and then close the result set unconditionally. @@ -809,7 +804,6 @@ class ResultProxy(object): self.connection._handle_dbapi_exception( e, None, None, self.cursor, self.context) - raise try: if row is not None: diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 71e617e365..91e4f97361 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -341,8 +341,8 @@ class SessionTransaction(object): for t in set(self._connections.values()): t[1].prepare() except: - self.rollback() - raise + with util.safe_reraise(): + self.rollback() self._state = PREPARED @@ -441,8 +441,8 @@ class SessionTransaction(object): try: self.commit() except: - self.rollback() - raise + with util.safe_reraise(): + self.rollback() else: self.rollback() @@ -1928,8 +1928,8 @@ class Session(_SessionClassMethods): transaction.commit() except: - transaction.rollback(_capture_exception=True) - raise + with util.safe_reraise(): + transaction.rollback(_capture_exception=True) def is_modified(self, instance, include_collections=True, passive=True): diff --git a/lib/sqlalchemy/util/__init__.py b/lib/sqlalchemy/util/__init__.py index 57bbdca85b..3fa06c7932 100644 --- a/lib/sqlalchemy/util/__init__.py +++ b/lib/sqlalchemy/util/__init__.py @@ -6,7 +6,8 @@ from .compat import callable, cmp, reduce, \ threading, py3k, py3k_warning, jython, pypy, cpython, win32, set_types, \ - pickle, dottedgetter, parse_qsl, namedtuple, next, WeakSet + pickle, dottedgetter, parse_qsl, namedtuple, next, WeakSet, reraise, \ + raise_from_cause from ._collections import KeyedTuple, ImmutableContainer, immutabledict, \ Properties, OrderedProperties, ImmutableProperties, OrderedDict, \ @@ -26,7 +27,7 @@ from .langhelpers import iterate_attributes, class_hierarchy, \ duck_type_collection, assert_arg_type, symbol, dictlike_iteritems,\ classproperty, set_creation_order, warn_exception, warn, NoneType,\ constructor_copy, methods_equivalent, chop_traceback, asint,\ - generic_repr, counter, PluginLoader, hybridmethod + generic_repr, counter, PluginLoader, hybridmethod, safe_reraise from .deprecations import warn_deprecated, warn_pending_deprecation, \ deprecated, pending_deprecation diff --git a/lib/sqlalchemy/util/compat.py b/lib/sqlalchemy/util/compat.py index 2a0f06f8ec..0f0a2f6742 100644 --- a/lib/sqlalchemy/util/compat.py +++ b/lib/sqlalchemy/util/compat.py @@ -140,3 +140,29 @@ else: def b(s): return s + +if py3k: + def reraise(tp, value, tb=None, cause=None): + if cause is not None: + value.__cause__ = cause + if value.__traceback__ is not tb: + raise value.with_traceback(tb) + raise value + + def raise_from_cause(exception, exc_info): + exc_type, exc_value, exc_tb = exc_info + reraise(type(exception), exception, tb=exc_tb, cause=exc_value) +else: + exec("""def reraise(tp, value, tb=None, cause=None): + raise tp, value, tb + """) + + def raise_from_cause(exception, exc_info): + # not as nice as that of Py3K, but at least preserves + # the code line where the issue occurred + exc_type, exc_value, exc_tb = exc_info + reraise(type(exception), exception, tb=exc_tb) + + + + diff --git a/lib/sqlalchemy/util/langhelpers.py b/lib/sqlalchemy/util/langhelpers.py index e3aed24d8a..bba8ad734e 100644 --- a/lib/sqlalchemy/util/langhelpers.py +++ b/lib/sqlalchemy/util/langhelpers.py @@ -20,6 +20,7 @@ from .compat import set_types, threading, \ from functools import update_wrapper from .. import exc import hashlib +from . import compat def md5_hex(x): # Py3K @@ -28,6 +29,34 @@ def md5_hex(x): m.update(x) return m.hexdigest() +class safe_reraise(object): + """Reraise an exception after invoking some + handler code. + + Stores the existing exception info before + invoking so that it is maintained across a potential + coroutine context switch. + + e.g.:: + + try: + sess.commit() + except: + with safe_reraise(): + sess.rollback() + + """ + + def __enter__(self): + self._exc_info = sys.exc_info() + + def __exit__(self, type_, value, traceback): + if type_ is None: + exc_type, exc_value, exc_tb = self._exc_info + compat.reraise(exc_type, exc_value, exc_tb) + else: + compat.reraise(type_, value, traceback) + def decode_slice(slc): """decode a slice object as sent to __getitem__. From 6d93918b9b68202fdcb1f76f865efd5cde154963 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 18 Apr 2013 11:04:51 -0400 Subject: [PATCH 04/14] - remove reference to _exc_info before reraise to reduce cycles --- lib/sqlalchemy/util/langhelpers.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/sqlalchemy/util/langhelpers.py b/lib/sqlalchemy/util/langhelpers.py index bba8ad734e..f6d9164e6c 100644 --- a/lib/sqlalchemy/util/langhelpers.py +++ b/lib/sqlalchemy/util/langhelpers.py @@ -51,10 +51,13 @@ class safe_reraise(object): self._exc_info = sys.exc_info() def __exit__(self, type_, value, traceback): + # see #2703 for notes if type_ is None: exc_type, exc_value, exc_tb = self._exc_info + self._exc_info = None # remove potential circular references compat.reraise(exc_type, exc_value, exc_tb) else: + self._exc_info = None # remove potential circular references compat.reraise(type_, value, traceback) def decode_slice(slc): From 1b9c1a2ecf549c54dbf0414fdf4ea197d4ba6dbd Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 18 Apr 2013 12:01:16 -0400 Subject: [PATCH 05/14] - test + changelog for [ticket:2691] --- doc/build/changelog/changelog_08.rst | 9 +++++++++ test/engine/test_reconnect.py | 24 +++++++++++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index a23d05fe79..224690cc29 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -6,6 +6,15 @@ .. changelog:: :version: 0.8.1 + .. change:: + :tags: bug, sql + :tickets: 2691 + + Fixed bug where disconnect detect on error would + raise an attribute error if the error were being + raised after the Connection object had already + been closed. + .. change:: :tags: bug, sql :tickets: 2703 diff --git a/test/engine/test_reconnect.py b/test/engine/test_reconnect.py index 86f646f33c..9aecb81a99 100644 --- a/test/engine/test_reconnect.py +++ b/test/engine/test_reconnect.py @@ -51,6 +51,7 @@ class MockCursor(object): def __init__(self, parent): self.explode = parent.explode self.description = () + self.closed = False def execute(self, *args, **kwargs): if self.explode == 'execute': raise MockDisconnect("Lost the DB connection on execute") @@ -60,10 +61,20 @@ class MockCursor(object): elif self.explode in ('rollback', 'rollback_no_disconnect'): raise MockError( "something broke on execute but we didn't lose the connection") + elif args and "select" in args[0]: + self.description = [('foo', None, None, None, None, None)] else: return + def fetchall(self): + if self.closed: + raise MockError("cursor closed") + return [] + def fetchone(self): + if self.closed: + raise MockError("cursor closed") + return None def close(self): - pass + self.closed = True db, dbapi = None, None class MockReconnectTest(fixtures.TestBase): @@ -294,6 +305,17 @@ class MockReconnectTest(fixtures.TestBase): conn.execute, select([1]) ) + def test_check_disconnect_no_cursor(self): + conn = db.connect() + result = conn.execute("select 1") + result.cursor.close() + conn.close() + assert_raises_message( + tsa.exc.DBAPIError, + "cursor closed", + list, result + ) + class CursorErrTest(fixtures.TestBase): def setup(self): From 835f0abb59c10c1204884df1a19f088cf55d49d7 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 18 Apr 2013 15:45:15 -0400 Subject: [PATCH 06/14] python2.5 fix --- lib/sqlalchemy/orm/session.py | 3 ++- lib/sqlalchemy/util/compat.py | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 91e4f97361..408b119a07 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -3,9 +3,10 @@ # # This module is part of SQLAlchemy and is released under # the MIT License: http://www.opensource.org/licenses/mit-license.php - """Provides the Session class and related utilities.""" +from __future__ import with_statement + import weakref from .. import util, sql, engine, exc as sa_exc, event from ..sql import util as sql_util, expression diff --git a/lib/sqlalchemy/util/compat.py b/lib/sqlalchemy/util/compat.py index 0f0a2f6742..033a87cc7b 100644 --- a/lib/sqlalchemy/util/compat.py +++ b/lib/sqlalchemy/util/compat.py @@ -153,9 +153,8 @@ if py3k: exc_type, exc_value, exc_tb = exc_info reraise(type(exception), exception, tb=exc_tb, cause=exc_value) else: - exec("""def reraise(tp, value, tb=None, cause=None): - raise tp, value, tb - """) + exec("def reraise(tp, value, tb=None, cause=None):\n" + " raise tp, value, tb\n") def raise_from_cause(exception, exc_info): # not as nice as that of Py3K, but at least preserves From 0790efcf87a268fd8bc810b711fe2b9760bcf1e6 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 18 Apr 2013 17:41:30 -0400 Subject: [PATCH 07/14] - this pymssql test needs to be against the pymssql dialect - Part of a longer series of fixes needed for pyodbc+ mssql, a CAST to NVARCHAR(max) has been added to the bound parameter for the table name and schema name in all information schema queries to avoid the issue of comparing NVARCHAR to NTEXT, which seems to be rejected by the ODBC driver in some cases, such as FreeTDS (0.91 only?) plus unicode bound parameters being passed. The issue seems to be specific to the SQL Server information schema tables and the workaround is harmless for those cases where the problem doesn't exist in the first place. [ticket:2355] --- doc/build/changelog/changelog_08.rst | 14 ++++++++++++++ .../dialects/mssql/information_schema.py | 4 ++++ test/dialect/test_mssql.py | 2 +- 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index 224690cc29..25a8d4c190 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -6,6 +6,20 @@ .. changelog:: :version: 0.8.1 + .. change:: + :tags: bug, mssql + :tickets: 2355 + + Part of a longer series of fixes needed for pyodbc+ + mssql, a CAST to NVARCHAR(max) has been added to the bound + parameter for the table name and schema name in all information schema + queries to avoid the issue of comparing NVARCHAR to NTEXT, + which seems to be rejected by the ODBC driver in some cases, + such as FreeTDS (0.91 only?) plus unicode bound parameters being passed. + The issue seems to be specific to the SQL Server information + schema tables and the workaround is harmless for those cases + where the problem doesn't exist in the first place. + .. change:: :tags: bug, sql :tickets: 2691 diff --git a/lib/sqlalchemy/dialects/mssql/information_schema.py b/lib/sqlalchemy/dialects/mssql/information_schema.py index 35ce2450e0..80e59d323e 100644 --- a/lib/sqlalchemy/dialects/mssql/information_schema.py +++ b/lib/sqlalchemy/dialects/mssql/information_schema.py @@ -8,6 +8,7 @@ from ... import Table, MetaData, Column from ...types import String, Unicode, Integer, TypeDecorator +from ... import cast ischema = MetaData() @@ -22,6 +23,9 @@ class CoerceUnicode(TypeDecorator): # end Py2K return value + def bind_expression(self, bindvalue): + return cast(bindvalue, Unicode) + schemata = Table("SCHEMATA", ischema, Column("CATALOG_NAME", CoerceUnicode, key="catalog_name"), Column("SCHEMA_NAME", CoerceUnicode, key="schema_name"), diff --git a/test/dialect/test_mssql.py b/test/dialect/test_mssql.py index 0dfda9015c..f1cd3fe85b 100644 --- a/test/dialect/test_mssql.py +++ b/test/dialect/test_mssql.py @@ -1949,7 +1949,7 @@ class TypeRoundTripTest(fixtures.TestBase, AssertsExecutionResults, ComparesTabl engine.execute(tbl.delete()) class MonkeyPatchedBinaryTest(fixtures.TestBase): - __only_on__ = 'mssql' + __only_on__ = 'mssql+pymssql' def test_unicode(self): module = __import__('pymssql') From 23c43e94b0d7c5d69595d63cc205f3c08fe78fa3 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 18 Apr 2013 18:35:45 -0400 Subject: [PATCH 08/14] - remove erroneous second RelationshipCache class --- examples/dogpile_caching/caching_query.py | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/examples/dogpile_caching/caching_query.py b/examples/dogpile_caching/caching_query.py index 9a705cf314..81ca310606 100644 --- a/examples/dogpile_caching/caching_query.py +++ b/examples/dogpile_caching/caching_query.py @@ -181,24 +181,6 @@ class FromCache(MapperOption): """Process a Query during normal loading operation.""" query._cache_region = self -class RelationshipCache(MapperOption): - """Specifies that a Query as called within a "lazy load" - should load results from a cache.""" - - propagate_to_loaders = True - - def __init__(self, attribute, region="default"): - self.region = region - self.cls_ = attribute.property.parent.class_ - self.key = attribute.property.key - - def process_query_conditionally(self, query): - if query._current_path: - mapper, key = query._current_path[-2:] - if issubclass(mapper.class_, self.cls_) and \ - key == self.key: - query._cache_region = self - class RelationshipCache(MapperOption): """Specifies that a Query as called within a "lazy load" should load results from a cache.""" From b6bf8c2a3001c38684ef806678dd187926e1910b Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 18 Apr 2013 20:11:08 -0400 Subject: [PATCH 09/14] Fixed a long-standing bug in the caching example, where the limit/offset parameter values wouldn't be taken into account when computing the cache key. The _key_from_query() function has been simplified to work directly from the final compiled statement in order to get at both the full statement as well as the fully processed parameter list. --- doc/build/changelog/changelog_08.rst | 11 +++++++++++ examples/dogpile_caching/caching_query.py | 19 +++++-------------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index 25a8d4c190..0cf5e1f13c 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -6,6 +6,17 @@ .. changelog:: :version: 0.8.1 + .. change:: + :tags: bug, examples + + Fixed a long-standing bug in the caching example, where + the limit/offset parameter values wouldn't be taken into + account when computing the cache key. The + _key_from_query() function has been simplified to work + directly from the final compiled statement in order to get + at both the full statement as well as the fully processed + parameter list. + .. change:: :tags: bug, mssql :tickets: 2355 diff --git a/examples/dogpile_caching/caching_query.py b/examples/dogpile_caching/caching_query.py index 81ca310606..f4724fb0b6 100644 --- a/examples/dogpile_caching/caching_query.py +++ b/examples/dogpile_caching/caching_query.py @@ -136,24 +136,15 @@ def _key_from_query(query, qualifier=None): """ - v = [] - def visit_bindparam(bind): - - if bind.key in query._params: - value = query._params[bind.key] - elif bind.callable: - value = bind.callable() - else: - value = bind.value - - v.append(unicode(value)) - stmt = query.statement - visitors.traverse(stmt, {}, {'bindparam': visit_bindparam}) + compiled = stmt.compile() + params = compiled.params # here we return the key as a long string. our "key mangler" # set up with the region will boil it down to an md5. - return " ".join([unicode(stmt)] + v) + return " ".join( + [unicode(compiled)] + + [unicode(params[k]) for k in sorted(params)]) class FromCache(MapperOption): """Specifies that a Query should load results from a cache.""" From d657cf0600e9fa1aa49a4e27ba2063073d879987 Mon Sep 17 00:00:00 2001 From: Dan Ring Date: Fri, 19 Apr 2013 18:01:39 -0700 Subject: [PATCH 10/14] Fix mysql+gaerdbms dialect for changed exception format googleappengine v1.7.5 changed the exception format to be incompatible with MySQLDialect_gaerdbms#_extract_error_code This fix works for both old- and new-style exceptions. Changes causing the breakage: /trunk/python/google/storage/speckle/python/api/rdbms.py at https://code.google.com/p/googleappengine/source/detail?r=318 --- lib/sqlalchemy/dialects/mysql/gaerdbms.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/sqlalchemy/dialects/mysql/gaerdbms.py b/lib/sqlalchemy/dialects/mysql/gaerdbms.py index a93a78b738..ad0ce76385 100644 --- a/lib/sqlalchemy/dialects/mysql/gaerdbms.py +++ b/lib/sqlalchemy/dialects/mysql/gaerdbms.py @@ -65,10 +65,10 @@ class MySQLDialect_gaerdbms(MySQLDialect_mysqldb): return [], opts def _extract_error_code(self, exception): - match = re.compile(r"^(\d+):").match(str(exception)) + match = re.compile(r"^(\d+):|^\((\d+),").match(str(exception)) # The rdbms api will wrap then re-raise some types of errors # making this regex return no matches. - code = match.group(1) if match else None + code = match.group(1) or match.group(2) if match else None if code: return int(code) From 15b3d2f5bd3ff24b06945a3243eef0d3f523db15 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 19 Apr 2013 21:41:11 -0400 Subject: [PATCH 11/14] changelog for pullreq 54 gaerdbms --- doc/build/changelog/changelog_08.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index 0cf5e1f13c..c443e2735d 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -6,6 +6,14 @@ .. changelog:: :version: 0.8.1 + .. change:: + :tags: bug, mysql + :pullreq: 54 + + Updated a regexp to correctly extract error code on + google app engine v1.7.5 and newer. Courtesy + Dan Ring. + .. change:: :tags: bug, examples From 6c1972e4ecd7f2d738aa1578bc95d4a77820278d Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sat, 20 Apr 2013 02:45:08 -0400 Subject: [PATCH 12/14] Improved the behavior of instance management regarding the creation of strong references within the Session; an object will no longer have an internal reference cycle created if it's in the transient state or moves into the detached state - the strong ref is created only when the object is attached to a Session and is removed when the object is detached. This makes it somewhat safer for an object to have a `__del__()` method, even though this is not recommended, as relationships with backrefs produce cycles too. A warning has been added when a class with a `__del__()` method is mapped. [ticket:2708] --- doc/build/changelog/changelog_08.rst | 16 +++ lib/sqlalchemy/orm/instrumentation.py | 7 ++ lib/sqlalchemy/orm/mapper.py | 3 +- lib/sqlalchemy/orm/session.py | 8 +- lib/sqlalchemy/orm/state.py | 27 ++--- test/orm/test_instrumentation.py | 14 +++ test/orm/test_session.py | 144 ++++++++++++++++++++++++++ 7 files changed, 202 insertions(+), 17 deletions(-) diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index c443e2735d..bdba906bb5 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -6,6 +6,22 @@ .. changelog:: :version: 0.8.1 + .. change:: + :tags: bug + :tickets: 2708 + + Improved the behavior of instance management regarding + the creation of strong references within the Session; + an object will no longer have an internal reference cycle + created if it's in the transient state or moves into the + detached state - the strong ref is created only when the + object is attached to a Session and is removed when the + object is detached. This makes it somewhat safer for an + object to have a `__del__()` method, even though this is + not recommended, as relationships with backrefs produce + cycles too. A warning has been added when a class with + a `__del__()` method is mapped. + .. change:: :tags: bug, mysql :pullreq: 54 diff --git a/lib/sqlalchemy/orm/instrumentation.py b/lib/sqlalchemy/orm/instrumentation.py index 51cf9edeb8..0e71494c44 100644 --- a/lib/sqlalchemy/orm/instrumentation.py +++ b/lib/sqlalchemy/orm/instrumentation.py @@ -72,6 +72,13 @@ class ClassManager(dict): self.manage() self._instrument_init() + if '__del__' in class_.__dict__: + util.warn("__del__() method on class %s will " + "cause unreachable cycles and memory leaks, " + "as SQLAlchemy instrumentation often creates " + "reference cycles. Please remove this method." % + class_) + dispatch = event.dispatcher(events.InstanceEvents) @property diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 914c29b7fa..c08d91b570 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -761,8 +761,9 @@ class Mapper(_InspectionAttr): del self._configure_failed if not self.non_primary and \ + self.class_manager is not None and \ self.class_manager.is_mapped and \ - self.class_manager.mapper is self: + self.class_manager.mapper is self: instrumentation.unregister_class(self.class_) def _configure_pks(self): diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 408b119a07..361ab65e6e 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -1727,13 +1727,13 @@ class Session(_SessionClassMethods): def _before_attach(self, state): if state.session_id != self.hash_key and \ - self.dispatch.before_attach: + self.dispatch.before_attach: self.dispatch.before_attach(self, state.obj()) def _attach(self, state, include_before=False): if state.key and \ state.key in self.identity_map and \ - not self.identity_map.contains_state(state): + not self.identity_map.contains_state(state): raise sa_exc.InvalidRequestError("Can't attach instance " "%s; another instance with key %s is already " "present in this session." @@ -1749,9 +1749,11 @@ class Session(_SessionClassMethods): if state.session_id != self.hash_key: if include_before and \ - self.dispatch.before_attach: + self.dispatch.before_attach: self.dispatch.before_attach(self, state.obj()) state.session_id = self.hash_key + if state.modified and not state._strong_obj: + state._strong_obj = state.obj() if self.dispatch.after_attach: self.dispatch.after_attach(self, state.obj()) diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py index 4bc689e94d..193678c2ff 100644 --- a/lib/sqlalchemy/orm/state.py +++ b/lib/sqlalchemy/orm/state.py @@ -164,7 +164,7 @@ class InstanceState(interfaces._InspectionAttr): return bool(self.key) def _detach(self): - self.session_id = None + self.session_id = self._strong_obj = None def _dispose(self): self._detach() @@ -176,7 +176,7 @@ class InstanceState(interfaces._InspectionAttr): instance_dict.discard(self) self.callables = {} - self.session_id = None + self.session_id = self._strong_obj = None del self.obj def obj(self): @@ -259,9 +259,6 @@ class InstanceState(interfaces._InspectionAttr): self.expired = state.get('expired', False) self.callables = state.get('callables', {}) - if self.modified: - self._strong_obj = inst - self.__dict__.update([ (k, state[k]) for k in ( 'key', 'load_options', @@ -322,6 +319,7 @@ class InstanceState(interfaces._InspectionAttr): modified_set.discard(self) self.modified = False + self._strong_obj = None self.committed_state.clear() @@ -335,7 +333,7 @@ class InstanceState(interfaces._InspectionAttr): for key in self.manager: impl = self.manager[key].impl if impl.accepts_scalar_loader and \ - (impl.expire_missing or key in dict_): + (impl.expire_missing or key in dict_): self.callables[key] = self old = dict_.pop(key, None) if impl.collection and old is not None: @@ -435,18 +433,22 @@ class InstanceState(interfaces._InspectionAttr): self.committed_state[attr.key] = previous - # the "or not self.modified" is defensive at - # this point. The assertion below is expected - # to be True: # assert self._strong_obj is None or self.modified - if self._strong_obj is None or not self.modified: + if (self.session_id and self._strong_obj is None) \ + or not self.modified: instance_dict = self._instance_dict() if instance_dict: instance_dict._modified.add(self) - self._strong_obj = self.obj() - if self._strong_obj is None: + # only create _strong_obj link if attached + # to a session + + inst = self.obj() + if self.session_id: + self._strong_obj = inst + + if inst is None: raise orm_exc.ObjectDereferencedError( "Can't emit change event for attribute '%s' - " "parent object of type %s has been garbage " @@ -467,7 +469,6 @@ class InstanceState(interfaces._InspectionAttr): this step if a value was not populated in state.dict. """ - class_manager = self.manager for key in keys: self.committed_state.pop(key, None) diff --git a/test/orm/test_instrumentation.py b/test/orm/test_instrumentation.py index 3b548f0cd1..3f8fc67b68 100644 --- a/test/orm/test_instrumentation.py +++ b/test/orm/test_instrumentation.py @@ -445,6 +445,20 @@ class MapperInitTest(fixtures.ORMTest): # C is not mapped in the current implementation assert_raises(sa.orm.exc.UnmappedClassError, class_mapper, C) + def test_del_warning(self): + class A(object): + def __del__(self): + pass + + assert_raises_message( + sa.exc.SAWarning, + r"__del__\(\) method on class " + " will cause " + "unreachable cycles and memory leaks, as SQLAlchemy " + "instrumentation often creates reference cycles. " + "Please remove this method.", + mapper, A, self.fixture() + ) class OnLoadTest(fixtures.ORMTest): """Check that Events.load is not hit in regular attributes operations.""" diff --git a/test/orm/test_session.py b/test/orm/test_session.py index 5c89688427..7c2e8a3b86 100644 --- a/test/orm/test_session.py +++ b/test/orm/test_session.py @@ -857,6 +857,150 @@ class SessionStateWFixtureTest(_fixtures.FixtureTest): assert sa.orm.attributes.instance_state(a).session_id is None +class NoCyclesOnTransientDetachedTest(_fixtures.FixtureTest): + """Test the instance_state._strong_obj link that it + is present only on persistent/pending objects and never + transient/detached. + + """ + run_inserts = None + + def setup(self): + mapper(self.classes.User, self.tables.users) + + def _assert_modified(self, u1): + assert sa.orm.attributes.instance_state(u1).modified + + def _assert_not_modified(self, u1): + assert not sa.orm.attributes.instance_state(u1).modified + + def _assert_cycle(self, u1): + assert sa.orm.attributes.instance_state(u1)._strong_obj is not None + + def _assert_no_cycle(self, u1): + assert sa.orm.attributes.instance_state(u1)._strong_obj is None + + def _persistent_fixture(self): + User = self.classes.User + u1 = User() + u1.name = "ed" + sess = Session() + sess.add(u1) + sess.flush() + return sess, u1 + + def test_transient(self): + User = self.classes.User + u1 = User() + u1.name = 'ed' + self._assert_no_cycle(u1) + self._assert_modified(u1) + + def test_transient_to_pending(self): + User = self.classes.User + u1 = User() + u1.name = 'ed' + self._assert_modified(u1) + self._assert_no_cycle(u1) + sess = Session() + sess.add(u1) + self._assert_cycle(u1) + sess.flush() + self._assert_no_cycle(u1) + self._assert_not_modified(u1) + + def test_dirty_persistent_to_detached_via_expunge(self): + sess, u1 = self._persistent_fixture() + u1.name = 'edchanged' + self._assert_cycle(u1) + sess.expunge(u1) + self._assert_no_cycle(u1) + + def test_dirty_persistent_to_detached_via_close(self): + sess, u1 = self._persistent_fixture() + u1.name = 'edchanged' + self._assert_cycle(u1) + sess.close() + self._assert_no_cycle(u1) + + def test_clean_persistent_to_detached_via_close(self): + sess, u1 = self._persistent_fixture() + self._assert_no_cycle(u1) + self._assert_not_modified(u1) + sess.close() + u1.name = 'edchanged' + self._assert_modified(u1) + self._assert_no_cycle(u1) + + def test_detached_to_dirty_deleted(self): + sess, u1 = self._persistent_fixture() + sess.expunge(u1) + u1.name = 'edchanged' + self._assert_no_cycle(u1) + sess.delete(u1) + self._assert_cycle(u1) + + def test_detached_to_dirty_persistent(self): + sess, u1 = self._persistent_fixture() + sess.expunge(u1) + u1.name = 'edchanged' + self._assert_modified(u1) + self._assert_no_cycle(u1) + sess.add(u1) + self._assert_cycle(u1) + self._assert_modified(u1) + + def test_detached_to_clean_persistent(self): + sess, u1 = self._persistent_fixture() + sess.expunge(u1) + self._assert_no_cycle(u1) + self._assert_not_modified(u1) + sess.add(u1) + self._assert_no_cycle(u1) + self._assert_not_modified(u1) + + def test_move_persistent_clean(self): + sess, u1 = self._persistent_fixture() + sess.close() + s2 = Session() + s2.add(u1) + self._assert_no_cycle(u1) + self._assert_not_modified(u1) + + def test_move_persistent_dirty(self): + sess, u1 = self._persistent_fixture() + u1.name = 'edchanged' + self._assert_cycle(u1) + self._assert_modified(u1) + sess.close() + self._assert_no_cycle(u1) + s2 = Session() + s2.add(u1) + self._assert_cycle(u1) + self._assert_modified(u1) + + @testing.requires.predictable_gc + def test_move_gc_session_persistent_dirty(self): + sess, u1 = self._persistent_fixture() + u1.name = 'edchanged' + self._assert_cycle(u1) + self._assert_modified(u1) + del sess + gc_collect() + self._assert_cycle(u1) + s2 = Session() + s2.add(u1) + self._assert_cycle(u1) + self._assert_modified(u1) + + def test_persistent_dirty_to_expired(self): + sess, u1 = self._persistent_fixture() + u1.name = 'edchanged' + self._assert_cycle(u1) + self._assert_modified(u1) + sess.expire(u1) + self._assert_no_cycle(u1) + self._assert_not_modified(u1) class WeakIdentityMapTest(_fixtures.FixtureTest): run_inserts = None From 39259d94f51f3ac86d0d4d1dc38410ba92152ce3 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sat, 20 Apr 2013 02:59:58 -0400 Subject: [PATCH 13/14] - dont do a boolean check on the mapped object --- lib/sqlalchemy/orm/session.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 361ab65e6e..f7a5558f17 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -1752,7 +1752,7 @@ class Session(_SessionClassMethods): self.dispatch.before_attach: self.dispatch.before_attach(self, state.obj()) state.session_id = self.hash_key - if state.modified and not state._strong_obj: + if state.modified and state._strong_obj is None: state._strong_obj = state.obj() if self.dispatch.after_attach: self.dispatch.after_attach(self, state.obj()) From 7f0ee900b6c35a9bff214f9ebb02c3fb98d1f7e1 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sat, 20 Apr 2013 03:09:10 -0400 Subject: [PATCH 14/14] - this issue is a bug, mention the apply_labels issue sooner --- doc/build/changelog/changelog_08.rst | 43 ++++++++++++++-------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index bdba906bb5..2a2925726c 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -7,7 +7,7 @@ :version: 0.8.1 .. change:: - :tags: bug + :tags: bug, orm :tickets: 2708 Improved the behavior of instance management regarding @@ -22,6 +22,27 @@ cycles too. A warning has been added when a class with a `__del__()` method is mapped. + .. change:: + :tags: bug, sql + :tickets: 2702 + + A major fix to the way in which a select() object produces + labeled columns when apply_labels() is used; this mode + produces a SELECT where each column is labeled as in + _, to remove column name collisions + for a multiple table select. The fix is that if two labels + collide when combined with the table name, i.e. + "foo.bar_id" and "foo_bar.id", anonymous aliasing will be + applied to one of the dupes. This allows the ORM to handle + both columns independently; previously, 0.7 + would in some cases silently emit a second SELECT for the + column that was "duped", and in 0.8 an ambiguous column error + would be emitted. The "keys" applied to the .c. collection + of the select() will also be deduped, so that the "column + being replaced" warning will no longer emit for any select() + that specifies use_labels, though the dupe key will be given + an anonymous label which isn't generally user-friendly. + .. change:: :tags: bug, mysql :pullreq: 54 @@ -118,26 +139,6 @@ handling routine fails and regardless of whether the condition is a disconnect or not. - .. change:: - :tags: bug, sql - :tickets: 2702 - - A major fix to the way in which a select() object produces - labeled columns when apply_labels() is used; this mode - produces a SELECT where each column is labeled as in - _, to remove column name collisions - for a multiple table select. The fix is that if two labels - collide when combined with the table name, i.e. - "foo.bar_id" and "foo_bar.id", anonymous aliasing will be - applied to one of the dupes. This allows the ORM to handle - both columns independently; previously, 0.7 - would in some cases silently emit a second SELECT for the - column that was "duped", and in 0.8 an ambiguous column error - would be emitted. The "keys" applied to the .c. collection - of the select() will also be deduped, so that the "column - being replaced" warning will no longer emit for any select() - that specifies use_labels, though the dupe key will be given - an anonymous label which isn't generally user-friendly. .. change:: :tags: bug, orm, declarative