Skip to content

Commit

Permalink
Restore Response#content_type to the original behavior
Browse files Browse the repository at this point in the history
Since rails#35709, `Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249

But unfortunately, some users expect this method to return all
Content-Type that does not contain charset. This seems to be breaking
changes.

We can change this behavior with the deprecate cycle.
But, in that case, a method needs that include Content-Type with
additional parameters. And that method name is probably the
`content_type` seems to properly.

So I changed the new behavior to more appropriate another
method(`mime_type`) and restored `Response#conten_type` to the original
behavior.

The original `conten_type` method seems to have some problems with
charset parsing (it implicitly expects charset to be at the end). So I
think we should consider the appropriate behavior again later.

Fixes rails#35709.
  • Loading branch information
y-yagi committed Apr 27, 2019
1 parent dbcaf9d commit c352562
Show file tree
Hide file tree
Showing 20 changed files with 141 additions and 120 deletions.
2 changes: 1 addition & 1 deletion actionpack/lib/action_controller/metal.rb
Expand Up @@ -148,7 +148,7 @@ def controller_name
attr_internal :response, :request
delegate :session, to: "@_request"
delegate :headers, :status=, :location=, :content_type=,
:status, :location, :content_type, to: "@_response"
:status, :location, :content_type, :media_type, to: "@_response"

def initialize
@_request = nil
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_controller/metal/mime_responds.rb
Expand Up @@ -205,7 +205,7 @@ def respond_to(*mimes)
yield collector if block_given?

if format = collector.negotiate_format(request)
if content_type && content_type != format
if media_type && media_type != format
raise ActionController::RespondToMismatchError
end
_process_format(format)
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_controller/metal/renderers.rb
Expand Up @@ -157,7 +157,7 @@ def _render_to_body_with_renderer(options)
json = json.to_json(options) unless json.kind_of?(String)

if options[:callback].present?
if content_type.nil? || content_type == Mime[:json]
if media_type.nil? || media_type == Mime[:json]
self.content_type = Mime[:js]
end

Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_controller/metal/rendering.rb
Expand Up @@ -73,7 +73,7 @@ def _set_html_content_type
end

def _set_rendered_content_type(format)
if format && !response.content_type
if format && !response.media_type
self.content_type = format.to_s
end
end
Expand Down
7 changes: 3 additions & 4 deletions actionpack/lib/action_dispatch/http/response.rb
Expand Up @@ -242,9 +242,8 @@ def content_type=(content_type)
set_content_type new_header_info.mime_type, charset
end

# Content type of response.
# It returns just MIME type and does NOT contain charset part.
def content_type
# Media type of response.
def media_type
parsed_content_type_header.mime_type
end

Expand Down Expand Up @@ -458,7 +457,7 @@ def munge_body_object(body)
end

def assign_default_content_type_and_charset!
return if content_type
return if media_type

ct = parsed_content_type_header
set_content_type(ct.mime_type || Mime[:html].to_s,
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_dispatch/testing/assertions.rb
Expand Up @@ -14,7 +14,7 @@ module Assertions
include Rails::Dom::Testing::Assertions

def html_document
@html_document ||= if @response.content_type.to_s.end_with?("xml")
@html_document ||= if @response.media_type.to_s.end_with?("xml")
Nokogiri::XML::Document.parse(@response.body)
else
Nokogiri::HTML::Document.parse(@response.body)
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_dispatch/testing/test_response.rb
Expand Up @@ -19,7 +19,7 @@ def parsed_body
end

def response_parser
@response_parser ||= RequestEncoder.parser(content_type)
@response_parser ||= RequestEncoder.parser(media_type)
end
end
end
28 changes: 14 additions & 14 deletions actionpack/test/controller/content_type_test.rb
Expand Up @@ -66,68 +66,68 @@ def setup
def test_render_defaults
get :render_defaults
assert_equal "utf-8", @response.charset
assert_equal Mime[:text], @response.content_type
assert_equal Mime[:text], @response.media_type
end

def test_render_changed_charset_default
with_default_charset "utf-16" do
get :render_defaults
assert_equal "utf-16", @response.charset
assert_equal Mime[:text], @response.content_type
assert_equal Mime[:text], @response.media_type
end
end

# :ported:
def test_content_type_from_body
get :render_content_type_from_body
assert_equal Mime[:rss], @response.content_type
assert_equal Mime[:rss], @response.media_type
assert_equal "utf-8", @response.charset
end

# :ported:
def test_content_type_from_render
get :render_content_type_from_render
assert_equal Mime[:rss], @response.content_type
assert_equal Mime[:rss], @response.media_type
assert_equal "utf-8", @response.charset
end

# :ported:
def test_charset_from_body
get :render_charset_from_body
assert_equal Mime[:text], @response.content_type
assert_equal Mime[:text], @response.media_type
assert_equal "utf-16", @response.charset
end

# :ported:
def test_nil_charset_from_body
get :render_nil_charset_from_body
assert_equal Mime[:text], @response.content_type
assert_equal Mime[:text], @response.media_type
assert_equal "utf-8", @response.charset, @response.headers.inspect
end

def test_nil_default_for_erb
with_default_charset nil do
get :render_default_for_erb
assert_equal Mime[:html], @response.content_type
assert_equal Mime[:html], @response.media_type
assert_nil @response.charset, @response.headers.inspect
end
end

def test_default_for_erb
get :render_default_for_erb
assert_equal Mime[:html], @response.content_type
assert_equal Mime[:html], @response.media_type
assert_equal "utf-8", @response.charset
end

def test_default_for_builder
get :render_default_for_builder
assert_equal Mime[:xml], @response.content_type
assert_equal Mime[:xml], @response.media_type
assert_equal "utf-8", @response.charset
end

def test_change_for_builder
get :render_change_for_builder
assert_equal Mime[:html], @response.content_type
assert_equal Mime[:html], @response.media_type
assert_equal "utf-8", @response.charset
end

Expand All @@ -148,22 +148,22 @@ class AcceptBasedContentTypeTest < ActionController::TestCase
def test_render_default_content_types_for_respond_to
@request.accept = Mime[:html].to_s
get :render_default_content_types_for_respond_to
assert_equal Mime[:html], @response.content_type
assert_equal Mime[:html], @response.media_type

@request.accept = Mime[:js].to_s
get :render_default_content_types_for_respond_to
assert_equal Mime[:js], @response.content_type
assert_equal Mime[:js], @response.media_type
end

def test_render_default_content_types_for_respond_to_with_template
@request.accept = Mime[:xml].to_s
get :render_default_content_types_for_respond_to
assert_equal Mime[:xml], @response.content_type
assert_equal Mime[:xml], @response.media_type
end

def test_render_default_content_types_for_respond_to_with_overwrite
@request.accept = Mime[:rss].to_s
get :render_default_content_types_for_respond_to
assert_equal Mime[:xml], @response.content_type
assert_equal Mime[:xml], @response.media_type
end
end
8 changes: 4 additions & 4 deletions actionpack/test/controller/integration_test.rb
Expand Up @@ -522,11 +522,11 @@ def test_accept_not_overridden_when_xhr_true
with_test_route_set do
get "/get", headers: { "Accept" => "application/json" }, xhr: true
assert_equal "application/json", request.accept
assert_equal "application/json", response.content_type
assert_equal "application/json", response.media_type

get "/get", headers: { "HTTP_ACCEPT" => "application/json" }, xhr: true
assert_equal "application/json", request.accept
assert_equal "application/json", response.content_type
assert_equal "application/json", response.media_type
end
end

Expand Down Expand Up @@ -986,7 +986,7 @@ def test_standard_json_encoding_works
def test_encoding_as_json
post_to_foos as: :json do
assert_response :success
assert_equal "application/json", request.content_type
assert_equal "application/json", request.media_type
assert_equal "application/json", request.accepts.first.to_s
assert_equal :json, request.format.ref
assert_equal({ "foo" => "fighters" }, request.request_parameters)
Expand Down Expand Up @@ -1025,7 +1025,7 @@ def test_registering_custom_encoder
post_to_foos as: :wibble do
assert_response :success
assert_equal "/foos_wibble", request.path
assert_equal "text/wibble", request.content_type
assert_equal "text/wibble", request.media_type
assert_equal "text/wibble", request.accepts.first.to_s
assert_equal :wibble, request.format.ref
assert_equal Hash.new, request.request_parameters # Unregistered MIME Type can't be parsed.
Expand Down
2 changes: 1 addition & 1 deletion actionpack/test/controller/localized_templates_test.rb
Expand Up @@ -43,6 +43,6 @@ def test_localized_template_has_correct_header_with_no_format_in_template_name
I18n.locale = :it
get :hello_world
assert_equal "Ciao Mondo", @response.body
assert_equal "text/html", @response.content_type
assert_equal "text/html", @response.media_type
end
end
4 changes: 2 additions & 2 deletions actionpack/test/controller/metal/renderers_test.rb
Expand Up @@ -38,13 +38,13 @@ def test_render_json
get :one
assert_response :success
assert_equal({ a: "b" }.to_json, @response.body)
assert_equal "application/json", @response.content_type
assert_equal "application/json", @response.media_type
end

def test_render_xml
get :two
assert_response :success
assert_equal(" ", @response.body)
assert_equal "text/plain", @response.content_type
assert_equal "text/plain", @response.media_type
end
end

0 comments on commit c352562

Please sign in to comment.