Rack rack → 1.1

Comments and changes to this ticket

• You flagged this item as spam.
  

  Jonathan del Strother November 11th, 2009 @ 03:25 PM

  FWIW, I still consider this a bug & my comments in the google groups page still stand. Would be great to get it fixed, but in the meantime, we're hacking around it in our Rails app using something like the following:

  class ActionController::Request
    private
    def normalize_parameters_with_stringified_files(params)
      if params.is_a?(Hash) and params[:tempfile] and params[:type]=~/^text\/plain(?:; charset=([^;]+))/
        param = params[:tempfile].read
        if encoding = $1
          param = Iconv.iconv('utf-8', encoding, param).to_s
        end
        param
      else
        normalize_parameters_without_stringified_files(params)
      end
    end
    alias_method_chain :normalize_parameters, :stringified_files
  end
  

• 

  josh December 11th, 2009 @ 03:20 AM

  • Assigned user set to “chneukirchen (at gmail)”
  • State changed from “new” to “open”
  • Milestone set to “1.1”

• You flagged this item as spam.
  

  Paul January 8th, 2010 @ 04:09 PM

  the version of http://jupload.sourceforge.net/ i am using is sending content-types with parameters too / so this bug (i think this is a bug) appears there too.
  for another example you might look at http://blogs.sun.com/mandy/entry/rack_and_content_type_for

  +1 for 'if filename' and nothing more...

• 

  chneukirchen (at gmail) January 8th, 2010 @ 04:56 PM

  Proposal 1: remove the check for parsable_data?, only check for filename.
  Proposal 2: (parsable_data? && content_type) || (form_data? && filename)

  The second option allows this parser to be used for ws-* stuff, which
  might be useful to some people.

• 

  chneukirchen (at gmail) January 8th, 2010 @ 06:00 PM

  Proposal 1: remove the check for parsable_data?, only check for filename.
  Proposal 2: (parsable_data? && content_type) || (form_data? && filename)

  The second option allows this parser to be used for ws-* stuff, which
  might be useful to some people.

• 

  chneukirchen (at gmail) January 8th, 2010 @ 06:01 PM

  Proposal 1: remove the check for parsable_data?, only check for filename.
  Proposal 2: (parsable_data? && content_type) || (form_data? && filename)

  The second option allows this parser to be used for ws-* stuff, which
  might be useful to some people.

• You flagged this item as spam.
  

  Mark Goris January 9th, 2010 @ 01:02 AM

  I haven't looked at all the code in detail, but I'm a little confused about how the "parsable_data?" check factors in here. In looking at the specific commit I referenced, it looked like lib/rack/utils.rb was make a decision about using tempfiles based on the presences of either a content_type field or a filename. So I think the other check that would be removed is "content_type", not "parsable_data?" (which is what you stated in Proposal 1). That's the way it reads in today's code as well.

  As for Proposal 2, obviously there is some other consideration you want to make regarding WS-* stuff that I don't have insight into, so it's hard for me to judge it's merit. At the highest level, I would mostly likely be satisfied as long as the presence of a content_type attribute didn't immediately trigger Rack to save off data in a tempfile. If Proposal 2 would satisfy that (it appears like it might), then that is fine by me.

• You flagged this item as spam.
  

  Mateo Murphy February 18th, 2010 @ 10:45 PM

  I've just been bit by this behavior myself, and coded a work around in my application; I can submit a patch if it would save someone else the time.

  Regarding the proposal 2; Yes, it would fix the bug, but I'm not certain that this behavior would be correct for the ws-* cases either. I'm not familiar with those specs to say for sure, but I think it would be wise to be absolutely certain of this before implementing it.

• 

  chneukirchen (at gmail) February 19th, 2010 @ 01:34 PM

  • State changed from “open” to “resolved”

  This issue has been decided.

  http://github.com/rack/rack/commit/acffe8ef5ea6de74fe306f2dd908b768...

• You flagged this item as spam.
  

  Mateo Murphy February 19th, 2010 @ 03:48 PM

  Great! Correct me if I'm wrong though, but shouldn't the following else statement be removed as well?

  http://github.com/rack/rack/blob/acffe8ef5ea6de74fe306f2dd908b7681a...

  I'm pretty sure body will not exist at this point, resulting in an exception thrown

• You flagged this item as spam.
  

  Jeremy Kemper June 28th, 2010 @ 12:17 AM

  • Milestone order changed from “0” to “0”

  Mateo, yes. Body will be a string at that point, which can't be rewound.

  This fix also breaks apps that expect to use the Content-Type of the MIME part, such as an app parsing multipart/mixed.

  I think it should be reverted.

• You flagged this item as spam.
  

  Jeremy Kemper June 28th, 2010 @ 12:28 AM

  A better choice would be to skip the Tempfile for text/plain only

• You flagged this item as spam.
  

  Jeremy Kemper June 28th, 2010 @ 12:43 AM

  Fix and regression test.

  • 0001-fix-multipart-mixed-parsing-regression-introduced-by.patch 2.4 KB

• You flagged this item as spam.
  

  Jeremy Kemper June 28th, 2010 @ 12:44 AM

  Losing the metadata for these MIME parts is painful, though. I think the right fix is to treat text/plain specially, given that's the content type in all the examples above.

• You flagged this item as spam.
  

  Jeremy Kemper September 21st, 2010 @ 11:57 PM

  Fix has been pending here for a few months. Could someone please reopen this ticket until it's addressed?

• You flagged this item as spam.
  

  Jeremy Kemper May 17th, 2011 @ 08:49 PM

  The multipart parser was recently refactored, fixing this issue.

  Here's a test verifying it (against master).

  • 0001-test-that-parsing-a-multip.patch 1.9 KB