quetz-mod_python-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Gallacher <...@jgassociates.ca>
Subject 3.2.9-rc2 FieldStorage Problems (was Re: mod_python 3.2.9-rc2 available for testing)
Date Fri, 23 Jun 2006 17:09:24 GMT
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

Mime
View raw message