[Zope3-dev] Weird error within __cmp__ method of KeyReferenceToPersistent

[Gary Poster]

On Jul 28, 2006, at 5:05 AM, Dominik Huber wrote:

> Gary Poster wrote:
>> I object. :-)
> Thanks for the feedback!

:-) Thanks for the really clear discussion below.  I think I have a  
better idea about what is going on.

BTW, I'm trying to squeeze a reply in before I (might) become  
unavailable for a few days.  I should be back online no later than  
next Tuesday.

>> First, your change effectively adds _database_name to an unspoken  
>> interface for persistent IKeyReferences.  That's not a good idea.
> Yup, I did not respect that point.
>> Second, the error you are getting is an error condition and should  
>> be raised as one.  Maybe a more helpful error would be an  
>> improvement.  But your __cmp__ is somehow comparing against a  
>> persistent keyreference with an object that appears to have not  
>> yet been added to a connection (_p_jar)--something that should not  
>> have been able to happen with the default persistent adapter to  
>> keyreference.
> Yes, that was my opinion too. But the error happen anyhow...
>> Are you working with a custom keyreference adaptor anywhere?
> No
>> In any case, shuffling over this problem as your change does would  
>> likely cause incorrect sorting/indexing in the intids btree, which  
>> would be very bad.  This is an error, and should be dealt with as  
>> one.
> I tried this approach before. But how can I remove an incorrect  
> sorting/indexing respecting the dict api without invoking the  
> compare function?
>
> class IntIds(Persistent, Contained):
>   [...]
>    def register(self, ob):
>       [...]
>       try:
>            if key in self.ids: # <- causing the comparision error
>                 return self.ids[key]
>       except SpecialCmpError:
>                 # clean up incorrect sorting/indexing
>                 uid = self.ids[key] # <- causing the comparision  
> error too
>                 del self.refs[uid]
>                 del self.ids[key]
>       [...]

Well, first of all, I suspect your situation, based on what I've seen  
so far, looks something like this:

- code creates obj P1
- code puts P1 in intids, with connection.
- code does something incorrect (either your code or code that you  
are relying on), probably involving a savepoint because of your  
discussion below, that keeps keyreference to P1 in intids BTree, but  
rolls back addition of P1 to connection.  Right now we have the bad  
data (a key reference to an object without a connection in the  
BTree), but we don't know about it yet.
- Now code creates obj P2.
- code tries to put P2 in intids.  If the BTree logic happens to make  
it compare against the bad keyreference to P1, you get the __cmp__  
error.  This is a red herring for debugging: the problem has already  
occurred (see above).

I'm pretty sure your BTree is already bad by the time you get to the  
failing __cmp__.  The only way I know to fix it at this late stage in  
the story is drastic: you'll need to create a new BTree, iterate over  
the old one and add all the contents except for the broken  
keyreferences, and replace the old intid BTree with the new one.

I don't think that's appropriate error catching for the BTree code.   
I think if you get the error you describe, you have a serious problem  
in "your" code (i.e., code that you've written or code that you're  
relying on in your stack, like Zope).  How--and when--to handle it is  
application policy, not something that should be in the base intids,  
I think.

> The error occurs after a savepoint.rollback. I can reproduce the  
> error within the basic error story (-> see testcase). It happens if  
> we compare a commited persistent object with a rollbacked  
> persistent object.
>
> The current implementation hurts the demeter's law. A fresh  
> instance does not support the db attribute, even though the  
> situation showed within the basic error story
>
>    >>> from persistent import Persistent
>    >>> p = Persistent()
>    >>> p._p_jar.db().database_name
>    Traceback (most recent call last):
>    ...
>    AttributeError: 'NoneType' object has no attribute 'db'

I remember knowing Demeter's law. ;-)  I should look it up, but I've  
already spent more time than I should have this morning.  The above  
behavior is expected.  The connection.add(obj) code in the persistent  
keyreference code gives the obj a _p_jar.


> IMO the error should still be handled by KeyReferenceToPersistent  
> __cmp__ method.

This is too late.  Anything at this stage in the game is going to be  
a dangerous cover-up of an already serious problem.

HTH; I'll reply again next week if others haven't piped up.

Gary