From 7c56371f81707b5979249b2f2b056f65488f1bab Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Tue, 20 Jan 2009 21:35:57 +0000 Subject: [PATCH] - 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. 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 | 20 ++++ doc/build/mappers.rst | 2 - doc/build/session.rst | 2 + lib/sqlalchemy/orm/__init__.py | 10 +- lib/sqlalchemy/orm/attributes.py | 12 ++- lib/sqlalchemy/orm/properties.py | 8 +- lib/sqlalchemy/orm/strategies.py | 26 ++++- lib/sqlalchemy/orm/unitofwork.py | 3 +- test/orm/cascade.py | 174 ++++++++++++++++++++++++------- test/orm/relationships.py | 2 +- 10 files changed, 207 insertions(+), 52 deletions(-) diff --git a/CHANGES b/CHANGES index 74ae62752f..5ff51fa8cf 100644 --- 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] diff --git a/doc/build/mappers.rst b/doc/build/mappers.rst index 07b89da604..cb770415e9 100644 --- a/doc/build/mappers.rst +++ b/doc/build/mappers.rst @@ -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: diff --git a/doc/build/session.rst b/doc/build/session.rst index 96463b6a86..a71b6b4858 100644 --- a/doc/build/session.rst +++ b/doc/build/session.rst @@ -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 diff --git a/lib/sqlalchemy/orm/__init__.py b/lib/sqlalchemy/orm/__init__.py index e9d98ac343..7e64bda7ab 100644 --- a/lib/sqlalchemy/orm/__init__.py +++ b/lib/sqlalchemy/orm/__init__.py @@ -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 diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index 3f2fc9b122..70b0738c95 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -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): diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py index c83e03599d..22806e364a 100644 --- a/lib/sqlalchemy/orm/properties.py +++ b/lib/sqlalchemy/orm/properties.py @@ -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: diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 7195310cdf..22ef7ded25 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -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) + + diff --git a/lib/sqlalchemy/orm/unitofwork.py b/lib/sqlalchemy/orm/unitofwork.py index 5f32884e76..c756045a1e 100644 --- a/lib/sqlalchemy/orm/unitofwork.py +++ b/lib/sqlalchemy/orm/unitofwork.py @@ -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 diff --git a/test/orm/cascade.py b/test/orm/cascade.py index 10de5cce74..3345a5d8cf 100644 --- a/test/orm/cascade.py +++ b/test/orm/cascade.py @@ -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() diff --git a/test/orm/relationships.py b/test/orm/relationships.py index 32a5cce1ff..532203ce20 100644 --- a/test/orm/relationships.py +++ b/test/orm/relationships.py @@ -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) -- 2.47.3