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 R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
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
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/testCatalo... (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@nuxeo.com
Florent Guillaume 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?
You probably had the unauthorized *after* the getObject, when it returned to you an object you weren't actually supposed to try to access.
possible, but at least I had an unauthorized generated, rather than just getting a useless None.
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.
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?!
(but zope-checkins list had problems that day, I don't know why, and the checkin mail never appeared).
Apologies, that'd explain it.
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? Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
Chris Withers wrote:
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?
Most of my queries, where I need to wake up objects, has the form:: brains = catalog(portal_type='Document') objects = [] for brain in brains: try: obj = brain.getObject() if not obj is None: objects.append(obj) except: pass So it will not break any of my code, and I guess that most do something similar. Another thing is that I think that this pattern is so common that the catalog should have a query method where brains are automatically converted to objects. Like: objects = catalog.getObjects(portal_type='Document') That would be a handy time saver. -- hilsen/regards Max M, Denmark http://www.mxm.dk/ IT's Mad Science
Max M wrote at 2005-3-31 14:48 +0200:
... Most of my queries, where I need to wake up objects, has the form::
brains = catalog(portal_type='Document') objects = [] for brain in brains: try: obj = brain.getObject() if not obj is None: objects.append(obj) except: pass
So it will not break any of my code, and I guess that most do something similar.
Hopefully not: "try: ... except: ..." is a powerful receipe to hide problems and to get low quality code. They should be used *very* rarely... -- Dieter
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
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...
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 R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
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
Florent Guillaume <fg@nuxeo.com> 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 R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Florent Guillaume wrote:
Florent Guillaume <fg@nuxeo.com> 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.)
We really need to follow a deprecation-style model here: the risk of breaking major third party components is pretty high. Could we use a module-scope global, settable from zope.conf, to indicate which strategy to use? It should (for 2.8) default to raising, but we need to be prepared for an onslaught of breakage reports. The CHANGELOG should highlight the change, and include the zope.conf snippet required to restore the old behavior. We could add a deprecation warning (if that entry is activated), that the old-style option would be removed in 2.10. Tres. - -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCTWhlGqWXf00rNCgRAsZ8AKChKf3YvASZ1jmJGeeN4Y3PN9/0rACbBRgi nGNOyVocQywRINr8FnyNTHg= =cUAs -----END PGP SIGNATURE-----
Tres Seaver <tseaver@zope.com> 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 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.)
We really need to follow a deprecation-style model here: the risk of breaking major third party components is pretty high.
Could we use a module-scope global, settable from zope.conf, to indicate which strategy to use? It should (for 2.8) default to raising, but we need to be prepared for an onslaught of breakage reports.
The CHANGELOG should highlight the change, and include the zope.conf snippet required to restore the old behavior. We could add a deprecation warning (if that entry is activated), that the old-style option would be removed in 2.10.
Ok, thanks a lot to Tres for having gone ahead and done that. I just merged his branch. All 5645 tests pass (man, with Zope 3 included that's way more than before!) Florent -- Florent Guillaume, Nuxeo (Paris, France) CTO, Director of R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
Florent Guillaume wrote:
Ok, thanks a lot to Tres for having gone ahead and done that. I just merged his branch. All 5645 tests pass (man, with Zope 3 included that's way more than before!)
Did you check with Tres that his branc hwas ready to merge? ;-) cheers, Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
Chris Withers wrote:
Florent Guillaume wrote:
Ok, thanks a lot to Tres for having gone ahead and done that. I just merged his branch. All 5645 tests pass (man, with Zope 3 included that's way more than before!)
Did you check with Tres that his branc hwas ready to merge? ;-)
Yes. Florent -- Florent Guillaume, Nuxeo (Paris, France) CTO, Director of R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
Hi Tres,
We really need to follow a deprecation-style model here: the risk of breaking major third party components is pretty high.
Agreed. I see you started working on this, thanks! Since this is a bug, and it looks like it's going to be fixed with a config option, would anyone mind if I ported this code to the 2.7 branch with the option set to do whatever 2.7.5 does?
The CHANGELOG should highlight the change, and include the zope.conf snippet required to restore the old behavior. We could add a deprecation warning (if that entry is activated), that the old-style option would be removed in 2.10.
Sounds good to me. cheers, Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Chris Withers wrote:
We really need to follow a deprecation-style model here: the risk of breaking major third party components is pretty high.
Agreed. I see you started working on this, thanks!
Since this is a bug, and it looks like it's going to be fixed with a config option, would anyone mind if I ported this code to the 2.7 branch with the option set to do whatever 2.7.5 does?
- -0. This change is not a bugfix -- this is a new feature, which changes the documented behavior of the catalog brains. It is really up to Andreas whether or not to accept such a change on the 2.7 line.
The CHANGELOG should highlight the change, and include the zope.conf snippet required to restore the old behavior. We could add a deprecation warning (if that entry is activated), that the old-style option would be removed in 2.10.
Tres. - -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.5 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCUU1UGqWXf00rNCgRAlG7AJ4w88icN4H4pw7/ZtDSV22RlR41OACgoU9R Ia2qEpT7DHGRKY7VbwYwxrk= =NqFr -----END PGP SIGNATURE-----
Tres Seaver wrote:
Since this is a bug, and it looks like it's going to be fixed with a config option, would anyone mind if I ported this code to the 2.7 branch with the option set to do whatever 2.7.5 does?
- -0. This change is not a bugfix
If removing bare try: excepts: is a bugfix, then this is too, can't remember when and how the big try-fix-geddon was done though...
-- this is a new feature, which changes the documented behavior of the catalog brains.
Which documentation are you referring to? I want to make sure it gets updated too...
It is really up to Andreas whether or not to accept such a change on the 2.7 line.
Andreas, whatcha think? cheers, Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
--On Mittwoch, 6. April 2005 10:16 Uhr +0100 Chris Withers <chris@simplistix.co.uk> wrote:
It is really up to Andreas whether or not to accept such a change on the 2.7 line.
Andreas, whatcha think?
Sorry, I have to catch up with this thread. I thought the problem was solved by Tres new configuration option. -aj
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Andreas Jung wrote:
--On Mittwoch, 6. April 2005 10:16 Uhr +0100 Chris Withers <chris@simplistix.co.uk> wrote:
It is really up to Andreas whether or not to accept such a change on the 2.7 line.
Andreas, whatcha think?
Sorry, I have to catch up with this thread. I thought the problem was solved by Tres new configuration option.
Chris wants to backport it to 2.7 x; I'm opposed. Your call. Tres. - -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.5 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCU+T4GqWXf00rNCgRAmunAJ9KhHjABM6gcjsdFpfjz3OfQISUVACdFLQy J9CiMRtEQu0CV4CRJxVEX+c= =vinQ -----END PGP SIGNATURE-----
--On Mittwoch, 6. April 2005 9:32 Uhr -0400 Tres Seaver <tseaver@zope.com> wrote:
Chris wants to backport it to 2.7 x; I'm opposed. Your call.
If it does not change the default behaviour we have in 2.7.5... why not... Andreas
Andreas Jung wrote:
Chris wants to backport it to 2.7 x; I'm opposed. Your call.
If it does not change the default behaviour we have in 2.7.5... why not...
Cool, thanks, I'll look at merging for 2.7.6 :-) cheers, Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
Florent Guillaume wrote at 2005-4-1 13:21 +0200:
Florent Guillaume <fg@nuxeo.com> 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
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
participants (7)
-
Andreas Jung -
Chris Withers -
Dieter Maurer -
Florent Guillaume -
Max M -
Roché Compaan -
Tres Seaver