Re: Status of mod_python 3.3.
Rereading Trac code, the req.args in constructor is still an issue as
what
the code is actually doing is prevent util.FieldStorage processing
req.args
and for it to do it itself, only adding args where they aren't also
defined in
POST body.
The only way to provide a fiddle for this would be to use a special
derived
instance of the list object which redefines append and translates
arguments
as necessary on the fly so it would be compatible.
Graham
On 20/10/2006, at 10:08 AM, Graham Dumpleton wrote:
Jim Gallacher wrote ..
# Populate FieldStorage with the original query string
parameters, if
# they aren't already defined through the request body
if req.args:
qsargs = []
for pair in util.parse_qsl(req.args, 1):
if self.has_key(pair[0]):
continue
qsargs.append(util.Field(pair[0], StringIO(pair[1]),
"text/plain", {}, None,
{}))
self.list += qsargs
def get(self, key, default=None):
# Work around a quirk with the ModPython FieldStorage class.
# Instances of a string subclass are returned instead of
real
# strings, this confuses psycopg2 among others.
v = util.FieldStorage.get(self, key, default)
if isinstance(v, util.StringField):
return v.value
else:
return v
def __setitem__(self, key, value):
if value is not None and key not in self:
self.list.append(util.Field(key, StringIO(value),
'text/plain',
{}, None, {}))
Presuming I am correct, I interpret the problem being with
__setitem__().
This is because the req.args in constructor shouldn't matter as latest
mod_python FieldStorage adds them anyway so it shouldn't try and add
them again.
If I remember, someone said that Trac added extra key/values after the
FieldStorage instance is created, presumably by __setitem__ being
invoked.
Thus, how the badly constructed data gets in there.
Is this correct?
I know it is a real hack, but we could perhaps change the
constructor of
util.FieldStorage to include:
self.__setitem__ = self.add_field
Since the __setitem__ in the derived class has already been added
to the
instance at this point, this line in the util.FieldStorage class
effectively
overrides it and we can replace it with the method that actually does
what is required.
For a working test, try:
class X:
def __init__(self):
self._dict = {}
#self.__setitem__ = self.add_field
def add_field(self, key, value):
self._dict[key] = value
def __getitem__(self, key):
return self._dict[key]
class Y(X):
def __init__(self):
X.__init__(self)
def __setitem__(self, key, value):
self._dict[key] = (value, value)
y = Y()
y._dict[1] = 1
print y[1]
y[2] = 2
print y[2]
Run that and then uncomment the override and see how now uses the
base class add_field.
Okay, its a hack, but in practice would it be a workable solution?
Adding the ability to use [] instead of add_field() may be a desirable
feature for the default implementation anyway. Doing it this way we
fix Trac in the process presuming I am correct about req.args in the
constructor not being an issue.
Graham
Re: Status of mod_python 3.3.
Jim Gallacher wrote: Graham Dumpleton wrote: Jim Gallacher wrote .. Although there is no JIRA issue for it, I'd like to see us do a quick code cleanup. I see lots of complier warnings about unused variables and it would be nice to excise the offending bits of code. I figure we are more likely to spot real problems if the compiler is spewing less noise. If there are no objections I'll do this over the weekend. Hmmm, I thought I had gone through and got rid of just about all of them. The only ones which I didn't fix up were some of the magic Python callbacks for deleting objects or something. I couldn't change these as I don't think some appropriate typedef or something existed in older versions of Python we still support. You may well be correct. It's been a while since I sat and watched the compiler output and that may have been when I was doing some leak testing on 3.2.10. I did a quick check on r466129 and it looks pretty good. There are still a couple outstanding instances however. It looks like they are in code that you worked on, Graham. (Not really a surprise, since you've worked on almost all of the code. ;) ) I can't see any harm in deleting the offending bits, but perhaps you could review just to make sure lines are in fact superfluous, as opposed to a lurking bug. mod_python.c:2437: warning: unused variable 'srv_conf' mod_python.c:2677: warning: unused variable 'ppath' requestobject.c:535: warning: unused variable 'result' requestobject.c:554: warning: unused variable 'result' requestobject.c:658: warning: unused variable 'result' Jim
Re: Status of mod_python 3.3.
Jim Gallacher wrote ..
> # Populate FieldStorage with the original query string
> parameters, if
> # they aren't already defined through the request body
> if req.args:
> qsargs = []
> for pair in util.parse_qsl(req.args, 1):
> if self.has_key(pair[0]):
> continue
> qsargs.append(util.Field(pair[0], StringIO(pair[1]),
> "text/plain", {}, None, {}))
> self.list += qsargs
>
> def get(self, key, default=None):
> # Work around a quirk with the ModPython FieldStorage class.
> # Instances of a string subclass are returned instead of real
> # strings, this confuses psycopg2 among others.
> v = util.FieldStorage.get(self, key, default)
> if isinstance(v, util.StringField):
> return v.value
> else:
> return v
>
> def __setitem__(self, key, value):
> if value is not None and key not in self:
> self.list.append(util.Field(key, StringIO(value), 'text/plain',
> {}, None, {}))
Presuming I am correct, I interpret the problem being with __setitem__().
This is because the req.args in constructor shouldn't matter as latest
mod_python FieldStorage adds them anyway so it shouldn't try and add
them again.
If I remember, someone said that Trac added extra key/values after the
FieldStorage instance is created, presumably by __setitem__ being invoked.
Thus, how the badly constructed data gets in there.
Is this correct?
I know it is a real hack, but we could perhaps change the constructor of
util.FieldStorage to include:
self.__setitem__ = self.add_field
Since the __setitem__ in the derived class has already been added to the
instance at this point, this line in the util.FieldStorage class effectively
overrides it and we can replace it with the method that actually does
what is required.
For a working test, try:
class X:
def __init__(self):
self._dict = {}
#self.__setitem__ = self.add_field
def add_field(self, key, value):
self._dict[key] = value
def __getitem__(self, key):
return self._dict[key]
class Y(X):
def __init__(self):
X.__init__(self)
def __setitem__(self, key, value):
self._dict[key] = (value, value)
y = Y()
y._dict[1] = 1
print y[1]
y[2] = 2
print y[2]
Run that and then uncomment the override and see how now uses the
base class add_field.
Okay, its a hack, but in practice would it be a workable solution?
Adding the ability to use [] instead of add_field() may be a desirable
feature for the default implementation anyway. Doing it this way we
fix Trac in the process presuming I am correct about req.args in the
constructor not being an issue.
Graham
Re: Status of mod_python 3.3.
Graham Dumpleton wrote:
Jim Gallacher wrote ..
Graham Dumpleton wrote:
On JIRA, the following issues are still marked as incomplete for mod_python
version 3.3. I have noted my own comments about where they are up to
and
what I think still needs to be done.
MODPYTHON-93 Improve util.FieldStorage efficiency.
This was actually marked as resolved but reopened because it was discovered
that changes meant that Trac <=0.9.6 would no longer work. The changes
were
also backed out of mod_python 3.2.X branch and not released in 3.2.10.
At this point I believe we have agreed that code in 3.3 would be left
as is and
people would need to use Trac >=0.10, which has now been release, with
mod_python 3.3 or later.
I know we've hashed this over a couple of times, but creating this
dependency still makes me nervous.
I'll see if I can get a chance to see what the issue is and what it would take
to still support Trac then.
As I recall, Trac 0.9.6 wraps FieldStorage and wants to manipulate the
the internal list. We changed the behaviour of that list, so now it fails.
The relevant bit of code is in trac/web/modpython_frontend.py in their
FieldStorageWrapper class.
class FieldStorageWrapper(util.FieldStorage):
"""
mod_python FieldStorage wrapper that improves compatibility with
the other
front-ends.
"""
def __init__(self, req):
"""
The mod_python FieldStorage implementation, unlike cgi.py, always
includes GET parameters, even if they are also defined in the
body of
a POST request. We work around this to provide the behaviour of
cgi.py
here.
"""
class RequestWrapper(object):
def __init__(self, req):
self.req = req
self.args = ''
def __getattr__(self, name):
return getattr(self.req, name)
util.FieldStorage.__init__(self, RequestWrapper(req),
keep_blank_values=1)
# Populate FieldStorage with the original query string
parameters, if
# they aren't already defined through the request body
if req.args:
qsargs = []
for pair in util.parse_qsl(req.args, 1):
if self.has_key(pair[0]):
continue
qsargs.append(util.Field(pair[0], StringIO(pair[1]),
"text/plain", {}, None, {}))
self.list += qsargs
def get(self, key, default=None):
# Work around a quirk with the ModPython FieldStorage class.
# Instances of a string subclass are returned instead of real
# strings, this confuses psycopg2 among others.
v = util.FieldStorage.get(self, key, default)
if isinstance(v, util.StringField):
return v.value
else:
return v
def __setitem__(self, key, value):
if value is not None and key not in self:
self.list.append(util.Field(key, StringIO(value), 'text/plain',
{}, None, {}))
There are a couple of things that can be cleaned up and marked as closed.
Add get_session() method to request object.
http://issues.apache.org/jira/browse/MODPYTHON-59
This idea was pretty much shot down, but there is still a bit of
residual code that should be cleaned up. I'll do that and mark it as
closed. I think the idea still has merit, but it would be better to
start from scratch at some future date.
Agree, code should be taken out and issue revisited at a later time if
warranted.
Although there is no JIRA issue for it, I'd like to see us do a quick
code cleanup. I see lots of complier warnings about unused variables and
it would be nice to excise the offending bits of code. I figure we are
more likely to spot real problems if the compiler is spewing less noise.
If there are no objections I'll do this over the weekend.
Hmmm, I thought I had gone through and got rid of just about all of them.
The only ones which I didn't fix up were some of the magic Python callbacks
for deleting objects or something. I couldn't change these as I don't think
some appropriate typedef or something existed in older versions of Python
we still support.
You may well be correct. It's been a while since I sat and watched the
compiler output and that may have been when I was doing some leak
testing on 3.2.10.
Overall I'd say we are in pretty good shape - thanks mainly to Graham's
hard work. If we defer the Python 2.5 / 64-bit support though I think we
should aim for a quick turnaround on a mod_python 3.4 release. Maybe a
January or February time frame?
Jim
Re: Status of mod_python 3.3.
Graham Dumpleton wrote: ... MODPYTHON-93 Improve util.FieldStorage efficiency. This was actually marked as resolved but reopened because it was discovered that changes meant that Trac <=0.9.6 would no longer work. The changes were also backed out of mod_python 3.2.X branch and not released in 3.2.10. At this point I believe we have agreed that code in 3.3 would be left as is and people would need to use Trac >=0.10, which has now been release, with mod_python 3.3 or later. There was comments as to whether util.FieldStorage needs to have more dictionary like access, but at this point I believe we should mark this issue as resolved and if people want dictionary like access, they can open a separate JIRA issue for that and we deal with it in a future release. I give it a +1 to mark it resolved. There's more than enough good stuff in the 93 fix (such as being able to upload large amounts of data to file-like objects) to justify breaking just one hack (which is easy to fix on the Trac side anyway). It would be logical to upgrade both the mod_python and trac releases on the machine simultaneously, so I doubt there will be any real hits in the field. Mike.
Re: Status of mod_python 3.3.
Graham Dumpleton wrote: On JIRA, the following issues are still marked as incomplete for mod_python version 3.3. I have noted my own comments about where they are up to and what I think still needs to be done. MODPYTHON-93 Improve util.FieldStorage efficiency. This was actually marked as resolved but reopened because it was discovered that changes meant that Trac <=0.9.6 would no longer work. The changes were also backed out of mod_python 3.2.X branch and not released in 3.2.10. At this point I believe we have agreed that code in 3.3 would be left as is and people would need to use Trac >=0.10, which has now been release, with mod_python 3.3 or later. I know we've hashed this over a couple of times, but creating this dependency still makes me nervous. There was comments as to whether util.FieldStorage needs to have more dictionary like access, but at this point I believe we should mark this issue as resolved and if people want dictionary like access, they can open a separate JIRA issue for that and we deal with it in a future release. In summary, I believe we should mark this as resolved. MODPYTHON-104 Allow Python code callouts with mod_include (SSI). The code for all this has been done for some time. The only reason it hasn't been marked as resolved as no documentation has been added into core mod_python documentation. I have separately written a article on the new feature which is available at: http://www.dscpl.com.au/wiki/ModPython/Articles/BasicsOfServerSideIncludes I have no problem as this being used as basis for core documentation. I had been holding off integrating it because of contention over whether we could use wiki for documentation or not. In summary, need to still keep this open until some documentation added to core mod_python documentation. MODPYTHON-127 Use namespace for mod_python PythonOption settings. I have made code changes but not committed them back to repository. Jim has committed some documentation changes related to it already though. Some more documentation changes are probably required where options are mentioned in relation to features they affect. I had posed question about whether mod_python.session.database_directory should also be added as a general fallback in cases where which type of filesystem based session was not going to be known. Jim responded with +1, but his explicit vs implicit comment made me unsure which proposal he was agreeing with. Thus nothing done about that yet. Sorry about that. :) I 100% agree with your proposal. By explicit I meant using your proposed scheme makes it more obvious to the user what they are configuring. So yes, we should include the additional PythonOption settings. In summary, bit more work to do. MODPYTHON-143 Implement and integrate a new module importer. Code has been done, except for extra bit more logging of exceptions for when modules hooks are called and a problem occurs. Also need to update the documentation. In summary, more work to do but mainly documentation. MODPYTHON-186 Build process not using correct values from Python config Makefile. This is only known to be an issue on Mac OS X when the very latest compiler tool chain software given out at Mac developer conference is used. It may only affect new Intel Macs. Someone did suggest they would come back with required changes but that hasn't happened. Personally I would say we don't attempt to address this in 3.3 and defer it till later. And I'm Mac-less, so I can't help there anyway. MODPYTHON-190 Python 2.5 support. From what I have seen, people are already using Python 2.5, thus is there any urgency on this? All I can figure is that by not making changes you will not be able to work with really large data, or will it all crash badly on a 64 bit platform with 64 bit support compiled in. I don't know the answers to this and no one else (doesn't even have to be one of the committers) has stepped up to do the work and work out what the required changes are. Personally I would say we don't attempt to address this in 3.3 and defer it till later. I've actually being going through the code in the last couple of days and was about to ask some questions about this. So I've added comments to code sections identified by ssizecheck.py. Should I check in the code with these comments so we can keep track, or just attach a patch to the JIRA issue? Personally, I would like to see this fixed for 3.3. The biggest problem I have is the lack of a 64-bit machine to test on. On the other hand, I don't have a 64-bit machine, so it doesn't affect me personally if we don't deal with it. At the very least we should make sure we include 32-bit python 2.5 in our testing round. MODPYTHON-193 Add req.hlist.location to mirror req.hlist.directory. I have done most of the code for this and now just sorting out some problems with trailing slashes getting added when they shouldn't. Also need to still update Session code and ensure None is retu
