Consider reverting multipart parsing change

Consider reverting multipart parsing change

[Jeremy Kemper]

I think acffe8ef5 should be reverted.

To support an edge casey user-agent it removed the metadata from all
MIME parts with a Content-Type but no filename.

The idea was to have text/plain parts parse as regular params instead
of file uploads. That's reasonable, but it should be limited to
text/plain parts.

The original issue: http://rack.lighthouseapp.com/projects/22435-rack/tickets/79

jeremy

Re: Consider reverting multipart parsing change

[Mark Goris]

Hi Jeremy,

It's been a long time since I created the initial ticket, so I've lost
some of the context (especially in the subsequent discussion that led
to this change that you've suggested to revert).  It seemed best to me
that tempfiles only be used in the case of having a filename attribute
be present, regardless of content-type.  Your suggestion seems to
focus on the specific type of content-type driving logic regarding
whether or not a part is treated as a file upload.  I guess my
question is, why not just use the filename attribute to drive this
logic?  I don't know of a downside to this approach (meaning, I'm not
exactly clear on the intention for the change that prompted ticket 79
in the first place).

JMG

On Jul 4, 3:20 pm, Jeremy Kemper <jer...@bitsweat.net> wrote:
> I think acffe8ef5 should be reverted.
>
> To support an edge casey user-agent it removed the metadata from all
> MIME parts with a Content-Type but no filename.
>
> The idea was to have text/plain parts parse as regular params instead
> of file uploads. That's reasonable, but it should be limited to
> text/plain parts.
>
> The original issue:http://rack.lighthouseapp.com/projects/22435-rack/tickets/79
>
> jeremy

Re: Consider reverting multipart parsing change

[Jeremy Kemper]

On Mon, Jul 5, 2010 at 3:57 PM, Mark Goris <mark.goris@gmail.com> wrote:
> It's been a long time since I created the initial ticket, so I've lost
> some of the context (especially in the subsequent discussion that led
> to this change that you've suggested to revert).  It seemed best to me
> that tempfiles only be used in the case of having a filename attribute
> be present, regardless of content-type.  Your suggestion seems to
> focus on the specific type of content-type driving logic regarding
> whether or not a part is treated as a file upload.  I guess my
> question is, why not just use the filename attribute to drive this
> logic?  I don't know of a downside to this approach (meaning, I'm not
> exactly clear on the intention for the change that prompted ticket 79
> in the first place).

Checking for filename works for file uploads from web browsers. All
other MIME parts are flattened to a "normal" form parameter even it it
was neither a form param or a file upload. For example, see the
multipart/mixed regression on #79.

The fix for #79 restricts what may be considered a file upload rather
than broadening what may be considered a normal form param to include
MIME parts with a text/plain Content-Type.

Rather than treating all MIME parts as form params and making a
special case for file uploads by looking for a filename, we should be
doing the opposite, making a special case to identify form param MIME
parts by their missing filename and missing or text/plain
Content-Type.

jeremy

Re: Consider reverting multipart parsing change

[Joshua Ballanco]

On Jul 6, 2010, at 12:54 AM, Jeremy Kemper wrote:

> On Mon, Jul 5, 2010 at 3:57 PM, Mark Goris <mark.goris@gmail.com> wrote:
>> It's been a long time since I created the initial ticket, so I've lost
>> some of the context (especially in the subsequent discussion that led
>> to this change that you've suggested to revert).  It seemed best to me
>> that tempfiles only be used in the case of having a filename attribute
>> be present, regardless of content-type.  Your suggestion seems to
>> focus on the specific type of content-type driving logic regarding
>> whether or not a part is treated as a file upload.  I guess my
>> question is, why not just use the filename attribute to drive this
>> logic?  I don't know of a downside to this approach (meaning, I'm not
>> exactly clear on the intention for the change that prompted ticket 79
>> in the first place).
> 
> Checking for filename works for file uploads from web browsers. All
> other MIME parts are flattened to a "normal" form parameter even it it
> was neither a form param or a file upload. For example, see the
> multipart/mixed regression on #79.
> 
> The fix for #79 restricts what may be considered a file upload rather
> than broadening what may be considered a normal form param to include
> MIME parts with a text/plain Content-Type.
> 
> Rather than treating all MIME parts as form params and making a
> special case for file uploads by looking for a filename, we should be
> doing the opposite, making a special case to identify form param MIME
> parts by their missing filename and missing or text/plain
> Content-Type.

Just wanted to give my +1 to this solution. I actually have a real-life use case where this becomes an issue. I have a client (not a web browser) which generates data dynamically to upload to an HTTP server as a file upload. Because there was no original file from which to extract a filename parameter (and because the server has logic to assign filenames based on other parameters), my client does a multipart-upload without a filename parameter. However, the file parts can be rather large (up to 100s of MB). Having these passed in as strings instead of using tempfiles seems less than ideal.

Cheers,

Josh

Re: Consider reverting multipart parsing change

[Mateo Murphy]

On 8-Jul-10, at 1:03 AM, Joshua Ballanco wrote:

> On Jul 6, 2010, at 12:54 AM, Jeremy Kemper wrote:
>
>> On Mon, Jul 5, 2010 at 3:57 PM, Mark Goris <mark.goris@gmail.com>  
>> wrote:
>>> It's been a long time since I created the initial ticket, so I've  
>>> lost
>>> some of the context (especially in the subsequent discussion that  
>>> led
>>> to this change that you've suggested to revert).  It seemed best  
>>> to me
>>> that tempfiles only be used in the case of having a filename  
>>> attribute
>>> be present, regardless of content-type.  Your suggestion seems to
>>> focus on the specific type of content-type driving logic regarding
>>> whether or not a part is treated as a file upload.  I guess my
>>> question is, why not just use the filename attribute to drive this
>>> logic?  I don't know of a downside to this approach (meaning, I'm  
>>> not
>>> exactly clear on the intention for the change that prompted ticket  
>>> 79
>>> in the first place).
>>
>> Checking for filename works for file uploads from web browsers. All
>> other MIME parts are flattened to a "normal" form parameter even it  
>> it
>> was neither a form param or a file upload. For example, see the
>> multipart/mixed regression on #79.
>>
>> The fix for #79 restricts what may be considered a file upload rather
>> than broadening what may be considered a normal form param to include
>> MIME parts with a text/plain Content-Type.
>>
>> Rather than treating all MIME parts as form params and making a
>> special case for file uploads by looking for a filename, we should be
>> doing the opposite, making a special case to identify form param MIME
>> parts by their missing filename and missing or text/plain
>> Content-Type.
>
> Just wanted to give my +1 to this solution. I actually have a real- 
> life use case where this becomes an issue. I have a client (not a  
> web browser) which generates data dynamically to upload to an HTTP  
> server as a file upload. Because there was no original file from  
> which to extract a filename parameter (and because the server has  
> logic to assign filenames based on other parameters), my client does  
> a multipart-upload without a filename parameter. However, the file  
> parts can be rather large (up to 100s of MB). Having these passed in  
> as strings instead of using tempfiles seems less than ideal.
>
> Cheers,
>
> Josh

Looking at the relevant RFC (http://www.ietf.org/rfc/rfc1867.txt), the  
filename is clearly optional, so relying on it is not a good idea.  
Treating everything that has a filename or a content-type other than  
text/plain as a file would be a sensible solution, although that  
creates a problem when trying to upload very large text files; in that  
case I think also added a content-length check might be wise?