From a477f8a61ec60b2fc343d87aa30ef6595c77727d Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 24 Jan 2013 21:31:23 -0500 Subject: [PATCH] the consideration of a pending object as an "orphan" has been modified to more closely match the behavior as that of persistent objects, which is that the object is expunged from the :class:`.Session` as soon as it is de-associated from any of its orphan-enabled parents. Previously, the pending object would be expunged only if de-associated from all of its orphan-enabled parents. The new flag ``legacy_is_orphan`` is added to :func:`.orm.mapper` which re-establishes the legacy behavior. [ticket:2655] --- doc/build/changelog/changelog_08.rst | 17 ++ doc/build/changelog/migration_08.rst | 118 +++++++++++++ lib/sqlalchemy/orm/__init__.py | 26 +++ lib/sqlalchemy/orm/mapper.py | 25 ++- lib/sqlalchemy/orm/properties.py | 2 +- test/orm/test_cascade.py | 244 ++++++++++++++++++++++----- 6 files changed, 383 insertions(+), 49 deletions(-) diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index 052dfaaf1f..f6881f9583 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -6,6 +6,23 @@ .. changelog:: :version: 0.8.0 + .. change:: + :tags: orm, bug + :tickets: 2655 + + the consideration of a pending object as + an "orphan" has been modified to more closely match the + behavior as that of persistent objects, which is that the object + is expunged from the :class:`.Session` as soon as it is + de-associated from any of its orphan-enabled parents. Previously, + the pending object would be expunged only if de-associated + from all of its orphan-enabled parents. The new flag ``legacy_is_orphan`` + is added to :func:`.orm.mapper` which re-establishes the + legacy behavior. + + See the change note and example case at :ref:`legacy_is_orphan_addition` + for a detailed discussion of this change. + .. change:: :tags: orm, bug :tickets: 2653 diff --git a/doc/build/changelog/migration_08.rst b/doc/build/changelog/migration_08.rst index 33e80839f7..06a9402f6a 100644 --- a/doc/build/changelog/migration_08.rst +++ b/doc/build/changelog/migration_08.rst @@ -964,6 +964,124 @@ on :func:`.insert`, :func:`.select` and :class:`.Query`. Behavioral Changes ================== +.. _legacy_is_orphan_addition: + +The consideration of a "pending" object as an "orphan" has been made more aggressive +------------------------------------------------------------------------------------ + +This is a late add to the 0.8 series, however it is hoped that the new behavior +is generally more consistent and intuitive in a wider variety of +situations. The ORM has since at least version 0.4 included behavior +such that an object that's "pending", meaning that it's +associated with a :class:`.Session` but hasn't been inserted into the database +yet, is automatically expunged from the :class:`.Session` when it becomes an "orphan", +which means it has been de-associated with a parent object that refers to it +with ``delete-orphan`` cascade on the configured :func:`.relationship`. This +behavior is intended to approximately mirror the behavior of a persistent +(that is, already inserted) object, where the ORM will emit a DELETE for such +objects that become orphans based on the interception of detachment events. + +The behavioral change comes into play for objects that +are referred to by multiple kinds of parents that each specify ``delete-orphan``; the +typical example is an :ref:`association object ` that bridges two other kinds of objects +in a many-to-many pattern. Previously, the behavior was such that the +pending object would be expunged only when de-associated with *all* of its parents. +With the behavioral change, the pending object +is expunged as soon as it is de-associated from *any* of the parents that it was +previously associated with. This behavior is intended to more closely +match that of persistent objects, which are deleted as soon +as they are de-associated from any parent. + +The rationale for the older behavior dates back +at least to version 0.4, and was basically a defensive decision to try to alleviate +confusion when an object was still being constructed for INSERT. But the reality +is that the object is re-associated with the :class:`.Session` as soon as it is +attached to any new parent in any case. + +It's still possible to flush an object +that is not associated with all of its required parents, if the object was either +not associated with those parents in the first place, or if it was expunged, but then +re-associated with a :class:`.Session` via a subsequent attachment event but still +not fully associated. In this situation, it is expected that the database +would emit an integrity error, as there are likely NOT NULL foreign key columns +that are unpopulated. The ORM makes the decision to let these INSERT attempts +occur, based on the judgment that an object that is only partially associated with +its required parents but has been actively associated with some of them, +is more often than not a user error, rather than an intentional +omission which should be silently skipped - silently skipping the INSERT here would +make user errors of this nature very hard to debug. + +The old behavior, for applications that might have been relying upon it, can be re-enabled for +any :class:`.Mapper` by specifying the flag ``legacy_is_orphan`` as a mapper +option. + +The new behavior allows the following test case to work:: + + from sqlalchemy import Column, Integer, String, ForeignKey + from sqlalchemy.orm import relationship, backref + from sqlalchemy.ext.declarative import declarative_base + + Base = declarative_base() + + class User(Base): + __tablename__ = 'user' + id = Column(Integer, primary_key=True) + name = Column(String(64)) + + class UserKeyword(Base): + __tablename__ = 'user_keyword' + user_id = Column(Integer, ForeignKey('user.id'), primary_key=True) + keyword_id = Column(Integer, ForeignKey('keyword.id'), primary_key=True) + + user = relationship(User, + backref=backref("user_keywords", + cascade="all, delete-orphan") + ) + + keyword = relationship("Keyword", + backref=backref("user_keywords", + cascade="all, delete-orphan") + ) + + # uncomment this to enable the old behavior + # __mapper_args__ = {"legacy_is_orphan": True} + + class Keyword(Base): + __tablename__ = 'keyword' + id = Column(Integer, primary_key=True) + keyword = Column('keyword', String(64)) + + from sqlalchemy import create_engine + from sqlalchemy.orm import Session + + # note we're using Postgresql to ensure that referential integrity + # is enforced, for demonstration purposes. + e = create_engine("postgresql://scott:tiger@localhost/test", echo=True) + + Base.metadata.drop_all(e) + Base.metadata.create_all(e) + + session = Session(e) + + u1 = User(name="u1") + k1 = Keyword(keyword="k1") + + session.add_all([u1, k1]) + + uk1 = UserKeyword(keyword=k1, user=u1) + + # previously, if session.flush() were called here, + # this operation would succeed, but if session.flush() + # were not called here, the operation fails with an + # integrity error. + # session.flush() + del u1.user_keywords[0] + + session.commit() + + +:ticket:`2655` + The after_attach event fires after the item is associated with the Session instead of before; before_attach added ----------------------------------------------------------------------------------------------------------------- diff --git a/lib/sqlalchemy/orm/__init__.py b/lib/sqlalchemy/orm/__init__.py index 0b035ef9b3..e9dde3ca78 100644 --- a/lib/sqlalchemy/orm/__init__.py +++ b/lib/sqlalchemy/orm/__init__.py @@ -947,6 +947,32 @@ def mapper(class_, local_table=None, *args, **params): this parameter can be used to specify which columns are "foreign". In most cases can be left as ``None``. + :param legacy_is_orphan: Boolean, defaults to ``False``. + When ``True``, specifies that "legacy" orphan consideration + is to be applied to objects mapped by this mapper, which means + that a pending (that is, not persistent) object is auto-expunged + from an owning :class:`.Session` only when it is de-associated + from *all* parents that specify a ``delete-orphan`` cascade towards + this mapper. The new default behavior is that the object is auto-expunged + when it is de-associated with *any* of its parents that specify + ``delete-orphan`` cascade. This behavior is more consistent with + that of a persistent object, and allows behavior to be consistent + in more scenarios independently of whether or not an orphanable + object has been flushed yet or not. + + See the change note and example at :ref:`legacy_is_orphan_addition` + for more detail on this change. + + .. versionadded:: 0.8 - the consideration of a pending object as + an "orphan" has been modified to more closely match the + behavior as that of persistent objects, which is that the object + is expunged from the :class:`.Session` as soon as it is + de-associated from any of its orphan-enabled parents. Previously, + the pending object would be expunged only if de-associated + from all of its orphan-enabled parents. The new flag ``legacy_is_orphan`` + is added to :func:`.orm.mapper` which re-establishes the + legacy behavior. + :param non_primary: Specify that this :class:`.Mapper` is in addition to the "primary" mapper, that is, the one used for persistence. The :class:`.Mapper` created here may be used for ad-hoc diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index eb43cc53e3..447a5fce12 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -113,6 +113,7 @@ class Mapper(_InspectionAttr): exclude_properties=None, passive_updates=True, eager_defaults=False, + legacy_is_orphan=False, _compiled_cache_size=100, ): """Construct a new mapper. @@ -145,7 +146,7 @@ class Mapper(_InspectionAttr): self.inherit_condition = inherit_condition self.inherit_foreign_keys = inherit_foreign_keys self._init_properties = properties or {} - self.delete_orphans = [] + self._delete_orphans = [] self.batch = batch self.eager_defaults = eager_defaults self.column_prefix = column_prefix @@ -154,6 +155,7 @@ class Mapper(_InspectionAttr): self._dependency_processors = [] self.validators = util.immutabledict() self.passive_updates = passive_updates + self.legacy_is_orphan = legacy_is_orphan self._clause_adapter = None self._requires_row_aliasing = False self._inherits_equated_pairs = None @@ -1292,14 +1294,23 @@ class Mapper(_InspectionAttr): ) def _is_orphan(self, state): - o = False + orphan_possible = False for mapper in self.iterate_to_root(): - for (key, cls) in mapper.delete_orphans: - if attributes.manager_of_class(cls).has_parent( - state, key, optimistic=bool(state.key)): + for (key, cls) in mapper._delete_orphans: + orphan_possible = True + + has_parent = attributes.manager_of_class(cls).has_parent( + state, key, optimistic=state.has_identity) + + if self.legacy_is_orphan and has_parent: return False - o = o or bool(mapper.delete_orphans) - return o + elif not self.legacy_is_orphan and not has_parent: + return True + + if self.legacy_is_orphan: + return orphan_possible + else: + return False def has_property(self, key): return key in self._props diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py index bc238d9734..c618e89b22 100644 --- a/lib/sqlalchemy/orm/properties.py +++ b/lib/sqlalchemy/orm/properties.py @@ -1081,7 +1081,7 @@ class RelationshipProperty(StrategizedProperty): self.target = self.mapper.mapped_table if self.cascade.delete_orphan: - self.mapper.primary_mapper().delete_orphans.append( + self.mapper.primary_mapper()._delete_orphans.append( (self.key, self.parent.class_) ) diff --git a/test/orm/test_cascade.py b/test/orm/test_cascade.py index f681fc4988..00d19e7928 100644 --- a/test/orm/test_cascade.py +++ b/test/orm/test_cascade.py @@ -6,7 +6,8 @@ from sqlalchemy.testing.schema import Table, Column from sqlalchemy.orm import mapper, relationship, create_session, \ sessionmaker, class_mapper, backref, Session, util as orm_util,\ configure_mappers -from sqlalchemy.orm import attributes, exc as orm_exc +from sqlalchemy.orm.attributes import instance_state +from sqlalchemy.orm import attributes, exc as orm_exc, object_mapper from sqlalchemy import testing from sqlalchemy.testing import eq_ from sqlalchemy.testing import fixtures @@ -2261,10 +2262,7 @@ class DoubleParentO2MOrphanTest(fixtures.MappedTest): Column('account_id', Integer, ForeignKey('accounts.account_id'))) - def test_double_parent_expunge_o2m(self): - """test the delete-orphan uow event for multiple delete-orphan - parent relationships.""" - + def _fixture(self, legacy_is_orphan, uselist): sales_reps, customers, accounts = (self.tables.sales_reps, self.tables.customers, self.tables.accounts) @@ -2277,15 +2275,17 @@ class DoubleParentO2MOrphanTest(fixtures.MappedTest): class SalesRep(fixtures.ComparableEntity): pass - mapper(Customer, customers) + mapper(Customer, customers, legacy_is_orphan=legacy_is_orphan) mapper(Account, accounts, properties=dict( customers=relationship(Customer, cascade="all,delete-orphan", - backref="account"))) + backref="account", + uselist=uselist))) mapper(SalesRep, sales_reps, properties=dict( customers=relationship(Customer, cascade="all,delete-orphan", - backref="sales_rep"))) + backref="sales_rep", + uselist=uselist))) s = create_session() a = Account(balance=0) @@ -2295,9 +2295,21 @@ class DoubleParentO2MOrphanTest(fixtures.MappedTest): c = Customer(name="Jane") - a.customers.append(c) - sr.customers.append(c) + if uselist: + a.customers.append(c) + sr.customers.append(c) + else: + a.customers = c + sr.customers = c + assert c in s + return s, c, a, sr + + def test_double_parent_expunge_o2m_legacy(self): + """test the delete-orphan uow event for multiple delete-orphan + parent relationships.""" + + s, c, a, sr = self._fixture(True, True) a.customers.remove(c) assert c in s, "Should not expunge customer yet, still has one parent" @@ -2306,48 +2318,44 @@ class DoubleParentO2MOrphanTest(fixtures.MappedTest): assert c not in s, \ 'Should expunge customer when both parents are gone' - def test_double_parent_expunge_o2o(self): + def test_double_parent_expunge_o2m_current(self): """test the delete-orphan uow event for multiple delete-orphan parent relationships.""" - sales_reps, customers, accounts = (self.tables.sales_reps, - self.tables.customers, - self.tables.accounts) - + s, c, a, sr = self._fixture(False, True) - class Customer(fixtures.ComparableEntity): - pass - class Account(fixtures.ComparableEntity): - pass - class SalesRep(fixtures.ComparableEntity): - pass + a.customers.remove(c) + assert c not in s, "Should expunge customer when either parent is gone" - mapper(Customer, customers) - mapper(Account, accounts, properties=dict( - customer=relationship(Customer, - cascade="all,delete-orphan", - backref="account", uselist=False))) - mapper(SalesRep, sales_reps, properties=dict( - customer=relationship(Customer, - cascade="all,delete-orphan", - backref="sales_rep", uselist=False))) - s = create_session() + sr.customers.remove(c) + assert c not in s, \ + 'Should expunge customer when both parents are gone' - a = Account(balance=0) - sr = SalesRep(name="John") - s.add_all((a, sr)) - s.flush() + def test_double_parent_expunge_o2o_legacy(self): + """test the delete-orphan uow event for multiple delete-orphan + parent relationships.""" - c = Customer(name="Jane") + s, c, a, sr = self._fixture(True, False) - a.customer = c - sr.customer = c - assert c in s - a.customer = None + a.customers = None assert c in s, "Should not expunge customer yet, still has one parent" - sr.customer = None + sr.customers = None + assert c not in s, \ + 'Should expunge customer when both parents are gone' + + def test_double_parent_expunge_o2o_current(self): + """test the delete-orphan uow event for multiple delete-orphan + parent relationships.""" + + s, c, a, sr = self._fixture(False, False) + + + a.customers = None + assert c not in s, "Should expunge customer when either parent is gone" + + sr.customers = None assert c not in s, \ 'Should expunge customer when both parents are gone' @@ -2499,6 +2507,160 @@ class CollectionAssignmentOrphanTest(fixtures.MappedTest): eq_(sess.query(A).get(a1.id), A(name='a1', bs=[B(name='b1'), B(name='b2'), B(name='b3')])) +class OrphanCriterionTest(fixtures.MappedTest): + @classmethod + def define_tables(self, metadata): + Table("core", metadata, + Column("id", Integer, + primary_key=True, test_needs_autoincrement=True), + Column("related_one_id", Integer, ForeignKey("related_one.id")), + Column("related_two_id", Integer, ForeignKey("related_two.id")) + ) + + Table("related_one", metadata, + Column("id", Integer, + primary_key=True, test_needs_autoincrement=True), + ) + + Table("related_two", metadata, + Column("id", Integer, + primary_key=True, test_needs_autoincrement=True), + ) + + def _fixture(self, legacy_is_orphan, persistent, + r1_present, r2_present, detach_event=True): + class Core(object): + pass + + class RelatedOne(object): + def __init__(self, cores): + self.cores = cores + + class RelatedTwo(object): + def __init__(self, cores): + self.cores = cores + + mapper(Core, self.tables.core, legacy_is_orphan=legacy_is_orphan) + mapper(RelatedOne, self.tables.related_one, properties={ + 'cores': relationship(Core, cascade="all, delete-orphan", + backref="r1") + }) + mapper(RelatedTwo, self.tables.related_two, properties={ + 'cores': relationship(Core, cascade="all, delete-orphan", + backref="r2") + }) + c1 = Core() + if detach_event: + r1 = RelatedOne(cores=[c1]) + r2 = RelatedTwo(cores=[c1]) + else: + if r1_present: + r1 = RelatedOne(cores=[c1]) + if r2_present: + r2 = RelatedTwo(cores=[c1]) + + if persistent: + s = Session() + s.add(c1) + s.flush() + + if detach_event: + if not r1_present: + c1.r1 = None + if not r2_present: + c1.r2 = None + return c1 + + def _assert_not_orphan(self, c1): + mapper = object_mapper(c1) + state = instance_state(c1) + assert not mapper._is_orphan(state) + + def _assert_is_orphan(self, c1): + mapper = object_mapper(c1) + state = instance_state(c1) + assert mapper._is_orphan(state) + + def test_leg_pers_r1_r2(self): + c1 = self._fixture(True, True, True, True) + + self._assert_not_orphan(c1) + + def test_current_pers_r1_r2(self): + c1 = self._fixture(False, True, True, True) + + self._assert_not_orphan(c1) + + def test_leg_pers_r1_notr2(self): + c1 = self._fixture(True, True, True, False) + + self._assert_not_orphan(c1) + + def test_current_pers_r1_notr2(self): + c1 = self._fixture(False, True, True, False) + + self._assert_is_orphan(c1) + + def test_leg_pers_notr1_notr2(self): + c1 = self._fixture(True, True, False, False) + + self._assert_is_orphan(c1) + + def test_current_pers_notr1_notr2(self): + c1 = self._fixture(False, True, True, False) + + self._assert_is_orphan(c1) + + def test_leg_transient_r1_r2(self): + c1 = self._fixture(True, False, True, True) + + self._assert_not_orphan(c1) + + def test_current_transient_r1_r2(self): + c1 = self._fixture(False, False, True, True) + + self._assert_not_orphan(c1) + + def test_leg_transient_r1_notr2(self): + c1 = self._fixture(True, False, True, False) + + self._assert_not_orphan(c1) + + def test_current_transient_r1_notr2(self): + c1 = self._fixture(False, False, True, False) + + self._assert_is_orphan(c1) + + def test_leg_transient_notr1_notr2(self): + c1 = self._fixture(True, False, False, False) + + self._assert_is_orphan(c1) + + def test_current_transient_notr1_notr2(self): + c1 = self._fixture(False, False, False, False) + + self._assert_is_orphan(c1) + + def test_leg_transient_notr1_notr2_noevent(self): + c1 = self._fixture(True, False, False, False, False) + + self._assert_is_orphan(c1) + + def test_current_transient_notr1_notr2_noevent(self): + c1 = self._fixture(False, False, False, False, False) + + self._assert_is_orphan(c1) + + def test_leg_persistent_notr1_notr2_noevent(self): + c1 = self._fixture(True, True, False, False, False) + + self._assert_not_orphan(c1) + + def test_current_persistent_notr1_notr2_noevent(self): + c1 = self._fixture(False, True, False, False, False) + + self._assert_not_orphan(c1) + class O2MConflictTest(fixtures.MappedTest): """test that O2M dependency detects a change in parent, does the right thing, and updates the collection/attribute. -- 2.47.2