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

[Dominik Huber]

Gary Poster wrote:
> I object. :-)
Thanks for the feedback!
> 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]
       [...]

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'

IMO the error should still be handled by KeyReferenceToPersistent 
__cmp__ method. What do you think about the following implementation 
respecting your first argument (unspoken interface extension):

class KeyReferenceToPersistent(object):
    [...]
    def __cmp__(self, other):
        if self.key_type_id == other.key_type_id:
            try:
                return cmp(
                    (self.object._p_jar.db().database_name,  
self.object._p_oid),
                    (other.object._p_jar.db().database_name, 
other.object._p_oid),
                    )
            except AttributeError:
                # a transaction might be rollbacked
                self_db = getattr(self.object._p_jar, 'db', '')
                if self_db:
                    self_db = self_db().database_name
                other_db = getattr(other.object._p_jar, 'db', '')
                if other_db:
                    other_db = other_db().database_name

                return cmp(
                    (self_db,  self.object._p_oid),
                    (other_db, other.object._p_oid),
                    )

Gary, thank you very much for your support!

Regards,
Dominik


======================================
KeyReferenceToPersistent __cmp__ error
======================================

    >>> from ZODB.tests.util import DB
    >>> from zope import component
    >>> from zope.app.component.hooks import setSite
    >>> from zope.app.folder import Folder, rootFolder
    >>> from zope.app.testing import setup
    >>> import sys
    >>> import traceback
    >>> import transaction

Basic Error Story
-----------------

We declare an persistent example class:

    >>> from persistent import Persistent
    >>> from zope.app.container.contained import Contained
    >>> class P(Contained, Persistent):
    ...    pass

    >>> p1 = P()

The instance does not have an associatet db yet that would be called by the
__cmp__ function:

    >>> p1._p_jar.db().database_name
    Traceback (most recent call last):
    ...
    AttributeError: 'NoneType' object has no attribute 'db'

This is not a reail problem because the KeyReferenceToPersistent asserts 
that the
db can be evaluated. Otherwise it raises the NotYet error:

    >>> from zope.app.keyreference.persistent import 
KeyReferenceToPersistent

    >>> keyref1 = KeyReferenceToPersistent(p1)
    Traceback (most recent call last):
    ...
    NotYet: <example.P object at ...>

For the further exploration we have to do a placeful setup:

    >>> setup.placefulSetUp()
    >>> db = DB()
    >>> conn = db.open()
    >>> root = conn.root()

    >>> root['top'] = top = rootFolder()
    >>> top.__name__ = 'top'
    >>> sm = setup.createSiteManager(top, True)
    >>> transaction.commit()
    >>> setSite(top)

An register the relevant adapters to IConnection and IKeyReference:

    >>> from ZODB.interfaces import IConnection
    >>> from persistent.interfaces import IPersistent
    >>> from zope.app.keyreference.interfaces import IKeyReference
    >>> from zope.app.keyreference.persistent import connectionOfPersistent

    >>> component.provideAdapter(KeyReferenceToPersistent, 
adapts=(IPersistent,), provides=IKeyReference)
    >>> component.provideAdapter(connectionOfPersistent, 
adapts=(IPersistent,), provides=IConnection)

Now we can drive on. After the __parent__ got set a persistent object can be
adapted to IKeyReference using the underlying connection:

    >>> p1.__parent__ = top

    >>> keyref1 = KeyReferenceToPersistent(p1)

We commit the transaction and create a savepoint for the further experiment:

    >>> transaction.commit()
    >>> savepoint = transaction.savepoint()

If we create an other P instance, we are able to compare p1 and p2:

    >>> p2 = P()
    >>> p2.__parent__ = top
    >>> keyref2 = KeyReferenceToPersistent(p2)
    >>> cmp(keyref1, keyref2)
    -1

But if we rollback to the savepoint the comparison will crash:

    >>> savepoint.rollback()
    >>> cmp(keyref1, keyref2)
    Traceback (most recent call last):
    ...
    AttributeError: 'NoneType' object has no attribute 'db'

Error Story using IntIds utility (That does not reproduce the error)
--------------------------------------------------------------------

Add intid utility and its subscriber:

    >>> from zope.app.container.interfaces import IObjectAddedEvent
    >>> from zope.app.container.interfaces import IObjectRemovedEvent
    >>> from zope.app.intid import IntIds
    >>> from zope.app.intid import addIntIdSubscriber
    >>> from zope.app.intid import removeIntIdSubscriber
    >>> from zope.app.intid.interfaces import IIntIdAddedEvent
    >>> from zope.app.intid.interfaces import IIntIdRemovedEvent
    >>> from zope.app.intid.interfaces import IIntIds
    >>> from zope.location.interfaces import ILocation

    >>> component.provideHandler(removeIntIdSubscriber, (ILocation, 
IObjectRemovedEvent))
    >>> component.provideHandler(addIntIdSubscriber, (ILocation, 
IObjectAddedEvent))

    >>> top['intids'] = intids = IntIds()
    >>> sm.registerUtility(intids, IIntIds, '')
    >>> transaction.commit()

Provide a notification mechanism using a simple subscription handler:

    >>> def notifyRegistration(e):
    ...    print "Registered '%s'." % e.object.__name__

    >>> def notifyUnRegistration(e):
    ...    print "Unregistered '%s'." % e.object.__name__

    >>> component.provideHandler(notifyRegistration, (IIntIdAddedEvent,))
    >>> component.provideHandler(notifyUnRegistration, 
(IIntIdRemovedEvent,))

We add our p1 to the top folder:

    >>> top['p1'] = p1
    Registered 'p1'.

We commit the transaction and create a savepoint for the further experiment:

    >>> transaction.commit()
    >>> savepoint = transaction.savepoint()

We create p2 instance of P:

    >>> p2 = P()
    >>> top['p2'] = p2
    Registered 'p2'.

In our real application an error occurs within a initialisation-handler 
which is
listen to ObjectAddedEvent. Therefore we have to rollback the transaction to
the savepoint:

    >>> savepoint.rollback()

Now we try to add a further p. Within this test it works, ...:

    >>> p3 = P()
    >>> top['p3'] = p3
    Registered 'p3'.

... but in our application we get the same error as in the Basic Error Story

After the addition of p2 was failed the following failes too, because of the
__cmp__ error. The incorrect sorting/indexing is not persistent if the
application is restarted no further error occurs. The __cmp__ error appears
only once after an unsuccessfull import. Therefore the __cmp__ error causes
something like a chain reaction. That means if we can ommit the __cmp__
error once and commit an import correctly and *no* incorrect 
sorting/indexing
are stored within the intids utitility.

    >>> setup.placefulTearDown()