From: Mike Bayer Date: Sun, 28 Nov 2010 21:27:44 +0000 (-0500) Subject: after some usage, its clear that [ticket:1974] should not be implemented. backrefs X-Git-Tag: rel_0_7b1~221^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ca9d3cea39a8cd2758ec6981872c38b7b3f59079;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git after some usage, its clear that [ticket:1974] should not be implemented. backrefs add to collections so its expected that collection membership would mirror in session membership. Backed out changeset e836366c843cd64a0df569582534868e3fb00f3b --- diff --git a/doc/build/orm/session.rst b/doc/build/orm/session.rst index d40ada7b2b..061f16f7ed 100644 --- a/doc/build/orm/session.rst +++ b/doc/build/orm/session.rst @@ -428,7 +428,7 @@ Lets use the canonical example of the User and Address objects:: id = Column(Integer, primary_key=True) name = Column(String(50), nullable=False) - addresses = relationship("Address", backref="user", cascade_backrefs=True) + addresses = relationship("Address", backref="user") class Address(Base): __tablename__ = 'address' @@ -472,17 +472,21 @@ and made our ``a1`` object pending, as though we had added it. Now we have >>> a1 is existing_a1 False -Above, our ``a1`` is already pending in the session. The subsequent -:meth:`~.Session.merge` operation essentially does nothing. To resolve this -issue, the ``cascade_backrefs`` flag should be set to its default of -``False``. Further detail on cascade operation is at -:ref:`unitofwork_cascades`. - +Above, our ``a1`` is already pending in the session. The +subsequent :meth:`~.Session.merge` operation essentially +does nothing. Cascade can be configured via the ``cascade`` +option on :func:`.relationship`, although in this case it +would mean removing the ``save-update`` cascade from the +``User.addresses`` relationship - and usually, that behavior +is extremely convenient. The solution here would usually be to not assign +``a1.user`` to an object already persistent in the target +session. -.. note:: Up until version 0.7 of SQLAlchemy, the "cascade backrefs" flag - defaulted to ``True`` and prior to 0.6.5 there was no way to disable it, - without turning off "save-update" cascade entirely. +Note that a new :func:`.relationship` option introduced in 0.6.5, +``cascade_backrefs=False``, will also prevent the ``Address`` from +being added to the session via the ``a1.user = u1`` assignment. +Further detail on cascade operation is at :ref:`unitofwork_cascades`. Another example of unexpected state:: @@ -880,8 +884,8 @@ objects to allow attachment to only one parent at a time. The default value for ``cascade`` on :func:`~sqlalchemy.orm.relationship` is ``save-update, merge``. -``save-update`` cascade, by default, does not take place on backrefs (new in 0.7). -This means that, given a mapping such as this:: +``save-update`` cascade also takes place on backrefs by default. This means +that, given a mapping such as this:: mapper(Order, order_table, properties={ 'items' : relationship(Item, items_table, backref='order') @@ -889,8 +893,8 @@ This means that, given a mapping such as this:: If an ``Order`` is already in the session, and is assigned to the ``order`` attribute of an ``Item``, the backref appends the ``Item`` to the ``orders`` -collection of that ``Order``, however, the ``Item`` will not yet be present -in the session:: +collection of that ``Order``, resulting in the ``save-update`` cascade taking +place:: >>> o1 = Order() >>> session.add(o1) @@ -902,34 +906,22 @@ in the session:: >>> i1 in o1.orders True >>> i1 in session - False - -You can of course :func:`~.Session.add` ``i1`` to the session at a later -point. SQLAlchemy defaults to this behavior as of 0.7 as it is helpful for -situations where an object needs to be kept out of a session until it's -construction is completed, but still needs to be given associations to objects -which are already persistent in the target session. It's more intuitive -that "cascades" into a :class:`.Session` work from left to right only, and also -allows session membership behavior to be more compatible with relationships that -don't have backrefs configured. - -The save-update cascade can be made bi-directional using ``cascade_backrefs`` flag:: + True + +This behavior can be disabled as of 0.6.5 using the ``cascade_backrefs`` flag:: mapper(Order, order_table, properties={ 'items' : relationship(Item, items_table, backref='order', - cascade_backrefs=True) + cascade_backrefs=False) }) -Using the above mapping with the previous usage example, -the assignment of ``i1.order = o1`` will append ``i1`` to the ``orders`` -collection of ``o1``, and will add ``i1`` to the session. ``cascade_backrefs`` -is specific to just one direction, so the above configuration still would not -cascade an ``Order`` into the session if it were associated with an ``Item`` -already in the session, unless ``cascade_backrefs`` were also configured on the -"order" side of the relationship. +So above, the assignment of ``i1.order = o1`` will append ``i1`` to the ``orders`` +collection of ``o1``, but will not add ``i1`` to the session. You can of +course :func:`~.Session.add` ``i1`` to the session at a later point. This option +may be helpful for situations where an object needs to be kept out of a +session until it's construction is completed, but still needs to be given +associations to objects which are already persistent in the target session. -Setting ``cascade_backrefs=True`` on a relationship and its backref makes the -operation compatible with the default behavior of SQLAlchemy 0.6 and earlier. .. _unitofwork_transaction: diff --git a/lib/sqlalchemy/orm/__init__.py b/lib/sqlalchemy/orm/__init__.py index d2f5de2b92..659484691e 100644 --- a/lib/sqlalchemy/orm/__init__.py +++ b/lib/sqlalchemy/orm/__init__.py @@ -267,18 +267,17 @@ def relationship(argument, secondary=None, **kwargs): * ``all`` - shorthand for "save-update,merge, refresh-expire, expunge, delete" - :param cascade_backrefs=False: + :param cascade_backrefs=True: a boolean value indicating if the ``save-update`` cascade should - operate along a backref event. When set to ``True`` on a + operate along a backref event. When set to ``False`` on a one-to-many relationship that has a many-to-one backref, assigning a persistent object to the many-to-one attribute on a transient object - will add the transient to the session. Similarly, when - set to ``True`` on a many-to-one relationship that has a one-to-many + will not add the transient to the session. Similarly, when + set to ``False`` on a many-to-one relationship that has a one-to-many backref, appending a persistent object to the one-to-many collection - on a transient object will add the transient to the session. + on a transient object will not add the transient to the session. - ``cascade_backrefs`` is new in 0.6.5. The default value changes - from ``True`` to ``False`` as of the 0.7 series. + ``cascade_backrefs`` is new in 0.6.5. :param collection_class: a class or callable that returns a new list-holding object. will diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py index 664eba62f5..b68290dbdd 100644 --- a/lib/sqlalchemy/orm/properties.py +++ b/lib/sqlalchemy/orm/properties.py @@ -452,7 +452,7 @@ class RelationshipProperty(StrategizedProperty): single_parent=False, innerjoin=False, doc=None, active_history=False, - cascade_backrefs=False, + cascade_backrefs=True, load_on_pending=False, strategy_class=None, _local_remote_pairs=None, query_class=None): diff --git a/test/orm/inheritance/test_polymorph.py b/test/orm/inheritance/test_polymorph.py index 33646c9227..7296bec98e 100644 --- a/test/orm/inheritance/test_polymorph.py +++ b/test/orm/inheritance/test_polymorph.py @@ -258,7 +258,6 @@ def _generate_round_trip_test(include_base, lazy_relationship, redefine_colprop, c = session.query(Company).first() daboss.company = c - session.add(daboss) manager_list = [e for e in c.employees if isinstance(e, Manager)] session.flush() session.expunge_all() diff --git a/test/orm/test_assorted_eager.py b/test/orm/test_assorted_eager.py index ff9234d701..0e389b74ba 100644 --- a/test/orm/test_assorted_eager.py +++ b/test/orm/test_assorted_eager.py @@ -2,9 +2,6 @@ Derived from mailing list-reported problems and trac tickets. -These are generally very old 0.1-era tests and at some point should -be cleaned up and modernized. - """ import datetime @@ -583,6 +580,12 @@ class EagerTest7(_base.MappedTest): Column('company_id', Integer, ForeignKey("companies.company_id")), Column('date', sa.DateTime)) + Table('items', metadata, + Column('item_id', Integer, primary_key=True, test_needs_autoincrement=True), + Column('invoice_id', Integer, ForeignKey('invoices.invoice_id')), + Column('code', String(20)), + Column('qty', Integer)) + @classmethod def setup_classes(cls): class Company(_base.ComparableEntity): @@ -594,11 +597,14 @@ class EagerTest7(_base.MappedTest): class Phone(_base.ComparableEntity): pass + class Item(_base.ComparableEntity): + pass + class Invoice(_base.ComparableEntity): pass @testing.resolve_artifact_names - def test_load_m2o_attached_to_o2m(self): + def testone(self): """ Tests eager load of a many-to-one attached to a one-to-many. this testcase illustrated the bug, which is that when the single Company is @@ -632,12 +638,66 @@ class EagerTest7(_base.MappedTest): session.expunge_all() i = session.query(Invoice).get(invoice_id) - - def go(): - eq_(c, i.company) - eq_(c.addresses, i.company.addresses) - self.assert_sql_count(testing.db, go, 0) + eq_(c, i.company) + + @testing.resolve_artifact_names + def testtwo(self): + """The original testcase that includes various complicating factors""" + + mapper(Phone, phone_numbers) + + mapper(Address, addresses, properties={ + 'phones': relationship(Phone, lazy='joined', backref='address', + order_by=phone_numbers.c.phone_id)}) + + mapper(Company, companies, properties={ + 'addresses': relationship(Address, lazy='joined', backref='company', + order_by=addresses.c.address_id)}) + + mapper(Item, items) + + mapper(Invoice, invoices, properties={ + 'items': relationship(Item, lazy='joined', backref='invoice', + order_by=items.c.item_id), + 'company': relationship(Company, lazy='joined', backref='invoices')}) + + c1 = Company(company_name='company 1', addresses=[ + Address(address='a1 address', + phones=[Phone(type='home', number='1111'), + Phone(type='work', number='22222')]), + Address(address='a2 address', + phones=[Phone(type='home', number='3333'), + Phone(type='work', number='44444')]) + ]) + + session = create_session() + session.add(c1) + session.flush() + + company_id = c1.company_id + + session.expunge_all() + + a = session.query(Company).get(company_id) + + # set up an invoice + i1 = Invoice(date=datetime.datetime.now(), company=a) + + item1 = Item(code='aaaa', qty=1, invoice=i1) + item2 = Item(code='bbbb', qty=2, invoice=i1) + item3 = Item(code='cccc', qty=3, invoice=i1) + + session.flush() + invoice_id = i1.invoice_id + + session.expunge_all() + c = session.query(Company).get(company_id) + + session.expunge_all() + i = session.query(Invoice).get(invoice_id) + + eq_(c, i.company) class EagerTest8(_base.MappedTest): diff --git a/test/orm/test_backref_mutations.py b/test/orm/test_backref_mutations.py index dd48df592d..42b6054b37 100644 --- a/test/orm/test_backref_mutations.py +++ b/test/orm/test_backref_mutations.py @@ -438,7 +438,6 @@ class O2OScalarOrphanTest(_fixtures.FixtureTest): # the _SingleParent extension sets the backref get to "active" ! # u1 gets loaded and deleted u2.address = a1 - sess.add(u2) sess.commit() assert sess.query(User).count() == 1 diff --git a/test/orm/test_cascade.py b/test/orm/test_cascade.py index bedff21704..0deef18fd1 100644 --- a/test/orm/test_cascade.py +++ b/test/orm/test_cascade.py @@ -1472,15 +1472,13 @@ class NoBackrefCascadeTest(_fixtures.FixtureTest): def setup_mappers(cls): mapper(Address, addresses) mapper(User, users, properties={ - 'addresses':relationship(Address, - backref=backref('user', cascade_backrefs=True), - ) + 'addresses':relationship(Address, backref='user', + cascade_backrefs=False) }) mapper(Dingaling, dingalings, properties={ - 'address' : relationship(Address, - backref=backref('dingalings', cascade_backrefs=True), - ) + 'address' : relationship(Address, backref='dingalings', + cascade_backrefs=False) }) @testing.resolve_artifact_names diff --git a/test/orm/test_expire.py b/test/orm/test_expire.py index 0d9f5e7453..c8bdf1719e 100644 --- a/test/orm/test_expire.py +++ b/test/orm/test_expire.py @@ -973,7 +973,6 @@ class ExpiredPendingTest(_fixtures.FixtureTest): u1 = User(name='u1') a1.user = u1 - sess.flush() # expire 'addresses'. backrefs @@ -984,8 +983,7 @@ class ExpiredPendingTest(_fixtures.FixtureTest): # in user.addresses a2 = Address(email_address='a2') a2.user = u1 - sess.add(a2) - + # expire u1.addresses again. this expires # "pending" as well. sess.expire(u1, ['addresses']) @@ -996,7 +994,6 @@ class ExpiredPendingTest(_fixtures.FixtureTest): # only two addresses pulled from the DB, no "pending" assert len(u1.addresses) == 2 - # flushes a2 sess.flush() sess.expire_all() assert len(u1.addresses) == 3 diff --git a/test/orm/test_mapper.py b/test/orm/test_mapper.py index 78e0f22063..4438ab8f70 100644 --- a/test/orm/test_mapper.py +++ b/test/orm/test_mapper.py @@ -3068,7 +3068,7 @@ class RequirementsTest(_base.MappedTest): h6 = H6() h6.h1a = h1 h6.h1b = x = H1() - s.add(x) + assert x in s h6.h1b.h2s.append(H2()) diff --git a/test/orm/test_merge.py b/test/orm/test_merge.py index 50d001a8c2..15c52a1e01 100644 --- a/test/orm/test_merge.py +++ b/test/orm/test_merge.py @@ -4,9 +4,8 @@ from sqlalchemy import Integer, PickleType, String import operator from sqlalchemy.test import testing from sqlalchemy.util import OrderedSet -from sqlalchemy.orm import mapper, relationship, create_session, \ - PropComparator, synonym, comparable_property, sessionmaker, \ - attributes, Session +from sqlalchemy.orm import mapper, relationship, create_session, PropComparator, \ + synonym, comparable_property, sessionmaker, attributes from sqlalchemy.orm.collections import attribute_mapped_collection from sqlalchemy.orm.interfaces import MapperOption from sqlalchemy.test.testing import eq_, ne_ @@ -877,22 +876,13 @@ class MergeTest(_fixtures.FixtureTest): def test_cascade_doesnt_blowaway_manytoone(self): """a merge test that was fixed by [ticket:1202]""" - s = Session() - - mapper(Address, addresses) + s = create_session(autoflush=True) mapper(User, users, properties={ - 'addresses':relationship(Address,backref='user') - }) + 'addresses':relationship(mapper(Address, addresses),backref='user')}) - u1 = s.merge(User(id=1, name='ed')) - a1 = Address(user=u1, email_address='x') - s.add(a1) - + a1 = Address(user=s.merge(User(id=1, name='ed')), email_address='x') before_id = id(a1.user) - - # autoflushes a1, u1 - u2 = s.merge(User(id=1, name='jack')) - a2 = Address(user=u2, email_address='x') + a2 = Address(user=s.merge(User(id=1, name='jack')), email_address='x') after_id = id(a1.user) other_id = id(a2.user) eq_(before_id, other_id)