[Zope-dev] brain.getObject and traversal

[Florent Guillaume]

Chris Withers wrote:
> Just because that's what it did before doesn't mean we should leave it 
> like that. I can see absolutely no benefit in returning None over 
> raising a specific error.
> 
> Also, the original behaviour of getObject, prior to Casey's drastic and 
> unexpected switch to restrictedTraverse(path,None) was to raise 
> Unauthorized, NOT to return None.

You're mistaken. The old code did:

   def getObject(self, REQUEST=None):
         """Try to return the object for this record"""
         try:
             obj = self.aq_parent.unrestrictedTraverse(self.getPath())
             if not obj:
                 if REQUEST is None:
                     REQUEST = self.REQUEST
                 obj = self.aq_parent.resolve_url(self.getPath(), REQUEST)
             return obj
         except:
             zLOG.LOG('CatalogBrains', zLOG.INFO, 'getObject raised an 
error',
                      error=sys.exc_info())
             pass

Which effectively returns None.

> I first became aware of Casey's changes when I upgraded a production 
> Zope instance and started getting loads of random attribute errors where 
>  I'd happilly been dealing with the Unauthorized's before.

You probably had the unauthorized *after* the getObject, when it 
returned to you an object you weren't actually supposed to try to access.

> So, for me, returning None is just plain evil. All it serves to do is 
> mask an exception that's likely to be useful. If people are relying on 
> it returning None, then it's a one line change in their code.

All robust old code had to be able to test for None, because it was 
returned in many cases (when indexes got desynchronised, due to 
transaction bugs for instance, or manage_beforeDelete swallowing stuff, 
or conflict errors happening...). I know I had to add lots in my code.

> Now, the other problem I have with this fix is no tests were checked in 
> for this. We should have tests that verify this behaves as it should, 
> since those tests are likely to be the only formal documentation this 
> issue ever receives ;-)

Tests were checked in:
http://cvs.zope.org/Zope/lib/python/Products/ZCatalog/tests/Attic/testCatalog.py.diff?r1=1.22.12.6&r2=1.22.12.7

(but zope-checkins list had problems that day, I don't know why, and the 
checkin mail never appeared).

> Would anyone object if I wrote tests and changed the implementation to 
> raise exceptions, including Unauthorized, instead of returning None?

Unauthorized in getObject is out of the question, that would be new 
behaviour.

Florent

-- 
Florent Guillaume, Nuxeo (Paris, France)   CTO, Director of R&D
+33 1 40 33 71 59   http://nuxeo.com   fg at nuxeo.com