Re: Streaming Uploads Discussion

On Mon, Mar 24, 2008 at 1:02 PM, Mike Axiak <mcaxiak@...> wrote:
>  Now that we actually have a working patch [1], there are a few details
>  I'd like to raise here.

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

>  Supporting dictionaries in form code
>  ------------------------------------
>  [...]
>    TextFileForm(data={'description': u'Assistance'}, files={'file':
>  {'filename': 'test1.txt', 'content': 'hello world'}})

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

>  I see three options to deal with this:
>  [...]
>   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.

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 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?

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.

>  What do people think about this?

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.

>  (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).

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.

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

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

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers@...
To unsubscribe from this group, send email to django-developers-unsubscribe@...
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---