Hi Dieter, *,
Dieter Maurer wrote:
> The "FieldStorage" must be rebuilt, because it can contain
> file objects. These file objects must be reset as they may
> have been (partially) read in the previous request.
> This prevent reusing the previous "FieldStorage".
As you may have seen in my first patch proposal I actually tried to
address this problem, although halfbaken and buggy. So here's the
next trial. I hope I've got it straight now.
Why didn't I do something to reset the files, FiledStorage creates?
Because I thought it wouldn't be needed. If you look at
cgi.FieldStorage you will see, that accessing FieldStorage's 'value'
field, will always reset the file before reading it and HTTPRequest's
'BODY' field does the same.
Cause of your hint I looked again and realised that I was wrong.
HTTPRquest's 'BODYFILE' field fails to reset the file before accessing
it (as in OFS/Image.py the File class does). That could be easily
fixed. What's worse is that FiledStorage can possibly create
subinstances of itsellf which can possibly contain other fileobjects.
This is the case for multipart-type requests.
How to solve this?
I can see three alternative aproaches:
1) In HTTPRequest.retry loop through self._fs.list (if it exists) and
call seek(0) on every fileobject, you can find there. This can
only work if the maximum depth of the FiledStorage tree is
predictable, which in turn depends on the possible input. I don't
know enough about the possible HTTP requests to determine such a
limit or to even 'prove' its existance.
2) In HTTPRequest.retry recursively walk through the FieldStorage
trees in self._fs.list (if it exists). I wouldn't do that in
Python without being able to predict the (worst case) number of
function calls needed to do this recursion. And without refreshing
my knowledge of compiler techniques I wouldn't be able to roll the
recursion out to a loop.
3) This is a somewhat drastic (and possibly conceived as hackish?)
approach which OTOH is independent of the input and should perform
quite well. I overwrite cgi.FieldStorage, esp. its make_file
method, such that it adds all fileobjects, that it creates, to a
(possibly caller provided) list. Thus the caller
(i.e. HTTPRequest) has a list of all those fileobjects that need to
be reset if the FieldStorage should be reused (as in ZPublisher's
retries).
For the patch attached I chose the third approach. Please let me
know, what you think about it. If it is assessed to be ok I will
update the collctor item, I opened yesterday.
cheers,
andreas
--- lib.orig/python/ZPublisher/HTTPRequest.py 2004-08-18 16:37:18.000000000 +0200
+++ lib/python/ZPublisher/HTTPRequest.py 2004-08-19 17:42:31.000000000 +0200
@@ -60,6 +60,37 @@
class NestedLoopExit( Exception ):
pass
+FILELIST_ENV_ENTRY = 'request.filestorage.filelist'
+
+class ZFieldStorage(cgi.FieldStorage):
+ """\
+ Tame the FieldStorage.
+
+ This class overrides the stock implementation to add all file
+ objects, generated while parsing a request, to a list. This list
+ is given by the caller.
+
+ Unfortunately cgi.FieldStorage is not generic enough, to adapt the
+ list of constructor parameters, by just deriving from it. Thus
+ the list is specified within the environ dict, given to the
+ constructor. This is ugly and could be fixed by a simple patch
+ against cgi.FieldStorage.
+ """
+
+ def __init__(self, fp=None, headers=None, outerboundary="",
+ environ=os.environ, keep_blank_values=0, strict_parsing=0):
+ self._filelist = environ.get(FILELIST_ENV_ENTRY, [])
+ cgi.FieldStorage.__init__(self, fp, headers, outerboundary,
+ environ, keep_blank_values,
+ strict_parsing)
+
+ def make_file(self, binary=None):
+ import tempfile
+ res = tempfile.TemporaryFile("w+b")
+ self._filelist.append(res)
+ return res
+
+
class HTTPRequest(BaseRequest):
"""\
Model HTTP request data.
@@ -125,10 +156,15 @@
def retry(self):
self.retry_count=self.retry_count+1
- self.stdin.seek(0)
+ if not self._fs:
+ raise ValueError, "Retrying on partially initialized request is not supported. Call processInputs()."
+ for f in self._fs_filelist:
+ f.seek(0)
r=self.__class__(stdin=self.stdin,
environ=self._orig_env,
- response=self.response.retry()
+ response=self.response.retry(),
+ fs = self._fs,
+ flist = self._fs_filelist
)
r.retry_count=self.retry_count
return r
@@ -138,6 +174,8 @@
# removing tempfiles.
self.stdin = None
self._file = None
+ self._fs = None
+ self._fs_filelist = None
self.form.clear()
# we want to clear the lazy dict here because BaseRequests don't have
# one. Without this, there's the possibility of memory leaking
@@ -237,7 +275,7 @@
"""
return self._client_addr
- def __init__(self, stdin, environ, response, clean=0):
+ def __init__(self, stdin, environ, response, clean=0, fs=None, flist=None):
self._orig_env=environ
# Avoid the overhead of scrubbing the environment in the
# case of request cloning for traversal purposes. If the
@@ -261,7 +299,11 @@
self.steps=[]
self._steps=[]
self._lazies={}
-
+ self._fs = fs
+ if flist is None:
+ self._fs_filelist = []
+ else:
+ self._fs_filelist = flist
if environ.has_key('REMOTE_ADDR'):
self._client_addr = environ['REMOTE_ADDR']
@@ -382,23 +424,26 @@
taintedform=self.taintedform
meth=None
- fs=FieldStorage(fp=fp,environ=environ,keep_blank_values=1)
- if not hasattr(fs,'list') or fs.list is None:
+ if not self._fs:
+ fs_env = environ
+ fs_env[FILELIST_ENV_ENTRY] = self._fs_filelist
+ self._fs=ZFieldStorage(fp=fp,environ=fs_env,keep_blank_values=1)
+ if not hasattr(self._fs,'list') or self._fs.list is None:
# Hm, maybe it's an XML-RPC
- if (fs.headers.has_key('content-type') and
- fs.headers['content-type'] == 'text/xml' and
+ if (self._fs.headers.has_key('content-type') and
+ self._fs.headers['content-type'] == 'text/xml' and
method == 'POST'):
# Ye haaa, XML-RPC!
global xmlrpc
if xmlrpc is None: import xmlrpc
- meth, self.args = xmlrpc.parse_input(fs.value)
+ meth, self.args = xmlrpc.parse_input(self._fs.value)
response=xmlrpc.response(response)
other['RESPONSE']=self.response=response
self.maybe_webdav_client = 0
else:
- self._file=fs.file
+ self._file=self._fs.file
else:
- fslist=fs.list
+ fslist=self._fs.list
tuple_items={}
lt=type([])
CGI_name=isCGI_NAME
_______________________________________________
Zope-Dev maillist - [EMAIL PROTECTED]
http://mail.zope.org/mailman/listinfo/zope-dev
** No cross posts or HTML encoding! **
(Related lists -
http://mail.zope.org/mailman/listinfo/zope-announce
http://mail.zope.org/mailman/listinfo/zope )
