Re: [Zope-dev] brain.getObject and traversal
Florent Guillaume wrote: Is everyone ok with returning - the object if it can be accessed - raise Unauthorized if it can't be accessed - raise NotFound if it's not there Please don't catch any exceptions and re-raise them in a different type, just let them pass through. I specifically don't think raising a normal NotFound when the catalog has a brain that doesn't map to an object is the right thing to do. I'd much prefer a BrainHasNoMatchingObject exception or some such which is nice and clear... cheers, Chris -- Simplistix - Content Management, Zope Python Consulting - http://www.simplistix.co.uk ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] brain.getObject and traversal
Florent Guillaume wrote: Well of course no, but I never had to check a getObject() against Unauthorized. Maybe it's because I only use it in a CMF setting where unaccessible objects are filtered anyway. Maybe, but CMF isn't the only use of Zope ;-) OTOH you're a bit excessive in your Whole array of possible distinct errors. Not really. I want to know the difference between: - unauthorized: my object has problems and probably needs its permissions fixing - not found: I want to know why an object is indexed that doesn't exist and fix that - other random error: damn, big problems with catalog, need to post to lists In all three cases, knowing the type of error and getting a sensible traceback in my logs is infinitely more useful than a random... AttributeError: None has no attribute x ...error at some point in the future, which may be no-where near the actual catalog search. cheers, Chris -- Simplistix - Content Management, Zope Python Consulting - http://www.simplistix.co.uk ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] brain.getObject and traversal
Florent Guillaume [EMAIL PROTECTED] wrote: Unauthorized in getObject is out of the question, that would be new behaviour. Well, in 2.8, new behaviour is expected, right? I really passionately believe that we should not be returnining None in Zope 2.8, and since 2.8 hasn't quite hit beta yet I'm very keen to see it fixed asap. Any objections? I'm ok for 2.8. I'll look at it. Is everyone ok with returning - the object if it can be accessed - raise Unauthorized if it can't be accessed - raise NotFound if it's not there and never return None ? I'll change that before tomorrow, for 2.8a2. (I'll change NotFound in Traversal.py to a real exception instead of a string too, I thought we'd killed those.) Florent -- Florent Guillaume, Nuxeo (Paris, France) CTO, Director of RD +33 1 40 33 71 59 http://nuxeo.com [EMAIL PROTECTED] ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] brain.getObject and traversal
Florent Guillaume wrote at 2005-4-1 13:21 +0200: Florent Guillaume [EMAIL PROTECTED] wrote: Unauthorized in getObject is out of the question, that would be new behaviour. Well, in 2.8, new behaviour is expected, right? I really passionately believe that we should not be returnining None in Zope 2.8, and since 2.8 hasn't quite hit beta yet I'm very keen to see it fixed asap. Any objections? I'm ok for 2.8. I'll look at it. Is everyone ok with returning - the object if it can be accessed - raise Unauthorized if it can't be accessed - raise NotFound if it's not there and never return None ? Remapping exceptions is a bad idea. Usually, valuable information is lost in the process. If you change the returning None, then you should let through whatever exception is caused. -- Dieter ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] brain.getObject and traversal
On Thu, 2005-03-31 at 13:02 +0100, Chris Withers wrote: 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. Well, in 2.8, new behaviour is expected, right? I really passionately believe that we should not be returnining None in Zope 2.8, and since 2.8 hasn't quite hit beta yet I'm very keen to see it fixed asap. Any objections? No, I agree that it should raise Unauthorized. Just make sure that this is communicated properly at release time. -- Roché Compaan Upfront Systems http://www.upfrontsystems.co.za ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] brain.getObject and traversal
Chris Withers wrote:
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.
Interesting. Where did that code come from?
It's been there for longer than I've being doing Zope.
http://cvs.zope.org/Zope/lib/python/Products/ZCatalog/Attic/CatalogBrains.py?hideattic=1content-type=text/vnd.viewcvs-markuprev=1.1.4.4
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.
Just because you had to do it in old code doesn't make it not evil.
Returning a meaningless value that masks a whole array of possible
distinct errors that should all be handled in different ways is mind
numbingly stupid IMNSHO. Why you feel so passionately that this should
be the case is beyond me. Do you like using bare try: except:s
throughout your code too?!
Well of course no, but I never had to check a getObject() against
Unauthorized. Maybe it's because I only use it in a CMF setting where
unaccessible objects are filtered anyway.
OTOH you're a bit excessive in your Whole array of possible distinct
errors.
Unauthorized in getObject is out of the question, that would be new
behaviour.
Well, in 2.8, new behaviour is expected, right?
I really passionately believe that we should not be returnining None in
Zope 2.8, and since 2.8 hasn't quite hit beta yet I'm very keen to see
it fixed asap.
Any objections?
I'm ok for 2.8. I'll look at it.
Florent
--
Florent Guillaume, Nuxeo (Paris, France) CTO, Director of RD
+33 1 40 33 71 59 http://nuxeo.com [EMAIL PROTECTED]
___
Zope-Dev maillist - Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
** No cross posts or HTML encoding! **
(Related lists -
http://mail.zope.org/mailman/listinfo/zope-announce
http://mail.zope.org/mailman/listinfo/zope )
[Zope-dev] brain.getObject and traversal
To try to clarify things even more: The implementation of getObject I checked in a few days ago has the following properties: 1. it checks permissions only on the last step of the traversal, 2. it returns None if for some reason the object cannot be retrieved. Now for the rationale: 1. is necessary in the presence of rights granted deeper in the hierarchy. There's no going around it. 2. is necessary for backward compatibility. *all* the previous implementations of getObject returned None in case of problems. The implementation of 1. looks slightly convoluted but is necessary because we want to leave the details of the traversal (involving __bobo_traverse__, getitem, and checking security with the proper 'accessed' and 'container') to (un)restrictedTraverse. Florent -- Florent Guillaume, Nuxeo (Paris, France) CTO, Director of RD +33 1 40 33 71 59 http://nuxeo.com [EMAIL PROTECTED] ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] brain.getObject and traversal
Florent Guillaume wrote: 2. is necessary for backward compatibility. *all* the previous implementations of getObject returned None in case of problems. This is the only bit I'm asking about, I accept that I'm in the insane minority on the other point ;-) 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. 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. 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. 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 ;-) Would anyone object if I wrote tests and changed the implementation to raise exceptions, including Unauthorized, instead of returning None? cheers, Chris -- Simplistix - Content Management, Zope Python Consulting - http://www.simplistix.co.uk ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] brain.getObject and traversal
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.6r2=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 RD
+33 1 40 33 71 59 http://nuxeo.com [EMAIL PROTECTED]
___
Zope-Dev maillist - Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
** No cross posts or HTML encoding! **
(Related lists -
http://mail.zope.org/mailman/listinfo/zope-announce
http://mail.zope.org/mailman/listinfo/zope )
