parent tracking tripped up fairly easily upon remove events

[Former user]

(behavior for both SQLAlchemy-0.6.4 and SQLAlchemy-0.7.3dev)

The following script illustrates the problem I've run into.

from sqlalchemy import *
from sqlalchemy.orm import *

engine = create_engine('sqlite:///', echo=True) 
metadata = MetaData(engine)
Session = sessionmaker(bind=engine)

rocks_table = Table("rocks", metadata,
    Column("id", Integer, primary_key=True),
)

bugs_table = Table("bugs", metadata,
    Column("rockid", Integer, ForeignKey('rocks.id'), primary_key=True,),
    Column("id", Integer, primary_key=True),
)

class Rock(object):
    pass

class Bug(object):
    pass


mapper(Rock, rocks_table,
    properties={'bugs': relationship(Bug,
                    cascade='all,delete-orphan', 
                    single_parent=True,
                    lazy=False,
                    backref='rock')
                })

mapper(Bug, bugs_table,
    allow_partial_pks=False)

metadata.create_all()

session = Session()


# add a rock and bug
rock=Rock()
rock.id = 0
bug=Bug()
bug.id = 0
rock.bugs.append(bug)
session.add(rock)

# add another rock and bug
rock=Rock()
rock.id = 1
bug=Bug()
bug.id = 1
rock.bugs.append(bug)
session.add(rock)

session.commit()
session.expunge_all()

# later... new session, we are moving second bug to the first rock using merge()
session = Session()

rocka=Rock()
rocka.id = 0

buga=Bug()
buga.id=0
buga.rockid=0
bugb=Bug()
bugb.id=1
bugb.rockid=1

rocka.bugs = [buga,bugb](buga,bugb)

rockb=Rock()
rockb.id = 1
rockb.bugs = [NOTE: if this line is commented out, the behavior changes completely (seems to work)
currentobjects = session.query(Rock).all()

mergeda = session.merge(rocka)

assert mergeda.bugs[0](]

#).rock is mergeda
assert mergeda.bugs[1](1).rock is mergeda

mergedb = session.merge(rockb)

# Same tests as before:
assert mergeda.bugs[0](0).rock is mergeda
# now, mergeda.bugs[1](1).rock is None and this fails: ############
assert mergeda.bugs[1](1).rock is mergeda

# flush incorrectly DELETES bug
session.flush()

NOTE: commenting out the line that loads all the objects to currentobjects seems to make the orm deal with this correctly.

[Mike Bayer]

similar issue as that of #2221, in that the patch of #2221 does extra work which resolves this quirk. you're merging along one-to-many then testing assertions on reverse many-to-ones that are in some cases loaded. the lack of expiration on reverse relationships causes conflicting activities.

However, merge has nothing to do with this. The equivalent activity without using merge at all has the same effect. The flush() at the end also appears to have a dictionary-ordering-affected behavior whereby the flush tries to emit an UPDATE SET NULL on the PK col in some cases, in others emits DELETE:

session = Session()

currentobjects = session.query(Rock).all()

allbugs = session.query(Bug).all()
r1 = session.query(Rock).get(0)
r1.bugs = allbugs

assert r1.bugs[0](0).rock is r1
assert r1.bugs[1](1).rock is r1

r2 = session.query(Rock).get(1)
# emits delete cascade
r2.bugs = [r1.bugs[0](]

assert).rock is r1

# hasn't been refreshed...so bug1.rock
# stays on None
#assert r1.bugs[1](1).rock is r1
assert r1.bugs[1](1).rock is None

# flush incorrectly DELETES bug
session.flush()

allowing it to reload fixes:

currentobjects = session.query(Rock).all()

allbugs = session.query(Bug).all()
r1 = session.query(Rock).get(0)
r1.bugs = allbugs

assert r1.bugs[0](0).rock is r1
assert r1.bugs[1](1).rock is r1

session.flush()
session.expire_all()

r2 = session.query(Rock).get(1)
# emits delete cascade
r2.bugs = [r1.bugs[0](]

assert).rock is r1
assert r1.bugs[1](1).rock is r1

# no-op
session.flush()

[Mike Bayer]

I'd add that I'm not convinced this is a bug in that SQLA doesn't update remotely affected collections in response to a collection mutation. It waits for the database to tell us about the new state. The example here relies on in-Python behavior to update the state of remote collections.

[Former user]

Replying to zzzeek:

I'd add that I'm not convinced this is a bug in that SQLA doesn't update remotely affected collections in response to a collection mutation. It waits for the database to tell us about the new state. The example here relies on in-Python behavior to update the state of remote collections.

• It seemed like a bug because there was different behavior based on whether the objects were already loaded in the session. So you suggest that collection mutation that may affect another collection needs be programmatically dealt with (and is outside scope of orm)? (I assumed that ''single_parent=True'' included such functionality.)
• Does this seem like SQLA functionality that ''should'' occur for single_parent relationships?
• In my case, I am merging many objects at once as described above and, as such, I can't picture when I would expire the offending relationships. (Particularly since merge() won't cause events, will it?) Can you help me figure out how I could go about detecting that a change within merge has affected certain (single_parent) relationships, so I can expire them as needed?
• In any case, if merge() is irrelevant, feel free to change this ticket's Summary.

[Mike Bayer]

Single parent only deals with in-python manipulations - its documentation states that it installs a "validator" - perhaps this should be clarified, but we mean effectively this validator. It only works upon set/append events. It doesn't know about things that were loaded in the session elsewhere- only when you set x1.y = y1 and x2.y = y1, those two set/append events collaborate to produce an assertion. It can't scan through N number of objects in the session, load every single unloaded (or even loaded) "x.y", and establish that none of those "y" objects reference "y1" already. Even in the case of checking only "loaded", this would require a large dictionary of record keeping which would be quite complex and scale quite poorly.

The use case for "single parent" is only as an assurance for "delete-orphan" cascade in the case of a many-to-one or foreign-key-side one-to-one relationship, that is, one where the database-side foreign keys would actually allow the target object to have multiple parents, so specifically not a one-to-many relationship.

With single parent, delete-orphan cascade won't have to make a decision that an object X has many parents, and only after all those parents have been deleted does the delete of X take place - this is not a supported feature and never will be. So ironically the "single parent" flag exists strictly because of this limitation, that SQLAlchemy does not track/correlate collections and references across all objects.

The difference between the merge use case and the get() use case is that in the get() use case its more explicit that you're pulling from the identity map. The fix suggested in #2221, that backrefs are checked without SQL being emitted, would be the way to go here; until then you should expire the reverse collection on each Bug, that is, session.expire(bug.rock, ['bugs']('bugs')) for each Bug.

[Mike Bayer]

Here's a patch I can do that would at least detect the condition where it goes wrong. That is, when you merge the second time, the "rock.bugs = [hits the backref and tries to set "Bug.rock = None", the backref at least can detect it:

diff -r 9b33b23a13446680c03974b497fc8436fe6c81e9 lib/sqlalchemy/orm/attributes.py
--- a/lib/sqlalchemy/orm/attributes.py  Mon Aug 22 11:26:52 2011 -0400
+++ b/lib/sqlalchemy/orm/attributes.py  Tue Aug 23 18:39:52 2011 -0400
@@ -17,7 +17,7 @@
 from operator import itemgetter

 from sqlalchemy import util, event, exc as sa_exc
-from sqlalchemy.orm import interfaces, collections, events
+from sqlalchemy.orm import interfaces, collections, events, exc as orm_exc


 mapperutil = util.importlater("sqlalchemy.orm", "util")
@@ -445,7 +445,7 @@
         self.set(state, dict_, value, initiator, passive=passive)

     def remove(self, state, dict_, value, initiator, passive=PASSIVE_OFF):
-        self.set(state, dict_, None, initiator, passive=passive)
+        self.set(state, dict_, None, initiator, passive=passive, check_old=value)

     def set(self, state, dict_, value, initiator, passive=PASSIVE_OFF):
         raise NotImplementedError()
@@ -623,7 +623,7 @@
         else:
             return [](]")

-    def set(self, state, dict_, value, initiator, passive=PASSIVE_OFF):
+    def set(self, state, dict_, value, initiator, passive=PASSIVE_OFF, check_old=None):
         """Set a value on the given InstanceState.

         `initiator` is the ``InstrumentedAttribute`` that initiated the
@@ -638,7 +638,11 @@
             old = self.get(state, dict_, passive=PASSIVE_ONLY_PERSISTENT)
         else:
             old = self.get(state, dict_, passive=PASSIVE_NO_FETCH)
-
+        if check_old is not None and \
+            old is not PASSIVE_NO_RESULT and \
+            check_old is not old:
+            raise orm_exc.StaleDataError("Expected object remove of %s, but %s is present" % 
+                            (mapperutil.instance_str(check_old), mapperutil.instance_str(old)))
         value = self.fire_replace_event(state, dict_, value, old, initiator)
         dict_[self.key](self.key) = value

Something like this could also be approximated with attribute events, though events don't propagate that far (i.e. when the backref does its set in response to an append, there's no events fired beyond that), so it would have to be organized on the "append"/"set collection" side. Maybe just to expire the "Rock.bugs" collection on each "Bug.rock" being affected.

I think its possible that I don't really rely on merge() to this degree for complicated things. There's too many details to manage.

[Former user]

Replying to zzzeek:

Something like this could also be approximated with attribute events, though events don't propagate that far (i.e. when the backref does its set in response to an append, there's no events fired beyond that), so it would have to be organized on the "append"/"set collection" side. Maybe just to expire the "Rock.bugs" collection on each "Bug.rock" being affected.

But those don't fire at all during merge(), do they?

P.S. As always, thank you so much for your prompt attention and your time!

[Former user]

Replying to zzzeek:

The use case for "single parent" is only as an assurance for "delete-orphan" cascade in the case of a many-to-one or foreign-key-side one-to-one relationship, that is, one where the database-side foreign keys would actually allow the target object to have multiple parents, so specifically not a one-to-many relationship.

Does this mean that where I have single_parent=True above for the 'bugs' relationship is actually meaningless because, by virtue of the Foreign Key, it can only have one parent anyway? Is it ignored in this case?

[Mike Bayer]

merge() when the "load" flag stays on its default of True is doing regular set events on the destination object, in the same way as a regular attribute set/append operation does. so it really is roughly equivalent to if you wrote your own object-copying routine using regular x2.y = x1.y types of operations.

[Mike Bayer]

per #2314 the same issue occurs without the backref, exhibits non-deterministic behavior upon flush since it's not a given which of "r1" or "r2" wins:

from sqlalchemy.orm import unitofwork, session, mapper, dependency
from sqlalchemy.util import topological
from test.lib.util import RandomSet
topological.set = unitofwork.set = session.set = mapper.set = dependency.set = RandomSet

from sqlalchemy import *
from sqlalchemy.orm import *

engine = create_engine('sqlite:///', echo=True) 
metadata = MetaData(engine)
Session = sessionmaker(bind=engine)

rocks_table = Table("rocks", metadata,
    Column("id", Integer, primary_key=True),
)

bugs_table = Table("bugs", metadata,
    Column("rockid", Integer, ForeignKey('rocks.id'), primary_key=True,),
    Column("id", Integer, primary_key=True),
)

class Rock(object):
    pass

class Bug(object):
    pass


mapper(Rock, rocks_table,
    properties={'bugs': relationship(Bug,
                    cascade='all,delete-orphan', 
                    single_parent=True,
                    lazy=False,
                    #backref='rock'
                    )
                })

mapper(Bug, bugs_table)

metadata.create_all()

session = Session()

rock=Rock()
rock.id = 1
bug=Bug()
bug.id = 1
rock.bugs.append(bug)
session.add(rock)

# add another rock and bug
rock=Rock()
rock.id = 2
bug=Bug()
bug.id = 2
rock.bugs.append(bug)
session.add(rock)

session.commit()
session.expunge_all()


session = Session()


currentobjects = session.query(Rock).all()

allbugs = session.query(Bug).all()
r1 = session.query(Rock).get(1)
r1.bugs = allbugs

r2 = session.query(Rock).get(2)
r2.bugs = []

# flush incorrectly DELETES bug...sometimes !
session.flush()

assert session.query(Bug).count() == 2

[Mike Bayer]

the issue is ultimately that we don't track "parents" carefully. This patch attempts to make a better approach at this, but im very concerned about serialize behavior, mutable primary keys behavior. I'm not sure what kind of record we can put in "parents" that's absolutely foolproof. The sethasparent() routine might want to raise an error if it really doesn't know if it's making the right choice or not and would need tests to illustrate these conditions:

diff -r e6a5ea8fa7d10078c8d3f1bf6cabc4399cac3e70 lib/sqlalchemy/orm/attributes.py
--- a/lib/sqlalchemy/orm/attributes.py  Sat Oct 29 17:38:56 2011 -0400
+++ b/lib/sqlalchemy/orm/attributes.py  Sun Oct 30 00:11:43 2011 -0400
@@ -17,7 +17,7 @@
 from operator import itemgetter

 from sqlalchemy import util, event, exc as sa_exc
-from sqlalchemy.orm import interfaces, collections, events
+from sqlalchemy.orm import interfaces, collections, events, exc as orm_exc


 mapperutil = util.importlater("sqlalchemy.orm", "util")
@@ -126,7 +126,7 @@
         return op(other, self.comparator, **kwargs)

     def hasparent(self, state, optimistic=False):
-        return self.impl.hasparent(state, optimistic=optimistic)
+        return self.impl.hasparent(state, optimistic=optimistic) is not False

     def __getattr__(self, key):
         try:
@@ -346,15 +346,27 @@
         will also not have a `hasparent` flag.

         """
-        return state.parents.get(id(self.parent_token), optimistic)
+        return state.parents.get(id(self.parent_token), optimistic) \
+                is not False

-    def sethasparent(self, state, value):
+    def sethasparent(self, state, parent_state, value):
         """Set a boolean flag on the given item corresponding to
         whether or not it is attached to a parent object via the
         attribute represented by this ``InstrumentedAttribute``.

         """
-        state.parents[id(self.parent_token)](id(self.parent_token)) = value
+        id_ = id(self.parent_token)
+        if value:
+            state.parents[id_](id_) = parent_state
+        else:
+            if id_ in state.parents and \
+                state.parents[id_](id_) is not False and \
+                state.parents[id_](id_) is not parent_state and \
+                state.parents[id_](id_).key != parent_state.key:
+                return
+
+            state.parents[id_](id_) = False
+

     def set_callable(self, state, callable_):
         """Set a callable function for this attribute on the given object.
@@ -449,7 +461,7 @@
         self.set(state, dict_, value, initiator, passive=passive)

     def remove(self, state, dict_, value, initiator, passive=PASSIVE_OFF):
-        self.set(state, dict_, None, initiator, passive=passive)
+        self.set(state, dict_, None, initiator, passive=passive, check_old=value)

     def set(self, state, dict_, value, initiator, passive=PASSIVE_OFF):
         raise NotImplementedError()
@@ -627,7 +639,7 @@
         else:
             return [
-    def set(self, state, dict_, value, initiator, passive=PASSIVE_OFF):
+    def set(self, state, dict_, value, initiator, passive=PASSIVE_OFF, check_old=None):
         """Set a value on the given InstanceState.

         `initiator` is the ``InstrumentedAttribute`` that initiated the
@@ -642,13 +654,16 @@
             old = self.get(state, dict_, passive=PASSIVE_ONLY_PERSISTENT)
         else:
             old = self.get(state, dict_, passive=PASSIVE_NO_FETCH)
-
+        if check_old is not None and \
+             old is not PASSIVE_NO_RESULT and \
+             check_old is not old:
+             return
         value = self.fire_replace_event(state, dict_, value, old, initiator)
         dict_[self.key](]
) = value

     def fire_remove_event(self, state, dict_, value, initiator):
         if self.trackparent and value is not None:
-            self.sethasparent(instance_state(value), False)
+            self.sethasparent(instance_state(value), state, False)

         for fn in self.dispatch.remove:
             fn(state, value, initiator or self)
@@ -660,7 +675,7 @@
             if (previous is not value and
                 previous is not None and
                 previous is not PASSIVE_NO_RESULT):
-                self.sethasparent(instance_state(previous), False)
+                self.sethasparent(instance_state(previous), state, False)

         for fn in self.dispatch.set:
             value = fn(state, value, previous, initiator or self)
@@ -669,7 +684,7 @@

         if self.trackparent:
             if value is not None:
-                self.sethasparent(instance_state(value), True)
+                self.sethasparent(instance_state(value), state, True)

         return value

@@ -751,7 +766,7 @@
         state.modified_event(dict_, self, NEVER_SET, True)

         if self.trackparent and value is not None:
-            self.sethasparent(instance_state(value), True)
+            self.sethasparent(instance_state(value), state, True)

         return value

@@ -760,7 +775,7 @@

     def fire_remove_event(self, state, dict_, value, initiator):
         if self.trackparent and value is not None:
-            self.sethasparent(instance_state(value), False)
+            self.sethasparent(instance_state(value), state, False)

         for fn in self.dispatch.remove:
             fn(state, value, initiator or self)
diff -r e6a5ea8fa7d10078c8d3f1bf6cabc4399cac3e70 lib/sqlalchemy/orm/dynamic.py
--- a/lib/sqlalchemy/orm/dynamic.py Sat Oct 29 17:38:56 2011 -0400
+++ b/lib/sqlalchemy/orm/dynamic.py Sun Oct 30 00:11:43 2011 -0400
@@ -80,14 +80,14 @@
             value = fn(state, value, initiator or self)

         if self.trackparent and value is not None:
-            self.sethasparent(attributes.instance_state(value), True)
+            self.sethasparent(attributes.instance_state(value), state, True)

     def fire_remove_event(self, state, dict_, value, initiator):
         collection_history = self._modified_event(state, dict_)
         collection_history.deleted_items.append(value)

         if self.trackparent and value is not None:
-            self.sethasparent(attributes.instance_state(value), False)
+            self.sethasparent(attributes.instance_state(value), state, False)

         for fn in self.dispatch.remove:
             fn(state, value, initiator or self)
diff -r e6a5ea8fa7d10078c8d3f1bf6cabc4399cac3e70 lib/sqlalchemy/orm/state.py
--- a/lib/sqlalchemy/orm/state.py   Sat Oct 29 17:38:56 2011 -0400
+++ b/lib/sqlalchemy/orm/state.py   Sun Oct 30 00:11:43 2011 -0400
@@ -132,11 +132,11 @@

     def __getstate__(self):
         d = {'instance':self.obj()}
-
         d.update(
             (k, self.__dict__[k](k)) for k in (
-                'committed_state', 'pending', 'parents', 'modified', 'expired', 
-                'callables', 'key', 'load_options', 'mutable_dict'
+                'committed_state', 'pending', 'modified', 'expired', 
+                'callables', 'key', 'parents', 'load_options', 'mutable_dict',
+                'class_',
             ) if k in self.__dict__ 
         )
         if self.load_path:
@@ -148,8 +148,11 @@

     def __setstate__(self, state):
         from sqlalchemy.orm import instrumentation
-        self.obj = weakref.ref(state['instance']('instance'), self._cleanup)
-        self.class_ = state['instance']('instance').__class__
+        if state['instance']('instance') is not None:
+            self.obj = weakref.ref(state['instance']('instance'), self._cleanup)
+        else:
+            self.obj = None
+        self.class_ = state['class_']('class_')
         self.manager = manager = instrumentation.manager_of_class(self.class_)
         if manager is None:
             raise orm_exc.UnmappedInstanceError(
diff -r e6a5ea8fa7d10078c8d3f1bf6cabc4399cac3e70 test/orm/test_relationships.py
--- a/test/orm/test_relationships.py    Sat Oct 29 17:38:56 2011 -0400
+++ b/test/orm/test_relationships.py    Sun Oct 30 00:11:43 2011 -0400
@@ -2738,7 +2738,6 @@
         self._test_attribute(o1, "composite", MyComposite('bar', 1))


-
 class RelationDeprecationTest(fixtures.MappedTest):
     """test usage of the old 'relation' function."""

[Mike Bayer]

another idea: each time a state is flushed or expired, assume it's "valid", wipe out the "parents" collection. "parents" then becomes strictly tracking of changes within a session. it would only need "state" to be present when the object is transient, pending, or dirty, hopefully reducing complications that can occur via pickling etc.

diff -r e6a5ea8fa7d10078c8d3f1bf6cabc4399cac3e70 lib/sqlalchemy/orm/attributes.py
--- a/lib/sqlalchemy/orm/attributes.py  Sat Oct 29 17:38:56 2011 -0400
+++ b/lib/sqlalchemy/orm/attributes.py  Sun Oct 30 00:23:07 2011 -0400
@@ -17,7 +17,7 @@
 from operator import itemgetter

 from sqlalchemy import util, event, exc as sa_exc
-from sqlalchemy.orm import interfaces, collections, events
+from sqlalchemy.orm import interfaces, collections, events, exc as orm_exc


 mapperutil = util.importlater("sqlalchemy.orm", "util")
@@ -126,7 +126,7 @@
         return op(other, self.comparator, **kwargs)

     def hasparent(self, state, optimistic=False):
-        return self.impl.hasparent(state, optimistic=optimistic)
+        return self.impl.hasparent(state, optimistic=optimistic) is not False

     def __getattr__(self, key):
         try:
@@ -346,15 +346,27 @@
         will also not have a `hasparent` flag.

         """
-        return state.parents.get(id(self.parent_token), optimistic)
+        return state.parents.get(id(self.parent_token), optimistic) \
+                is not False

-    def sethasparent(self, state, value):
+    def sethasparent(self, state, parent_state, value):
         """Set a boolean flag on the given item corresponding to
         whether or not it is attached to a parent object via the
         attribute represented by this ``InstrumentedAttribute``.

         """
-        state.parents[id(self.parent_token)](id(self.parent_token)) = value
+        id_ = id(self.parent_token)
+        if value:
+            state.parents[id_](id_) = parent_state
+        else:
+            if id_ in state.parents and \
+                state.parents[id_](id_) is not False and \
+                state.parents[id_](id_) is not parent_state and \
+                state.parents[id_](id_).key != parent_state.key:
+                return
+
+            state.parents[id_](id_) = False
+

     def set_callable(self, state, callable_):
         """Set a callable function for this attribute on the given object.
@@ -449,7 +461,7 @@
         self.set(state, dict_, value, initiator, passive=passive)

     def remove(self, state, dict_, value, initiator, passive=PASSIVE_OFF):
-        self.set(state, dict_, None, initiator, passive=passive)
+        self.set(state, dict_, None, initiator, passive=passive, check_old=value)

     def set(self, state, dict_, value, initiator, passive=PASSIVE_OFF):
         raise NotImplementedError()
@@ -627,7 +639,7 @@
         else:
             return [
-    def set(self, state, dict_, value, initiator, passive=PASSIVE_OFF):
+    def set(self, state, dict_, value, initiator, passive=PASSIVE_OFF, check_old=None):
         """Set a value on the given InstanceState.

         `initiator` is the ``InstrumentedAttribute`` that initiated the
@@ -642,13 +654,16 @@
             old = self.get(state, dict_, passive=PASSIVE_ONLY_PERSISTENT)
         else:
             old = self.get(state, dict_, passive=PASSIVE_NO_FETCH)
-
+        if check_old is not None and \
+             old is not PASSIVE_NO_RESULT and \
+             check_old is not old:
+             return
         value = self.fire_replace_event(state, dict_, value, old, initiator)
         dict_[self.key](]
) = value

     def fire_remove_event(self, state, dict_, value, initiator):
         if self.trackparent and value is not None:
-            self.sethasparent(instance_state(value), False)
+            self.sethasparent(instance_state(value), state, False)

         for fn in self.dispatch.remove:
             fn(state, value, initiator or self)
@@ -660,7 +675,7 @@
             if (previous is not value and
                 previous is not None and
                 previous is not PASSIVE_NO_RESULT):
-                self.sethasparent(instance_state(previous), False)
+                self.sethasparent(instance_state(previous), state, False)

         for fn in self.dispatch.set:
             value = fn(state, value, previous, initiator or self)
@@ -669,7 +684,7 @@

         if self.trackparent:
             if value is not None:
-                self.sethasparent(instance_state(value), True)
+                self.sethasparent(instance_state(value), state, True)

         return value

@@ -751,7 +766,7 @@
         state.modified_event(dict_, self, NEVER_SET, True)

         if self.trackparent and value is not None:
-            self.sethasparent(instance_state(value), True)
+            self.sethasparent(instance_state(value), state, True)

         return value

@@ -760,7 +775,7 @@

     def fire_remove_event(self, state, dict_, value, initiator):
         if self.trackparent and value is not None:
-            self.sethasparent(instance_state(value), False)
+            self.sethasparent(instance_state(value), state, False)

         for fn in self.dispatch.remove:
             fn(state, value, initiator or self)
diff -r e6a5ea8fa7d10078c8d3f1bf6cabc4399cac3e70 lib/sqlalchemy/orm/dynamic.py
--- a/lib/sqlalchemy/orm/dynamic.py Sat Oct 29 17:38:56 2011 -0400
+++ b/lib/sqlalchemy/orm/dynamic.py Sun Oct 30 00:23:07 2011 -0400
@@ -80,14 +80,14 @@
             value = fn(state, value, initiator or self)

         if self.trackparent and value is not None:
-            self.sethasparent(attributes.instance_state(value), True)
+            self.sethasparent(attributes.instance_state(value), state, True)

     def fire_remove_event(self, state, dict_, value, initiator):
         collection_history = self._modified_event(state, dict_)
         collection_history.deleted_items.append(value)

         if self.trackparent and value is not None:
-            self.sethasparent(attributes.instance_state(value), False)
+            self.sethasparent(attributes.instance_state(value), state, False)

         for fn in self.dispatch.remove:
             fn(state, value, initiator or self)
diff -r e6a5ea8fa7d10078c8d3f1bf6cabc4399cac3e70 lib/sqlalchemy/orm/state.py
--- a/lib/sqlalchemy/orm/state.py   Sat Oct 29 17:38:56 2011 -0400
+++ b/lib/sqlalchemy/orm/state.py   Sun Oct 30 00:23:07 2011 -0400
@@ -132,11 +132,11 @@

     def __getstate__(self):
         d = {'instance':self.obj()}
-
         d.update(
             (k, self.__dict__[k](k)) for k in (
-                'committed_state', 'pending', 'parents', 'modified', 'expired', 
-                'callables', 'key', 'load_options', 'mutable_dict'
+                'committed_state', 'pending', 'modified', 'expired', 
+                'callables', 'key', 'parents', 'load_options', 'mutable_dict',
+                'class_',
             ) if k in self.__dict__ 
         )
         if self.load_path:
@@ -148,8 +148,11 @@

     def __setstate__(self, state):
         from sqlalchemy.orm import instrumentation
-        self.obj = weakref.ref(state['instance']('instance'), self._cleanup)
-        self.class_ = state['instance']('instance').__class__
+        if state['instance']('instance') is not None:
+            self.obj = weakref.ref(state['instance']('instance'), self._cleanup)
+        else:
+            self.obj = None
+        self.class_ = state['class_']('class_')
         self.manager = manager = instrumentation.manager_of_class(self.class_)
         if manager is None:
             raise orm_exc.UnmappedInstanceError(
@@ -220,13 +223,11 @@

         self.modified = False

-        pending = self.__dict__.get('pending', None)
-        mutable_dict = self.mutable_dict
         self.committed_state.clear()
-        if mutable_dict:
-            mutable_dict.clear()
-        if pending:
-            pending.clear()
+
+        self.__dict__.pop('pending', None)
+        self.__dict__.pop('parents', None)
+        self.__dict__.pop('mutable_dict', None)

         for key in self.manager:
             impl = self.manager[key](key).impl
@@ -239,6 +240,7 @@

     def expire_attributes(self, dict_, attribute_names):
         pending = self.__dict__.get('pending', None)
+        parents = self.__dict__.get('parents', None)
         mutable_dict = self.mutable_dict

         for key in attribute_names:
@@ -252,6 +254,8 @@
                 mutable_dict.pop(key, None)
             if pending:
                 pending.pop(key, None)
+            if parents:
+                parents.pop(id(impl.parent_token), None)

         self.manager.dispatch.expire(self, attribute_names)

diff -r e6a5ea8fa7d10078c8d3f1bf6cabc4399cac3e70 test/orm/test_relationships.py
--- a/test/orm/test_relationships.py    Sat Oct 29 17:38:56 2011 -0400
+++ b/test/orm/test_relationships.py    Sun Oct 30 00:23:07 2011 -0400
@@ -2738,7 +2738,6 @@
         self._test_attribute(o1, "composite", MyComposite('bar', 1))


-
 class RelationDeprecationTest(fixtures.MappedTest):
     """test usage of the old 'relation' function."""

[Mike Bayer]

patch for this with notes in acc5277a811576a302465225e4e715b0c4c42c91

[Former user]

I haven't attempted to make sense of these code changes, but two questions:

1. My incorrect assumption from the beginning was that single_parent=True, among other things, would track when an instance was added to a parent and automatically remove it from its original parent. Is that effectively what 'trackparent' is accomplishing now?
2. When merging a one to many relationship, since its backref is a single object and we already know what it is, is that automatically set now? (Or still concerned about circular references when serializing?)

[Mike Bayer]

Replying to guest:

I haven't attempted to make sense of these code changes, but two questions:

1. My incorrect assumption from the beginning was that single_parent=True, among other things, would track when an instance was added to a parent and automatically remove it from its original parent. Is that effectively what 'trackparent' is accomplishing now?

whether or not an object has a "parent" is always tracked when the child is known to have a foreign key attribute that's managed, i.e. a one-to-many. The unit of work always sets a foreign key attribute to NULL, or if configured for delete deletes the row, when the parent primary key is being deleted or when the child is detached from the parent.

single_parent=True only refers to a many-to-many or many-to-one relationship, where you'd like to enforce that the non-foreign key child has only one parent, essentially one-to-one. the single_parent flag only exists so that people can use "delete-orphan" cascade in the other direction, since SQLA doesn't track multiple parents.

1. When merging a one to many relationship, since its backref is a single object and we already know what it is, is that automatically set now? (Or still concerned about circular references when serializing?)

that sounds like #2221 to me, which is just an optimization - to copy relationships in both directions rather than waiting for a SQL load later on. As it de-optimizes other cases, it remains to be seen how that one will pan out. Depends on which case is more common as well as if the de-optimizing case can be ameliorated. the issues here had to do with decisions being made against stale state, its more cut and dry that this should never be done.

[Mike Bayer]

Removing milestone: 0.7.4 (automated comment)