3.2.9-rc2 FieldStorage Problems (was Re: mod_python 3.2.9-rc2 available for testing)

[Jim Gallacher <jp...@jgassociates.ca>]

Max Bowsher wrote:
> Jim Gallacher wrote:
>> Max Bowsher wrote:
>>> Jim Gallacher wrote:
>>>> The mod_python 3.2.9-rc2 tarball is available for testing.
>>> Something about the mod_python.util changes has either exposed a bug in
>>> Trac, or introduced a bug into mod_python - I'm not sure which yet.
>>>
>>> 3.2.x r416547 with r393781 reverted works fine for me
>>> 3.2.x r416548 seems to be behaving as if any path info in the URL beyond
>>> the object designated as a mod_python handler is being truncated after
>>> the first slash - i.e.:
>>>
>>> If the configuration is:
>>>
>>> <Location /trac>
>>>   SetHandler mod_python
>>>   ...
>>> </Location>
>>>
>>> then an URL like:
>>>
>>> http://host/trac/foo/bar/baz
>>>
>>> gets treated as:
>>>
>>> http://host/trac/foo/
>>>
>>>
>>> I'll try to narrow down the source of the problem.
>> I installed trac and can reproduce the behaviour. The latest changes to
>> Field *should* work, so I'm thinking something is a little wonky in
>> FieldStorage. No time right now but I'll be able to dig into it later
>> today - if you haven't found the solution by then Max. ;)
> 
> OK, I've found the solution.
> 
> The root of the problem is that Trac wants to be able to add extra
> fields to a FieldStorage itself, and has been jumping through all sorts
> of crazy hoops in the internals of FieldStorage to make this happen.

Which suggests bad design in either Trac or FieldStorage. Personally I
think FieldStorage is an ugly piece of work so we'll let the blame fall
there for now. ;)

> The recent changes have moved all the hoops, and Trac is now failing
> utterly.
> 
> First problem, is that the new FieldStorage memoizes self.dictionary the
> first time it creates it. However, Trac expects to be able to append to
> FieldStorage.list to add new fields. Since the list member is documented
> in the public API docs, this isn't entirely unreasonable. In any case,
> list ought to have a leading underscore if it is supposed to be private.
> 
> OK, so suppose you comment out the line "self.dictionary = result", so
> that the dictionary gets rebuilt every time it is needed. Does it work? No!
> 
> The fun doesn't stop there. The second problem is related to the way
> that pre-3.2.9 mod_python likes to stuff everything into unnecessary
> StringIO objects. Trac tries to replicate this, and this causes problems
> since 3.2.9 no longer expects StringIO objects everywhere.  Trac is
> actually relying on pre-3.2.9 behaviour of transforming
> string-in-StringIO Fields into StringFields during
> FieldStorage.__getitem__, but this no longer happens in 3.2.9.
> 
> 
> Here is the patch that seems to make things work again:
> --- util-bad.py 2006-06-23 08:12:00.000000000 -0500
> +++ util.py     2006-06-23 09:51:32.000000000 -0500
> @@ -323,10 +323,17 @@
>     def __getitem__(self, key):
>         """Dictionary style indexing."""
>         found = self.dictionary[key]
> -       if len(found) == 1:
> -           return found[0]
> +       newfound = []
> +       for item in found:
> +         if isinstance(item.file, FileType) or \
> +             isinstance(getattr(item.file, 'file', None), FileType):
> +             newfound.append(item)
> +         else:
> +             newfound.append(StringField(item.value))
> +       if len(newfound) == 1:
> +           return newfound[0]
>         else:
> -           return found
> +           return newfound
> 
>     def get(self, key, default):
>         try:
> @@ -374,7 +381,6 @@
>         if list is None:
>             raise TypeError, "not indexable"
>         result = {}
> -       self.dictionary = result
>         for item in list:
>             if item.name in result:
>                result[item.name].append(item)
> 
> 
> 
> Yes, ugh.

Such meddling in FieldStorage makes me nervous. I'd rather revert all of
the changes corresponding to MODPYTHON-93 (Improve util.FieldStorage
efficiency) and proceed with a 3.2.9 release. The old code works even if
it is not the most elegant.

> So, I guess the two questions to answer now are:
> 
> * How much non-compatibility is acceptable in a patch release?

I'd say none, unless the non-compatibility is absolutely essential to
fix some critical bug. You should be able to just drop in a patch
release and carry on without any fuss. The problems we've created for
Trac here are unacceptable.

> * How are applications supposed to perform write operations on a
> FieldStorage, in 3.3 and the future?

Personally I never considered writing to FieldStorage. I always thought
of it as a read-only representation of a submitted form, but then that's
just my mental map.

The docs state "The FieldStorage class has a mapping object interface,
i.e. it can be treated like a dictionary". There is no mention that
FieldStorage is immutable, and I certainly think from the description
that most people would assume it could be used like a dictionary. Also
as Max points out there are no implied restrictions on the list
attribute, so an application should be free to modify it if desired.

Since a 3.3 release is at least a few months away, I think we can take
our time and give this some careful consideration. Maybe the best plan
is to leave FieldStorage as-is for legacy applications and start fresh
on a brand new FieldStorageNG class,  without worrying about backwards
compatibility?

Jim

Re: 3.2.9-rc2 FieldStorage Problems (was Re: mod_python 3.2.9-rc2 available for testing)

[Nicolas Lehuen <ni...@lehuen.com>]

> >> * How are applications supposed to perform write operations on a
> >> FieldStorage, in 3.3 and the future?
> >
> > Personally I never considered writing to FieldStorage. I always thought
> > of it as a read-only representation of a submitted form, but then that's
> > just my mental map.
>
> It's a pretty uncommon usage - it surprised me when I saw it in Trac.
> Trac is using it to bundle additional request information gathered from
> sources other than the form itself - it makes me suspect that someone in
> the past thought "hey, we have this dictionary-like thing we are passing
> around already - wouldn't it be nice if we could shove extra things in
> it, rather than passing around another object too?", and then proceeded
> to hack things so it was possible.

What I don't understand is why they don't put extra attributes in the
request object itself, since it supports that. This is done everywhere
in the publisher and the PSP handler. Fiddling with the FieldStorage
looks messy to me too - then again, if we promise a dict-like
interface we should make sure it is supported.

Regards,
Nicolas

Re: 3.2.9-rc2 FieldStorage Problems (was Re: mod_python 3.2.9-rc2 available for testing)

[Jim Gallacher <jp...@jgassociates.ca>]

Max Bowsher wrote:
> Jim Gallacher wrote

>> Since a 3.3 release is at least a few months away, I think we can take
>> our time and give this some careful consideration. Maybe the best plan
>> is to leave FieldStorage as-is for legacy applications and start fresh
>> on a brand new FieldStorageNG class,  without worrying about backwards
>> compatibility?
> 
> There are projects which allow compatibility breakage only on major
> (i.e. 3.x -> 4.x) version bumps [e.g. APR, Subversion] and those which
> allow it on minor (i.e. 3.2.x -> 3.3.x) version bumps [e.g. Apache
> HTTPD, Berkeley DB]. Into which category does mod_python place itself?

I don't think we have any formal mod_python policy. If I had to guess
I'd say allow breakage on minor version bumps, for 2 reasons:

1. We are an Apache httpd subproject, so that would be consistent with
our parent project.

2. API changes should be fairly rare and of a minor nature anyway and
requiring a major version bump seems like overkill. Major bumps should
be reserved for the really big events, such as 2.7 -> 3.0, where apache
1.3 support was dropped as mod_python transitioned to apache 2.0.

My personal feeling is that we should be very conservative about API
changes. Users should be able to depend on mod_python as a stable
platform on which to build their frameworks and applications, and not
have to worry about refactoring their code every 6 months because we've
run amok with our changes.

Jim

Re: 3.2.9-rc2 FieldStorage Problems (was Re: mod_python 3.2.9-rc2 available for testing)

[Max Bowsher <ma...@ukf.net>]

Jim Gallacher wrote:
> Max Bowsher wrote:
>> The root of the problem is that Trac wants to be able to add extra
>> fields to a FieldStorage itself, and has been jumping through all sorts
>> of crazy hoops in the internals of FieldStorage to make this happen.
> 
> Which suggests bad design in either Trac or FieldStorage. Personally I
> think FieldStorage is an ugly piece of work so we'll let the blame fall
> there for now. ;)
> 
...
> 
> Such meddling in FieldStorage makes me nervous. I'd rather revert all of
> the changes corresponding to MODPYTHON-93 (Improve util.FieldStorage
> efficiency) and proceed with a 3.2.9 release. The old code works even if
> it is not the most elegant.

Excellent - 3.2.9 is valuable for all sorts of other reasons, not least
apache 2.2 and bash 3.1 compatibility, and then non-trivial FieldStorage
tidying can happen in mod_python 3.3.

>> So, I guess the two questions to answer now are:
>>
>> * How much non-compatibility is acceptable in a patch release?
> 
> I'd say none, unless the non-compatibility is absolutely essential to
> fix some critical bug. You should be able to just drop in a patch
> release and carry on without any fuss. The problems we've created for
> Trac here are unacceptable.
> 
>> * How are applications supposed to perform write operations on a
>> FieldStorage, in 3.3 and the future?
> 
> Personally I never considered writing to FieldStorage. I always thought
> of it as a read-only representation of a submitted form, but then that's
> just my mental map.

It's a pretty uncommon usage - it surprised me when I saw it in Trac.
Trac is using it to bundle additional request information gathered from
sources other than the form itself - it makes me suspect that someone in
the past thought "hey, we have this dictionary-like thing we are passing
around already - wouldn't it be nice if we could shove extra things in
it, rather than passing around another object too?", and then proceeded
to hack things so it was possible.

> The docs state "The FieldStorage class has a mapping object interface,
> i.e. it can be treated like a dictionary". There is no mention that
> FieldStorage is immutable, and I certainly think from the description
> that most people would assume it could be used like a dictionary. Also
> as Max points out there are no implied restrictions on the list
> attribute, so an application should be free to modify it if desired.
> 
> Since a 3.3 release is at least a few months away, I think we can take
> our time and give this some careful consideration. Maybe the best plan
> is to leave FieldStorage as-is for legacy applications and start fresh
> on a brand new FieldStorageNG class,  without worrying about backwards
> compatibility?

There are projects which allow compatibility breakage only on major
(i.e. 3.x -> 4.x) version bumps [e.g. APR, Subversion] and those which
allow it on minor (i.e. 3.2.x -> 3.3.x) version bumps [e.g. Apache
HTTPD, Berkeley DB]. Into which category does mod_python place itself?

Max.