From 51c7ac142d31095d4c699f44cc44ddea627da1eb Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 25 Aug 2015 18:35:44 -0700 Subject: [PATCH] provide a request and response to all controllers Controllers should always have a request and response when responding. Since we make this The Rule(tm), then controllers don't need to be somewhere in limbo between "asking a response object for a rack response" or "I, myself contain a rack response". This duality leads to conditionals spread through the codebase that we can delete: * https://github.com/rails/rails/blob/85a78d9358aa728298cd020cdc842b55c16f9549/actionpack/lib/action_controller/metal.rb#L221-L223 --- actionpack/lib/action_controller/metal.rb | 37 ++++++++++++++----- .../lib/action_controller/metal/head.rb | 5 +-- .../metal/rack_delegation.rb | 17 --------- .../lib/action_dispatch/http/response.rb | 3 +- .../lib/action_dispatch/routing/route_set.rb | 10 +++-- actionpack/test/abstract_unit.rb | 8 +++- actionpack/test/controller/base_test.rb | 2 + .../controller/new_base/bare_metal_test.rb | 2 + .../controller/new_base/render_html_test.rb | 2 +- .../controller/new_base/render_plain_test.rb | 2 +- 10 files changed, 49 insertions(+), 39 deletions(-) diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb index 54980aa453..129e0bbd3c 100644 --- a/actionpack/lib/action_controller/metal.rb +++ b/actionpack/lib/action_controller/metal.rb @@ -132,6 +132,12 @@ def self.controller_name @controller_name ||= name.demodulize.sub(/Controller$/, '').underscore end + def self.make_response!(request) + ActionDispatch::Response.new.tap do |res| + res.request = request + end + end + # Delegates to the class' controller_name def controller_name self.class.controller_name @@ -143,11 +149,10 @@ def controller_name # and response object available. You might wish to control the # environment and response manually for performance reasons. - attr_internal :headers, :response, :request - delegate :session, :to => "@_request" + attr_internal :response, :request + delegate :session, :headers, :to => "@_request" def initialize - @_headers = {"Content-Type" => "text/html"} @_status = 200 @_request = nil @_response = nil @@ -168,7 +173,7 @@ def params=(val) # in Renderer and Redirector. def content_type=(type) - headers["Content-Type"] = type.to_s + response.content_type = type end def content_type @@ -199,6 +204,7 @@ def status=(status) def response_body=(body) body = [body] unless body.nil? || body.respond_to?(:each) + response.body = body super end @@ -207,12 +213,17 @@ def performed? response_body || (response && response.committed?) end - def dispatch(name, request) #:nodoc: + def dispatch(name, request, response) #:nodoc: set_request!(request) + set_response!(response) process(name) to_a end + def set_response!(response) # :nodoc: + @_response = response + end + def set_request!(request) #:nodoc: @_request = request @_request.controller_instance = self @@ -253,20 +264,26 @@ class << self; deprecate :call; end def self.action(name) if middleware_stack.any? middleware_stack.build(name) do |env| - new.dispatch(name, ActionDispatch::Request.new(env)) + req = ActionDispatch::Request.new(env) + res = make_response! req + new.dispatch(name, req, res) end else - lambda { |env| new.dispatch(name, ActionDispatch::Request.new(env)) } + lambda { |env| + req = ActionDispatch::Request.new(env) + res = make_response! req + new.dispatch(name, req, res) + } end end # Direct dispatch to the controller. Instantiates the controller, then # executes the action named +name+. - def self.dispatch(name, req) + def self.dispatch(name, req, res) if middleware_stack.any? - middleware_stack.build(name) { |env| new.dispatch(name, req) }.call req.env + middleware_stack.build(name) { |env| new.dispatch(name, req, res) }.call req.env else - new.dispatch(name, req) + new.dispatch(name, req, res) end end end diff --git a/actionpack/lib/action_controller/metal/head.rb b/actionpack/lib/action_controller/metal/head.rb index 056962b38c..7dbd5ef328 100644 --- a/actionpack/lib/action_controller/metal/head.rb +++ b/actionpack/lib/action_controller/metal/head.rb @@ -36,6 +36,8 @@ def head(status, options = {}) headers[key.to_s.dasherize.split('-').each { |v| v[0] = v[0].chr.upcase }.join('-')] = value.to_s end + response.status = Rack::Utils.status_code(status) + self.status = status self.location = url_for(location) if location @@ -44,9 +46,6 @@ def head(status, options = {}) if include_content?(self.response_code) self.content_type = content_type || (Mime[formats.first] if formats) self.response.charset = false if self.response - else - headers.delete('Content-Type') - headers.delete('Content-Length') end true diff --git a/actionpack/lib/action_controller/metal/rack_delegation.rb b/actionpack/lib/action_controller/metal/rack_delegation.rb index eb8bca1d92..5ba9a47d63 100644 --- a/actionpack/lib/action_controller/metal/rack_delegation.rb +++ b/actionpack/lib/action_controller/metal/rack_delegation.rb @@ -12,17 +12,6 @@ module ClassMethods def build_with_env(env = {}) #:nodoc: new.tap { |c| c.set_request! ActionDispatch::Request.new(env) } end - - def make_response!(request) - ActionDispatch::Response.new.tap do |res| - res.request = request - end - end - end - - def set_request!(request) #:nodoc: - super - set_response!(request) end def response_body=(body) @@ -33,11 +22,5 @@ def response_body=(body) def reset_session @_request.reset_session end - - private - - def set_response!(request) - @_response = self.class.make_response! request - end end end diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb index fd92e89231..c83b682f69 100644 --- a/actionpack/lib/action_dispatch/http/response.rb +++ b/actionpack/lib/action_dispatch/http/response.rb @@ -65,7 +65,7 @@ class Response CONTENT_TYPE = "Content-Type".freeze SET_COOKIE = "Set-Cookie".freeze LOCATION = "Location".freeze - NO_CONTENT_CODES = [204, 304] + NO_CONTENT_CODES = [100, 101, 102, 204, 205, 304] cattr_accessor(:default_charset) { "utf-8" } cattr_accessor(:default_headers) @@ -396,6 +396,7 @@ def rack_response(status, header) if NO_CONTENT_CODES.include?(@status) header.delete CONTENT_TYPE + header.delete 'Content-Length' [status, header, []] else [status, header, RackBody.new(self)] diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 3e3a424df3..e4b8d5993e 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -27,8 +27,10 @@ def initialize(raise_on_name_error) def dispatcher?; true; end def serve(req) - params = req.path_parameters - dispatch(controller(req), params[:action], req) + params = req.path_parameters + controller = controller req + res = controller.make_response! req + dispatch(controller, params[:action], req, res) rescue NameError => e if @raise_on_name_error raise ActionController::RoutingError, e.message, e.backtrace @@ -43,8 +45,8 @@ def controller(req) req.controller_class end - def dispatch(controller, action, req) - controller.dispatch(action, req) + def dispatch(controller, action, req, res) + controller.dispatch(action, req, res) end end diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index 39ae8cf899..1954324222 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -121,12 +121,16 @@ def self.build_app(routes = nil) class DeadEndRoutes < ActionDispatch::Routing::RouteSet # Stub Rails dispatcher so it does not get controller references and # simply return the controller#action as Rack::Body. - class NullController + class NullController < ::ActionController::Metal def initialize(controller_name) @controller = controller_name end - def dispatch(action, req) + def make_response!(request) + self.class.make_response! request + end + + def dispatch(action, req, res) [200, {'Content-Type' => 'text/html'}, ["#{@controller}##{action}"]] end end diff --git a/actionpack/test/controller/base_test.rb b/actionpack/test/controller/base_test.rb index d9374ce9c3..fb60dbd993 100644 --- a/actionpack/test/controller/base_test.rb +++ b/actionpack/test/controller/base_test.rb @@ -93,6 +93,8 @@ def test_no_deprecation_when_action_view_record_identifier_is_included class ControllerInstanceTests < ActiveSupport::TestCase def setup @empty = EmptyController.new + @empty.set_request!(ActionDispatch::Request.new({})) + @empty.set_response!(EmptyController.make_response!(@empty.request)) @contained = Submodule::ContainedEmptyController.new @empty_controllers = [@empty, @contained] end diff --git a/actionpack/test/controller/new_base/bare_metal_test.rb b/actionpack/test/controller/new_base/bare_metal_test.rb index 951674a399..77c9c13e96 100644 --- a/actionpack/test/controller/new_base/bare_metal_test.rb +++ b/actionpack/test/controller/new_base/bare_metal_test.rb @@ -28,6 +28,8 @@ class BareTest < ActiveSupport::TestCase test "response_body value is wrapped in an array when the value is a String" do controller = BareController.new + controller.set_request!(ActionDispatch::Request.new({})) + controller.set_response!(BareController.make_response!(controller.request)) controller.index assert_equal ["Hello world"], controller.response_body end diff --git a/actionpack/test/controller/new_base/render_html_test.rb b/actionpack/test/controller/new_base/render_html_test.rb index fe11501eeb..e9ea57e329 100644 --- a/actionpack/test/controller/new_base/render_html_test.rb +++ b/actionpack/test/controller/new_base/render_html_test.rb @@ -179,7 +179,7 @@ class RenderHtmlTest < Rack::TestCase test "rendering from minimal controller returns response with text/html content type" do get "/render_html/minimal/index" - assert_content_type "text/html" + assert_content_type "text/html; charset=utf-8" end test "rendering from normal controller returns response with text/html content type" do diff --git a/actionpack/test/controller/new_base/render_plain_test.rb b/actionpack/test/controller/new_base/render_plain_test.rb index 0e36d36b50..0881442bd0 100644 --- a/actionpack/test/controller/new_base/render_plain_test.rb +++ b/actionpack/test/controller/new_base/render_plain_test.rb @@ -157,7 +157,7 @@ class RenderPlainTest < Rack::TestCase test "rendering from minimal controller returns response with text/plain content type" do get "/render_plain/minimal/index" - assert_content_type "text/plain" + assert_content_type "text/plain; charset=utf-8" end test "rendering from normal controller returns response with text/plain content type" do -- GitLab