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

Fix Accept header overwritten issue when "xhr: true" in integration test #26003

Merged
merged 1 commit into from Aug 7, 2016

Conversation

Projects
None yet
5 participants
@darkbaby123
Contributor

darkbaby123 commented Jul 31, 2016

Summary

Fixes #25859

@rails-bot

This comment has been minimized.

rails-bot commented Jul 31, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@kaspth

View changes

actionpack/CHANGELOG.md Outdated
@@ -1,3 +1,10 @@
* Fix `xhr: true` issue in `ActionDispatch::Integration::RequestHelpers`.

This comment has been minimized.

@kaspth

kaspth Aug 2, 2016

Member

Don't think the title says much. I think we could condense this to:

*   Don't override the `Accept` header in integration tests when called with `xhr: true`.

    Fixes #25859.

    *David Chen*
@kaspth

View changes

actionpack/lib/action_dispatch/testing/integration.rb Outdated
@@ -360,15 +360,17 @@ def process(method, path, params: nil, headers: nil, env: nil, xhr: false, as: n
"HTTP_ACCEPT" => accept
}
# # Create a wrapped_headers for key transformation

This comment has been minimized.

@kaspth

kaspth Aug 2, 2016

Member

This comment doesn't add much, let's just remove it.

@kaspth

This comment has been minimized.

Member

kaspth commented Aug 2, 2016

We should write regression tests for this as well to prove neither HTTP_ACCEPT nor Accept are overriden.

Can we also put that lovely investigative work you did in the issue into your commit message? That way future readers won't have to go to GitHub for info.

@kaspth kaspth self-assigned this Aug 2, 2016

@kaspth kaspth added this to the 5.0.1 milestone Aug 2, 2016

@darkbaby123

This comment has been minimized.

Contributor

darkbaby123 commented Aug 3, 2016

Can you tell me which file the test should be written? If I should create a new test file, which file name and path do you suggest? I thought to write a test before but didn't find a proper place. Because this is a issue in test helper.

@kaspth

This comment has been minimized.

Member

kaspth commented Aug 7, 2016

@darkbaby123 somewhere in this test case class might be a good place:

class IntegrationProcessTest < ActionDispatch::IntegrationTest

@darkbaby123 darkbaby123 force-pushed the darkbaby123:fix_xhr_overwrite_headers_in_test branch 3 times, most recently Aug 7, 2016

@darkbaby123

This comment has been minimized.

Contributor

darkbaby123 commented Aug 7, 2016

@kaspth Yes. I just finished the work and put the test into that file. Thanks for the test code so I can find a bug for my previous solution! The changelog and commit and also modified.

@@ -727,6 +728,16 @@ def test_respect_removal_of_default_headers_by_a_controller_action
assert_includes @response.headers, "c"
end
def test_accept_not_overriden_when_xhr_true
with_test_route_set do
get "/get", headers: { "Accept" => "application/json" }, xhr: true

This comment has been minimized.

@kaspth

kaspth Aug 7, 2016

Member

Let's add a test for HTTP_ACCEPT too.

Fix Accept header overridden when "xhr: true" in integration test
In integration test when specify the "Accept" header with "xhr: true"
option, the Accept header is overridden with a default xhr Accept
header. The issue only affects HTTP header "Accept" but not CGI variable
"HTTP_ACCEPT".

For example:

    get '/page', headers: { 'Accept' => 'application/json' }, xhr: true

    # This is WRONG! And the response.content_type is also affected.
    # It should be "application/json"
    assert_equal "text/javascript, text/html, ...", request.accept
    assert_equal 'text/html', response.content_type

The issue is in `ActionDispatch::Integration::RequestHelpers`. When
setting "xhr: true" the helper sets a default HTTP_ACCEPT if blank.
But the code doesn't consider supporting both HTTP header style and
CGI variable style.

For detail see this GitHub issue:
#25859

@darkbaby123 darkbaby123 force-pushed the darkbaby123:fix_xhr_overwrite_headers_in_test branch to 23ce9e9 Aug 7, 2016

@kaspth kaspth merged commit 15a600f into rails:master Aug 7, 2016

2 checks passed

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

kaspth added a commit that referenced this pull request Aug 7, 2016

Merge pull request #26003 from darkbaby123/fix_xhr_overwrite_headers_…
…in_test

Fix Accept header overwritten issue when "xhr: true" in integration test
@kaspth

This comment has been minimized.

Member

kaspth commented Aug 7, 2016

Thanks for the help! I've backported this to the pending 5.0.1 as well: 9047d9e

@darkbaby123 darkbaby123 deleted the darkbaby123:fix_xhr_overwrite_headers_in_test branch Aug 8, 2016

@darkbaby123

This comment has been minimized.

Contributor

darkbaby123 commented Aug 8, 2016

@kaspth Glad to help!

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