Skip to content

Commit

Permalink
Fix default headers in test responses
Browse files Browse the repository at this point in the history
Fixes regression in #18423. Merge default headers for new responses,
but don't merge when creating a response from the last session request.

hat tip @senny ❤️
  • Loading branch information
jeremy committed Feb 25, 2015
1 parent 2abed7a commit f6e293e
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 34 deletions.
8 changes: 3 additions & 5 deletions actionpack/lib/action_dispatch/http/response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,10 @@ def closed?
# The underlying body, as a streamable object.
attr_reader :stream

def initialize(status = 200, header = {}, body = [])
def initialize(status = 200, header = {}, body = [], default_headers: self.class.default_headers)
super()

header = merge_default_headers(header, self.class.default_headers)
header = merge_default_headers(header, default_headers)

self.body, self.header, self.status = body, header, status

Expand Down Expand Up @@ -308,9 +308,7 @@ def before_sending
end

def merge_default_headers(original, default)
return original unless default.respond_to?(:merge)

default.merge(original)
default.respond_to?(:merge) ? default.merge(original) : original
end

def build_buffer(response, body)
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_dispatch/testing/integration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ def process(method, path, params: nil, headers: nil, env: nil, xhr: false)
@request_count += 1
@request = ActionDispatch::Request.new(session.last_request.env)
response = _mock_session.last_response
@response = ActionDispatch::TestResponse.new(response.status, response.headers, response.body)
@response = ActionDispatch::TestResponse.from_response(response)
@html_document = nil
@url_options = nil

Expand Down
13 changes: 1 addition & 12 deletions actionpack/lib/action_dispatch/testing/test_response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,7 @@ module ActionDispatch
# See Response for more information on controller response objects.
class TestResponse < Response
def self.from_response(response)
new.tap do |resp|
resp.status = response.status
resp.headers = response.headers
resp.body = response.body
end
new response.status, response.headers, response.body, default_headers: nil
end

# Was the response successful?
Expand All @@ -25,12 +21,5 @@ def self.from_response(response)

# Was there a server-side error?
alias_method :error?, :server_error?

def merge_default_headers(original, *args)
# Default headers are already applied, no need to merge them a second time.
# This makes sure that default headers, removed in controller actions, will
# not be reapplied to the test response.
original
end
end
end
34 changes: 18 additions & 16 deletions actionpack/test/controller/integration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -397,9 +397,9 @@ def redirect
redirect_to action_url('get')
end

def remove_default_header
response.headers.except! 'X-Frame-Options'
head :ok
def remove_header
response.headers.delete params[:header]
head :ok, 'c' => '3'
end
end

Expand Down Expand Up @@ -640,25 +640,27 @@ def test_https_and_port_via_process
end
end

def test_removed_default_headers_on_test_response_are_not_reapplied
def test_respect_removal_of_default_headers_by_a_controller_action
with_test_route_set do
begin
header_to_remove = 'X-Frame-Options'
original_default_headers = ActionDispatch::Response.default_headers
ActionDispatch::Response.default_headers = {
'X-Content-Type-Options' => 'nosniff',
header_to_remove => 'SAMEORIGIN',
}
get '/remove_default_header'
assert_includes headers, 'X-Content-Type-Options'
assert_not_includes headers, header_to_remove, "Should not contain removed default header"
ensure
ActionDispatch::Response.default_headers = original_default_headers
with_default_headers 'a' => '1', 'b' => '2' do
get '/remove_header', params: { header: 'a' }
end
end

assert_not_includes @response.headers, 'a', 'Response should not include default header removed by the controller action'
assert_includes @response.headers, 'b'
assert_includes @response.headers, 'c'
end

private
def with_default_headers(headers)
original = ActionDispatch::Response.default_headers
ActionDispatch::Response.default_headers = headers
yield
ensure
ActionDispatch::Response.default_headers = original
end

def with_test_route_set
with_routing do |set|
controller = ::IntegrationProcessTest::IntegrationController.clone
Expand Down
46 changes: 46 additions & 0 deletions actionpack/test/controller/test_case_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -965,6 +965,52 @@ def test_redirect_url_only_cares_about_location_header
end
end

class ResponseDefaultHeadersTest < ActionController::TestCase
class TestController < ActionController::Base
def remove_header
headers.delete params[:header]
head :ok, 'C' => '3'
end
end

setup do
@original = ActionDispatch::Response.default_headers
@defaults = { 'A' => '1', 'B' => '2' }
ActionDispatch::Response.default_headers = @defaults
end

teardown do
ActionDispatch::Response.default_headers = @original
end

def setup
super
@controller = TestController.new
@request = ActionController::TestRequest.new
@response = ActionController::TestResponse.new
@request.env['PATH_INFO'] = nil
@routes = ActionDispatch::Routing::RouteSet.new.tap do |r|
r.draw do
get ':controller(/:action(/:id))'
end
end
end

test "response contains default headers" do
# Response headers start out with the defaults
assert_equal @defaults, response.headers

get :remove_header, params: { header: 'A' }
assert_response :ok

# After a request, the response in the test case doesn't have the
# defaults merged on top again.
assert_not_includes response.headers, 'A'
assert_includes response.headers, 'B'
assert_includes response.headers, 'C'
end
end

module EngineControllerTests
class Engine < ::Rails::Engine
isolate_namespace EngineControllerTests
Expand Down

3 comments on commit f6e293e

@eileencodes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeremy this is causing a segmentation fault on ActionPack tests. I did a bisect and found it was this commit. I haven't investigated yet as to what's causing it. You can see the failure in Travis here: https://travis-ci.org/rails/rails/jobs/52558658#L324

@eileencodes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeremy a test was missing the new default_headers argument. I fixed it in 22e0a22 - let me know if that was incorrect and I'll revert 😄

@eileencodes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: https://bugs.ruby-lang.org/issues/10685, I tested ruby 2.3.0-dev and this problem is fixed there

Please sign in to comment.