- clarified the error message which occurs when you try to update()

an instance with the same identity key as an instance already present
in the session.
- opened up the recursive checks in session.merge() a little bit

diff --git a/CHANGES b/CHANGES
index a44c402b5ddd5267f32ec6893f0969053e00aa19..46272c5868de90af7eac0a951729cc4be41da58b 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -8,7 +8,11 @@ CHANGES
 
    - fixed endless loop issue when using lazy="dynamic" on both 
      sides of a bi-directional relationship [ticket:872]
-     
+
+   - clarified the error message which occurs when you try to update()
+     an instance with the same identity key as an instance already present
+     in the session.
+ 
 0.4.1
 -----
 

diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py
index df52512eed263e01fb046bf3f847b934f0cade4a..cbc21a2c3fbdecdf4360f5971662e1d04080cb29 100644 (file)
--- a/lib/sqlalchemy/orm/properties.py
+++ b/lib/sqlalchemy/orm/properties.py
@@ -281,7 +281,7 @@ class PropertyLoader(StrategizedProperty):
         return str(self.parent.class_.__name__) + "." + self.key + " (" + str(self.mapper.class_.__name__)  + ")"
 
     def merge(self, session, source, dest, dont_load, _recursive):
-        if not "merge" in self.cascade or self.mapper in _recursive:
+        if not "merge" in self.cascade:
             return
         childlist = sessionlib.attribute_manager.get_history(source, self.key, passive=True)
         if childlist is None:
@@ -300,7 +300,6 @@ class PropertyLoader(StrategizedProperty):
                 if obj is not None:
                     setattr(dest, self.key, obj)
 
-
     def cascade_iterator(self, type, object, recursive, halt_on=None):
         if not type in self.cascade:
             return

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index 0b87b39614805e351115100819bf9785152e4e99..1007275204f43b9ec1c79a38a905a86a6369f638 100644 (file)
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -866,40 +866,35 @@ class Session(object):
         """
 
         if _recursive is None:
-            _recursive = util.Set()
+            _recursive = {}  #TODO: this should be an IdentityDict
         if entity_name is not None:
             mapper = _class_mapper(object.__class__, entity_name=entity_name)
         else:
             mapper = _object_mapper(object)
-        if mapper in _recursive or object in _recursive:
-            return None
-        _recursive.add(mapper)
-        _recursive.add(object)
-        try:
-            key = getattr(object, '_instance_key', None)
-            if key is None:
+        if object in _recursive:
+            return _recursive[object]
+        
+        key = getattr(object, '_instance_key', None)
+        if key is None:
+            merged = attribute_manager.new_instance(mapper.class_)
+        else:
+            if key in self.identity_map:
+                merged = self.identity_map[key]
+            elif dont_load:
                 merged = attribute_manager.new_instance(mapper.class_)
+                merged._instance_key = key
+                self._update_impl(merged, entity_name=mapper.entity_name)
+                merged._state.committed_state = object._state.committed_state.copy()
             else:
-                if key in self.identity_map:
-                    merged = self.identity_map[key]
-                elif dont_load:
-                    merged = attribute_manager.new_instance(mapper.class_)
-                    merged._instance_key = key
-                    self.update(merged, entity_name=mapper.entity_name)
-                    merged._state.committed_state = object._state.committed_state.copy()
-                else:
-                    merged = self.get(mapper.class_, key[1])
-                    if merged is None:
-                        raise exceptions.AssertionError("Instance %s has an instance key but is not persisted" % mapperutil.instance_str(object))
-            for prop in mapper.iterate_properties:
-                prop.merge(self, object, merged, dont_load, _recursive)
-            if dont_load:
-                merged._state.modified = object._state.modified
-            if key is None:
-                self.save(merged, entity_name=mapper.entity_name)
-            return merged
-        finally:
-            _recursive.remove(mapper)
+                merged = self.get(mapper.class_, key[1])
+                if merged is None:
+                    raise exceptions.AssertionError("Instance %s has an instance key but is not persisted" % mapperutil.instance_str(object))
+        _recursive[object] = merged
+        for prop in mapper.iterate_properties:
+            prop.merge(self, object, merged, dont_load, _recursive)
+        if key is None:
+            self.save(merged, entity_name=mapper.entity_name)
+        return merged
             
     def identity_key(cls, *args, **kwargs):
         """Get an identity key.
@@ -987,7 +982,7 @@ class Session(object):
         if not hasattr(obj, '_instance_key'):
             raise exceptions.InvalidRequestError("Instance '%s' is not persisted" % mapperutil.instance_str(obj))
         elif self.identity_map.get(obj._instance_key, obj) is not obj:
-            raise exceptions.InvalidRequestError("Instance '%s' is with key %s already persisted with a different identity" % (mapperutil.instance_str(obj), obj._instance_key))
+            raise exceptions.InvalidRequestError("Could not update instance '%s', identity key %s; a different instance with the same identity key already exists in this session." % (mapperutil.instance_str(obj), obj._instance_key))
         self._attach(obj)
 
     def _save_or_update_impl(self, object, entity_name=None):

diff --git a/test/orm/merge.py b/test/orm/merge.py
index fd3e0f446c1a03e7753b5e3bb4f84f147bf7ea76..ed55e1f49b5f757537de87deb49f934e74d93ece 100644 (file)
--- a/test/orm/merge.py
+++ b/test/orm/merge.py
@@ -104,6 +104,9 @@ class MergeTest(AssertMixin):
         # merge persistent object into another session
         sess4 = create_session()
         u = sess4.merge(u)
+        assert len(u.addresses)
+        for a in u.addresses:
+            assert a.user is u
         def go():
             sess4.flush()
         # no changes; therefore flush should do nothing
@@ -111,7 +114,11 @@ class MergeTest(AssertMixin):
         
         # test with "dontload" merge
         sess5 = create_session()
+        print "------------------"
         u = sess5.merge(u, dont_load=True)
+        assert len(u.addresses)
+        for a in u.addresses:
+            assert a.user is u
         def go():
             sess5.flush()
         # no changes; therefore flush should do nothing