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
