Hello,

It's become clear to me that the API for 2070 [1] (which is currently
a mess :) has never really been discussed by the developers, and I've
been thinking about ways of organizing it such that one can add
features to file uploading without modifying the core.
Here's an overview of what I had in mind.

(Any comments/suggestions/angry cries are greatly appreciated.)

Extremely Visible Changes
================

request.set_upload_handler(<upload_handler>)
--------------------------------------------------------------
This new function will allow one to register a new FileUploadHandler
object to deal with the incoming data.
The API for the FileUploadHandler (and the default child
TemporaryFileUploadHandler) is discussed below.

request.FILES
-----------------
This is no longer a MultiValueDict of raw content, but a
MultiValueDict of UploadedFile objects.
This will probably hurt the most, as there are probably applications
assuming that they can take the content from this dict.
The API of the UploadedFile (and the default child
TemporaryUploadedFile) is discussed below.

APIs for New Objects
=============

FileUploadHandler
-----------------------
Objects of this type must define the following methods:
   1. new_file(filename, content_length) -- A signal that a new file
       has been reached by the parser.
   2. receive_data_chunk(raw_data, start, stop) -- Some data has been
       received by the parser. A non-nonzero return value indicates
that
       the file upload should cease.
   3. file_complete(file_size) -- A signal that the current file has
       completed (no more than one file will ever be dealt with).
       The expected return value is an object of UploadedFile.
   4. upload_complete() -- A signal that all uploads have finished
successfully.
   5. get_chunk_size() -- Returns a number that represents the maximum
number
       of bytes read into memory for each chunk.

By adding a set_upload_handler() method to request, anyone can
override the default upload handler. However, this must be done before
the POST was accessed, and it is probably recommended we raise an
error if someone tries to set a new upload handler after the FILES
MultiValueDict is populated.

The default handler (TemporaryFileUploadHandler) will stream the data
to a temporary file for each part of the upload, and return a
corresponding TemporaryUploadedFile object that represents that data.
In addition, the TemporaryFileUploadHandler can know to stream it to
memory if the content_size is small enough (and start streaming it
into disk if the header lied for some grace period).

UploadedFile
----------------
Objects of this class represent files that were just uploaded and
handled by some handler above. I'm assuming that this will be the most
difficult class to nail, since it has to balance a few different
aspects:
   1. Encapsulating what it means to be a uploaded file.
   2. Making it easy for a developer to send it to a FileBackend and
       have it Do The Right Thing.
   3. Making it easy for a developer to get the content and do
       something with it.

Methods of the UploadedFile:
   1. open() -- Open the file for reading.
   2. read([num_bytes]) -- Read a number of bytes from the file.
   3. close() -- Round out the standard read file operations.
   4. chunk([chunk_size]) -- Generates chunks from the file (if the
       content is in memory already, it should ignore chunk_size).
   5. filename() -- The filename from the content-disposition header.
   6. multiple_chunks([chunk_size]) -- Whether or not you can expect
       multiple chunks from chunk() (Boolean). Useful if you want to
do
       something particularly when you know you can put it all into
       memory at once.
   7. file_size() -- The size in bytes of the uploaded file.

Added method of a TemporaryUploadedFile:
   8. temporary_file_path() -- The whole path (directory and basename)
       of the temporary file on disk. A FileBackend may decide to move
       a file (using the OS move operations) if it has a non-empty
       temporary_file_path.

Notes: After talking with Marty online, it seems that this will work
with the FileBackends, provided that they know how to deal with it.
For instance, if the upload is going to a temporary file, and the
FileBackend is a standard file, then the FileBackend for standard
files should know from temporary_file_path() that it can move, and
perform an OS move operation. Otherwise, the FileBackends should use
the chunk() iterator and do whatever it needs in chunks.

It's interesting to note that with this framework a lot of interesting
possibilities open up. I will not write any of the code to do anything
but the basic temporary disk storage, but here are a few interesting
examples of what can happen:
   - Gzipping data on the fly [GZipFileUploadHandler +
GZipFileBackend].
   - Saving file to another FTP Server on the fly
                                            [FTPFileUploadHandler +
NoOpFileBackend].
   - Having Cool Ajax-y file uploads [AjaxProgressUploadHandler + Any
Backend].
   - Having user-based quotas [QuotaUploadHandler + Any Backend].
   ...

Anyway, the list immediately above isn't what's important right now,
but I'd like to get the API
close enough so we can break things once.

Cheers,
Michael Axiak

One slight modification so far...

On Mar 19, 12:30 am, Mike Axiak <mcax...@gmail.com> wrote:

Scratch that return value. You will have to raise StopUpload and
SkipFile to stop the entire upload and skip the file respectively.

-Mike

The filename returned by UploadedFile, will that be an unicode object
or a string? Currently request.FILES uses string objects for the
filenames, which results in some problems with uploaded files with
non-latin filenames. See #6009.

I remember long time ago I had a bit different idea.

There are actually two aspects of what happens to an uploaded file:

- its data should be stored somewhere
- a user might want to do some additional processing (count bytes, unzip
it on the fly, resend it to a remote machine)

And I thought that for the first part -- storing it somewhere -- there
shouldn't be actually any handlers, Django should just store it in temp
files indefinitely. However this your proposal may be better. Because,
given my example of unzipping files on the fly, user might not want
event store original file as it is. What do you think about it?

I believe this can be made backwards compatible. In my patch[1] to
ticket 1484 (which was duplicated by 2070 long ago) I had a FileDict
class that was lazily fetching file content upon accessing
request.FILES['some_file']['content']. Have a look at it.

Am I right thinking that raw_data is not a raw data from socket but is
already decoded from whatever content-transfer-encoding it might be in
(i.e. base64)?

Why not just an attribute chunk_size? It's shorter and it's almost
always will be just a plain class attribute.

Instead of having users figure out where to stick a call to
set_upload_handler we could steal the design from middleware and just
have UPLOAD_HANDLERS setting... It might not be such a good idea if
people will often want different logic per view. However I think a
single global setting is needed for the most common use case: store
everything in temp files.

Heh :-). I was inventing my use cases before I get to this point :-).

[1]: http://code.djangoproject.com/attachment/ticket/1484/1484.m-r.6.diff

Hi Mike --

*Very* well done -- I'm in agreement with nearly every aspect of your
proposal. Major props for taking on such a sticky issue -- walking
into #2070 is a bit like exploring an overgrown jungle :) A few
comments inline below, but in general I quite like your API and would
like to see your code.

I especially like this -- it neatly encapsulates the fact that
different folks are going to want quite different file upload
behavior. A few things to think about:

* Do you think it's worth firing a signal
(file_upload_started/file_upload_finished) from request or from the
base upload handler? This would let decentralized processing of file
uploads, but could get confusing.
* Do you think we should allow multiple upload handlers (which makes
this call into something like request.add_upload_handler(handler))?
The other option would be to just make folks compose upload handlers
with a wrapper class, which is hardly a hardship.

I don't have answers to either of these questions; something to think
about, at least.

It seems to me that you could pretty easily provide a
backwards-compatible API by defining __getitem__ on UploadedFile;
raise a DeprecationWarning there but provide the data in the "old"
style.

Just make this FileUploadHandler.chunk_size -- no reason for getters/setters.

Again, thanks -- this is good stuff.

Jacob

But what about default behaviour? There should be some place to say "all
file uploads should go on disk".

P.S. In my other response we seem to agree on __getitem__ for
request.FILES and an attribute .chunk_size almost exactly :-).

Good question. The issue of encoding is a little murky. As of now, my
(almost working) patch will pretty much try to encode everything
(using force_to_unicode) *except* data in a file upload.
I'm not exactly sure we'll ever support encoding data in file uploads,
but if we do it's definitely a tricky problem.
The issue is two-fold:
   - What files should we encode and which shouldn't we? (do we do
matching on the mimetype? that doesn't sound nice)
   - Dealing with multibyte boundaries in the strings while streaming
the data. (Not a question, just a fun night of work.)

Anyway, I'll go back to ignoring that issue for now unless I get angry
cries for it.

-Mike

On Mar 19, 7:06 am, "Casper Jensen" <t...@sema.dk> wrote:

On Mar 19, 7:31 am, Ivan Sagalaev <man...@softwaremaniacs.org> wrote:

Yes. The idea is to abstract away much of the HTTP work and just get
the raw data after it's been pulled from whatever casing it's in.

The idea is that you can set this anywhere, so long as you haven't
accessed POST and FILES yet. So a view will be a place you can do
this.
The TemporaryFileUploadHandler will be default, and is what will
happen "out-of-the-box".
(Well...there's the idea of using the MemoryFileUploadHandler if the
Content-Length exists and is small...but that's beside the point. :)

-Mike

On Wed, Mar 19, 2008 at 8:19 AM, Ivan Sagalaev

I think the default behavior doesn't need to be a setting:
upload-to-disk for all files over N bytes (1M, maybe?), and
upload-to-RAM for anything smaller.

Jacob

My concern is for this "out-of-the-box" default not to be hard-coded.
There should be some way for a user to set its own global (i.e. non
per-view) handler. I understand that everyone can write a one-line
middleware that sets the handler but while we're at designing stage I
thought we could make it a setting instead of another FAQ :-)

This is the right way. Uploaded file is almost never will be treated as
text and unicode doesn't make sense for binary data. It's also
especially useless for the default behaviour of storing uploaded data to
disk: you'll have to encode just decoded data back to a stream of bytes
to store it in a file.

Sorry, I didn't mean "default" in a sense of absent user settings. I
want to have a way to set a user-specific handler not only on per-view
basis but as a "default" for all upload requests.

To think of a use-case... Imagine some system where /tmp directory is on
a very small partition. An admin would want to direct all uploads
(potentially big) to some other device. And he wants to do it only for
this Django site, not the whole system. Something like this...

Twas brillig at 19:53:00 19.03.2008 UTC+03 when Ivan Sagalaev did gyre and gimble:

 IS> An admin would want to direct all uploads (potentially big) to some
 IS> other device. And he wants to do it only for this Django site, not
 IS> the whole system.

Security also comes in mind: it may be undesirable to use same directory
for different sites to prevent information leakage (even the fact of
file upload may be security-sensitive).

--

application_pgp-signature_part < 1K Download

On Mar 19, 12:53 pm, Ivan Sagalaev <man...@softwaremaniacs.org> wrote:

Since this is part of the request, the user is welcome to write a
middleware class that sets this before any view gets started in the
entire project.
Does this not do what you want?

-Mike

It does. But I was thinking of a setting. However I don't insist on it.
Since you and Jacob seem to not feel that it's necessary let it be just
a method on request. I was merely raising a concern.

The way I see it is that there's no reason we can't add a contrib
middleware later that adds a setting for the default request handler.

-Mike

On Mar 19, 1:49 pm, Ivan Sagalaev <man...@softwaremaniacs.org> wrote:

This looks great.  I expect it will make proper integration with something
like Tramline (www.infrae.com/products/*tramline*) very easy to do but
perhaps no longer necessary.

Chad

Now that we actually have a working patch [1], there are a few details
I'd like to raise here.

Major Issues
=======

Supporting dictionaries in form code
------------------------------------
While we can have file objects that support the dictionary lookup to
make those backwards compatible, there's still the API of the forms
code.
As an example, there are a lot of tests that look like::

   TextFileForm(data={'description': u'Assistance'}, files={'file':
{'filename': 'test1.txt', 'content': 'hello world'}})

This is an example of something using a dictionary instead of a
UploadedFile instance when using the forms. However, the forms should
*probably* be using the new fileobject API (use .chunks()/.read()
rather than ['content']), so that would mean they would break if fed a
dictionary like the above example. I see three options to deal with
this:

 1. Leave forms code alone, causing (a) DeprecationWarnings and (b)
the files to be read into memory when saving.
 2. Modify the form code to access the attributes of the
UploadedFile, but on AttributeError use the old dict-style interface
and emit a DeprecationWarning.
 3. Modify form code to use the new UploadedFile object, but just
break on the old dictionary access as it's largely an internal API
anyway.

Without thinking about it, I wrote the patch for (3) and modified the
tests appropriately. I'm now thinking people will probably want (2)
more. The other issue is what we should do with the tests. Should we
leave them? Should we copy them and create a copy of them for the new
style? Should we replace them with the new style?

Having upload_handlers be a list object
---------------------------------------
I know that .set_upload_handler() was really cool, but I've discarded
it to support multiple handlers (see below for details). So now I had
to think of what to replace it with. At first I thought:
.append_upload_handler(), but then I realized that I may want
.insert_upload_handler(), and .delete_upload_handler(). So before long
I realized that I just want people to have a list interface.
Therefore, I decided to just leave it as a plain old list. Thus, to
add an upload handler, you'd just write::

   request.upload_handlers.append(some_upload_handler)

And to replace them all::

   request.upload_handlers = [some_upload_handler]

I've made a few efforts to ensure that it will raise an error if the
upload has already been handled. I know this isn't as simple as the
.set_upload_handler() interface, but I think it's the simplest way we
can support the list modification/replacement in a useful fashion.
What do people think about this?

(Mis)Handling Content-Length
----------------------------
There's probably not much room for argument here, but it's worth
asking a larger group. Currently when you try uploading Large files
(~2GB and greater), you will get a weird Content-Length header (less
than zero, overflowing). The problem is two-fold:
 1. We can't easily exhaust the request data without knowing its
length.
 2. If we don't exhaust the request data, we get a sad Connect Reset
error from the browsers.
This means that even if we wanted to display a useful error page to
people telling them why this happened, it won't happen because we'll
just get Connection Reset errors. We can just ignore it, or we can try
modifying the request streams to set timeouts and exhaust the input.
My perusal of Apache docs tells me that there's discard_request_body,
but unfortunately even this seems to depend on Content-Length. Should
I/we just ignore this and expect people to be sane and upload
reasonable data?

Revised API
===========

I was about to write a huge document that described the API in here.
But then I realized I might as well write it in restructuredtext. Then
I realized I might as well make it formatted like a Django document.
And well...you can see the outcome at:
   http://orodruin.axiak.net/upload_handling.html

Let me know if you have any comments on the API. (Things like how it
deals with multiple handlers could be up for discussion.)

Addendum
========

One last note: In order to test the API and everything, I created a
google code project 'django-uploadutils' [2] which I hope will become
a collection of upload handlers for doing common tasks. If anyone is
interested in helping out with this, I'd be happy to add you as a
contributor.

Regards,
Mike

1: http://code.djangoproject.com/ticket/2070
2: http://code.google.com/p/django-uploadutils/

I don't like #1 because there's no point to keep deprecated code in
Django where we can fix it. And #3 is indeed broken because there is
much code in the wild that uses request.FILES directly. It's a public
API after all.

#2 looks reasonable to me.

It would be good to invent a declarative setting for this but since it's
a per-view thing it would be hard. So may be indeed a list would be enough.

If Content-Length header is crewed then neither we, nor users can do
anything about it. When these file volumes become more widespread I
believe browsers will fix themselves to send correct Content-Length.

Hey,

On Mar 25, 6:04 am, Ivan Sagalaev <man...@softwaremaniacs.org> wrote:

I thought this would be confused. Currently when you want to upload
something to a form, you would do something along the lines of::

    f = SomeForm(request.POST, request.FILES)

This actually invokes two separate interfaces: the objects inside
request.FILES, and the way the form deals with it. Currently, the form
assumes request.FILES contains dictionaries, and so (like the
documentation) you can feed it dictionaries of data and it will work.
How the form treats this data is the specific interface I'm talking
about, not what request.FILES contains.

Anyway, I'll modify the patch to do #2 anyway. Thanks for the input.

-Mike

I haven't had time to sit down and devote to reading through the whole
patch, but I have a possibly very easy question that I can't answer from
the docs at the moment.

I'm a simple guy. I like simple stuff. So if I'm deploying a Django
system and it will handle file uploads from some forms and all I want to
do is ensure that large file uploads aren't held in memory, how do I do
that? In other words, I want to avoid the problem that originally
prompted this ticket and the related one.

Does this Just Work(tm) out of the box?

Malcolm

--
Remember that you are unique. Just like everyone else.
http://www.pointy-stick.com/blog/

Hey,

On Mar 25, 3:32 pm, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:

Yes. :-)

-Mike

On Mar 25, 3:38 pm, Mike Axiak <mcax...@gmail.com> wrote:
Doh. I didn't realize you actually wanted me to do more work.

The newly updated doc can be found at the same URL:
http://orodruin.axiak.net/upload_handling.html

Be sure to refresh as the browser caching is high on that page.

-Mike

I didn't either. I thought I may have just been missing something
obvious. But now that you've written the extra bits at the top of the
document, it makes more sense to me as a user. Thanks.

Okay, so this all has to go on the review pile now I guess. That should
be fun, for some value of "fun". Nice work, Mike.

Regards,
Malcolm

--
No one is listening until you make a mistake.
http://www.pointy-stick.com/blog/

Woo! Thanks for your hard work. My thoughts on your questions follow inline:

What would an equivalent line look like under the new system? That is, what do
folks need to change their tests to?

Yes, this is correct approach. Something along these lines:

   if isinstance(uploadedfile, dict):
        warn(...)
        uploadedfile = uploadedfile_from_dict(uploadedfile)

Option #3 is unacceptable: if at all possible we want people's tests to
not break. Warnings are fine; breakage if avoidable is a bad idea.

The latter -- fix all the tests to use the new syntax as a demo of how it's
supposed to be done.

If we do indeed need this -- see below -- then this is the right way to do it.

I'm thinking YAGNI here. Why would I need multiple upload handlers? I think you
need to talk me through your thinking here, because at first glance this smacks
of overegineering. Remember that multiple handlers can always be accomplished by
composition anyway, so unless there's a good reason I think set_upload_handler()
is just much cleaner.

Similarly, I'm a bit suspicious of FILE_UPLOAD_HANDLERS. Couldn't you just write
a simple middleware to do the same thing? As a general principle, you should try
as hard as you can to avoid introducing new settings; if there's another way
just do it that way. In this case, I'd just document that if you want to use a
custom upload handler globally that you should write a middleware class.

Personally, I can't see any reason to care too much about people trying
to upload 2GB files through the web, anyway. Anyone allowing uploads should
have a sane upload size limit set at the web server level. Anyone who's allowing
uploads of over 2GB is just asking to get DOSed.

I think this is a place where we just add a note to the docs about setting the
Apache/lighttpd/etc. upload limit and move on.

I quite like the API. I'm not sure why you'd need to return a custom
UploadedFile from an upload handler, but props for documenting the interface
anyway :)

Thanks again for the hard work -- this looks very good!

Jacob