[Zope-Coders] Re: Confusing code in ObjectManager manage_FTPstat

[Jim Fulton]

Sidnei da Silva wrote:
> On Mon, Oct 31, 2005 at 10:05:45AM -0500, Jim Fulton wrote:
> | Sidnei da Silva wrote:
> | >Found some lovely piece of code deep into the FTP parts of Zope 2 last
> | >saturday, one of them is truely ugly. It's listing the contents of
> | >the current and parent folders for no apparent reason (or at least, it
> | >didn't make sense either to me or Chris McDonough).
> | 
> | There's a comment stating the intent.
> 
> Yes, but the original code does the check in a truely obscure way, at
> least to me. I've thought of spelling:
> 
>         # check to see if we are acquiring our objectValues or not
>         if not (len(REQUEST.PARENTS) > 1 and
>                 self.objectValues() == REQUEST.PARENTS[1].objectValues()):
> 
> As:
> 
>         if self.objectValues.im_self is not self:

That might be better.

> However I'm not sure that would be the be the correct
> replacement. There doesn't seem to exist any test for this.

Of course, there's no test for this.  We weren't doing automated
testing back then.  Feel free to write one.


> | >The code in question is in the ``manage_FTPstat`` method of
> | >``OFS.ObjectManager``. Tracing back the source of this code, it seems
> | >to have been (surprisingly) introduced by Jim Fulton, back in
> | >1999. [1]
> | >
> | >Later on, Amos Latteier changed part of the code (namely
> | >``manage_FTPlist``) to use a slightly more friendlier, yet equally
> | >confusing ``is_acquired`` method. [2]
> | >
> | >I'm now sitting here, trying to make sense of this code and wondering
> | >what was the original intention in the first place. Would anyone have
> | >a clue?
> | 
> | I think the comments make this pretyu clear.  The intent is to avoid
> | providing listings of acquired objects.  In fact, the intent of both
> | bits of code, I think is to prevent FTP access to acquired objects.
> 
> Wouldn't that be better implemented by making the same check as it's
> done for WebDAV and set 'maybe_webdav_client' (abusing the name)
> so that it sets the 'no_acquire_flag'?

Possibly.

> | I suspect that there is a clearer way to implement the intent.
> 
> I have three suggestions right now:
> 
>  1. change manage_FTPstat to use the same thing as manage_FTPlist
>     (using App.Common.is_acquired, and possibly rewriting is_acquired
>     to a simpler check)

That's an improvement,

> OR
> 
>  2. Throw away the `is_acquired` check and replace it by a much
>     simpler check

Sounds good, doing so consistently.

> OR
> 
>  3. Setting 'maybe_webdav_client' in the request object so the object
>     is never acquired for FTP, the same way as it's done for WebDAV.

Possibly.  I don't have time to look at that.

Jim


-- 
Jim Fulton           mailto:jim at zope.com       Python Powered!
CTO                  (540) 361-1714            http://www.python.org
Zope Corporation     http://www.zope.com       http://www.zope.org