[Zope-CMF] Re: Bad interaction between CMF 1.4 and Zope 2.8 (catalog-getObject-raises)

[Chris Withers]

Florent Guillaume wrote:
> <Sigh> Please read and understand the code more carefully.
> 
> There is no expected error. reindexObjectSecurity (which I introduced
> more than 2 years ago because CMF was buggy without it) needs access to
> all subobjects to reindex them. To do that it has to:
> 
> 1. use an unrestricted searchResults, which was introduced with the name
>    unrestrictedSearchResults 8 months ago in the types tool, and used in
>    reindexObjectSecurity 3 weeks ago,
> 
> 2. use an unrestricted equivalent of getObject. Before, getObject
>    returned the object without security checks. Now, it does security
>    checks. So we can't use it. And there's no unrestrictedGetObject.
> 
> That's why we have to do the equivalent using unrestrictedTraverse and
> brain.getPath()

Okay, all this I agree with.

> The None issue is a red herring, it's just that we *don't* want to fail
> even if there are broken indexes. No we don't. We're reindexing what we
> can find in the catalog. We don't want an exception.

Yes we do! I really really really want to know if:
- I have a catalog entry that points to an object that no longer exists. 
This can ONLY happen due to a bug somewhere and needs to be fixed
- I have index corruption or other weirdness
- any of the above are happening, and be able to descriminate between them.

I honestly can thing of no sane justification for putting that None 
there. It's tantamount to a bare try except, and without even any logging.

PLEASE provide some sane justification for this, 'cos I'm obviously 
missing something since you're a bright guy and can't seriously be 
advocating sweeping unspecified bugs under the carpet...

> I don't want the users to find that out when they change the local roles
> somewhere.

When DO you want them to find out?

> If you want to audit your catalog, just write a recursive
> method that checks the objects. 

Why should anyone have to do an explicit audit just to find errors that 
are being reported but hidden by day to day use of the catalog?

What you're saying is like "yes, I want to catch and ignore conflict 
errors when a user saves a document, 'cos you should go and check for 
resulting data inconsistencies in an offline process"...

> Don't do it on the fly and have it fail
> on your users when ignoring broken object would have no actual impact on
> the functionality of the code.

It has impact on the funcionality of the system as a whole and gets 
worse the longer you leave it...

>>Now, this particular case seems to back up my point even more. It looks 
>>like the actual problem here is crufty code in 
>>Products.CMFDefault.DiscussionItem which has been happilly masked up 
>>until now.
> 
> This is unrelated. Don't mix theses issues, I fixed a real bug that may
> or may not have been visible because of DiscussionItem.

How so? Sidnei would never have seen the traceback if you had swallowed 
the problem by replacing getObject with unrestrictedTraverse(path,None), 
he would have had something else weird happen later down the line and 
have to try and track it back to this action going wrong...

Really, what am I missing here?

cheers,

Chris

-- 
Simplistix - Content Management, Zope & Python Consulting
            - http://www.simplistix.co.uk