From 6fa74de09da864c8f9a1d9db5cb7d8d4132ad6cd Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Tue, 20 Nov 2007 15:55:36 +0000 Subject: [PATCH] - 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 --- CHANGES | 6 +++- lib/sqlalchemy/orm/properties.py | 3 +- lib/sqlalchemy/orm/session.py | 53 +++++++++++++++----------------- test/orm/merge.py | 7 +++++ 4 files changed, 37 insertions(+), 32 deletions(-) diff --git a/CHANGES b/CHANGES index a44c402b5d..46272c5868 100644 --- 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 df52512eed..cbc21a2c3f 100644 --- 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 0b87b39614..1007275204 100644 --- 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 fd3e0f446c..ed55e1f49b 100644 --- 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 -- 2.47.2