Rack::Request#content_charset is always blank due to ActionDispatch::MimeNegotation#content_type #1018

Closed
lighthouse-import opened this Issue May 16, 2011 · 1 comment

Projects

None yet

1 participant

@lighthouse-import

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/6698
Created by Ryan D Johnson - 2011-04-13 04:14:41 UTC

It looks like #4109 was intended to fix this in commit 77a2a3d -- but the content_type method introduced there doesn't do what it needs to. Probably it should be omitted in favor of letting Rack::Request#content_type do its normal thing.

Here's an overview of the problem:

ActionDispatch::Request includes ActionDispatch::Http::MimeNegotiation, which overrides content_type from Rack::Request

Rack::Requset#content_type just dumps env['CONTENT_TYPE'], which will include the charset:

def content_type
  content_type = @env['CONTENT_TYPE']
  content_type.nil? || content_type.empty? ? nil : content_type
end

ActionDispatch::Http::MimeNegotiation does a Mime::Type#lookup as content_mime_type (the Mime::Type used to come back directly from #content_type; that's what 77a2a3 fixed, returning the mime type string instead).

  def content_mime_type
    @env["action_dispatch.request.content_type"] ||= begin
      if @env['CONTENT_TYPE'] =~ /^([^,\;]*)/
        Mime::Type.lookup($1.strip.downcase)
      else
        nil
      end
    end
  end

  def content_type
    content_mime_type && content_mime_type.to_s
  end

Unfortunately, although the new content_type implementation returns a string, it's stripped of the charset. This renders Rack::Request#content_charset (and #media_type_params) useless:

def media_type_params
  return {} if content_type.nil?
  Hash[*content_type.split(/\s*[;,]\s*/)[1..-1].
    collect { |s| s.split('=', 2) }.
    map { |k,v| [k.downcase, v] }.flatten]
end

def content_charset
  media_type_params['charset']
end

It would be really great if request.body could end up with the proper encoding at the end of the day. But even if that isn't going to happen right away, at least request.raw_post.force_encoding( request.content_charset ) should work. As is, I have to reparse the request.env['CONTENT_TYPE'] to get the charset.

Attaching a patch which implements the suggestion. The unit test modification leads to a failure without the lib change and passes with it. All other action pack tests pass with the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment