New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow `send_file` to declare a charset #26317

Merged
merged 3 commits into from Aug 29, 2016

Conversation

Projects
None yet
2 participants
@maclover7
Member

maclover7 commented Aug 29, 2016

Summary

Before this PR, if you tried to do the pass in type: "text/calendar; charset=utf-8" as an option to send_file:, the charset would be deleted. This PR ensures that the user provided charset is not cleared.

Other Information

This helps restore behavior that was altered by 1cc315c.

@tenderlove

View changes

actionpack/lib/action_dispatch/http/response.rb Outdated
if content_type
# Mime::Type objects are sometimes passed as arguments, so we need to
# call #to_s on them, to be able to call #split a few lines later.

This comment has been minimized.

@tenderlove

tenderlove Aug 29, 2016

Member

Why are Mime::Type objects sometimes passed as arguments? We should always be getting a string.

@tenderlove

This comment has been minimized.

Member

tenderlove commented Aug 29, 2016

@maclover7 I think we can fix this with a less invasive patch:

diff --git a/actionpack/lib/action_controller/metal/data_streaming.rb b/actionpack/lib/action_controller/metal/data_streaming.rb
index b598dd4..6a1acb6 100644
--- a/actionpack/lib/action_controller/metal/data_streaming.rb
+++ b/actionpack/lib/action_controller/metal/data_streaming.rb
@@ -70,6 +70,7 @@ def send_file(path, options = {}) #:doc:
         send_file_headers! options

         self.status = options[:status] || 200
+        self.content_type = options[:type] if options.key?(:type)
         self.content_type = options[:content_type] if options.key?(:content_type)
         response.send_file path
       end
diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb
index 9788431..c8f0644 100644
--- a/actionpack/lib/action_dispatch/http/response.rb
+++ b/actionpack/lib/action_dispatch/http/response.rb
@@ -224,8 +224,10 @@ def status=(status)

     # Sets the HTTP content type.
     def content_type=(content_type)
-      header_info = parse_content_type
-      set_content_type content_type.to_s, header_info.charset || self.class.default_charset
+      return unless content_type # maybe this should raise
+      new_header_info = parse_content_type(content_type.to_s)
+      prev_header_info = parse_content_type(get_header(CONTENT_TYPE))
+      set_content_type new_header_info.mime_type, new_header_info.charset || prev_header_info.charset || self.class.default_charset
     end

     # Sets the HTTP response's content MIME type. For example, in the controller
@@ -403,8 +405,7 @@ def cookies
     ContentTypeHeader = Struct.new :mime_type, :charset
     NullContentTypeHeader = ContentTypeHeader.new nil, nil

-    def parse_content_type
-      content_type = get_header CONTENT_TYPE
+    def parse_content_type(content_type = get_header(CONTENT_TYPE))
       if content_type
         type, charset = content_type.split(/;\s*charset=/)
         type = nil if type.empty?

Can you test out the above patch?

Also, in general, when we do argument mangling (for example when we sometimes get a mime type object) it's best to do that as close as possible to the place where we're getting that input. Handling that case in parse_content_type can lead future developers to believe that the header hash could contain a mime type object as a value when this is a) definitely not the case, and b) would be very bad if it actually happened (we would want that situation to raise an exception). It seems like those type of objects are getting passed to content_type=, so I called to_s on the parameter, but I think it would be totally legit to do an is_a? test there instead (and we could avoid reparsing the Mime object though that leads to a question of "Is it common to pass in a Mime object? If not, it's probably more performant to leave the to_s version. If so, then maybe we need to rethink the API"

I'd also rather we pass the header in to parse_content_type rather than use a default param. I think we can change to do that since this method is private, but I left the default to keep the patch short.

If the above patch works, can you apply it and add OPs test? Thanks!

@tenderlove

This comment has been minimized.

Member

tenderlove commented Aug 29, 2016

Tracking #26298

@maclover7 maclover7 force-pushed the maclover7:jm-fix-26298 branch Aug 29, 2016

Allow `send_file` to declare a charset
Removed my patch in favor of @tenderlove's less invasive approach.

[Aaron Patterson & Jon Moss]

@maclover7 maclover7 force-pushed the maclover7:jm-fix-26298 branch Aug 29, 2016

@tenderlove

This comment has been minimized.

Member

tenderlove commented Aug 29, 2016

@maclover7 looks great! Ping me when you're ready for a merge.

@maclover7 maclover7 force-pushed the maclover7:jm-fix-26298 branch Aug 29, 2016

@maclover7 maclover7 force-pushed the maclover7:jm-fix-26298 branch to 92f8b99 Aug 29, 2016

@maclover7

This comment has been minimized.

Member

maclover7 commented Aug 29, 2016

@tenderlove updated, just waiting on test suite to wrap up. :)

@tenderlove

This comment has been minimized.

Member

tenderlove commented Aug 29, 2016

@maclover7 sounds great! I'll merged when it's 🍏

@tenderlove tenderlove merged commit 6949f8e into rails:master Aug 29, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
codeclimate Code Climate didn't find any new or fixed issues.
Details

maclover7 added a commit to maclover7/rails that referenced this pull request Aug 29, 2016

Merge pull request rails#26317 from maclover7/jm-fix-26298
Allow `send_file` to declare a charset

@maclover7 maclover7 referenced this pull request Aug 29, 2016

Merged

Backport PR #26317 #26322

@maclover7 maclover7 changed the title from [WIP] Allow `send_file` to declare a charset to Allow `send_file` to declare a charset Aug 29, 2016

@maclover7

This comment has been minimized.

Member

maclover7 commented Aug 29, 2016

👍 thanks! I opened up PR #26322 to backport this PR to 5-0-stable.

@maclover7 maclover7 deleted the maclover7:jm-fix-26298 branch Aug 29, 2016

tenderlove added a commit that referenced this pull request Aug 29, 2016

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