diff options
Diffstat (limited to 'actionpack')
19 files changed, 124 insertions, 111 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index e250450a76..de9722c392 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,16 @@ +* Deprecate implicit Array conversion for Response objects. It was added + (using `#to_ary`) so we could conveniently use implicit splatting: + + status, headers, body = response + + But it also means `response + response` works and `[response].flatten` + cascades down to the Rack body. Nonsense behavior. Instead, rely on + explicit conversion and splatting with `#to_a`: + + status, header, body = *response + + *Jeremy Kemper* + * Don't rescue `IPAddr::InvalidAddressError`. `IPAddr::InvalidAddressError` does not exist in Ruby 1.9.3 diff --git a/actionpack/Rakefile b/actionpack/Rakefile index cf20751687..af075f82ad 100644 --- a/actionpack/Rakefile +++ b/actionpack/Rakefile @@ -9,7 +9,10 @@ task :default => :test # Run the unit tests Rake::TestTask.new do |t| t.libs << 'test' - t.test_files = test_files + + # make sure we include the tests in alphabetical order as on some systems + # this will not happen automatically and the tests (as a whole) will error + t.test_files = test_files.sort t.warning = true t.verbose = true diff --git a/actionpack/actionpack.gemspec b/actionpack/actionpack.gemspec index a39b3e86d4..69a160fdc8 100644 --- a/actionpack/actionpack.gemspec +++ b/actionpack/actionpack.gemspec @@ -23,8 +23,8 @@ Gem::Specification.new do |s| s.add_dependency 'rack', '~> 1.6.0.beta' s.add_dependency 'rack-test', '~> 0.6.2' - s.add_dependency 'rails-deprecated_sanitizer', '~> 1.0', '>= 1.0.2' - s.add_dependency 'rails-dom-testing', '~> 1.0', '>= 1.0.2' + s.add_dependency 'rails-html-sanitizer', '~> 1.0', '>= 1.0.1' + s.add_dependency 'rails-dom-testing', '~> 1.0', '>= 1.0.3' s.add_dependency 'actionview', version s.add_development_dependency 'activemodel', version diff --git a/actionpack/lib/action_controller/metal/renderers.rb b/actionpack/lib/action_controller/metal/renderers.rb index 02c4e563f5..bc94536c8c 100644 --- a/actionpack/lib/action_controller/metal/renderers.rb +++ b/actionpack/lib/action_controller/metal/renderers.rb @@ -34,14 +34,15 @@ module ActionController end def render_to_body(options) - _handle_render_options(options) || super + _render_to_body_with_renderer(options) || super end - def _handle_render_options(options) + def _render_to_body_with_renderer(options) _renderers.each do |name| if options.key?(name) _process_options(options) - return send("_render_option_#{name}", options.delete(name), options) + method_name = Renderers._render_with_renderer_method_name(name) + return send(method_name, options.delete(name), options) end end nil @@ -51,6 +52,10 @@ module ActionController # Default values are <tt>:json</tt>, <tt>:js</tt>, <tt>:xml</tt>. RENDERERS = Set.new + def self._render_with_renderer_method_name(key) + "_render_with_renderer_#{key}" + end + # Adds a new renderer to call within controller actions. # A renderer is invoked by passing its name as an option to # <tt>AbstractController::Rendering#render</tt>. To create a renderer @@ -84,7 +89,7 @@ module ActionController # <tt>ActionController::MimeResponds::ClassMethods.respond_to</tt> and # <tt>ActionController::MimeResponds#respond_with</tt> def self.add(key, &block) - define_method("_render_option_#{key}", &block) + define_method(_render_with_renderer_method_name(key), &block) RENDERERS << key.to_sym end @@ -95,8 +100,8 @@ module ActionController # ActionController::Renderers.remove(:csv) def self.remove(key) RENDERERS.delete(key.to_sym) - method = "_render_option_#{key}" - remove_method(method) if method_defined?(method) + method_name = _render_with_renderer_method_name(key) + remove_method(method_name) if method_defined?(method_name) end module All @@ -112,7 +117,7 @@ module ActionController json = json.to_json(options) unless json.kind_of?(String) if options[:callback].present? - if self.content_type.nil? || self.content_type == Mime::JSON + if content_type.nil? || content_type == Mime::JSON self.content_type = Mime::JS end diff --git a/actionpack/lib/action_dispatch/http/mime_type.rb b/actionpack/lib/action_dispatch/http/mime_type.rb index 9450be838c..b9d5009683 100644 --- a/actionpack/lib/action_dispatch/http/mime_type.rb +++ b/actionpack/lib/action_dispatch/http/mime_type.rb @@ -45,8 +45,8 @@ module Mime # # respond_to do |format| # format.html - # format.ics { render text: post.to_ics, mime_type: Mime::Type["text/calendar"] } - # format.xml { render xml: @people } + # format.ics { render text: @post.to_ics, mime_type: Mime::Type["text/calendar"] } + # format.xml { render xml: @post } # end # end # end diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index c441f7f95b..e854dd266c 100644 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -298,7 +298,7 @@ module ActionDispatch # Override Rack's GET method to support indifferent access def GET @env["action_dispatch.request.query_parameters"] ||= Utils.deep_munge(normalize_encode_params(super || {})) - rescue TypeError, Rack::Utils::InvalidParameterError => e + rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e raise ActionController::BadRequest.new(:query, e) end alias :query_parameters :GET @@ -306,7 +306,7 @@ module ActionDispatch # Override Rack's POST method to support indifferent access def POST @env["action_dispatch.request.request_parameters"] ||= Utils.deep_munge(normalize_encode_params(super || {})) - rescue TypeError, Rack::Utils::InvalidParameterError => e + rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e raise ActionController::BadRequest.new(:request, e) end alias :request_parameters :POST diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb index 2fab6be1a5..c5e18048da 100644 --- a/actionpack/lib/action_dispatch/http/response.rb +++ b/actionpack/lib/action_dispatch/http/response.rb @@ -1,4 +1,5 @@ require 'active_support/core_ext/module/attribute_accessors' +require 'active_support/deprecation' require 'action_dispatch/http/filter_redirect' require 'monitor' @@ -274,12 +275,22 @@ module ActionDispatch # :nodoc: end # Turns the Response into a Rack-compatible array of the status, headers, - # and body. + # and body. Allows explict splatting: + # + # status, headers, body = *response def to_a rack_response @status, @header.to_hash end alias prepare! to_a - alias to_ary to_a + + # Be super clear that a response object is not an Array. Defining this + # would make implicit splatting work, but it also makes adding responses + # as arrays work, and "flattening" responses, cascading to the rack body! + # Not sensible behavior. + def to_ary + ActiveSupport::Deprecation.warn 'ActionDispatch::Response#to_ary no longer performs implicit conversion to an Array. Please use response.to_a instead, or a splat like `status, headers, body = *response`' + to_a + end # Returns the response cookies, converted to a Hash of (name => value) pairs # @@ -369,6 +380,10 @@ module ActionDispatch # :nodoc: def to_path @response.stream.to_path end + + def to_ary + nil + end end def rack_response(status, header) diff --git a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb index f15868d37e..0847842fa2 100644 --- a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb +++ b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb @@ -116,7 +116,6 @@ module ActionDispatch action, type, opts - end # Returns the path component of a URL for the given record. It uses @@ -159,8 +158,7 @@ module ActionDispatch end def polymorphic_path_for_action(action, record_or_hash, options) - options = options.merge(:action => action, :routing_type => :path) - polymorphic_path(record_or_hash, options) + polymorphic_path(record_or_hash, options.merge(:action => action)) end class HelperMethodBuilder # :nodoc: diff --git a/actionpack/lib/action_dispatch/testing/assertions/routing.rb b/actionpack/lib/action_dispatch/testing/assertions/routing.rb index 2cf38a9c2d..e06f7037c6 100644 --- a/actionpack/lib/action_dispatch/testing/assertions/routing.rb +++ b/actionpack/lib/action_dispatch/testing/assertions/routing.rb @@ -38,7 +38,7 @@ module ActionDispatch # # Test a custom route # assert_recognizes({controller: 'items', action: 'show', id: '1'}, 'view/item1') def assert_recognizes(expected_options, path, extras={}, msg=nil) - request = recognized_request_for(path, extras) + request = recognized_request_for(path, extras, msg) expected_options = expected_options.clone @@ -69,9 +69,9 @@ module ActionDispatch # # # Asserts that the generated route gives us our custom route # assert_generates "changesets/12", { controller: 'scm', action: 'show_diff', revision: "12" } - def assert_generates(expected_path, options, defaults={}, extras = {}, message=nil) + def assert_generates(expected_path, options, defaults={}, extras={}, message=nil) if expected_path =~ %r{://} - fail_on(URI::InvalidURIError) do + fail_on(URI::InvalidURIError, message) do uri = URI.parse(expected_path) expected_path = uri.path.to_s.empty? ? "/" : uri.path end @@ -174,7 +174,7 @@ module ActionDispatch private # Recognizes the route for a given path. - def recognized_request_for(path, extras = {}) + def recognized_request_for(path, extras = {}, msg) if path.is_a?(Hash) method = path[:method] path = path[:path] @@ -186,7 +186,7 @@ module ActionDispatch request = ActionController::TestRequest.new if path =~ %r{://} - fail_on(URI::InvalidURIError) do + fail_on(URI::InvalidURIError, msg) do uri = URI.parse(path) request.env["rack.url_scheme"] = uri.scheme || "http" request.host = uri.host if uri.host @@ -200,7 +200,7 @@ module ActionDispatch request.request_method = method if method - params = fail_on(ActionController::RoutingError) do + params = fail_on(ActionController::RoutingError, msg) do @routes.recognize_path(path, { :method => method, :extras => extras }) end request.path_parameters = params.with_indifferent_access @@ -208,10 +208,10 @@ module ActionDispatch request end - def fail_on(exception_class) + def fail_on(exception_class, message) yield rescue exception_class => e - raise Minitest::Assertion, e.message + raise Minitest::Assertion, message || e.message end end end diff --git a/actionpack/lib/action_dispatch/testing/assertions/selector.rb b/actionpack/lib/action_dispatch/testing/assertions/selector.rb index 19eca60f70..7361e6c44b 100644 --- a/actionpack/lib/action_dispatch/testing/assertions/selector.rb +++ b/actionpack/lib/action_dispatch/testing/assertions/selector.rb @@ -1,3 +1,3 @@ require 'active_support/deprecation' -ActiveSupport::Deprecation.warn("ActionDispatch::Assertions::SelectorAssertions has been has been extracted to the rails-dom-testing gem.")
\ No newline at end of file +ActiveSupport::Deprecation.warn("ActionDispatch::Assertions::SelectorAssertions has been extracted to the rails-dom-testing gem.") diff --git a/actionpack/lib/action_dispatch/testing/assertions/tag.rb b/actionpack/lib/action_dispatch/testing/assertions/tag.rb index d5348d80e1..5c2905d1ac 100644 --- a/actionpack/lib/action_dispatch/testing/assertions/tag.rb +++ b/actionpack/lib/action_dispatch/testing/assertions/tag.rb @@ -1,3 +1,3 @@ require 'active_support/deprecation' -ActiveSupport::Deprecation.warn("ActionDispatch::Assertions::TagAssertions has been has been extracted to the rails-dom-testing gem.") +ActiveSupport::Deprecation.warn("ActionDispatch::Assertions::TagAssertions has been extracted to the rails-dom-testing gem.") diff --git a/actionpack/lib/action_pack/gem_version.rb b/actionpack/lib/action_pack/gem_version.rb index 280d35adcb..6834845d4f 100644 --- a/actionpack/lib/action_pack/gem_version.rb +++ b/actionpack/lib/action_pack/gem_version.rb @@ -8,7 +8,7 @@ module ActionPack MAJOR = 4 MINOR = 2 TINY = 0 - PRE = "beta1" + PRE = "beta2" STRING = [MAJOR, MINOR, TINY, PRE].compact.join(".") end diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index 799c17119d..69312e4c22 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -516,4 +516,4 @@ end # FIXME: we have tests that depend on run order, we should fix that and # remove this method call. require 'active_support/test_case' -ActiveSupport::TestCase.my_tests_are_order_dependent! +ActiveSupport::TestCase.test_order = :sorted diff --git a/actionpack/test/controller/new_base/render_file_test.rb b/actionpack/test/controller/new_base/render_file_test.rb index a961cbf849..0c21bb0719 100644 --- a/actionpack/test/controller/new_base/render_file_test.rb +++ b/actionpack/test/controller/new_base/render_file_test.rb @@ -13,15 +13,6 @@ module RenderFile render :file => File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_ivar') end - def without_file_key - render File.join(File.dirname(__FILE__), *%w[.. .. fixtures test hello_world]) - end - - def without_file_key_with_instance_variable - @secret = 'in the sauce' - render File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_ivar') - end - def relative_path @secret = 'in the sauce' render :file => '../../fixtures/test/render_file_with_ivar' @@ -41,11 +32,6 @@ module RenderFile path = File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_locals') render :file => path, :locals => {:secret => 'in the sauce'} end - - def without_file_key_with_locals - path = FIXTURES.join('test/render_file_with_locals').to_s - render path, :locals => {:secret => 'in the sauce'} - end end class TestBasic < Rack::TestCase @@ -61,16 +47,6 @@ module RenderFile assert_response "The secret is in the sauce\n" end - test "rendering path without specifying the :file key" do - get :without_file_key - assert_response "Hello world!" - end - - test "rendering path without specifying the :file key with ivar" do - get :without_file_key_with_instance_variable - assert_response "The secret is in the sauce\n" - end - test "rendering a relative path" do get :relative_path assert_response "The secret is in the sauce\n" @@ -90,10 +66,5 @@ module RenderFile get :with_locals assert_response "The secret is in the sauce\n" end - - test "rendering path without specifying the :file key with locals" do - get :without_file_key_with_locals - assert_response "The secret is in the sauce\n" - end end end diff --git a/actionpack/test/controller/new_base/render_template_test.rb b/actionpack/test/controller/new_base/render_template_test.rb index e87811776a..42a86b1d0d 100644 --- a/actionpack/test/controller/new_base/render_template_test.rb +++ b/actionpack/test/controller/new_base/render_template_test.rb @@ -45,6 +45,10 @@ module RenderTemplate render :template => "locals", :locals => { :secret => 'area51' } end + def with_locals_without_key + render "locals", :locals => { :secret => 'area51' } + end + def builder_template render :template => "xml_template" end @@ -101,6 +105,11 @@ module RenderTemplate assert_response "The secret is area51" end + test "rendering a template with local variables without key" do + get :with_locals + assert_response "The secret is area51" + end + test "rendering a builder template" do get :builder_template, "format" => "xml" assert_response "<html>\n <p>Hello</p>\n</html>\n" diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb index 00b80c7357..a58306ea0a 100644 --- a/actionpack/test/dispatch/request_test.rb +++ b/actionpack/test/dispatch/request_test.rb @@ -915,7 +915,7 @@ class RequestParameters < BaseRequestTest 2.times do assert_raises(ActionController::BadRequest) do - # rack will raise a TypeError when parsing this query string + # rack will raise a Rack::Utils::ParameterTypeError when parsing this query string request.parameters end end @@ -941,7 +941,7 @@ class RequestParameters < BaseRequestTest ) assert_raises(ActionController::BadRequest) do - # rack will raise a TypeError when parsing this query string + # rack will raise a Rack::Utils::ParameterTypeError when parsing this query string request.parameters end end @@ -950,7 +950,7 @@ class RequestParameters < BaseRequestTest request = stub_request("QUERY_STRING" => "x[y]=1&x[y][][w]=2") e = assert_raises(ActionController::BadRequest) do - # rack will raise a TypeError when parsing this query string + # rack will raise a Rack::Utils::ParameterTypeError when parsing this query string request.parameters end diff --git a/actionpack/test/dispatch/response_test.rb b/actionpack/test/dispatch/response_test.rb index 187b9a2420..48342e252a 100644 --- a/actionpack/test/dispatch/response_test.rb +++ b/actionpack/test/dispatch/response_test.rb @@ -1,4 +1,6 @@ require 'abstract_unit' +require 'timeout' +require 'rack/content_length' class ResponseTest < ActiveSupport::TestCase def setup @@ -220,9 +222,9 @@ class ResponseTest < ActiveSupport::TestCase assert @response.respond_to?(:method_missing, true) end - test "can be destructured into status, headers and an enumerable body" do + test "can be explicitly destructured into status, headers and an enumerable body" do response = ActionDispatch::Response.new(404, { 'Content-Type' => 'text/plain' }, ['Not Found']) - status, headers, body = response + status, headers, body = *response assert_equal 404, status assert_equal({ 'Content-Type' => 'text/plain' }, headers) @@ -231,12 +233,38 @@ class ResponseTest < ActiveSupport::TestCase test "[response].flatten does not recurse infinitely" do Timeout.timeout(1) do # use a timeout to prevent it stalling indefinitely - status, headers, body = [@response].flatten + status, headers, body = assert_deprecated { [@response].flatten } assert_equal @response.status, status assert_equal @response.headers, headers assert_equal @response.body, body.each.to_a.join end end + + test "compatibility with Rack::ContentLength" do + @response.body = 'Hello' + app = lambda { |env| @response.to_a } + env = Rack::MockRequest.env_for("/") + + status, headers, body = app.call(env) + assert_nil headers['Content-Length'] + + status, headers, body = Rack::ContentLength.new(app).call(env) + assert_equal '5', headers['Content-Length'] + end + + test "implicit destructuring and Array conversion is deprecated" do + response = ActionDispatch::Response.new(404, { 'Content-Type' => 'text/plain' }, ['Not Found']) + + assert_deprecated do + status, headers, body = response + + assert_equal 404, status + assert_equal({ 'Content-Type' => 'text/plain' }, headers) + assert_equal ['Not Found'], body.each.to_a + end + + assert_deprecated { response.to_ary } + end end class ResponseIntegrationTest < ActionDispatch::IntegrationTest diff --git a/actionpack/test/dispatch/routing_assertions_test.rb b/actionpack/test/dispatch/routing_assertions_test.rb index aea4489852..56ea644f22 100644 --- a/actionpack/test/dispatch/routing_assertions_test.rb +++ b/actionpack/test/dispatch/routing_assertions_test.rb @@ -74,10 +74,26 @@ class RoutingAssertionsTest < ActionController::TestCase assert_recognizes({ :controller => 'query_articles', :action => 'index', :use_query => 'true' }, '/query/articles', { :use_query => 'true' }) end + def test_assert_recognizes_raises_message + err = assert_raise(Assertion) do + assert_recognizes({ :controller => 'secure_articles', :action => 'index' }, 'http://test.host/secure/articles', {}, "This is a really bad msg") + end + + assert_match err.message, "This is a really bad msg" + end + def test_assert_routing assert_routing('/articles', :controller => 'articles', :action => 'index') end + def test_assert_routing_raises_message + err = assert_raise(Assertion) do + assert_routing('/thisIsNotARoute', { :controller => 'articles', :action => 'edit', :id => '1' }, { :id => '1' }, {}, "This is a really bad msg") + end + + assert_match err.message, "This is a really bad msg" + end + def test_assert_routing_with_defaults assert_routing('/articles/1/edit', { :controller => 'articles', :action => 'edit', :id => '1' }, { :id => '1' }) end diff --git a/actionpack/test/lib/controller/fake_models.rb b/actionpack/test/lib/controller/fake_models.rb index b8b51d86c2..ce9522d12a 100644 --- a/actionpack/test/lib/controller/fake_models.rb +++ b/actionpack/test/lib/controller/fake_models.rb @@ -28,30 +28,6 @@ class Customer < Struct.new(:name, :id) end end -class ValidatedCustomer < Customer - def errors - if name =~ /Sikachu/i - [] - else - [{:name => "is invalid"}] - end - end -end - -module Quiz - class Question < Struct.new(:name, :id) - extend ActiveModel::Naming - include ActiveModel::Conversion - - def persisted? - id.present? - end - end - - class Store < Question - end -end - class Post < Struct.new(:title, :author_name, :body, :secret, :persisted, :written_on, :cost) extend ActiveModel::Naming include ActiveModel::Conversion @@ -95,24 +71,3 @@ class Comment attr_accessor :body end - -module Blog - def self.use_relative_model_naming? - true - end - - class Post < Struct.new(:title, :id) - extend ActiveModel::Naming - include ActiveModel::Conversion - - def persisted? - id.present? - end - end -end - -class RenderJsonTestException < Exception - def as_json(options = nil) - { :error => self.class.name, :message => self.to_s } - end -end |