Skip to content
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

Change ActionDispatch::Response#content_type returning Content-Type header as it is #36034

Merged
merged 1 commit into from Jun 1, 2019

Conversation

@y-yagi
Copy link
Member

@y-yagi y-yagi commented Apr 19, 2019

Since #35549, 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.

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

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 #35709.

@rails-bot rails-bot bot added the actionpack label Apr 19, 2019
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 25, 2019

See #16018.

I'm not sure how we end up with that much code since I opened that PR but it seems we are doing too much work to get the content_type. Do we need all that code?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 25, 2019

Since #35709, Response#conten_type returns only MIME type correctly.

That was always the case. Unless I'm missing something. That was the behavior I found when I opened #16018.

@y-yagi
Copy link
Member Author

@y-yagi y-yagi commented Apr 25, 2019

In my understanding, this not always the case. Until #35709, Response#conten_type returned media type parameters.
Following script passes with Rails 5.2.2.1, but not with Rails 6.0.0.

require "bundler/inline"

gemfile(true) do
  # gem "rails", "6.0.0.rc1"
  gem "rails", "5.2.2.1"
end

require "minitest/autorun"

class BugTest < Minitest::Test
  def test_content_type
    resp = ActionDispatch::Response.new(200, "Content-Type" => "application/fhir+json; fhirVersion=1; charset=utf-8;")
    assert_equal "application/fhir+json; fhirVersion=1", resp.content_type
  end
end

5.2.2.1

$ ruby 36034.rb
Run options: --seed 30174

# Running:

.

Finished in 0.009994s, 100.0630 runs/s, 100.0630 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

6.0.0

$ ruby 36034.rb
# Running:

F

Failure:
BugTest#test_content_type [36034.rb:13]:
--- expected
+++ actual
@@ -1 +1 @@
-"application/fhir+json; fhirVersion=1"
+"application/fhir+json"



rails test 36034.rb:11



Finished in 0.021647s, 46.1949 runs/s, 46.1949 assertions/s.
1 runs, 1 assertions, 1 failures, 0 errors, 0 skips

I have doubts whether this was an intentional behavior, but I'm concerned about incompatibilities due to a change in behavior.
(I missed #16018. It seems to me that it is the good approach.)

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 26, 2019

👍 I think we should just do what #16018 did. It means that charset will be now in the content_type return, but that will also allow all the methods that I tested in #16018 will work again. With a good CHANGELOG entry and an entry in the upgrade guide I think we can get far with #16018.

@kaspth
Copy link
Member

@kaspth kaspth commented Apr 26, 2019

@rafaelfranca so that would mean apps should generally use request.media_type instead of request.content_type?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 26, 2019

Depends on what they want. If they want all the things they expect in #35709 they should be using content_type. If they only want the mime type like application/json they should be using mime_type.

@y-yagi y-yagi force-pushed the fixes_35709 branch 3 times, most recently from 5ec5adb to 724b564 May 2, 2019
@y-yagi y-yagi changed the title Restore Response#content_type to the original behavior Change ActionDispatch::Response#content_type returning Content-Type header as it is May 2, 2019
@y-yagi
Copy link
Member Author

@y-yagi y-yagi commented May 2, 2019

I updated PR based on #16018.

… header as it is

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 `media_type` method.
And `Response#content_type` changed (as the method name) to return Content-Type
header as it is.

Fixes rails#35709.

[Rafael Mendonça França & Yuuji Yaginuma ]
@y-yagi y-yagi merged commit 2a015f6 into rails:master Jun 1, 2019
2 checks passed
@y-yagi y-yagi deleted the fixes_35709 branch Jun 1, 2019
y-yagi added a commit that referenced this issue Jun 1, 2019
Change `ActionDispatch::Response#content_type` returning Content-Type header as it is
def content_type
type = super
type&.empty? ? nil : type
Copy link
Member

@kamipo kamipo Jun 1, 2019

This line is a bit hard to understand. Is it not enough super.presence?

Copy link
Member Author

@y-yagi y-yagi Jun 1, 2019

👍 I fixed with 22274d3. Thanks!

@@ -92,11 +92,11 @@ def test_should_render_formatted_xml_erb_template

def test_should_render_xml_but_keep_custom_content_type
Copy link
Contributor

@abhaynikam abhaynikam Jun 2, 2019

@y-yagi : Should we rename the test description here from content_type to media_type? I made the changes but was not sure to raise PR.

At other places, it looked fine to me. Only test desc here should maybe change.

Copy link
Member Author

@y-yagi y-yagi Jun 2, 2019

This is test is to confirm that content_type takes precedence when specifying render and content_type.

render xml: "<blah/>", content_type: "application/atomsvc+xml"

So I think the current test name is appropriate.

@cpruitt
Copy link
Contributor

@cpruitt cpruitt commented Jun 4, 2019

This change just landed into our latest 6-0-stable update at GitHub and turned out to be a breaking change causing quite a few failures. Overall I think the changes to content_type are probably justified, but it's a difficult switch to make for us while we're running our app against 5.2 at the same time. This change means 5.2 and 6.0 have different content_type return values making for a non-trivial number of conditionals. I'm wondering if there's a better way to roll this change out that has a cleaner upgrade path for apps expecting the previous content_type values.

@y-yagi
Copy link
Member Author

@y-yagi y-yagi commented Jun 5, 2019

Thanks for your feedback. I think your point is correct. I will try to add a config to control the behavior.

wasifhossain added a commit to rails-api/active_model_serializers that referenced this issue Jun 8, 2019
y-yagi added a commit to y-yagi/rails that referenced this issue Jun 15, 2019
I changed return value of `ActionDispatch::Response#content_type` in rails#36034.
But this change seems to an obstacle to upgrading. rails#36034 (comment)

Therefore, I restored the behavior of `ActionDispatch::Response#content_type`
to 5.2 and deprecated old behavior. Also, made it possible to control the
behavior with the config.
y-yagi added a commit to y-yagi/rails that referenced this issue Jun 15, 2019
I changed return value of `ActionDispatch::Response#content_type` in rails#36034.
But this change seems to an obstacle to upgrading. rails#36034 (comment)

Therefore, I restored the behavior of `ActionDispatch::Response#content_type`
to 5.2 and deprecated old behavior. Also, made it possible to control the
behavior with the config.
y-yagi added a commit to y-yagi/rails that referenced this issue Jun 15, 2019
I changed return value of `ActionDispatch::Response#content_type` in rails#36034.
But this change seems to an obstacle to upgrading. rails#36034 (comment)

Therefore, I restored the behavior of `ActionDispatch::Response#content_type`
to 5.2 and deprecated old behavior. Also, made it possible to control the
behavior with the config.
wasifhossain added a commit to rails-api/active_model_serializers that referenced this issue Jun 15, 2019
y-yagi added a commit to y-yagi/rails that referenced this issue Jun 20, 2019
I changed return value of `ActionDispatch::Response#content_type` in rails#36034.
But this change seems to an obstacle to upgrading. rails#36034 (comment)

Therefore, I restored the behavior of `ActionDispatch::Response#content_type`
to 5.2 and deprecated old behavior. Also, made it possible to control the
behavior with the config.
tegon added a commit to heartcombo/devise that referenced this issue Aug 6, 2019
Before Rails 6 RC2, the `ActionDispatch::Response#content_type` method
would return only the media part of the `Content-Type` header, without any
other parts. Now the `#content_type` method returns the entire header -
as it is - and `#media_type` should be used instead to get the previous
behavior.

Ref:
- rails/rails#36034
- rails/rails#36854
tegon added a commit to heartcombo/devise that referenced this issue Aug 6, 2019
Before Rails 6 RC2, the `ActionDispatch::Response#content_type` method
would return only the media part of the `Content-Type` header, without any
other parts. Now the `#content_type` method returns the entire header -
as it is - and `#media_type` should be used instead to get the previous
behavior.

Ref:
- rails/rails#36034
- rails/rails#36854
tegon added a commit to heartcombo/devise that referenced this issue Aug 7, 2019
* Fix specs on Rails 6 RC2

`ActiveRecord::MigrationContext` now has a `schema_migration` attribute.
Ref: https://github.com/rails/rails/pull/36439/files#diff-8d3c44120f7b67ff79e2fbe6a40d0ad6R1018

* Use `media_type` instead of `content_type`

Before Rails 6 RC2, the `ActionDispatch::Response#content_type` method
would return only the media part of the `Content-Type` header, without any
other parts. Now the `#content_type` method returns the entire header -
as it is - and `#media_type` should be used instead to get the previous
behavior.

Ref:
- rails/rails#36034
- rails/rails#36854

* Use render template instead of render file

Render file will need the full path in order to avoid security breaches.
In this particular case, there's no need to use render file, it's ok to
use render template.

Ref: rails/rails#35688

* Don't set `represent_boolean_as_integer` on Rails 6

* Update comments [ci skip]
tvdeyen added a commit to tvdeyen/alchemy_cms that referenced this issue Aug 19, 2019
Rails 6 changed the usage of content_type. It now includes the complete header
in contrary to media_type that only returns the MIME type.

See rails/rails#36034
wasifhossain added a commit to rails-api/active_model_serializers that referenced this issue Aug 23, 2019
benoittgt added a commit to benoittgt/rspec-rails that referenced this issue Aug 24, 2019
@themilkman
Copy link

@themilkman themilkman commented Sep 17, 2019

I will try to add a config to control the behavior.

@y-yagi is there a way to configure the old behavior? As mentioned in #35709 this has an big impact.

suketa added a commit to suketa/rails_sandbox that referenced this issue Oct 14, 2019
Change `ActionDispatch::Response#content_type` returning Content-Type
header as it is
rails/rails#36034
suketa added a commit to suketa/rails_sandbox that referenced this issue Oct 14, 2019
Change `ActionDispatch::Response#content_type` returning Content-Type
header as it is
rails/rails#36034
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants