From 52735e75c527dd24a793ae7ee0b1978e8aacacb5 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Wed, 15 Sep 2010 22:20:01 -0400 Subject: [PATCH] - fix test_single test to use default dialect - The exception raised by Session when it is used subsequent to a subtransaction rollback (which is what happens when a flush fails in autocommit=False mode) has now been reworded (this is the "inactive due to a rollback in a subtransaction" message). In particular, if the rollback was due to an exception during flush(), the message states this is the case, and reiterates the string form of the original exception that occurred during flush. If the session is closed due to explicit usage of subtransactions (not very common), the message just states this is the case. - The exception raised by Mapper when repeated requests to its initialization are made after initialization already failed no longer assumes the "hasattr" case, since there's other scenarios in which this message gets emitted, and the message also does not compound onto itself multiple times - you get the same message for each attempt at usage. The misnomer "compiles" is being traded out for "initialize". --- CHANGES | 31 +++++++++++++++++++---- lib/sqlalchemy/ext/declarative.py | 2 +- lib/sqlalchemy/orm/dependency.py | 6 +++-- lib/sqlalchemy/orm/mapper.py | 18 ++++++++------ lib/sqlalchemy/orm/session.py | 38 ++++++++++++++++++++++------- test/ext/test_declarative.py | 13 ++++++---- test/orm/inheritance/test_single.py | 6 +++-- test/orm/test_mapper.py | 23 +++++++++++++---- test/orm/test_session.py | 33 ++++++++++++++++++++++--- 9 files changed, 130 insertions(+), 40 deletions(-) diff --git a/CHANGES b/CHANGES index 9c1f0a1fc9..35255cba42 100644 --- a/CHANGES +++ b/CHANGES @@ -15,6 +15,27 @@ CHANGES - Fixed a regression in 0.6.4 which occurred if you passed an empty list to "include_properties" on mapper() [ticket:1918] + + - The exception raised by Session when it is used + subsequent to a subtransaction rollback (which is what + happens when a flush fails in autocommit=False mode) has + now been reworded (this is the "inactive due to a + rollback in a subtransaction" message). In particular, + if the rollback was due to an exception during flush(), + the message states this is the case, and reiterates the + string form of the original exception that occurred + during flush. If the session is closed due to explicit + usage of subtransactions (not very common), the message + just states this is the case. + + - The exception raised by Mapper when repeated requests to + its initialization are made after initialization already + failed no longer assumes the "hasattr" case, since + there's other scenarios in which this message gets + emitted, and the message also does not compound onto + itself multiple times - you get the same message for + each attempt at usage. The misnomer "compiles" is being + traded out for "initialize". - Added an assertion during flush which ensures that no NULL-holding identity keys were generated @@ -42,11 +63,11 @@ CHANGES after a flush. The flag is only intended for very specific use cases. - - Slight improvement to the behavior of "passive_updates=False" - when placed only on the many-to-one side of a - relationship; documentation has been clarified - that passive_updates=False should really be on the - one-to-many side. + - Slight improvement to the behavior of + "passive_updates=False" when placed only on the + many-to-one side of a relationship; documentation has + been clarified that passive_updates=False should really + be on the one-to-many side. - Placing passive_deletes=True on a many-to-one emits a warning, since you probably intended to put it on diff --git a/lib/sqlalchemy/ext/declarative.py b/lib/sqlalchemy/ext/declarative.py index 6d4bdda437..40263c7b80 100755 --- a/lib/sqlalchemy/ext/declarative.py +++ b/lib/sqlalchemy/ext/declarative.py @@ -1192,7 +1192,7 @@ def _deferred_relationship(cls, prop): return x except NameError, n: raise exceptions.InvalidRequestError( - "When compiling mapper %s, expression %r failed to " + "When initializing mapper %s, expression %r failed to " "locate a name (%r). If this is a class name, consider " "adding this relationship() to the %r class after " "both dependent classes have been defined." % diff --git a/lib/sqlalchemy/orm/dependency.py b/lib/sqlalchemy/orm/dependency.py index 662cfc67b0..4458a85470 100644 --- a/lib/sqlalchemy/orm/dependency.py +++ b/lib/sqlalchemy/orm/dependency.py @@ -806,8 +806,10 @@ class DetectKeySwitch(DependencyProcessor): if not issubclass(state.class_, self.parent.class_): continue dict_ = state.dict - related = state.get_impl(self.key).get(state, dict_, passive=self.passive_updates) - if related is not attributes.PASSIVE_NO_RESULT and related is not None: + related = state.get_impl(self.key).get(state, dict_, + passive=self.passive_updates) + if related is not attributes.PASSIVE_NO_RESULT and \ + related is not None: related_state = attributes.instance_state(dict_[self.key]) if related_state in switchers: uowcommit.register_object(state, diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 8d31dd89ad..49f5d2190f 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -30,6 +30,7 @@ from sqlalchemy.orm.util import ( ExtensionCarrier, _INSTRUMENTOR, _class_to_mapper, _state_mapper, class_mapper, instance_str, state_str, ) +import sys __all__ = ( 'Mapper', @@ -787,12 +788,13 @@ class Mapper(object): # the order of mapper compilation for mapper in list(_mapper_registry): if getattr(mapper, '_compile_failed', False): - raise sa_exc.InvalidRequestError( - "One or more mappers failed to compile. " - "Exception was probably " - "suppressed within a hasattr() call. " - "Message was: %s" % - mapper._compile_failed) + e = sa_exc.InvalidRequestError( + "One or more mappers failed to initialize - " + "can't proceed with initialization of other " + "mappers. Original exception was: %s" + % mapper._compile_failed) + e._compile_failed = mapper._compile_failed + raise e if not mapper.compiled: mapper._post_configure_properties() @@ -801,9 +803,9 @@ class Mapper(object): finally: _already_compiling = False except: - import sys exc = sys.exc_info()[1] - self._compile_failed = exc + if not hasattr(exc, '_compile_failed'): + self._compile_failed = exc raise finally: self._expire_memoizations() diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 54b41fcc61..bab98f4faf 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -22,6 +22,7 @@ from sqlalchemy.orm.util import ( from sqlalchemy.orm.mapper import Mapper, _none_set from sqlalchemy.orm.unitofwork import UOWTransaction from sqlalchemy.orm import identity +import sys __all__ = ['Session', 'SessionTransaction', 'SessionExtension'] @@ -206,7 +207,9 @@ class SessionTransaction(object): single: thread safety; SessionTransaction """ - + + _rollback_exception = None + def __init__(self, session, parent=None, nested=False): self.session = session self._connections = {} @@ -229,9 +232,21 @@ class SessionTransaction(object): def _assert_is_active(self): self._assert_is_open() if not self._active: - raise sa_exc.InvalidRequestError( - "The transaction is inactive due to a rollback in a " - "subtransaction. Issue rollback() to cancel the transaction.") + if self._rollback_exception: + raise sa_exc.InvalidRequestError( + "This Session's transaction has been rolled back " + "due to a previous exception during flush." + " To begin a new transaction with this Session, " + "first issue Session.rollback()." + " Original exception was: %s" + % self._rollback_exception + ) + else: + raise sa_exc.InvalidRequestError( + "This Session's transaction has been rolled back " + "by a nested rollback() call. To begin a new " + "transaction, issue Session.rollback() first." + ) def _assert_is_open(self, error_msg="The transaction is closed"): if self.session is None: @@ -288,14 +303,16 @@ class SessionTransaction(object): assert not self.session._deleted for s in self.session.identity_map.all_states(): - _expire_state(s, s.dict, None, instance_dict=self.session.identity_map) + _expire_state(s, s.dict, None, + instance_dict=self.session.identity_map) def _remove_snapshot(self): assert self._is_transaction_boundary if not self.nested and self.session.expire_on_commit: for s in self.session.identity_map.all_states(): - _expire_state(s, s.dict, None, instance_dict=self.session.identity_map) + _expire_state(s, s.dict, None, + instance_dict=self.session.identity_map) def _connection_for_bind(self, bind): self._assert_is_active() @@ -379,7 +396,7 @@ class SessionTransaction(object): self.close() return self._parent - def rollback(self): + def rollback(self, _capture_exception=False): self._assert_is_open() stx = self.session.transaction @@ -397,6 +414,8 @@ class SessionTransaction(object): transaction._deactivate() self.close() + if self._parent and _capture_exception: + self._parent._rollback_exception = sys.exc_info()[1] return self._parent def _rollback_impl(self): @@ -415,7 +434,8 @@ class SessionTransaction(object): def close(self): self.session.transaction = self._parent if self._parent is None: - for connection, transaction, autoclose in set(self._connections.values()): + for connection, transaction, autoclose in \ + set(self._connections.values()): if autoclose: connection.close() else: @@ -1451,7 +1471,7 @@ class Session(object): ext.after_flush(self, flush_context) transaction.commit() except: - transaction.rollback() + transaction.rollback(_capture_exception=True) raise flush_context.finalize_flush_changes() diff --git a/test/ext/test_declarative.py b/test/ext/test_declarative.py index 26e1563fe2..65ec92ca29 100644 --- a/test/ext/test_declarative.py +++ b/test/ext/test_declarative.py @@ -369,11 +369,14 @@ class DeclarativeTest(DeclarativeTestBase): hasattr(User.addresses, 'property') - # the exeption is preserved - - assert_raises_message(sa.exc.InvalidRequestError, - r"suppressed within a hasattr\(\)", - compile_mappers) + # the exception is preserved. Remains the + # same through repeated calls. + for i in range(3): + assert_raises_message(sa.exc.InvalidRequestError, + "^One or more mappers failed to initialize - " + "can't proceed with initialization of other " + "mappers. Original exception was: When initializing.*", + compile_mappers) def test_custom_base(self): class MyBase(object): diff --git a/test/orm/inheritance/test_single.py b/test/orm/inheritance/test_single.py index 4204309540..2efde2b32a 100644 --- a/test/orm/inheritance/test_single.py +++ b/test/orm/inheritance/test_single.py @@ -252,10 +252,12 @@ class RelationshipFromSingleTest(testing.AssertsCompiledSQL, MappedTest): 'employee_stuff_name, anon_1.employee_id ' 'AS anon_1_employee_id FROM (SELECT ' 'employee.id AS employee_id FROM employee ' - 'WHERE employee.type IN (?)) AS anon_1 ' + 'WHERE employee.type IN (:type_1)) AS anon_1 ' 'JOIN employee_stuff ON anon_1.employee_id ' '= employee_stuff.employee_id ORDER BY ' - 'anon_1.employee_id') + 'anon_1.employee_id', + use_default_dialect=True + ) class RelationshipToSingleTest(MappedTest): @classmethod diff --git a/test/orm/test_mapper.py b/test/orm/test_mapper.py index a19af5f4f4..f041c8896b 100644 --- a/test/orm/test_mapper.py +++ b/test/orm/test_mapper.py @@ -88,14 +88,25 @@ class MapperTest(_fixtures.FixtureTest): @testing.resolve_artifact_names def test_exceptions_sticky(self): - """test preservation of mapper compile errors raised during hasattr().""" + """test preservation of mapper compile errors raised during hasattr(), + as well as for redundant mapper compile calls. Test that + repeated calls don't stack up error messages. + + """ mapper(Address, addresses, properties={ 'user':relationship(User) }) hasattr(Address.user, 'property') - assert_raises_message(sa.exc.InvalidRequestError, r"suppressed within a hasattr\(\)", compile_mappers) + for i in range(3): + assert_raises_message(sa.exc.InvalidRequestError, + "^One or more mappers failed to " + "initialize - can't proceed with " + "initialization of other mappers. " + "Original exception was: Class " + "'test.orm._fixtures.User' is not mapped$" + , compile_mappers) @testing.resolve_artifact_names def test_column_prefix(self): @@ -157,13 +168,15 @@ class MapperTest(_fixtures.FixtureTest): @testing.resolve_artifact_names def test_column_not_present(self): - assert_raises_message(sa.exc.ArgumentError, "not represented in the mapper's table", mapper, User, users, properties={ - 'foo':addresses.c.user_id - }) + assert_raises_message(sa.exc.ArgumentError, + "not represented in the mapper's table", + mapper, User, users, properties={'foo' + : addresses.c.user_id}) @testing.resolve_artifact_names def test_bad_constructor(self): """If the construction of a mapped class fails, the instance does not get placed in the session""" + class Foo(object): def __init__(self, one, two, _sa_session=None): pass diff --git a/test/orm/test_session.py b/test/orm/test_session.py index b7ae104a16..6ac42a6b38 100644 --- a/test/orm/test_session.py +++ b/test/orm/test_session.py @@ -713,11 +713,38 @@ class SessionTest(_fixtures.FixtureTest): sess.flush() sess.rollback() assert_raises_message(sa.exc.InvalidRequestError, - 'inactive due to a rollback in a ' - 'subtransaction', sess.begin, - subtransactions=True) + "This Session's transaction has been " + r"rolled back by a nested rollback\(\) " + "call. To begin a new transaction, " + r"issue Session.rollback\(\) first.", + sess.begin, subtransactions=True) sess.close() + @testing.resolve_artifact_names + def test_preserve_flush_error(self): + mapper(User, users) + sess = Session() + + sess.add(User(id=5)) + assert_raises( + sa.exc.DBAPIError, + sess.commit + ) + + for i in range(5): + assert_raises_message(sa.exc.InvalidRequestError, + "^This Session's transaction has been " + r"rolled back due to a previous exception during flush. To " + "begin a new transaction with this " + "Session, first issue " + r"Session.rollback\(\). Original exception " + "was:", + sess.commit) + sess.rollback() + sess.add(User(id=5, name='some name')) + sess.commit() + + @testing.resolve_artifact_names def test_no_autocommit_with_explicit_commit(self): mapper(User, users) -- 2.47.2