]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
- Further refined 0.5.1's warning about delete-orphan cascade
authorMike Bayer <mike_mp@zzzcomputing.com>
Tue, 20 Jan 2009 21:35:57 +0000 (21:35 +0000)
committerMike Bayer <mike_mp@zzzcomputing.com>
Tue, 20 Jan 2009 21:35:57 +0000 (21:35 +0000)
placed on a many-to-many relation.   First, the bad news:
the warning will apply to both many-to-many as well as
many-to-one relations.  This is necessary since in both
cases, SQLA does not scan the full set of potential parents
when determining "orphan" status - for a persistent object
it only detects an in-python de-association event to establish
the object as an "orphan".  Next, the good news: to support
one-to-one via a foreign key or assocation table, or to
support one-to-many via an association table, a new flag
single_parent=True may be set which indicates objects
linked to the relation are only meant to have a single parent.
The relation will raise an error if multiple parent-association
events occur within Python.

- Fixed bug in delete-orphan cascade whereby two one-to-one
relations from two different parent classes to the same target
class would prematurely expunge the instance.  This is
an extension of the non-ticketed fix in r4247.

- the order of "sethasparent" flagging in relation to
AttributeExtensions has been refined such that false setparents
are issued before the event, true setparents issued afterwards.
event handlers "know" that a remove event originates
from a non-orphan but need to know if its become an orphan,
and that append events will become non-orphans but need to know
if the event originates from a non-orphan.

CHANGES
doc/build/mappers.rst
doc/build/session.rst
lib/sqlalchemy/orm/__init__.py
lib/sqlalchemy/orm/attributes.py
lib/sqlalchemy/orm/properties.py
lib/sqlalchemy/orm/strategies.py
lib/sqlalchemy/orm/unitofwork.py
test/orm/cascade.py
test/orm/relationships.py

diff --git a/CHANGES b/CHANGES
index 74ae62752f136613cdd8ae6b29cd668d96835745..5ff51fa8cfc176b0a1b9e92d3c6225d377c0498a 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -6,6 +6,26 @@ CHANGES
 0.5.2
 ======
 
+- orm
+    - Further refined 0.5.1's warning about delete-orphan cascade
+      placed on a many-to-many relation.   First, the bad news:
+      the warning will apply to both many-to-many as well as
+      many-to-one relations.  This is necessary since in both 
+      cases, SQLA does not scan the full set of potential parents
+      when determining "orphan" status - for a persistent object
+      it only detects an in-python de-association event to establish
+      the object as an "orphan".  Next, the good news: to support 
+      one-to-one via a foreign key or assocation table, or to 
+      support one-to-many via an association table, a new flag 
+      single_parent=True may be set which indicates objects 
+      linked to the relation are only meant to have a single parent.
+      The relation will raise an error if multiple parent-association
+      events occur within Python.
+
+    - Fixed bug in delete-orphan cascade whereby two one-to-one
+      relations from two different parent classes to the same target 
+      class would prematurely expunge the instance.
+      
 - sql
     - Further fixes to the "percent signs and spaces in column/table
        names" functionality. [ticket:1284]
index 07b89da60405e9d26d7c69ab058c8262073e926c..cb770415e9b47599637c65c8fe68cc064ee071b0 100644 (file)
@@ -1687,7 +1687,6 @@ Above, the ``children`` collection is fully writeable, and changes to it will be
 Using Passive Deletes 
 ~~~~~~~~~~~~~~~~~~~~~~
 
-
 Use ``passive_deletes=True`` to disable child object loading on a DELETE operation, in conjunction with "ON DELETE (CASCADE|SET NULL)" on your database to automatically cascade deletes to child objects.   Note that "ON DELETE" is not supported on SQLite, and requires ``InnoDB`` tables when using MySQL:
 
 .. sourcecode:: python+sql
@@ -1713,7 +1712,6 @@ When ``passive_deletes`` is applied, the ``children`` relation will not be loade
 Mutable Primary Keys / Update Cascades 
 ---------------------------------------
 
-
 As of SQLAlchemy 0.4.2, the primary key attributes of an instance can be changed freely, and will be persisted upon flush.  When the primary key of an entity changes, related items which reference the primary key must also be updated as well.  For databases which enforce referential integrity, it's required to use the database's ON UPDATE CASCADE functionality in order to propagate primary key changes.  For those which don't, the ``passive_cascades`` flag can be set to ``False`` which instructs SQLAlchemy to issue UPDATE statements individually.  The ``passive_cascades`` flag can also be ``False`` in conjunction with ON UPDATE CASCADE functionality, although in that case it issues UPDATE statements unnecessarily.
 
 A typical mutable primary key setup might look like:
index 96463b6a86e71fd0e71060b689a786c235c7e637..a71b6b48587200676cb0e1cc42127e5e9e9251db 100644 (file)
@@ -381,6 +381,8 @@ The above mapper specifies two relations, ``items`` and ``customer``.  The ``ite
 
 The ``customer`` relationship specifies only the "save-update" cascade value, indicating most operations will not be cascaded from a parent ``Order`` instance to a child ``User`` instance except for the ``add()`` operation.  "save-update" cascade indicates that an ``add()`` on the parent will cascade to all child items, and also that items added to a parent which is already present in the session will also be added.
 
+Note that the ``delete-orphan`` cascade only functions for relationships where the target object can have a single parent at a time, meaning it is only appropriate for one-to-one or one-to-many relationships.  For a :func:`~sqlalchemy.orm.relation` which establishes one-to-one via a local foreign key, i.e. a many-to-one that stores only a single parent, or one-to-one/one-to-many via a "secondary" (association) table, a warning will be issued if ``delete-orphan`` is configured.  To disable this warning, also specify the ``single_parent=True`` flag on the relationship, which constrains objects to allow attachment to only one parent at a time.
+
 The default value for ``cascade`` on :func:`~sqlalchemy.orm.relation()` is ``save-update, merge``.
 
 Managing Transactions
index e9d98ac3437bf144359196ec9ab58296f8db8757..7e64bda7ab6ed07ce13d68aeeacb52fcb5696133 100644 (file)
@@ -388,6 +388,14 @@ def relation(argument, secondary=None, **kwargs):
       based on the foreign key relationships of the association and
       child tables.
 
+    :param single_parent=(True|False):
+      when True, installs a validator which will prevent objects
+      from being associated with more than one parent at a time.
+      This is used for many-to-one or many-to-many relationships that
+      should be treated either as one-to-one or one-to-many.  Its
+      usage is optional unless delete-orphan cascade is also 
+      set on this relation(), in which case its required (new in 0.5.2).
+      
     :param uselist=(True|False):
       a boolean that indicates if this property should be loaded as a
       list or a scalar. In most cases, this value is determined
@@ -400,7 +408,7 @@ def relation(argument, secondary=None, **kwargs):
     :param viewonly=False:
       when set to True, the relation is used only for loading objects
       within the relationship, and has no effect on the unit-of-work
-      flush process.  Relations with viewonly can specify any kind of
+      flush process.  Relationships with viewonly can specify any kind of
       join conditions to provide additional views of related objects
       onto a parent object. Note that the functionality of a viewonly
       relationship has its limits - complicated join conditions may
index 3f2fc9b122083d02644e56fc53224af19b5f8b5c..70b0738c955399fa0a10ddd060de0065b154f282 100644 (file)
@@ -565,13 +565,16 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl):
         state.modified_event(self, False, previous)
 
         if self.trackparent:
-            if value is not None:
-                self.sethasparent(instance_state(value), True)
             if previous is not value and previous is not None:
                 self.sethasparent(instance_state(previous), False)
 
         for ext in self.extensions:
             value = ext.set(state, value, previous, initiator or self)
+
+        if self.trackparent:
+            if value is not None:
+                self.sethasparent(instance_state(value), True)
+
         return value
 
 
@@ -617,11 +620,12 @@ class CollectionAttributeImpl(AttributeImpl):
     def fire_append_event(self, state, value, initiator):
         state.modified_event(self, True, NEVER_SET, passive=PASSIVE_NO_INITIALIZE)
 
+        for ext in self.extensions:
+            value = ext.append(state, value, initiator or self)
+
         if self.trackparent and value is not None:
             self.sethasparent(instance_state(value), True)
 
-        for ext in self.extensions:
-            value = ext.append(state, value, initiator or self)
         return value
 
     def fire_pre_remove_event(self, state, initiator):
index c83e03599d08fd3e6eefb4f8b0cd5c52abcfc9b7..22806e364ae802052c786eeeae8342775cd89e9f 100644 (file)
@@ -359,6 +359,7 @@ class RelationProperty(StrategizedProperty):
         passive_updates=True, remote_side=None,
         enable_typechecks=True, join_depth=None,
         comparator_factory=None,
+        single_parent=False,
         strategy_class=None, _local_remote_pairs=None, query_class=None):
 
         self.uselist = uselist
@@ -370,6 +371,7 @@ class RelationProperty(StrategizedProperty):
         self.direction = None
         self.viewonly = viewonly
         self.lazy = lazy
+        self.single_parent = single_parent
         self._foreign_keys = foreign_keys
         self.collection_class = collection_class
         self.passive_deletes = passive_deletes
@@ -910,9 +912,11 @@ class RelationProperty(StrategizedProperty):
                     "the child's mapped tables.  Specify 'foreign_keys' "
                     "argument." % (str(self)))
         
-        if self.cascade.delete_orphan and self.direction is MANYTOMANY:
+        if self.cascade.delete_orphan and not self.single_parent and \
+            (self.direction is MANYTOMANY or self.direction is MANYTOONE):
             util.warn("On %s, delete-orphan cascade is not supported on a "
-                    "many-to-many relation.  This will raise an error in 0.6." % self)
+                    "many-to-many or many-to-one relationship when single_parent is not set.  "
+                    " Set single_parent=True on the relation()." % self)
         
     def _determine_local_remote_pairs(self):
         if not self.local_remote_pairs:
index 7195310cdf0ab9db0943876d332d91043a0552fa..22ef7ded25944505342e0ee9e86542ebf23e43f9 100644 (file)
@@ -10,7 +10,7 @@ import sqlalchemy.exceptions as sa_exc
 from sqlalchemy import sql, util, log
 from sqlalchemy.sql import util as sql_util
 from sqlalchemy.sql import visitors, expression, operators
-from sqlalchemy.orm import mapper, attributes
+from sqlalchemy.orm import mapper, attributes, interfaces
 from sqlalchemy.orm.interfaces import (
     LoaderStrategy, StrategizedOption, MapperOption, PropertyOption,
     serialize_path, deserialize_path, StrategizedProperty
@@ -33,6 +33,10 @@ def _register_attribute(strategy, useobject,
 
     prop = strategy.parent_property
     attribute_ext = util.to_list(prop.extension) or []
+
+    if useobject and prop.single_parent:
+        attribute_ext.append(_SingleParentValidator(prop))
+
     if getattr(prop, 'backref', None):
         attribute_ext.append(prop.backref.extension)
     
@@ -812,4 +816,24 @@ class LoadEagerFromAliasOption(PropertyOption):
         else:
             query._attributes[("user_defined_eager_row_processor", paths[-1])] = None
 
+class _SingleParentValidator(interfaces.AttributeExtension):
+    def __init__(self, prop):
+        self.prop = prop
+
+    def _do_check(self, state, value, oldvalue, initiator):
+        if value is not None:
+            hasparent = initiator.hasparent(attributes.instance_state(value))
+            if hasparent and oldvalue is not value: 
+                raise sa_exc.InvalidRequestError("Instance %s is already associated with an instance "
+                    "of %s via its %s attribute, and is only allowed a single parent." % 
+                    (mapperutil.instance_str(value), state.class_, self.prop)
+                )
+        return value
         
+    def append(self, state, value, initiator):
+        return self._do_check(state, value, None, initiator)
+
+    def set(self, state, value, oldvalue, initiator):
+        return self._do_check(state, value, oldvalue, initiator)
+
+
index 5f32884e76e32d1c0c88afc2b3e2be4a844341e4..c756045a1e8b26b928ebe3bdc53010023fb2b499 100644 (file)
@@ -65,7 +65,8 @@ class UOWEventHandler(interfaces.AttributeExtension):
             prop = _state_mapper(state).get_property(self.key)
             if newvalue is not None and prop.cascade.save_update and newvalue not in sess:
                 sess.add(newvalue)
-            if prop.cascade.delete_orphan and oldvalue in sess.new:
+            if prop.cascade.delete_orphan and oldvalue in sess.new and \
+                prop.mapper._is_orphan(attributes.instance_state(oldvalue)):
                 sess.expunge(oldvalue)
         return newvalue
 
index 10de5cce741b190ca9ae2b6f9002c61244bbd756..3345a5d8cf645455477d8675695864afdeb3fca0 100644 (file)
@@ -1,6 +1,6 @@
 import testenv; testenv.configure_for_tests()
 
-from testlib.sa import Table, Column, Integer, String, ForeignKey, Sequence
+from testlib.sa import Table, Column, Integer, String, ForeignKey, Sequence, exc as sa_exc
 from testlib.sa.orm import mapper, relation, create_session, class_mapper, backref
 from testlib.sa.orm import attributes, exc as orm_exc
 from testlib import testing
@@ -185,7 +185,30 @@ class O2MCascadeTest(_fixtures.FixtureTest):
         assert users.count().scalar() == 1
         assert orders.count().scalar() == 0
 
+class O2OCascadeTest(_fixtures.FixtureTest):
+    run_inserts = None
+    
+    @testing.resolve_artifact_names
+    def setup_mappers(self):
+        mapper(Address, addresses)
+        mapper(User, users, properties = {
+            'address':relation(Address, backref=backref("user", single_parent=True), uselist=False)
+        })
 
+    @testing.resolve_artifact_names
+    def test_single_parent_raise(self):
+        a1 = Address(email_address='some address')
+        u1 = User(name='u1', address=a1)
+        
+        self.assertRaises(sa_exc.InvalidRequestError, Address, email_address='asd', user=u1)
+        
+        a2 = Address(email_address='asd')
+        u1.address = a2
+        assert u1.address is not a1
+        assert a1.user is None
+        
+        
+        
 class O2MBackrefTest(_fixtures.FixtureTest):
     run_inserts = None
 
@@ -351,7 +374,7 @@ class M2OCascadeTest(_base.MappedTest):
             extra = relation(Extra, cascade="all, delete")
         ))
         mapper(User, users, properties = dict(
-            pref = relation(Pref, lazy=False, cascade="all, delete-orphan")
+            pref = relation(Pref, lazy=False, cascade="all, delete-orphan", single_parent=True  )
         ))
 
     @testing.resolve_artifact_names
@@ -566,9 +589,9 @@ class M2OCascadeDeleteOrphanTest(_base.MappedTest):
     @testing.resolve_artifact_names
     def setup_mappers(self):
         mapper(T1, t1, properties=dict(
-            t2=relation(T2, cascade="all, delete-orphan")))
+            t2=relation(T2, cascade="all, delete-orphan", single_parent=True)))
         mapper(T2, t2, properties=dict(
-            t3=relation(T3, cascade="all, delete-orphan")))
+            t3=relation(T3, cascade="all, delete-orphan", single_parent=True, backref=backref('t2', uselist=False))))
         mapper(T3, t3)
 
     @testing.resolve_artifact_names
@@ -625,9 +648,35 @@ class M2OCascadeDeleteOrphanTest(_base.MappedTest):
         eq_(sess.query(T2).all(), [T2()])
         eq_(sess.query(T3).all(), [])
 
+    @testing.resolve_artifact_names
+    def test_single_parent_raise(self):
+
+        sess = create_session()
+        
+        y = T2(data='T2a')
+        x = T1(data='T1a', t2=y)
+        self.assertRaises(sa_exc.InvalidRequestError, T1, data='T1b', t2=y)
+
+    @testing.resolve_artifact_names
+    def test_single_parent_backref(self):
+
+        sess = create_session()
+        
+        y = T3(data='T3a')
+        x = T2(data='T2a', t3=y)
+
+        # cant attach the T3 to another T2
+        self.assertRaises(sa_exc.InvalidRequestError, T2, data='T2b', t3=y)
+        
+        # set via backref tho is OK, unsets from previous parent
+        # first
+        z = T2(data='T2b')
+        y.t2 = z
+
+        assert z.t3 is y
+        assert x.t3 is None
+
 class M2MCascadeTest(_base.MappedTest):
-    """delete-orphan cascade is deprecated on many-to-many."""
-    
     def define_tables(self, metadata):
         Table('a', metadata,
             Column('id', Integer, primary_key=True),
@@ -662,13 +711,12 @@ class M2MCascadeTest(_base.MappedTest):
         class C(_fixtures.Base):
             pass
 
-    @testing.emits_warning(".*not supported on a many-to-many")
     @testing.resolve_artifact_names
     def test_delete_orphan(self):
         mapper(A, a, properties={
             # if no backref here, delete-orphan failed until [ticket:427] was
             # fixed
-            'bs': relation(B, secondary=atob, cascade="all, delete-orphan")
+            'bs': relation(B, secondary=atob, cascade="all, delete-orphan", single_parent=True)
         })
         mapper(B, b)
 
@@ -684,13 +732,12 @@ class M2MCascadeTest(_base.MappedTest):
         assert b.count().scalar() == 0
         assert a.count().scalar() == 1
 
-    @testing.emits_warning(".*not supported on a many-to-many")
     @testing.resolve_artifact_names
     def test_delete_orphan_cascades(self):
         mapper(A, a, properties={
             # if no backref here, delete-orphan failed until [ticket:427] was
             # fixed
-            'bs':relation(B, secondary=atob, cascade="all, delete-orphan")
+            'bs':relation(B, secondary=atob, cascade="all, delete-orphan", single_parent=True)
         })
         mapper(B, b, properties={'cs':relation(C, cascade="all, delete-orphan")})
         mapper(C, c)
@@ -708,11 +755,10 @@ class M2MCascadeTest(_base.MappedTest):
         assert a.count().scalar() == 1
         assert c.count().scalar() == 0
 
-    @testing.emits_warning(".*not supported on a many-to-many")
     @testing.resolve_artifact_names
     def test_cascade_delete(self):
         mapper(A, a, properties={
-            'bs':relation(B, secondary=atob, cascade="all, delete-orphan")
+            'bs':relation(B, secondary=atob, cascade="all, delete-orphan", single_parent=True)
         })
         mapper(B, b)
 
@@ -727,39 +773,46 @@ class M2MCascadeTest(_base.MappedTest):
         assert b.count().scalar() == 0
         assert a.count().scalar() == 0
 
-    @testing.emits_warning(".*not supported on a many-to-many")
-    @testing.fails_on_everything_except('sqlite')
     @testing.resolve_artifact_names
-    def test_this_doesnt_work(self):
-        """illustrates why cascade with m2m should not be supported
-            (i.e. many parents...)
-            
-        """
+    def test_single_parent_raise(self):
         mapper(A, a, properties={
-            'bs':relation(B, secondary=atob, cascade="all, delete-orphan")
+            'bs':relation(B, secondary=atob, cascade="all, delete-orphan", single_parent=True)
         })
         mapper(B, b)
 
         sess = create_session()
         b1 =B(data='b1')
         a1 = A(data='a1', bs=[b1])
-        a2 = A(data='a2', bs=[b1])
-        sess.add(a1)
-        sess.add(a2)
-        sess.flush()
+        
+        self.assertRaises(sa_exc.InvalidRequestError,
+                A, data='a2', bs=[b1]
+            )
 
-        sess.delete(a1)
+    @testing.resolve_artifact_names
+    def test_single_parent_backref(self):
+        """test that setting m2m via a uselist=False backref bypasses the single_parent raise"""
         
-        # this raises an integrity error on DBs that support FKs
-        sess.flush()
+        mapper(A, a, properties={
+            'bs':relation(B, 
+                secondary=atob, 
+                cascade="all, delete-orphan", single_parent=True,
+                backref=backref('a', uselist=False))
+        })
+        mapper(B, b)
+
+        sess = create_session()
+        b1 =B(data='b1')
+        a1 = A(data='a1', bs=[b1])
         
-        # still a row present !
-        assert atob.count().scalar() ==1
+        self.assertRaises(
+            sa_exc.InvalidRequestError,
+            A, data='a2', bs=[b1]
+        )
         
-        # but no bs !
-        assert b.count().scalar() == 0
-        assert a.count().scalar() == 1
-
+        a2 = A(data='a2')
+        b1.a = a2
+        assert b1 not in a1.bs
+        assert b1 in a2.bs
 
 class UnsavedOrphansTest(_base.MappedTest):
     """Pending entities that are orphans"""
@@ -927,9 +980,9 @@ class UnsavedOrphansTest3(_base.MappedTest):
                    ForeignKey('accounts.account_id')))
 
     @testing.resolve_artifact_names
-    def test_double_parent_expunge(self):
-        """Removing a pending item from a collection expunges it from the session."""
-
+    def test_double_parent_expunge_o2m(self):
+        """test the delete-orphan uow event for multiple delete-orphan parent relations."""
+        
         class Customer(_fixtures.Base):
             pass
         class Account(_fixtures.Base):
@@ -965,6 +1018,47 @@ class UnsavedOrphansTest3(_base.MappedTest):
         sr.customers.remove(c)
         assert c not in s, "Should expunge customer when both parents are gone"
 
+    @testing.resolve_artifact_names
+    def test_double_parent_expunge_o2o(self):
+        """test the delete-orphan uow event for multiple delete-orphan parent relations."""
+
+        class Customer(_fixtures.Base):
+            pass
+        class Account(_fixtures.Base):
+            pass
+        class SalesRep(_fixtures.Base):
+            pass
+
+        mapper(Customer, customers)
+        mapper(Account, accounts, properties=dict(
+            customer=relation(Customer,
+                               cascade="all,delete-orphan",
+                               backref="account", uselist=False)))
+        mapper(SalesRep, sales_reps, properties=dict(
+            customer=relation(Customer,
+                               cascade="all,delete-orphan",
+                               backref="sales_rep", uselist=False)))
+        s = create_session()
+
+        a = Account(balance=0)
+        sr = SalesRep(name="John")
+        s.add_all((a, sr))
+        s.flush()
+
+        c = Customer(name="Jane")
+
+        a.customer = c
+        sr.customer = c
+        assert c in s
+
+        a.customer = None
+        assert c in s, "Should not expunge customer yet, still has one parent"
+
+        sr.customer = None
+        assert c not in s, "Should expunge customer when both parents are gone"
+
+        
+        
 class DoubleParentOrphanTest(_base.MappedTest):
     """test orphan detection for an entity with two parent relations"""
 
@@ -1000,8 +1094,8 @@ class DoubleParentOrphanTest(_base.MappedTest):
             pass
 
         mapper(Address, addresses)
-        mapper(Home, homes, properties={'address':relation(Address, cascade="all,delete-orphan")})
-        mapper(Business, businesses, properties={'address':relation(Address, cascade="all,delete-orphan")})
+        mapper(Home, homes, properties={'address':relation(Address, cascade="all,delete-orphan", single_parent=True)})
+        mapper(Business, businesses, properties={'address':relation(Address, cascade="all,delete-orphan", single_parent=True)})
 
         session = create_session()
         h1 = Home(description='home1', address=Address(street='address1'))
@@ -1026,8 +1120,8 @@ class DoubleParentOrphanTest(_base.MappedTest):
             pass
 
         mapper(Address, addresses)
-        mapper(Home, homes, properties={'address':relation(Address, cascade="all,delete-orphan")})
-        mapper(Business, businesses, properties={'address':relation(Address, cascade="all,delete-orphan")})
+        mapper(Home, homes, properties={'address':relation(Address, cascade="all,delete-orphan", single_parent=True)})
+        mapper(Business, businesses, properties={'address':relation(Address, cascade="all,delete-orphan", single_parent=True)})
 
         session = create_session()
         a1 = Address()
index 32a5cce1ffc5ba3207e684fd52a760fe1c7d7823..532203ce2040d8defdbe221d5cdbba30c2c0c563 100644 (file)
@@ -452,7 +452,7 @@ class RelationTest4(_base.MappedTest):
                         #"save-update, delete-orphan",
                         "save-update, delete, delete-orphan"):
             mapper(B, tableB, properties={
-                'a':relation(A, cascade=cascade)
+                'a':relation(A, cascade=cascade, single_parent=True)
             })
             mapper(A, tableA)