From: Mike Bayer Date: Fri, 1 Sep 2006 17:01:55 +0000 (+0000) Subject: futher fix to the "orphan state" idea. to avoid setting tons of X-Git-Tag: rel_0_2_8~11 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=35e2f6680b17dcf0f65b4e392d2504bfe8000efb;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git futher fix to the "orphan state" idea. to avoid setting tons of "hasparent" flags on objects as they are loaded, both from lazy and eager loads, the "orphan" check now uses an "optimistic" flag to determine the result if no "hasparent" flag is found for a particular relationship on an instance. if the instance has an _instance_key and therefore was loaded from the database, it is assumed to not be an orphan unless a "False" hasparent flag has been set. if the instance does not have an _instance_key and is therefore transient/pending, it is assumed to be an orphan unless a "True" hasparent flag has been set. --- diff --git a/CHANGES b/CHANGES index c509caeb6d..79d8e3d6d5 100644 --- a/CHANGES +++ b/CHANGES @@ -26,6 +26,13 @@ persistent with a session already. - unit-of-work does a better check for "orphaned" objects that are part of a "delete-orphan" cascade, for certain conditions where the parent isnt available to cascade from. +- mappers can tell if one of their objects is an "orphan" based +on interactions with the attribute package. this check is based +on a status flag maintained for each relationship +when objects are attached and detached from each other. if the +status flag is not present, its assumed to be "False" for a +transient instance and assumed to be "True" for a persisted/detached + instance. - it is now invalid to declare a self-referential relationship with "delete-orphan" (as the abovementioned check would make them impossible to save) diff --git a/lib/sqlalchemy/attributes.py b/lib/sqlalchemy/attributes.py index 3aada1951a..8629d85a50 100644 --- a/lib/sqlalchemy/attributes.py +++ b/lib/sqlalchemy/attributes.py @@ -31,10 +31,13 @@ class InstrumentedAttribute(object): return self return self.get(obj) - def hasparent(self, item): - """returns True if the given item is attached to a parent object - via the attribute represented by this InstrumentedAttribute.""" - return item._state.get(('hasparent', id(self))) + def hasparent(self, item, optimistic=False): + """return True if the given item is attached to a parent object + via the attribute represented by this InstrumentedAttribute. + + optimistic indicates what we should return if the given item has no "hasparent" + record at all for the given attribute.""" + return item._state.get(('hasparent', id(self)), optimistic) def sethasparent(self, item, value): """sets a boolean flag on the given item corresponding to whether or not it is @@ -136,8 +139,12 @@ class InstrumentedAttribute(object): return InstrumentedAttribute.PASSIVE_NORESULT values = callable_() l = InstrumentedList(self, obj, self._adapt_list(values), init=False) - if self.trackparent and values is not None: - [self.sethasparent(v, True) for v in values if v is not None] + + # mark loaded instances with "hasparent" status. commented out + # because loaded objects use "optimistic" parent-checking + #if self.trackparent and values is not None: + # [self.sethasparent(v, True) for v in values if v is not None] + # if a callable was executed, then its part of the "committed state" # if any, so commit the newly loaded data orig = state.get('original', None) @@ -157,8 +164,12 @@ class InstrumentedAttribute(object): return InstrumentedAttribute.PASSIVE_NORESULT value = callable_() obj.__dict__[self.key] = value - if self.trackparent and value is not None: - self.sethasparent(value, True) + + # mark loaded instances with "hasparent" status. commented out + # because loaded objects use "optimistic" parent-checking + #if self.trackparent and value is not None: + # self.sethasparent(value, True) + # if a callable was executed, then its part of the "committed state" # if any, so commit the newly loaded data orig = state.get('original', None) diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 5fed121888..21d4c6e367 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -141,8 +141,9 @@ class Mapper(object): #self.compile() def _is_orphan(self, obj): + optimistic = hasattr(obj, '_instance_key') for (key,klass) in self.delete_orphans: - if not getattr(klass, key).hasparent(obj): + if not getattr(klass, key).hasparent(obj, optimistic=optimistic): return True else: return False diff --git a/test/base/attributes.py b/test/base/attributes.py index f281e487b5..831848cd99 100644 --- a/test/base/attributes.py +++ b/test/base/attributes.py @@ -186,10 +186,22 @@ class AttributesTest(PersistTest): assert p1.blog is b assert p1 in b.posts - # no orphans - assert getattr(Blog, 'posts').hasparent(p1) - assert getattr(Post, 'blog').hasparent(b) + # no orphans (but we are using optimistic checks) + assert getattr(Blog, 'posts').hasparent(p1, optimistic=True) + assert getattr(Post, 'blog').hasparent(b, optimistic=True) + # lazy loads currently not processed for "hasparent" status, so without + # optimistic, it returns false + assert not getattr(Blog, 'posts').hasparent(p1, optimistic=False) + assert not getattr(Post, 'blog').hasparent(b, optimistic=False) + + # ok what about non-optimistic. well, dont use lazy loaders, + # assign things manually, so the "hasparent" flags get set + b2 = Blog() + p2 = Post() + b2.posts.append(p2) + assert getattr(Blog, 'posts').hasparent(p2, optimistic=False) + assert getattr(Post, 'blog').hasparent(b2, optimistic=False) def testinheritance(self): """tests that attributes are polymorphic""" diff --git a/test/orm/mapper.py b/test/orm/mapper.py index 2704e8e7cc..e12e15139e 100644 --- a/test/orm/mapper.py +++ b/test/orm/mapper.py @@ -670,6 +670,18 @@ class LazyTest(MapperSuperTest): {'user_id' : 9, 'addresses' : (Address, [])}, ) + def testorphanstate(self): + """test that a lazily loaded child object is not marked as an orphan""" + m = mapper(User, users, properties={ + 'addresses':relation(Address, cascade="all,delete-orphan", lazy=True) + }) + mapper(Address, addresses) + + q = create_session().query(m) + user = q.get(7) + assert getattr(User, 'addresses').hasparent(user.addresses[0], optimistic=True) + assert not class_mapper(Address)._is_orphan(user.addresses[0]) + def testlimit(self): ordermapper = mapper(Order, orders, properties = dict( items = relation(mapper(Item, orderitems), lazy = True) @@ -881,6 +893,19 @@ class EagerTest(MapperSuperTest): }, ) + def testorphanstate(self): + """test that an eagerly loaded child object is not marked as an orphan""" + m = mapper(User, users, properties={ + 'addresses':relation(Address, cascade="all,delete-orphan", lazy=False) + }) + mapper(Address, addresses) + + s = create_session() + q = s.query(m) + user = q.get(7) + assert getattr(User, 'addresses').hasparent(user.addresses[0], optimistic=True) + assert not class_mapper(Address)._is_orphan(user.addresses[0]) + def testwithrepeat(self): """tests a one-to-many eager load where we also query on joined criterion, where the joined criterion is using the same tables that are used within the eager load. the mapper must insure that the