From f4f68885efd0e1135217433cafd368902b1fd58a Mon Sep 17 00:00:00 2001 From: Taryn East Date: Fri, 2 Oct 2009 10:13:01 -0500 Subject: [PATCH 01/61] update_attribute(s) added to Active Resource Signed-off-by: Joshua Peek --- activeresource/lib/active_resource/base.rb | 30 ++++++++++++ activeresource/test/cases/base_test.rb | 53 +++++++++++++++++++++- 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/activeresource/lib/active_resource/base.rb b/activeresource/lib/active_resource/base.rb index e5b8589fb3..8fc0999c70 100644 --- a/activeresource/lib/active_resource/base.rb +++ b/activeresource/lib/active_resource/base.rb @@ -1099,6 +1099,36 @@ module ActiveResource self end + # Updates a single attribute and then saves the object. + # + # Note: Unlike ActiveRecord::Base.update_attribute, this method is + # subject to normal validation routines as an update sends the whole body + # of the resource in the request. (See Validations). + # + # As such, this method is equivalent to calling update_attributes with a single attribute/value pair. + # + # If the saving fails because of a connection or remote service error, an + # exception will be raised. If saving fails because the resource is + # invalid then false will be returned. + def update_attribute(name, value) + self.send("#{name}=".to_sym, value) + self.save + end + + + # Updates this resource with all the attributes from the passed-in Hash + # and requests that the record be saved. + # + # If the saving fails because of a connection or remote service error, an + # exception will be raised. If saving fails because the resource is + # invalid then false will be returned. + # + # Note: Though this request can be made with a partial set of the + # resource's attributes, the full body of the request will still be sent + # in the save request to the remote service. + def update_attributes(attributes) + load(attributes) && save + end # For checking respond_to? without searching the attributes (which is faster). alias_method :respond_to_without_attributes?, :respond_to? diff --git a/activeresource/test/cases/base_test.rb b/activeresource/test/cases/base_test.rb index 8c0217aad6..96fc6fc2fe 100644 --- a/activeresource/test/cases/base_test.rb +++ b/activeresource/test/cases/base_test.rb @@ -11,12 +11,12 @@ class BaseTest < Test::Unit::TestCase @matz = { :id => 1, :name => 'Matz' }.to_xml(:root => 'person') @david = { :id => 2, :name => 'David' }.to_xml(:root => 'person') @greg = { :id => 3, :name => 'Greg' }.to_xml(:root => 'person') - @addy = { :id => 1, :street => '12345 Street' }.to_xml(:root => 'address') + @addy = { :id => 1, :street => '12345 Street', :country => 'Australia' }.to_xml(:root => 'address') @default_request_headers = { 'Content-Type' => 'application/xml' } @rick = { :name => "Rick", :age => 25 }.to_xml(:root => "person") @people = [{ :id => 1, :name => 'Matz' }, { :id => 2, :name => 'David' }].to_xml(:root => 'people') @people_david = [{ :id => 2, :name => 'David' }].to_xml(:root => 'people') - @addresses = [{ :id => 1, :street => '12345 Street' }].to_xml(:root => 'addresses') + @addresses = [{ :id => 1, :street => '12345 Street', :country => 'Australia' }].to_xml(:root => 'addresses') # - deep nested resource - # - Luis (Customer) @@ -813,6 +813,55 @@ class BaseTest < Test::Unit::TestCase assert_raise(ActiveResource::ResourceConflict) { Person.find(2).save } end + + ###### + # update_attribute(s)(!) + + def test_update_attribute_as_symbol + matz = Person.first + matz.expects(:save).returns(true) + + assert_equal "Matz", matz.name + assert matz.update_attribute(:name, "David") + assert_equal "David", matz.name + end + + def test_update_attribute_as_string + matz = Person.first + matz.expects(:save).returns(true) + + assert_equal "Matz", matz.name + assert matz.update_attribute('name', "David") + assert_equal "David", matz.name + end + + + def test_update_attributes_as_symbols + addy = StreetAddress.first(:params => {:person_id => 1}) + addy.expects(:save).returns(true) + + assert_equal "12345 Street", addy.street + assert_equal "Australia", addy.country + assert addy.update_attributes(:street => '54321 Street', :country => 'USA') + assert_equal "54321 Street", addy.street + assert_equal "USA", addy.country + end + + def test_update_attributes_as_strings + addy = StreetAddress.first(:params => {:person_id => 1}) + addy.expects(:save).returns(true) + + assert_equal "12345 Street", addy.street + assert_equal "Australia", addy.country + assert addy.update_attributes('street' => '54321 Street', 'country' => 'USA') + assert_equal "54321 Street", addy.street + assert_equal "USA", addy.country + end + + + ##### + # Mayhem and destruction + def test_destroy assert Person.find(1).destroy ActiveResource::HttpMock.respond_to do |mock| From 8377646d68b32de362fefad0d752a923f6b36da6 Mon Sep 17 00:00:00 2001 From: Taryn East Date: Fri, 2 Oct 2009 10:13:40 -0500 Subject: [PATCH 02/61] add indifferent access to the attributes Signed-off-by: Joshua Peek --- activeresource/lib/active_resource/base.rb | 3 ++- activeresource/test/cases/base/load_test.rb | 13 +++++++++++++ activeresource/test/cases/base_test.rb | 15 +++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/activeresource/lib/active_resource/base.rb b/activeresource/lib/active_resource/base.rb index 8fc0999c70..3a07811f26 100644 --- a/activeresource/lib/active_resource/base.rb +++ b/activeresource/lib/active_resource/base.rb @@ -1,6 +1,7 @@ require 'active_support' require 'active_support/core_ext/class/attribute_accessors' require 'active_support/core_ext/class/inheritable_attributes' +require 'active_support/core_ext/hash/indifferent_access' require 'active_support/core_ext/kernel/reporting' require 'active_support/core_ext/module/attr_accessor_with_default' require 'active_support/core_ext/module/delegation' @@ -770,7 +771,7 @@ module ActiveResource # my_other_course = Course.new(:name => "Philosophy: Reason and Being", :lecturer => "Ralph Cling") # my_other_course.save def initialize(attributes = {}) - @attributes = {} + @attributes = {}.with_indifferent_access @prefix_options = {} load(attributes) end diff --git a/activeresource/test/cases/base/load_test.rb b/activeresource/test/cases/base/load_test.rb index 1952f5b5f0..53fa94d07f 100644 --- a/activeresource/test/cases/base/load_test.rb +++ b/activeresource/test/cases/base/load_test.rb @@ -68,6 +68,19 @@ class BaseLoadTest < Test::Unit::TestCase assert_equal @matz.stringify_keys, @person.load(@matz).attributes end + def test_after_load_attributes_are_accessible + assert_equal Hash.new, @person.attributes + assert_equal @matz.stringify_keys, @person.load(@matz).attributes + assert_equal @matz[:name], @person.attributes['name'] + end + + def test_after_load_attributes_are_accessible_via_indifferent_access + assert_equal Hash.new, @person.attributes + assert_equal @matz.stringify_keys, @person.load(@matz).attributes + assert_equal @matz[:name], @person.attributes['name'] + assert_equal @matz[:name], @person.attributes[:name] + end + def test_load_one_with_existing_resource address = @person.load(:street_address => @first_address).street_address assert_kind_of StreetAddress, address diff --git a/activeresource/test/cases/base_test.rb b/activeresource/test/cases/base_test.rb index 96fc6fc2fe..fc366430d4 100644 --- a/activeresource/test/cases/base_test.rb +++ b/activeresource/test/cases/base_test.rb @@ -102,6 +102,9 @@ class BaseTest < Test::Unit::TestCase Person.password = nil end + ######################################################################## + # Tests relating to setting up the API-connection configuration + ######################################################################## def test_site_accessor_accepts_uri_or_string_argument site = URI.parse('http://localhost') @@ -509,6 +512,11 @@ class BaseTest < Test::Unit::TestCase assert_not_equal(first_connection, second_connection, 'Connection should be re-created') end + + ######################################################################## + # Tests for setting up remote URLs for a given model (including adding + # parameters appropriately) + ######################################################################## def test_collection_name assert_equal "people", Person.collection_name end @@ -637,6 +645,10 @@ class BaseTest < Test::Unit::TestCase assert_equal [:person_id].to_set, StreetAddress.__send__(:prefix_parameters) end + + ######################################################################## + # Tests basic CRUD functions (find/save/create etc) + ######################################################################## def test_respond_to matz = Person.find(1) assert matz.respond_to?(:name) @@ -910,6 +922,9 @@ class BaseTest < Test::Unit::TestCase assert_raise(ActiveResource::ResourceGone) { Person.find(1) } end + ######################################################################## + # Tests the more miscelaneous helper methods + ######################################################################## def test_exists # Class method. assert !Person.exists?(nil) From 89630a7c2c3d0f625742e2b84ec28479ff3ecef9 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Fri, 2 Oct 2009 10:19:30 -0500 Subject: [PATCH 03/61] Cleanup whitespace introduced in 8377646 and f4f6888 --- activeresource/lib/active_resource/base.rb | 69 ++++++++++----------- activeresource/test/cases/base/load_test.rb | 25 +++----- activeresource/test/cases/base_test.rb | 8 +-- 3 files changed, 47 insertions(+), 55 deletions(-) diff --git a/activeresource/lib/active_resource/base.rb b/activeresource/lib/active_resource/base.rb index 3a07811f26..b21d8db613 100644 --- a/activeresource/lib/active_resource/base.rb +++ b/activeresource/lib/active_resource/base.rb @@ -156,7 +156,7 @@ module ActiveResource # # 404 is just one of the HTTP error response codes that Active Resource will handle with its own exception. The # following HTTP response codes will also result in these exceptions: - # + # # * 200..399 - Valid response, no exception (other than 301, 302) # * 301, 302 - ActiveResource::Redirection # * 400 - ActiveResource::BadRequest @@ -415,7 +415,7 @@ module ActiveResource attr_accessor_with_default(:collection_name) { ActiveSupport::Inflector.pluralize(element_name) } #:nodoc: attr_accessor_with_default(:primary_key, 'id') #:nodoc: - + # Gets the \prefix for a resource's nested URL (e.g., prefix/collectionname/1.xml) # This method is regenerated at runtime based on what the \prefix is set to. def prefix(options={}) @@ -916,7 +916,7 @@ module ActiveResource def save new? ? create : update end - + # Saves the resource. # # If the resource is new, it is created via +POST+, otherwise the @@ -925,7 +925,7 @@ module ActiveResource # With save! validations always run. If any of them fail # ActiveResource::ResourceInvalid gets raised, and nothing is POSTed to # the remote system. - # See ActiveResource::Validations for more information. + # See ActiveResource::Validations for more information. # # There's a series of callbacks associated with save!. If any # of the before_* callbacks return +false+ the action is @@ -1100,36 +1100,36 @@ module ActiveResource self end - # Updates a single attribute and then saves the object. - # - # Note: Unlike ActiveRecord::Base.update_attribute, this method is - # subject to normal validation routines as an update sends the whole body - # of the resource in the request. (See Validations). - # - # As such, this method is equivalent to calling update_attributes with a single attribute/value pair. - # - # If the saving fails because of a connection or remote service error, an - # exception will be raised. If saving fails because the resource is - # invalid then false will be returned. - def update_attribute(name, value) - self.send("#{name}=".to_sym, value) - self.save - end - - - # Updates this resource with all the attributes from the passed-in Hash - # and requests that the record be saved. - # - # If the saving fails because of a connection or remote service error, an - # exception will be raised. If saving fails because the resource is - # invalid then false will be returned. - # - # Note: Though this request can be made with a partial set of the - # resource's attributes, the full body of the request will still be sent - # in the save request to the remote service. - def update_attributes(attributes) - load(attributes) && save - end + # Updates a single attribute and then saves the object. + # + # Note: Unlike ActiveRecord::Base.update_attribute, this method is + # subject to normal validation routines as an update sends the whole body + # of the resource in the request. (See Validations). + # + # As such, this method is equivalent to calling update_attributes with a single attribute/value pair. + # + # If the saving fails because of a connection or remote service error, an + # exception will be raised. If saving fails because the resource is + # invalid then false will be returned. + def update_attribute(name, value) + self.send("#{name}=".to_sym, value) + self.save + end + + # Updates this resource with all the attributes from the passed-in Hash + # and requests that the record be saved. + # + # If the saving fails because of a connection or remote service error, an + # exception will be raised. If saving fails because the resource is + # invalid then false will be returned. + # + # Note: Though this request can be made with a partial set of the + # resource's attributes, the full body of the request will still be sent + # in the save request to the remote service. + def update_attributes(attributes) + load(attributes) && save + end + # For checking respond_to? without searching the attributes (which is faster). alias_method :respond_to_without_attributes?, :respond_to? @@ -1150,7 +1150,6 @@ module ActiveResource super end - protected def connection(refresh = false) self.class.connection(refresh) diff --git a/activeresource/test/cases/base/load_test.rb b/activeresource/test/cases/base/load_test.rb index 53fa94d07f..189a4d81fe 100644 --- a/activeresource/test/cases/base/load_test.rb +++ b/activeresource/test/cases/base/load_test.rb @@ -15,26 +15,21 @@ module Highrise module Deeply module Nested - class Note < ActiveResource::Base self.site = "http://37s.sunrise.i:3000" end - class Comment < ActiveResource::Base - self.site = "http://37s.sunrise.i:3000" - end - - module TestDifferentLevels - - class Note < ActiveResource::Base - self.site = "http://37s.sunrise.i:3000" - end - - end + class Comment < ActiveResource::Base + self.site = "http://37s.sunrise.i:3000" + end + module TestDifferentLevels + class Note < ActiveResource::Base + self.site = "http://37s.sunrise.i:3000" + end + end end end - end @@ -156,7 +151,7 @@ class BaseLoadTest < Test::Unit::TestCase assert_kind_of String, places.first assert_equal @deep[:street][:state][:places].first, places.first end - + def test_nested_collections_within_the_same_namespace n = Highrise::Note.new(:comments => [{ :name => "1" }]) assert_kind_of Highrise::Comment, n.comments.first @@ -171,6 +166,4 @@ class BaseLoadTest < Test::Unit::TestCase n = Highrise::Deeply::Nested::TestDifferentLevels::Note.new(:comments => [{ :name => "1" }]) assert_kind_of Highrise::Deeply::Nested::Comment, n.comments.first end - - end diff --git a/activeresource/test/cases/base_test.rb b/activeresource/test/cases/base_test.rb index fc366430d4..1593e25595 100644 --- a/activeresource/test/cases/base_test.rb +++ b/activeresource/test/cases/base_test.rb @@ -828,7 +828,7 @@ class BaseTest < Test::Unit::TestCase ###### # update_attribute(s)(!) - + def test_update_attribute_as_symbol matz = Person.first matz.expects(:save).returns(true) @@ -837,7 +837,7 @@ class BaseTest < Test::Unit::TestCase assert matz.update_attribute(:name, "David") assert_equal "David", matz.name end - + def test_update_attribute_as_string matz = Person.first matz.expects(:save).returns(true) @@ -847,7 +847,7 @@ class BaseTest < Test::Unit::TestCase assert_equal "David", matz.name end - + def test_update_attributes_as_symbols addy = StreetAddress.first(:params => {:person_id => 1}) addy.expects(:save).returns(true) @@ -913,7 +913,7 @@ class BaseTest < Test::Unit::TestCase end assert_raise(ActiveResource::ResourceNotFound) { StreetAddress.find(1, :params => { :person_id => 1 }) } end - + def test_delete_with_410_gone assert Person.delete(1) ActiveResource::HttpMock.respond_to do |mock| From 84e94551f62d3bcbc71f1c6f3fda738342d984e2 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 20:45:49 -0500 Subject: [PATCH 04/61] Add custom "with_routing" to internal tests to fix reseting session after using with_routing. This only affects our internal AP tests. --- actionpack/test/abstract_unit.rb | 33 +++++++++++++++---- .../activerecord/active_record_store_test.rb | 1 - .../test/controller/integration_test.rb | 5 +-- actionpack/test/controller/rescue_test.rb | 1 - actionpack/test/controller/webservice_test.rb | 1 - .../request/json_params_parsing_test.rb | 1 - .../request/multipart_params_parsing_test.rb | 1 - .../request/query_string_parsing_test.rb | 1 - .../url_encoded_params_parsing_test.rb | 1 - .../request/xml_params_parsing_test.rb | 1 - .../dispatch/session/cookie_store_test.rb | 1 - .../dispatch/session/mem_cache_store_test.rb | 1 - .../test/lib/controller/fake_controllers.rb | 1 + actionpack/test/view/test_case_test.rb | 1 + 14 files changed, 30 insertions(+), 20 deletions(-) diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index aef3dd6165..b72dd4dd0f 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -53,12 +53,33 @@ ORIGINAL_LOCALES = I18n.available_locales.map {|locale| locale.to_s }.sort FIXTURE_LOAD_PATH = File.join(File.dirname(__FILE__), 'fixtures') class ActionController::IntegrationTest < ActiveSupport::TestCase - @@app = ActionDispatch::MiddlewareStack.new { |middleware| - middleware.use "ActionDispatch::ShowExceptions" - middleware.use "ActionDispatch::Callbacks" - middleware.use "ActionDispatch::ParamsParser" - middleware.use "Rack::Head" - }.build(ActionController::Routing::Routes) + def self.build_app(routes = nil) + ActionDispatch::MiddlewareStack.new { |middleware| + middleware.use "ActionDispatch::ShowExceptions" + middleware.use "ActionDispatch::Callbacks" + middleware.use "ActionDispatch::ParamsParser" + middleware.use "Rack::Head" + }.build(routes || ActionController::Routing::Routes) + end + + self.app = build_app + + def with_routing(&block) + real_routes = ActionController::Routing::Routes + ActionController::Routing.module_eval { remove_const :Routes } + + temporary_routes = ActionController::Routing::RouteSet.new + self.class.app = self.class.build_app(temporary_routes) + ActionController::Routing.module_eval { const_set :Routes, temporary_routes } + + yield temporary_routes + ensure + if ActionController::Routing.const_defined? :Routes + ActionController::Routing.module_eval { remove_const :Routes } + end + ActionController::Routing.const_set(:Routes, real_routes) if real_routes + self.class.app = self.class.build_app + end end module ActionView diff --git a/actionpack/test/activerecord/active_record_store_test.rb b/actionpack/test/activerecord/active_record_store_test.rb index 102b9cffdd..c6c079f88c 100644 --- a/actionpack/test/activerecord/active_record_store_test.rb +++ b/actionpack/test/activerecord/active_record_store_test.rb @@ -158,7 +158,6 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest map.connect "/:action", :controller => "active_record_store_test/test" end @app = ActiveRecord::SessionStore.new(set, options.reverse_merge(:key => '_session_id')) - reset! yield end end diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb index 0e4ca21143..508364d0b5 100644 --- a/actionpack/test/controller/integration_test.rb +++ b/actionpack/test/controller/integration_test.rb @@ -372,11 +372,8 @@ class IntegrationProcessTest < ActionController::IntegrationTest def with_test_route_set with_routing do |set| set.draw do |map| - map.with_options :controller => "IntegrationProcessTest::Integration" do |c| - c.connect "/:action" - end + map.connect "/:action", :controller => "integration_process_test/integration" end - reset! yield end end diff --git a/actionpack/test/controller/rescue_test.rb b/actionpack/test/controller/rescue_test.rb index 6ad708bba1..689359166f 100644 --- a/actionpack/test/controller/rescue_test.rb +++ b/actionpack/test/controller/rescue_test.rb @@ -347,7 +347,6 @@ class RescueTest < ActionController::IntegrationTest map.connect 'invalid', :controller => "rescue_test/test", :action => 'invalid' map.connect 'b00m', :controller => "rescue_test/test", :action => 'b00m' end - reset! yield end end diff --git a/actionpack/test/controller/webservice_test.rb b/actionpack/test/controller/webservice_test.rb index c04d20fbad..0514c098bf 100644 --- a/actionpack/test/controller/webservice_test.rb +++ b/actionpack/test/controller/webservice_test.rb @@ -259,7 +259,6 @@ class WebServiceTest < ActionController::IntegrationTest c.connect "/", :action => "assign_parameters" end end - reset! yield end end diff --git a/actionpack/test/dispatch/request/json_params_parsing_test.rb b/actionpack/test/dispatch/request/json_params_parsing_test.rb index 995f36bb29..db6cf7b330 100644 --- a/actionpack/test/dispatch/request/json_params_parsing_test.rb +++ b/actionpack/test/dispatch/request/json_params_parsing_test.rb @@ -59,7 +59,6 @@ class JsonParamsParsingTest < ActionController::IntegrationTest set.draw do |map| map.connect ':action', :controller => "json_params_parsing_test/test" end - reset! yield end end diff --git a/actionpack/test/dispatch/request/multipart_params_parsing_test.rb b/actionpack/test/dispatch/request/multipart_params_parsing_test.rb index d4ee4362eb..301080842e 100644 --- a/actionpack/test/dispatch/request/multipart_params_parsing_test.rb +++ b/actionpack/test/dispatch/request/multipart_params_parsing_test.rb @@ -153,7 +153,6 @@ class MultipartParamsParsingTest < ActionController::IntegrationTest set.draw do |map| map.connect ':action', :controller => "multipart_params_parsing_test/test" end - reset! yield end end diff --git a/actionpack/test/dispatch/request/query_string_parsing_test.rb b/actionpack/test/dispatch/request/query_string_parsing_test.rb index 2261934e45..a31e326ddf 100644 --- a/actionpack/test/dispatch/request/query_string_parsing_test.rb +++ b/actionpack/test/dispatch/request/query_string_parsing_test.rb @@ -111,7 +111,6 @@ class QueryStringParsingTest < ActionController::IntegrationTest set.draw do |map| map.connect ':action', :controller => "query_string_parsing_test/test" end - reset! get "/parse", actual assert_response :ok diff --git a/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb b/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb index 6c9967d26e..7167cdafac 100644 --- a/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb +++ b/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb @@ -132,7 +132,6 @@ class UrlEncodedParamsParsingTest < ActionController::IntegrationTest set.draw do |map| map.connect ':action', :controller => "url_encoded_params_parsing_test/test" end - reset! yield end end diff --git a/actionpack/test/dispatch/request/xml_params_parsing_test.rb b/actionpack/test/dispatch/request/xml_params_parsing_test.rb index 2f2dd695c4..521002b519 100644 --- a/actionpack/test/dispatch/request/xml_params_parsing_test.rb +++ b/actionpack/test/dispatch/request/xml_params_parsing_test.rb @@ -86,7 +86,6 @@ class XmlParamsParsingTest < ActionController::IntegrationTest set.draw do |map| map.connect ':action', :controller => "xml_params_parsing_test/test" end - reset! yield end end diff --git a/actionpack/test/dispatch/session/cookie_store_test.rb b/actionpack/test/dispatch/session/cookie_store_test.rb index 6241c79829..ab5fabde65 100644 --- a/actionpack/test/dispatch/session/cookie_store_test.rb +++ b/actionpack/test/dispatch/session/cookie_store_test.rb @@ -223,7 +223,6 @@ class CookieStoreTest < ActionController::IntegrationTest end options = {:key => SessionKey, :secret => SessionSecret}.merge(options) @app = ActionDispatch::Session::CookieStore.new(set, options) - reset! yield end end diff --git a/actionpack/test/dispatch/session/mem_cache_store_test.rb b/actionpack/test/dispatch/session/mem_cache_store_test.rb index c2d40ae24a..c7435bd06b 100644 --- a/actionpack/test/dispatch/session/mem_cache_store_test.rb +++ b/actionpack/test/dispatch/session/mem_cache_store_test.rb @@ -115,7 +115,6 @@ class MemCacheStoreTest < ActionController::IntegrationTest map.connect "/:action", :controller => "mem_cache_store_test/test" end @app = ActionDispatch::Session::MemCacheStore.new(set, :key => '_session_id') - reset! yield end end diff --git a/actionpack/test/lib/controller/fake_controllers.rb b/actionpack/test/lib/controller/fake_controllers.rb index 5dcca2e148..2e8c5090ca 100644 --- a/actionpack/test/lib/controller/fake_controllers.rb +++ b/actionpack/test/lib/controller/fake_controllers.rb @@ -18,6 +18,7 @@ class HiController < ActionController::Base; end class BraveController < ActionController::Base; end class ImageController < ActionController::Base; end class WeblogController < ActionController::Base; end +class BarController < ActionController::Base; end # For speed test class SpeedController < ActionController::Base; end diff --git a/actionpack/test/view/test_case_test.rb b/actionpack/test/view/test_case_test.rb index 3e974b87f7..5db42c4d68 100644 --- a/actionpack/test/view/test_case_test.rb +++ b/actionpack/test/view/test_case_test.rb @@ -1,4 +1,5 @@ require 'abstract_unit' +require 'controller/fake_controllers' module ActionView class TestCase From 018b79dd36d054d87fdc408d38dc9ac7f1b1500d Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 21:05:51 -0500 Subject: [PATCH 05/61] File extra test folders into controller, dispatch, or template --- actionpack/Rakefile | 2 +- .../{ => controller}/new_base/base_test.rb | 0 .../new_base/content_negotiation_test.rb | 0 .../new_base/content_type_test.rb | 0 .../{ => controller}/new_base/etag_test.rb | 0 .../{ => controller}/new_base/metal_test.rb | 0 .../new_base/middleware_test.rb | 0 .../new_base/render_action_test.rb | 0 .../new_base/render_file_test.rb | 18 +++++++++--------- .../new_base/render_implicit_action_test.rb | 0 .../new_base/render_layout_test.rb | 0 .../new_base/render_partial_test.rb | 0 .../new_base/render_rjs_test.rb | 0 .../new_base/render_template_test.rb | 0 .../{ => controller}/new_base/render_test.rb | 0 .../new_base/render_text_test.rb | 0 .../new_base/render_xml_test.rb | 0 .../test/{javascript => template}/ajax_test.rb | 15 +++++++-------- .../html-scanner/cdata_node_test.rb | 0 .../html-scanner/document_test.rb | 0 .../{ => template}/html-scanner/node_test.rb | 0 .../html-scanner/sanitizer_test.rb | 0 .../html-scanner/tag_node_test.rb | 0 .../html-scanner/text_node_test.rb | 0 .../html-scanner/tokenizer_test.rb | 0 .../test/{view => template}/test_case_test.rb | 0 26 files changed, 17 insertions(+), 18 deletions(-) rename actionpack/test/{ => controller}/new_base/base_test.rb (100%) rename actionpack/test/{ => controller}/new_base/content_negotiation_test.rb (100%) rename actionpack/test/{ => controller}/new_base/content_type_test.rb (100%) rename actionpack/test/{ => controller}/new_base/etag_test.rb (100%) rename actionpack/test/{ => controller}/new_base/metal_test.rb (100%) rename actionpack/test/{ => controller}/new_base/middleware_test.rb (100%) rename actionpack/test/{ => controller}/new_base/render_action_test.rb (100%) rename actionpack/test/{ => controller}/new_base/render_file_test.rb (75%) rename actionpack/test/{ => controller}/new_base/render_implicit_action_test.rb (100%) rename actionpack/test/{ => controller}/new_base/render_layout_test.rb (100%) rename actionpack/test/{ => controller}/new_base/render_partial_test.rb (100%) rename actionpack/test/{ => controller}/new_base/render_rjs_test.rb (100%) rename actionpack/test/{ => controller}/new_base/render_template_test.rb (100%) rename actionpack/test/{ => controller}/new_base/render_test.rb (100%) rename actionpack/test/{ => controller}/new_base/render_text_test.rb (100%) rename actionpack/test/{ => controller}/new_base/render_xml_test.rb (100%) rename actionpack/test/{javascript => template}/ajax_test.rb (90%) rename actionpack/test/{ => template}/html-scanner/cdata_node_test.rb (100%) rename actionpack/test/{ => template}/html-scanner/document_test.rb (100%) rename actionpack/test/{ => template}/html-scanner/node_test.rb (100%) rename actionpack/test/{ => template}/html-scanner/sanitizer_test.rb (100%) rename actionpack/test/{ => template}/html-scanner/tag_node_test.rb (100%) rename actionpack/test/{ => template}/html-scanner/text_node_test.rb (100%) rename actionpack/test/{ => template}/html-scanner/tokenizer_test.rb (100%) rename actionpack/test/{view => template}/test_case_test.rb (100%) diff --git a/actionpack/Rakefile b/actionpack/Rakefile index af39175047..547e74b5a1 100644 --- a/actionpack/Rakefile +++ b/actionpack/Rakefile @@ -34,7 +34,7 @@ end desc "Run all unit tests" task :test => [:test_action_pack, :test_active_record_integration] -TESTS_GLOB = "test/{abstract,controller,dispatch,new_base,template,html-scanner,view}/**/*_test.rb" +TESTS_GLOB = "test/{abstract,controller,dispatch,template}/**/*_test.rb" Rake::TestTask.new(:test_action_pack) do |t| t.libs << 'test' diff --git a/actionpack/test/new_base/base_test.rb b/actionpack/test/controller/new_base/base_test.rb similarity index 100% rename from actionpack/test/new_base/base_test.rb rename to actionpack/test/controller/new_base/base_test.rb diff --git a/actionpack/test/new_base/content_negotiation_test.rb b/actionpack/test/controller/new_base/content_negotiation_test.rb similarity index 100% rename from actionpack/test/new_base/content_negotiation_test.rb rename to actionpack/test/controller/new_base/content_negotiation_test.rb diff --git a/actionpack/test/new_base/content_type_test.rb b/actionpack/test/controller/new_base/content_type_test.rb similarity index 100% rename from actionpack/test/new_base/content_type_test.rb rename to actionpack/test/controller/new_base/content_type_test.rb diff --git a/actionpack/test/new_base/etag_test.rb b/actionpack/test/controller/new_base/etag_test.rb similarity index 100% rename from actionpack/test/new_base/etag_test.rb rename to actionpack/test/controller/new_base/etag_test.rb diff --git a/actionpack/test/new_base/metal_test.rb b/actionpack/test/controller/new_base/metal_test.rb similarity index 100% rename from actionpack/test/new_base/metal_test.rb rename to actionpack/test/controller/new_base/metal_test.rb diff --git a/actionpack/test/new_base/middleware_test.rb b/actionpack/test/controller/new_base/middleware_test.rb similarity index 100% rename from actionpack/test/new_base/middleware_test.rb rename to actionpack/test/controller/new_base/middleware_test.rb diff --git a/actionpack/test/new_base/render_action_test.rb b/actionpack/test/controller/new_base/render_action_test.rb similarity index 100% rename from actionpack/test/new_base/render_action_test.rb rename to actionpack/test/controller/new_base/render_action_test.rb diff --git a/actionpack/test/new_base/render_file_test.rb b/actionpack/test/controller/new_base/render_file_test.rb similarity index 75% rename from actionpack/test/new_base/render_file_test.rb rename to actionpack/test/controller/new_base/render_file_test.rb index c4098855e6..1c52d2b37d 100644 --- a/actionpack/test/new_base/render_file_test.rb +++ b/actionpack/test/controller/new_base/render_file_test.rb @@ -6,45 +6,45 @@ module RenderFile self.view_paths = File.dirname(__FILE__) def index - render :file => File.join(File.dirname(__FILE__), *%w[.. fixtures test hello_world]) + render :file => File.join(File.dirname(__FILE__), *%w[.. .. fixtures test hello_world]) end def with_instance_variables @secret = 'in the sauce' - render :file => File.join(File.dirname(__FILE__), '../fixtures/test/render_file_with_ivar.erb') + render :file => File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_ivar.erb') end def without_file_key - render File.join(File.dirname(__FILE__), *%w[.. fixtures test hello_world]) + 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.erb') + render File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_ivar.erb') end def relative_path @secret = 'in the sauce' - render :file => '../fixtures/test/render_file_with_ivar' + render :file => '../../fixtures/test/render_file_with_ivar' end def relative_path_with_dot @secret = 'in the sauce' - render :file => '../fixtures/test/dot.directory/render_file_with_ivar' + render :file => '../../fixtures/test/dot.directory/render_file_with_ivar' end def pathname @secret = 'in the sauce' - render :file => Pathname.new(File.dirname(__FILE__)).join(*%w[.. fixtures test dot.directory render_file_with_ivar.erb]) + render :file => Pathname.new(File.dirname(__FILE__)).join(*%w[.. .. fixtures test dot.directory render_file_with_ivar.erb]) end def with_locals - path = File.join(File.dirname(__FILE__), '../fixtures/test/render_file_with_locals.erb') + path = File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_locals.erb') render :file => path, :locals => {:secret => 'in the sauce'} end def without_file_key_with_locals - path = File.expand_path('../fixtures/test/render_file_with_locals.erb') + path = File.expand_path('../../fixtures/test/render_file_with_locals.erb') render path, :locals => {:secret => 'in the sauce'} end end diff --git a/actionpack/test/new_base/render_implicit_action_test.rb b/actionpack/test/controller/new_base/render_implicit_action_test.rb similarity index 100% rename from actionpack/test/new_base/render_implicit_action_test.rb rename to actionpack/test/controller/new_base/render_implicit_action_test.rb diff --git a/actionpack/test/new_base/render_layout_test.rb b/actionpack/test/controller/new_base/render_layout_test.rb similarity index 100% rename from actionpack/test/new_base/render_layout_test.rb rename to actionpack/test/controller/new_base/render_layout_test.rb diff --git a/actionpack/test/new_base/render_partial_test.rb b/actionpack/test/controller/new_base/render_partial_test.rb similarity index 100% rename from actionpack/test/new_base/render_partial_test.rb rename to actionpack/test/controller/new_base/render_partial_test.rb diff --git a/actionpack/test/new_base/render_rjs_test.rb b/actionpack/test/controller/new_base/render_rjs_test.rb similarity index 100% rename from actionpack/test/new_base/render_rjs_test.rb rename to actionpack/test/controller/new_base/render_rjs_test.rb diff --git a/actionpack/test/new_base/render_template_test.rb b/actionpack/test/controller/new_base/render_template_test.rb similarity index 100% rename from actionpack/test/new_base/render_template_test.rb rename to actionpack/test/controller/new_base/render_template_test.rb diff --git a/actionpack/test/new_base/render_test.rb b/actionpack/test/controller/new_base/render_test.rb similarity index 100% rename from actionpack/test/new_base/render_test.rb rename to actionpack/test/controller/new_base/render_test.rb diff --git a/actionpack/test/new_base/render_text_test.rb b/actionpack/test/controller/new_base/render_text_test.rb similarity index 100% rename from actionpack/test/new_base/render_text_test.rb rename to actionpack/test/controller/new_base/render_text_test.rb diff --git a/actionpack/test/new_base/render_xml_test.rb b/actionpack/test/controller/new_base/render_xml_test.rb similarity index 100% rename from actionpack/test/new_base/render_xml_test.rb rename to actionpack/test/controller/new_base/render_xml_test.rb diff --git a/actionpack/test/javascript/ajax_test.rb b/actionpack/test/template/ajax_test.rb similarity index 90% rename from actionpack/test/javascript/ajax_test.rb rename to actionpack/test/template/ajax_test.rb index b67a91dad3..670ba92697 100644 --- a/actionpack/test/javascript/ajax_test.rb +++ b/actionpack/test/template/ajax_test.rb @@ -32,7 +32,7 @@ class LinkToRemoteTest < AjaxTestCase end test "with no update" do - assert_html link, %w(href="/blog/destroy/3" Delete\ this\ post data-remote="true") + assert_html link, %w(href="/blog/destroy/4" Delete\ this\ post data-remote="true") end test "basic" do @@ -46,7 +46,7 @@ class LinkToRemoteTest < AjaxTestCase end test "with :html options" do - expected = %{Delete this post} + expected = %{Delete this post} assert_equal expected, link(:update => "#posts", :html => {"data-custom" => "me"}) end @@ -74,7 +74,7 @@ class LinkToRemoteTest < AjaxTestCase end test "basic link_to_remote with :url =>" do - expected = %{Delete this post} + expected = %{Delete this post} assert_equal expected, link_to_remote("Delete this post", :url => "/blog/destroy/3", :update => "#posts") end @@ -93,7 +93,7 @@ class ButtonToRemoteTest < AjaxTestCase def url_for(*) "/whatnot" end - + class StandardTest < ButtonToRemoteTest test "basic" do button = button({:url => {:action => "whatnot"}}, {:class => "fine"}) @@ -103,13 +103,12 @@ class ButtonToRemoteTest < AjaxTestCase end end end - + class LegacyButtonToRemoteTest < ButtonToRemoteTest include ActionView::Helpers::AjaxHelper::Rails2Compatibility - + assert_callbacks_work do |callback| button(callback => "undoRequestCompleted(request)") end end - -end \ No newline at end of file +end diff --git a/actionpack/test/html-scanner/cdata_node_test.rb b/actionpack/test/template/html-scanner/cdata_node_test.rb similarity index 100% rename from actionpack/test/html-scanner/cdata_node_test.rb rename to actionpack/test/template/html-scanner/cdata_node_test.rb diff --git a/actionpack/test/html-scanner/document_test.rb b/actionpack/test/template/html-scanner/document_test.rb similarity index 100% rename from actionpack/test/html-scanner/document_test.rb rename to actionpack/test/template/html-scanner/document_test.rb diff --git a/actionpack/test/html-scanner/node_test.rb b/actionpack/test/template/html-scanner/node_test.rb similarity index 100% rename from actionpack/test/html-scanner/node_test.rb rename to actionpack/test/template/html-scanner/node_test.rb diff --git a/actionpack/test/html-scanner/sanitizer_test.rb b/actionpack/test/template/html-scanner/sanitizer_test.rb similarity index 100% rename from actionpack/test/html-scanner/sanitizer_test.rb rename to actionpack/test/template/html-scanner/sanitizer_test.rb diff --git a/actionpack/test/html-scanner/tag_node_test.rb b/actionpack/test/template/html-scanner/tag_node_test.rb similarity index 100% rename from actionpack/test/html-scanner/tag_node_test.rb rename to actionpack/test/template/html-scanner/tag_node_test.rb diff --git a/actionpack/test/html-scanner/text_node_test.rb b/actionpack/test/template/html-scanner/text_node_test.rb similarity index 100% rename from actionpack/test/html-scanner/text_node_test.rb rename to actionpack/test/template/html-scanner/text_node_test.rb diff --git a/actionpack/test/html-scanner/tokenizer_test.rb b/actionpack/test/template/html-scanner/tokenizer_test.rb similarity index 100% rename from actionpack/test/html-scanner/tokenizer_test.rb rename to actionpack/test/template/html-scanner/tokenizer_test.rb diff --git a/actionpack/test/view/test_case_test.rb b/actionpack/test/template/test_case_test.rb similarity index 100% rename from actionpack/test/view/test_case_test.rb rename to actionpack/test/template/test_case_test.rb From 75a2f00c9790f2fd7cebb4337cab1404e9e03eae Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 21:33:06 -0500 Subject: [PATCH 06/61] Move improved isolated test runner to AP --- actionpack/Rakefile | 11 +++++------ actionpack/test/ts_isolated.rb | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) create mode 100644 actionpack/test/ts_isolated.rb diff --git a/actionpack/Rakefile b/actionpack/Rakefile index 547e74b5a1..e186037aeb 100644 --- a/actionpack/Rakefile +++ b/actionpack/Rakefile @@ -34,22 +34,21 @@ end desc "Run all unit tests" task :test => [:test_action_pack, :test_active_record_integration] -TESTS_GLOB = "test/{abstract,controller,dispatch,template}/**/*_test.rb" - Rake::TestTask.new(:test_action_pack) do |t| t.libs << 'test' # 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 = Dir.glob(TESTS_GLOB).sort + t.test_files = Dir.glob('test/{abstract,controller,dispatch,template}/**/*_test.rb').sort t.verbose = true # t.warning = true end -task :isolated_test do - ruby = File.join(*RbConfig::CONFIG.values_at('bindir', 'RUBY_INSTALL_NAME')) - Dir.glob(TESTS_GLOB).all? { |file| system(ruby, '-Ilib:test', file) } or raise "Failures" +namespace :test do + Rake::TestTask.new(:isolated) do |t| + t.pattern = 'test/ts_isolated.rb' + end end desc 'ActiveRecord Integration Tests' diff --git a/actionpack/test/ts_isolated.rb b/actionpack/test/ts_isolated.rb new file mode 100644 index 0000000000..4a321c631f --- /dev/null +++ b/actionpack/test/ts_isolated.rb @@ -0,0 +1,16 @@ +require 'test/unit' +require 'rbconfig' +require 'rubygems' +require 'active_support' + +class TestIsolated < Test::Unit::TestCase + ruby = File.join(*RbConfig::CONFIG.values_at('bindir', 'RUBY_INSTALL_NAME')) + + Dir["#{File.dirname(__FILE__)}/{abstract,controller,dispatch,template}/**/*_test.rb"].each do |file| + define_method("test #{file}") do + command = "#{ruby} -Ilib:test #{file}" + silence_stderr { `#{command}` } + assert_equal 0, $?.to_i, command + end + end +end From 660eb068d3aaed0ad7769fe9b258d7e471972cbc Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 21:51:34 -0500 Subject: [PATCH 07/61] Don't load rubygems for isolated tests --- actionpack/test/ts_isolated.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/actionpack/test/ts_isolated.rb b/actionpack/test/ts_isolated.rb index 4a321c631f..21d62f6aa7 100644 --- a/actionpack/test/ts_isolated.rb +++ b/actionpack/test/ts_isolated.rb @@ -1,7 +1,8 @@ +$:.unshift(File.dirname(__FILE__) + '/../../activesupport/lib') + require 'test/unit' require 'rbconfig' -require 'rubygems' -require 'active_support' +require 'active_support/core_ext/kernel/reporting' class TestIsolated < Test::Unit::TestCase ruby = File.join(*RbConfig::CONFIG.values_at('bindir', 'RUBY_INSTALL_NAME')) From 7eaed071a2c701552491767e42730cb6419cc6c3 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 22:02:51 -0500 Subject: [PATCH 08/61] Changing directories during the test breaks file loading when ran by itself --- actionpack/test/abstract_unit.rb | 1 + .../controller/new_base/render_file_test.rb | 53 ++++++++----------- 2 files changed, 22 insertions(+), 32 deletions(-) diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index b72dd4dd0f..c1153b01de 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -51,6 +51,7 @@ I18n.backend.store_translations 'pt-BR', {} ORIGINAL_LOCALES = I18n.available_locales.map {|locale| locale.to_s }.sort FIXTURE_LOAD_PATH = File.join(File.dirname(__FILE__), 'fixtures') +FIXTURES = Pathname.new(FIXTURE_LOAD_PATH) class ActionController::IntegrationTest < ActiveSupport::TestCase def self.build_app(routes = nil) diff --git a/actionpack/test/controller/new_base/render_file_test.rb b/actionpack/test/controller/new_base/render_file_test.rb index 1c52d2b37d..864469e0ae 100644 --- a/actionpack/test/controller/new_base/render_file_test.rb +++ b/actionpack/test/controller/new_base/render_file_test.rb @@ -1,110 +1,99 @@ require 'abstract_unit' module RenderFile - class BasicController < ActionController::Base - self.view_paths = File.dirname(__FILE__) - + self.view_paths = File.dirname(__FILE__) + def index render :file => File.join(File.dirname(__FILE__), *%w[.. .. fixtures test hello_world]) end - + def with_instance_variables @secret = 'in the sauce' render :file => File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_ivar.erb') 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.erb') end - + def relative_path @secret = 'in the sauce' render :file => '../../fixtures/test/render_file_with_ivar' end - + def relative_path_with_dot @secret = 'in the sauce' render :file => '../../fixtures/test/dot.directory/render_file_with_ivar' end - + def pathname @secret = 'in the sauce' render :file => Pathname.new(File.dirname(__FILE__)).join(*%w[.. .. fixtures test dot.directory render_file_with_ivar.erb]) end - + def with_locals path = File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_locals.erb') render :file => path, :locals => {:secret => 'in the sauce'} end - + def without_file_key_with_locals - path = File.expand_path('../../fixtures/test/render_file_with_locals.erb') + path = FIXTURES.join('test/render_file_with_locals.erb').to_s render path, :locals => {:secret => 'in the sauce'} end end - + class TestBasic < SimpleRouteCase testing RenderFile::BasicController - - def setup - @old_pwd = Dir.pwd - Dir.chdir(File.dirname(__FILE__)) - end - - def teardown - Dir.chdir(@old_pwd) - end - + test "rendering simple template" do get :index assert_response "Hello world!" end - + test "rendering template with ivar" do get :with_instance_variables 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" end - + test "rendering a relative path with dot" do get :relative_path_with_dot assert_response "The secret is in the sauce\n" end - + test "rendering a Pathname" do get :pathname assert_response "The secret is in the sauce\n" end - + test "rendering file with locals" do 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 From 31319b471b797c063ec13f1dd11a27d68cd671a7 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 22:06:25 -0500 Subject: [PATCH 09/61] NumberHelper depends on big decimal extensions --- actionpack/lib/action_view/helpers/number_helper.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/actionpack/lib/action_view/helpers/number_helper.rb b/actionpack/lib/action_view/helpers/number_helper.rb index 897a7cc348..397871b85e 100644 --- a/actionpack/lib/action_view/helpers/number_helper.rb +++ b/actionpack/lib/action_view/helpers/number_helper.rb @@ -1,3 +1,4 @@ +require 'active_support/core_ext/big_decimal/conversions' require 'active_support/core_ext/float/rounding' module ActionView From f5cba5e6b1f4ed830737c2f4716c88a245bce5c7 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 22:14:50 -0500 Subject: [PATCH 10/61] Moved shared form helper models into fake_models --- actionpack/test/lib/controller/fake_models.rb | 96 +++++++++++++++++ actionpack/test/template/form_helper_test.rb | 100 +----------------- .../test/template/record_tag_helper_test.rb | 1 + 3 files changed, 98 insertions(+), 99 deletions(-) diff --git a/actionpack/test/lib/controller/fake_models.rb b/actionpack/test/lib/controller/fake_models.rb index 18eff7516b..823de8bdc7 100644 --- a/actionpack/test/lib/controller/fake_models.rb +++ b/actionpack/test/lib/controller/fake_models.rb @@ -51,3 +51,99 @@ module Quiz end end +class Post < Struct.new(:title, :author_name, :body, :secret, :written_on, :cost) + extend ActiveModel::Naming + include ActiveModel::Conversion + + alias_method :secret?, :secret + + def new_record=(boolean) + @new_record = boolean + end + + def new_record? + @new_record + end + + attr_accessor :author + def author_attributes=(attributes); end + + attr_accessor :comments + def comments_attributes=(attributes); end + + attr_accessor :tags + def tags_attributes=(attributes); end +end + +class Comment + extend ActiveModel::Naming + include ActiveModel::Conversion + + attr_reader :id + attr_reader :post_id + def initialize(id = nil, post_id = nil); @id, @post_id = id, post_id end + def save; @id = 1; @post_id = 1 end + def new_record?; @id.nil? end + def to_param; @id; end + def name + @id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}" + end + + attr_accessor :relevances + def relevances_attributes=(attributes); end + +end + +class Tag + extend ActiveModel::Naming + include ActiveModel::Conversion + + attr_reader :id + attr_reader :post_id + def initialize(id = nil, post_id = nil); @id, @post_id = id, post_id end + def save; @id = 1; @post_id = 1 end + def new_record?; @id.nil? end + def to_param; @id; end + def value + @id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}" + end + + attr_accessor :relevances + def relevances_attributes=(attributes); end + +end + +class CommentRelevance + extend ActiveModel::Naming + include ActiveModel::Conversion + + attr_reader :id + attr_reader :comment_id + def initialize(id = nil, comment_id = nil); @id, @comment_id = id, comment_id end + def save; @id = 1; @comment_id = 1 end + def new_record?; @id.nil? end + def to_param; @id; end + def value + @id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}" + end +end + +class TagRelevance + extend ActiveModel::Naming + include ActiveModel::Conversion + + attr_reader :id + attr_reader :tag_id + def initialize(id = nil, tag_id = nil); @id, @tag_id = id, tag_id end + def save; @id = 1; @tag_id = 1 end + def new_record?; @id.nil? end + def to_param; @id; end + def value + @id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}" + end +end + +class Author < Comment + attr_accessor :post + def post_attributes=(attributes); end +end diff --git a/actionpack/test/template/form_helper_test.rb b/actionpack/test/template/form_helper_test.rb index be15b06372..6a08c99619 100644 --- a/actionpack/test/template/form_helper_test.rb +++ b/actionpack/test/template/form_helper_test.rb @@ -1,103 +1,5 @@ require 'abstract_unit' - -silence_warnings do - class Post < Struct.new(:title, :author_name, :body, :secret, :written_on, :cost) - extend ActiveModel::Naming - include ActiveModel::Conversion - - alias_method :secret?, :secret - - def new_record=(boolean) - @new_record = boolean - end - - def new_record? - @new_record - end - - attr_accessor :author - def author_attributes=(attributes); end - - attr_accessor :comments - def comments_attributes=(attributes); end - - attr_accessor :tags - def tags_attributes=(attributes); end - end - - class Comment - extend ActiveModel::Naming - include ActiveModel::Conversion - - attr_reader :id - attr_reader :post_id - def initialize(id = nil, post_id = nil); @id, @post_id = id, post_id end - def save; @id = 1; @post_id = 1 end - def new_record?; @id.nil? end - def to_param; @id; end - def name - @id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}" - end - - attr_accessor :relevances - def relevances_attributes=(attributes); end - - end - - class Tag - extend ActiveModel::Naming - include ActiveModel::Conversion - - attr_reader :id - attr_reader :post_id - def initialize(id = nil, post_id = nil); @id, @post_id = id, post_id end - def save; @id = 1; @post_id = 1 end - def new_record?; @id.nil? end - def to_param; @id; end - def value - @id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}" - end - - attr_accessor :relevances - def relevances_attributes=(attributes); end - - end - - class CommentRelevance - extend ActiveModel::Naming - include ActiveModel::Conversion - - attr_reader :id - attr_reader :comment_id - def initialize(id = nil, comment_id = nil); @id, @comment_id = id, comment_id end - def save; @id = 1; @comment_id = 1 end - def new_record?; @id.nil? end - def to_param; @id; end - def value - @id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}" - end - end - - class TagRelevance - extend ActiveModel::Naming - include ActiveModel::Conversion - - attr_reader :id - attr_reader :tag_id - def initialize(id = nil, tag_id = nil); @id, @tag_id = id, tag_id end - def save; @id = 1; @tag_id = 1 end - def new_record?; @id.nil? end - def to_param; @id; end - def value - @id.nil? ? "new #{self.class.name.downcase}" : "#{self.class.name.downcase} ##{@id}" - end - end - - class Author < Comment - attr_accessor :post - def post_attributes=(attributes); end - end -end +require 'controller/fake_models' class FormHelperTest < ActionView::TestCase tests ActionView::Helpers::FormHelper diff --git a/actionpack/test/template/record_tag_helper_test.rb b/actionpack/test/template/record_tag_helper_test.rb index 4144fea678..77d1374020 100644 --- a/actionpack/test/template/record_tag_helper_test.rb +++ b/actionpack/test/template/record_tag_helper_test.rb @@ -1,4 +1,5 @@ require 'abstract_unit' +require 'controller/fake_models' class Post extend ActiveModel::Naming From 2a938ad5e773c35bfaec282a150e77032563f059 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 22:14:58 -0500 Subject: [PATCH 11/61] Run AP isolated tests on CI --- ci/ci_build.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/ci_build.rb b/ci/ci_build.rb index fd55444889..b9c321efef 100755 --- a/ci/ci_build.rb +++ b/ci/ci_build.rb @@ -66,6 +66,7 @@ cd "#{root_dir}/actionpack" do puts "[CruiseControl] Building ActionPack" puts build_results[:actionpack] = system 'gem bundle && rake' + build_results[:actionpack_isolated] = system 'rake test:isolated' end cd "#{root_dir}/actionmailer" do From 8287a112c92aff73d45536f0942d9dc7ba107bd7 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 23:03:08 -0500 Subject: [PATCH 12/61] Avoid creating new controller constants during test runtime. All routable controllers should be defined beforehand. --- actionpack/test/controller/routing_test.rb | 62 +------------------ .../test/lib/controller/fake_controllers.rb | 33 ++++++---- 2 files changed, 24 insertions(+), 71 deletions(-) diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb index 1aabf71cad..edf243337f 100644 --- a/actionpack/test/controller/routing_test.rb +++ b/actionpack/test/controller/routing_test.rb @@ -669,21 +669,13 @@ class LegacyRouteSetTests < Test::Unit::TestCase %w(GET POST PUT DELETE).each do |request_method| define_method("test_request_method_recognized_with_#{request_method}") do - begin - Object.const_set(:BooksController, Class.new(ActionController::Base)) - - setup_request_method_routes_for(request_method) - - assert_nothing_raised { rs.recognize(@request) } - assert_equal request_method.downcase, @request.path_parameters[:action] - ensure - Object.send(:remove_const, :BooksController) rescue nil - end + setup_request_method_routes_for(request_method) + assert_nothing_raised { rs.recognize(@request) } + assert_equal request_method.downcase, @request.path_parameters[:action] end end def test_recognize_array_of_methods - Object.const_set(:BooksController, Class.new(ActionController::Base)) rs.draw do |r| r.connect '/match', :controller => 'books', :action => 'get_or_post', :conditions => { :method => [:get, :post] } r.connect '/match', :controller => 'books', :action => 'not_get_or_post' @@ -701,13 +693,9 @@ class LegacyRouteSetTests < Test::Unit::TestCase @request.request_uri = "/match" assert_nothing_raised { rs.recognize(@request) } assert_equal 'not_get_or_post', @request.path_parameters[:action] - ensure - Object.send(:remove_const, :BooksController) rescue nil end def test_subpath_recognized - Object.const_set(:SubpathBooksController, Class.new(ActionController::Base)) - rs.draw do |r| r.connect '/books/:id/edit', :controller => 'subpath_books', :action => 'edit' r.connect '/items/:id/:action', :controller => 'subpath_books' @@ -730,13 +718,9 @@ class LegacyRouteSetTests < Test::Unit::TestCase hash = rs.recognize_path "/posts/7" assert_not_nil hash assert_equal %w(subpath_books show 7), [hash[:controller], hash[:action], hash[:id]] - ensure - Object.send(:remove_const, :SubpathBooksController) rescue nil end def test_subpath_generated - Object.const_set(:SubpathBooksController, Class.new(ActionController::Base)) - rs.draw do |r| r.connect '/books/:id/edit', :controller => 'subpath_books', :action => 'edit' r.connect '/items/:id/:action', :controller => 'subpath_books' @@ -746,8 +730,6 @@ class LegacyRouteSetTests < Test::Unit::TestCase assert_equal "/books/7/edit", rs.generate(:controller => "subpath_books", :id => 7, :action => "edit") assert_equal "/items/15/complete", rs.generate(:controller => "subpath_books", :id => 15, :action => "complete") assert_equal "/posts/new/preview", rs.generate(:controller => "subpath_books", :action => "preview") - ensure - Object.send(:remove_const, :SubpathBooksController) rescue nil end def test_failed_requirements_raises_exception_with_violated_requirements @@ -1122,8 +1104,6 @@ class RouteSetTest < ActiveSupport::TestCase end def test_recognize_with_conditions - Object.const_set(:PeopleController, Class.new) - set.draw do |map| map.with_options(:controller => "people") do |people| people.people "/people", :action => "index", :conditions => { :method => :get } @@ -1183,14 +1163,9 @@ class RouteSetTest < ActiveSupport::TestCase assert_equal [:get, :put, :delete], e.allowed_methods end request.recycle! - - ensure - Object.send(:remove_const, :PeopleController) end def test_recognize_with_alias_in_conditions - Object.const_set(:PeopleController, Class.new) - set.draw do |map| map.people "/people", :controller => 'people', :action => "index", :conditions => { :method => :get } @@ -1208,13 +1183,9 @@ class RouteSetTest < ActiveSupport::TestCase assert_nothing_raised { set.recognize(request) } assert_equal("people", request.path_parameters[:controller]) assert_equal("index", request.path_parameters[:action]) - ensure - Object.send(:remove_const, :PeopleController) end def test_typo_recognition - Object.const_set(:ArticlesController, Class.new) - set.draw do |map| map.connect 'articles/:year/:month/:day/:title', :controller => 'articles', :action => 'permalink', @@ -1229,9 +1200,6 @@ class RouteSetTest < ActiveSupport::TestCase assert_equal("11", request.path_parameters[:month]) assert_equal("05", request.path_parameters[:day]) assert_equal("a-very-interesting-article", request.path_parameters[:title]) - - ensure - Object.send(:remove_const, :ArticlesController) end def test_routing_traversal_does_not_load_extra_classes @@ -1248,8 +1216,6 @@ class RouteSetTest < ActiveSupport::TestCase end def test_recognize_with_conditions_and_format - Object.const_set(:PeopleController, Class.new) - set.draw do |map| map.with_options(:controller => "people") do |people| people.person "/people/:id", :action => "show", :conditions => { :method => :get } @@ -1276,8 +1242,6 @@ class RouteSetTest < ActiveSupport::TestCase assert_equal("show", request.path_parameters[:action]) assert_equal("5", request.path_parameters[:id]) assert_equal("png", request.path_parameters[:_format]) - ensure - Object.send(:remove_const, :PeopleController) end def test_generate_with_default_action @@ -1291,8 +1255,6 @@ class RouteSetTest < ActiveSupport::TestCase end def test_root_map - Object.const_set(:PeopleController, Class.new) - set.draw { |map| map.root :controller => "people" } request.path = "" @@ -1300,13 +1262,9 @@ class RouteSetTest < ActiveSupport::TestCase assert_nothing_raised { set.recognize(request) } assert_equal("people", request.path_parameters[:controller]) assert_equal("index", request.path_parameters[:action]) - ensure - Object.send(:remove_const, :PeopleController) end def test_namespace - Object.const_set(:Api, Module.new { |m| m.const_set(:ProductsController, Class.new) }) - set.draw do |map| map.namespace 'api' do |api| @@ -1320,13 +1278,9 @@ class RouteSetTest < ActiveSupport::TestCase assert_nothing_raised { set.recognize(request) } assert_equal("api/products", request.path_parameters[:controller]) assert_equal("inventory", request.path_parameters[:action]) - ensure - Object.send(:remove_const, :Api) end def test_namespaced_root_map - Object.const_set(:Api, Module.new { |m| m.const_set(:ProductsController, Class.new) }) - set.draw do |map| map.namespace 'api' do |api| @@ -1340,13 +1294,9 @@ class RouteSetTest < ActiveSupport::TestCase assert_nothing_raised { set.recognize(request) } assert_equal("api/products", request.path_parameters[:controller]) assert_equal("index", request.path_parameters[:action]) - ensure - Object.send(:remove_const, :Api) end def test_namespace_with_path_prefix - Object.const_set(:Api, Module.new { |m| m.const_set(:ProductsController, Class.new) }) - set.draw do |map| map.namespace 'api', :path_prefix => 'prefix' do |api| api.route 'inventory', :controller => "products", :action => 'inventory' @@ -1358,13 +1308,9 @@ class RouteSetTest < ActiveSupport::TestCase assert_nothing_raised { set.recognize(request) } assert_equal("api/products", request.path_parameters[:controller]) assert_equal("inventory", request.path_parameters[:action]) - ensure - Object.send(:remove_const, :Api) end def test_namespace_with_blank_path_prefix - Object.const_set(:Api, Module.new { |m| m.const_set(:ProductsController, Class.new) }) - set.draw do |map| map.namespace 'api', :path_prefix => '' do |api| api.route 'inventory', :controller => "products", :action => 'inventory' @@ -1376,8 +1322,6 @@ class RouteSetTest < ActiveSupport::TestCase assert_nothing_raised { set.recognize(request) } assert_equal("api/products", request.path_parameters[:controller]) assert_equal("inventory", request.path_parameters[:action]) - ensure - Object.send(:remove_const, :Api) end def test_generate_changes_controller_module diff --git a/actionpack/test/lib/controller/fake_controllers.rb b/actionpack/test/lib/controller/fake_controllers.rb index 2e8c5090ca..7a0216e73c 100644 --- a/actionpack/test/lib/controller/fake_controllers.rb +++ b/actionpack/test/lib/controller/fake_controllers.rb @@ -1,24 +1,33 @@ class << Object; alias_method :const_available?, :const_defined?; end -class ContentController < ActionController::Base -end -class NotAController -end +class ContentController < ActionController::Base; end +class NotAController; end + module Admin class << self; alias_method :const_available?, :const_defined?; end class UserController < ActionController::Base; end class NewsFeedController < ActionController::Base; end end -class ElsewhereController < ActionController::Base; end + +module Api + class ProductsController < ActionController::Base; end +end + +# TODO: Reduce the number of test controllers we use class AddressesController < ActionController::Base; end -class SessionsController < ActionController::Base; end -class FooController < ActionController::Base; end -class CController < ActionController::Base; end -class HiController < ActionController::Base; end -class BraveController < ActionController::Base; end -class ImageController < ActionController::Base; end -class WeblogController < ActionController::Base; end +class ArticlesController < ActionController::Base; end class BarController < ActionController::Base; end +class BooksController < ActionController::Base; end +class BraveController < ActionController::Base; end +class CController < ActionController::Base; end +class ElsewhereController < ActionController::Base; end +class FooController < ActionController::Base; end +class HiController < ActionController::Base; end +class ImageController < ActionController::Base; end +class PeopleController < ActionController::Base; end +class SessionsController < ActionController::Base; end +class SubpathBooksController < ActionController::Base; end +class WeblogController < ActionController::Base; end # For speed test class SpeedController < ActionController::Base; end From 61411f2aeb29edba2d8cd2008020044799ea3d61 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 23:18:32 -0500 Subject: [PATCH 13/61] Redraw default routes on all internal integration tests. We don't need SimpleRouteCase anymore --- actionpack/test/abstract_unit.rb | 18 ++++++++---------- .../test/controller/new_base/base_test.rb | 2 +- .../new_base/content_negotiation_test.rb | 2 +- .../controller/new_base/content_type_test.rb | 6 +++--- .../test/controller/new_base/etag_test.rb | 2 +- .../controller/new_base/render_action_test.rb | 12 ++++++------ .../controller/new_base/render_file_test.rb | 2 +- .../new_base/render_implicit_action_test.rb | 2 +- .../controller/new_base/render_layout_test.rb | 6 +++--- .../controller/new_base/render_partial_test.rb | 2 +- .../controller/new_base/render_rjs_test.rb | 2 +- .../new_base/render_template_test.rb | 6 +++--- .../test/controller/new_base/render_test.rb | 6 +++--- .../controller/new_base/render_text_test.rb | 2 +- .../test/lib/controller/fake_controllers.rb | 5 ----- 15 files changed, 34 insertions(+), 41 deletions(-) diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index c1153b01de..950c85a34b 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -81,6 +81,12 @@ class ActionController::IntegrationTest < ActiveSupport::TestCase ActionController::Routing.const_set(:Routes, real_routes) if real_routes self.class.app = self.class.build_app end + + setup do + ActionController::Routing::Routes.draw do |map| + map.connect ':controller/:action/:id' + end + end end module ActionView @@ -160,9 +166,9 @@ module ActionController super end end - + Base.view_paths = FIXTURE_LOAD_PATH - + class TestCase include TestProcess @@ -214,11 +220,3 @@ module ActionController end end end - -class SimpleRouteCase < Rack::TestCase - setup do - ActionController::Routing::Routes.draw do |map| - map.connect ':controller/:action/:id' - end - end -end diff --git a/actionpack/test/controller/new_base/base_test.rb b/actionpack/test/controller/new_base/base_test.rb index effde324bc..1f9bf7f0fb 100644 --- a/actionpack/test/controller/new_base/base_test.rb +++ b/actionpack/test/controller/new_base/base_test.rb @@ -26,7 +26,7 @@ module Dispatching class ContainedEmptyController < ActionController::Base ; end end - class BaseTest < SimpleRouteCase + class BaseTest < Rack::TestCase # :api: plugin test "simple dispatching" do get "/dispatching/simple/index" diff --git a/actionpack/test/controller/new_base/content_negotiation_test.rb b/actionpack/test/controller/new_base/content_negotiation_test.rb index c43cb677f8..7b38a82f51 100644 --- a/actionpack/test/controller/new_base/content_negotiation_test.rb +++ b/actionpack/test/controller/new_base/content_negotiation_test.rb @@ -9,7 +9,7 @@ module ContentNegotiation )] end - class TestContentNegotiation < SimpleRouteCase + class TestContentNegotiation < Rack::TestCase test "A */* Accept header will return HTML" do get "/content_negotiation/basic/hello", {}, "HTTP_ACCEPT" => "*/*" assert_body "Hello world */*!" diff --git a/actionpack/test/controller/new_base/content_type_test.rb b/actionpack/test/controller/new_base/content_type_test.rb index 898d0bb9f3..0ff5552b08 100644 --- a/actionpack/test/controller/new_base/content_type_test.rb +++ b/actionpack/test/controller/new_base/content_type_test.rb @@ -44,7 +44,7 @@ module ContentType end end - class ExplicitContentTypeTest < SimpleRouteCase + class ExplicitContentTypeTest < Rack::TestCase test "default response is HTML and UTF8" do get "/content_type/base" @@ -67,7 +67,7 @@ module ContentType end end - class ImpliedContentTypeTest < SimpleRouteCase + class ImpliedContentTypeTest < Rack::TestCase test "sets Content-Type as text/html when rendering *.html.erb" do get "/content_type/implied/i_am_html_erb" @@ -93,7 +93,7 @@ module ContentType end end - class ExplicitCharsetTest < SimpleRouteCase + class ExplicitCharsetTest < Rack::TestCase test "setting the charset of the response directly on the response object" do get "/content_type/charset/set_on_response_obj" diff --git a/actionpack/test/controller/new_base/etag_test.rb b/actionpack/test/controller/new_base/etag_test.rb index d5b7942ab6..51bfb2278a 100644 --- a/actionpack/test/controller/new_base/etag_test.rb +++ b/actionpack/test/controller/new_base/etag_test.rb @@ -16,7 +16,7 @@ module Etags end end - class EtagTest < SimpleRouteCase + class EtagTest < Rack::TestCase describe "Rendering without any special etag options returns an etag that is an MD5 hash of its text" test "an action without a layout" do diff --git a/actionpack/test/controller/new_base/render_action_test.rb b/actionpack/test/controller/new_base/render_action_test.rb index d5896c1ebd..ecd29c4530 100644 --- a/actionpack/test/controller/new_base/render_action_test.rb +++ b/actionpack/test/controller/new_base/render_action_test.rb @@ -45,7 +45,7 @@ module RenderAction end - class RenderActionTest < SimpleRouteCase + class RenderActionTest < Rack::TestCase test "rendering an action using :action => " do get "/render_action/basic/hello_world" @@ -82,7 +82,7 @@ module RenderAction end end - class RenderLayoutTest < SimpleRouteCase + class RenderLayoutTest < Rack::TestCase describe "Both .html.erb and application.html.erb are missing" test "rendering with layout => true" do @@ -150,7 +150,7 @@ module RenderActionWithApplicationLayout end end - class LayoutTest < SimpleRouteCase + class LayoutTest < Rack::TestCase describe "Only application.html.erb is present and .html.erb is missing" test "rendering implicit application.html.erb as layout" do @@ -189,7 +189,7 @@ module RenderActionWithApplicationLayout end end - class TestLayout < SimpleRouteCase + class TestLayout < Rack::TestCase testing BasicController test "builder works with layouts" do @@ -228,7 +228,7 @@ module RenderActionWithControllerLayout end end - class ControllerLayoutTest < SimpleRouteCase + class ControllerLayoutTest < Rack::TestCase describe "Only .html.erb is present and application.html.erb is missing" test "render hello_world and implicitly use .html.erb as a layout." do @@ -286,7 +286,7 @@ module RenderActionWithBothLayouts end end - class ControllerLayoutTest < SimpleRouteCase + class ControllerLayoutTest < Rack::TestCase describe "Both .html.erb and application.html.erb are present" test "rendering implicitly use .html.erb over application.html.erb as a layout" do diff --git a/actionpack/test/controller/new_base/render_file_test.rb b/actionpack/test/controller/new_base/render_file_test.rb index 864469e0ae..8b2fdf8f96 100644 --- a/actionpack/test/controller/new_base/render_file_test.rb +++ b/actionpack/test/controller/new_base/render_file_test.rb @@ -48,7 +48,7 @@ module RenderFile end end - class TestBasic < SimpleRouteCase + class TestBasic < Rack::TestCase testing RenderFile::BasicController test "rendering simple template" do diff --git a/actionpack/test/controller/new_base/render_implicit_action_test.rb b/actionpack/test/controller/new_base/render_implicit_action_test.rb index 2b78fa7d4f..90cc7933ff 100644 --- a/actionpack/test/controller/new_base/render_implicit_action_test.rb +++ b/actionpack/test/controller/new_base/render_implicit_action_test.rb @@ -10,7 +10,7 @@ module RenderImplicitAction def hello_world() end end - class RenderImplicitActionTest < SimpleRouteCase + class RenderImplicitActionTest < Rack::TestCase test "render a simple action with new explicit call to render" do get "/render_implicit_action/simple/hello_world" diff --git a/actionpack/test/controller/new_base/render_layout_test.rb b/actionpack/test/controller/new_base/render_layout_test.rb index f840a47ecf..6a9668b81a 100644 --- a/actionpack/test/controller/new_base/render_layout_test.rb +++ b/actionpack/test/controller/new_base/render_layout_test.rb @@ -36,7 +36,7 @@ module ControllerLayouts end end - class RenderLayoutTest < SimpleRouteCase + class RenderLayoutTest < Rack::TestCase test "rendering a normal template, but using the implicit layout" do get "/controller_layouts/implicit/index" @@ -58,7 +58,7 @@ module ControllerLayouts end - class LayoutOptionsTest < SimpleRouteCase + class LayoutOptionsTest < Rack::TestCase testing ControllerLayouts::ImplicitController test "rendering with :layout => false leaves out the implicit layout" do @@ -79,7 +79,7 @@ module ControllerLayouts end end - class MismatchFormatTest < SimpleRouteCase + class MismatchFormatTest < Rack::TestCase testing ControllerLayouts::MismatchFormatController test "if JS is selected, an HTML template is not also selected" do diff --git a/actionpack/test/controller/new_base/render_partial_test.rb b/actionpack/test/controller/new_base/render_partial_test.rb index 7c2c20e1c7..8fddcbcd57 100644 --- a/actionpack/test/controller/new_base/render_partial_test.rb +++ b/actionpack/test/controller/new_base/render_partial_test.rb @@ -15,7 +15,7 @@ module RenderPartial end end - class TestPartial < SimpleRouteCase + class TestPartial < Rack::TestCase testing BasicController test "rendering a partial in ActionView doesn't pull the ivars again from the controller" do diff --git a/actionpack/test/controller/new_base/render_rjs_test.rb b/actionpack/test/controller/new_base/render_rjs_test.rb index 7b76c54ab9..8c47b38ab6 100644 --- a/actionpack/test/controller/new_base/render_rjs_test.rb +++ b/actionpack/test/controller/new_base/render_rjs_test.rb @@ -21,7 +21,7 @@ module RenderRjs end end - class TestBasic < SimpleRouteCase + class TestBasic < Rack::TestCase testing BasicController def setup diff --git a/actionpack/test/controller/new_base/render_template_test.rb b/actionpack/test/controller/new_base/render_template_test.rb index 3b24c2d75a..c81b951c0d 100644 --- a/actionpack/test/controller/new_base/render_template_test.rb +++ b/actionpack/test/controller/new_base/render_template_test.rb @@ -39,7 +39,7 @@ module RenderTemplate end end - class TestWithoutLayout < SimpleRouteCase + class TestWithoutLayout < Rack::TestCase testing RenderTemplate::WithoutLayoutController test "rendering a normal template with full path without layout" do @@ -107,7 +107,7 @@ module RenderTemplate end end - class TestWithLayout < SimpleRouteCase + class TestWithLayout < Rack::TestCase describe "Rendering with :template using implicit or explicit layout" test "rendering with implicit layout" do @@ -158,7 +158,7 @@ module RenderTemplate end end - class TestTemplateRenderWithForwardSlash < SimpleRouteCase + class TestTemplateRenderWithForwardSlash < Rack::TestCase test "rendering a normal template with full path starting with a leading slash" do get "/render_template/compatibility/without_layout/with_forward_slash" diff --git a/actionpack/test/controller/new_base/render_test.rb b/actionpack/test/controller/new_base/render_test.rb index 804be79d17..d985d9f9ad 100644 --- a/actionpack/test/controller/new_base/render_test.rb +++ b/actionpack/test/controller/new_base/render_test.rb @@ -35,7 +35,7 @@ module Render end end - class RenderTest < SimpleRouteCase + class RenderTest < Rack::TestCase test "render with blank" do get "/render/blank_render" @@ -50,7 +50,7 @@ module Render end end - class TestOnlyRenderPublicActions < SimpleRouteCase + class TestOnlyRenderPublicActions < Rack::TestCase describe "Only public methods on actual controllers are callable actions" test "raises an exception when a method of Object is called" do @@ -66,7 +66,7 @@ module Render end end - class TestVariousObjectsAvailableInView < SimpleRouteCase + class TestVariousObjectsAvailableInView < Rack::TestCase test "The request object is accessible in the view" do get "/render/blank_render/access_request" assert_body "The request: GET" diff --git a/actionpack/test/controller/new_base/render_text_test.rb b/actionpack/test/controller/new_base/render_text_test.rb index f5839ee16f..0e6f51c998 100644 --- a/actionpack/test/controller/new_base/render_text_test.rb +++ b/actionpack/test/controller/new_base/render_text_test.rb @@ -62,7 +62,7 @@ module RenderText end end - class RenderTextTest < SimpleRouteCase + class RenderTextTest < Rack::TestCase describe "Rendering text using render :text" test "rendering text from a action with default options renders the text with the layout" do diff --git a/actionpack/test/lib/controller/fake_controllers.rb b/actionpack/test/lib/controller/fake_controllers.rb index 7a0216e73c..9ec7f330b8 100644 --- a/actionpack/test/lib/controller/fake_controllers.rb +++ b/actionpack/test/lib/controller/fake_controllers.rb @@ -44,8 +44,3 @@ class ChannelsController < SpeedController; end class ChannelVideosController < SpeedController; end class LostPasswordsController < SpeedController; end class PagesController < SpeedController; end - -ActionController::Routing::Routes.draw do |map| - map.route_one 'route_one', :controller => 'elsewhere', :action => 'flash_me' - map.connect ':controller/:action/:id' -end From 86ed58d9121844e2d881be67e2d845ae721f72e7 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 23:31:38 -0500 Subject: [PATCH 14/61] Use with_routing helper in tests instead of modifying global route set --- actionpack/test/controller/caching_test.rb | 22 +-- .../test/controller/mime_responds_test.rb | 138 ++++++++++-------- .../test/controller/verification_test.rb | 17 +-- actionpack/test/template/test_test.rb | 37 +++-- 4 files changed, 119 insertions(+), 95 deletions(-) diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index 1a9f95e5e9..69b0eb5e3e 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -46,14 +46,8 @@ end class PageCachingTest < ActionController::TestCase def setup super - ActionController::Base.perform_caching = true - ActionController::Routing::Routes.draw do |map| - map.main '', :controller => 'posts', :format => nil - map.formatted_posts 'posts.:format', :controller => 'posts' - map.resources :posts - map.connect ':controller/:action/:id' - end + ActionController::Base.perform_caching = true @request = ActionController::TestRequest.new @request.host = 'hostname.com' @@ -74,10 +68,16 @@ class PageCachingTest < ActionController::TestCase end def test_page_caching_resources_saves_to_correct_path_with_extension_even_if_default_route - @params[:format] = 'rss' - assert_equal '/posts.rss', @rewriter.rewrite(@params) - @params[:format] = nil - assert_equal '/', @rewriter.rewrite(@params) + with_routing do |set| + set.draw do |map| + map.main '', :controller => 'posts', :format => nil + map.formatted_posts 'posts.:format', :controller => 'posts' + end + @params[:format] = 'rss' + assert_equal '/posts.rss', @rewriter.rewrite(@params) + @params[:format] = nil + assert_equal '/', @rewriter.rewrite(@params) + end end def test_should_cache_get_with_ok_status diff --git a/actionpack/test/controller/mime_responds_test.rb b/actionpack/test/controller/mime_responds_test.rb index 93a815adae..a79648396c 100644 --- a/actionpack/test/controller/mime_responds_test.rb +++ b/actionpack/test/controller/mime_responds_test.rb @@ -527,12 +527,6 @@ class RespondWithControllerTest < ActionController::TestCase super ActionController::Base.use_accept_header = true @request.host = "www.example.com" - - ActionController::Routing::Routes.draw do |map| - map.resources :customers - map.resources :quiz_stores, :has_many => :customers - map.connect ":controller/:action/:id" - end end def teardown @@ -593,53 +587,59 @@ class RespondWithControllerTest < ActionController::TestCase end def test_using_resource_for_post_with_html - post :using_resource - assert_equal "text/html", @response.content_type - assert_equal 302, @response.status - assert_equal "http://www.example.com/customers/13", @response.location - assert @response.redirect? + with_test_route_set do + post :using_resource + assert_equal "text/html", @response.content_type + assert_equal 302, @response.status + assert_equal "http://www.example.com/customers/13", @response.location + assert @response.redirect? - errors = { :name => :invalid } - Customer.any_instance.stubs(:errors).returns(errors) - post :using_resource - assert_equal "text/html", @response.content_type - assert_equal 200, @response.status - assert_equal "New world!\n", @response.body - assert_nil @response.location + errors = { :name => :invalid } + Customer.any_instance.stubs(:errors).returns(errors) + post :using_resource + assert_equal "text/html", @response.content_type + assert_equal 200, @response.status + assert_equal "New world!\n", @response.body + assert_nil @response.location + end end def test_using_resource_for_post_with_xml - @request.accept = "application/xml" + with_test_route_set do + @request.accept = "application/xml" - post :using_resource - assert_equal "application/xml", @response.content_type - assert_equal 201, @response.status - assert_equal "david", @response.body - assert_equal "http://www.example.com/customers/13", @response.location + post :using_resource + assert_equal "application/xml", @response.content_type + assert_equal 201, @response.status + assert_equal "david", @response.body + assert_equal "http://www.example.com/customers/13", @response.location - errors = { :name => :invalid } - Customer.any_instance.stubs(:errors).returns(errors) - post :using_resource - assert_equal "application/xml", @response.content_type - assert_equal 422, @response.status - assert_equal errors.to_xml, @response.body - assert_nil @response.location + errors = { :name => :invalid } + Customer.any_instance.stubs(:errors).returns(errors) + post :using_resource + assert_equal "application/xml", @response.content_type + assert_equal 422, @response.status + assert_equal errors.to_xml, @response.body + assert_nil @response.location + end end def test_using_resource_for_put_with_html - put :using_resource - assert_equal "text/html", @response.content_type - assert_equal 302, @response.status - assert_equal "http://www.example.com/customers/13", @response.location - assert @response.redirect? + with_test_route_set do + put :using_resource + assert_equal "text/html", @response.content_type + assert_equal 302, @response.status + assert_equal "http://www.example.com/customers/13", @response.location + assert @response.redirect? - errors = { :name => :invalid } - Customer.any_instance.stubs(:errors).returns(errors) - put :using_resource - assert_equal "text/html", @response.content_type - assert_equal 200, @response.status - assert_equal "Edit world!\n", @response.body - assert_nil @response.location + errors = { :name => :invalid } + Customer.any_instance.stubs(:errors).returns(errors) + put :using_resource + assert_equal "text/html", @response.content_type + assert_equal 200, @response.status + assert_equal "Edit world!\n", @response.body + assert_nil @response.location + end end def test_using_resource_for_put_with_xml @@ -660,11 +660,13 @@ class RespondWithControllerTest < ActionController::TestCase end def test_using_resource_for_delete_with_html - Customer.any_instance.stubs(:destroyed?).returns(true) - delete :using_resource - assert_equal "text/html", @response.content_type - assert_equal 302, @response.status - assert_equal "http://www.example.com/customers", @response.location + with_test_route_set do + Customer.any_instance.stubs(:destroyed?).returns(true) + delete :using_resource + assert_equal "text/html", @response.content_type + assert_equal 302, @response.status + assert_equal "http://www.example.com/customers", @response.location + end end def test_using_resource_for_delete_with_xml @@ -685,21 +687,23 @@ class RespondWithControllerTest < ActionController::TestCase end def test_using_resource_with_parent_for_post - @request.accept = "application/xml" + with_test_route_set do + @request.accept = "application/xml" - post :using_resource_with_parent - assert_equal "application/xml", @response.content_type - assert_equal 201, @response.status - assert_equal "david", @response.body - assert_equal "http://www.example.com/quiz_stores/11/customers/13", @response.location + post :using_resource_with_parent + assert_equal "application/xml", @response.content_type + assert_equal 201, @response.status + assert_equal "david", @response.body + assert_equal "http://www.example.com/quiz_stores/11/customers/13", @response.location - errors = { :name => :invalid } - Customer.any_instance.stubs(:errors).returns(errors) - post :using_resource - assert_equal "application/xml", @response.content_type - assert_equal 422, @response.status - assert_equal errors.to_xml, @response.body - assert_nil @response.location + errors = { :name => :invalid } + Customer.any_instance.stubs(:errors).returns(errors) + post :using_resource + assert_equal "application/xml", @response.content_type + assert_equal 422, @response.status + assert_equal errors.to_xml, @response.body + assert_nil @response.location + end end def test_using_resource_with_collection @@ -773,6 +777,18 @@ class RespondWithControllerTest < ActionController::TestCase get :default_overwritten assert_equal 406, @response.status end + + private + def with_test_route_set + with_routing do |set| + set.draw do |map| + map.resources :customers + map.resources :quiz_stores, :has_many => :customers + map.connect ":controller/:action/:id" + end + yield + end + end end class AbstractPostController < ActionController::Base diff --git a/actionpack/test/controller/verification_test.rb b/actionpack/test/controller/verification_test.rb index ee558f3465..1a9eb65f29 100644 --- a/actionpack/test/controller/verification_test.rb +++ b/actionpack/test/controller/verification_test.rb @@ -111,13 +111,6 @@ class VerificationTest < ActionController::TestCase tests TestController - setup do - ActionController::Routing::Routes.draw do |map| - map.foo '/foo', :controller => 'test', :action => 'foo' - map.connect ":controller/:action/:id" - end - end - def test_using_symbol_back_with_no_referrer assert_raise(ActionController::RedirectBackError) { get :guarded_with_back } end @@ -130,8 +123,14 @@ class VerificationTest < ActionController::TestCase def test_no_deprecation_warning_for_named_route assert_not_deprecated do - get :guarded_one_for_named_route_test, :two => "not one" - assert_redirected_to '/foo' + with_routing do |set| + set.draw do |map| + map.foo '/foo', :controller => 'test', :action => 'foo' + map.connect ":controller/:action/:id" + end + get :guarded_one_for_named_route_test, :two => "not one" + assert_redirected_to '/foo' + end end end diff --git a/actionpack/test/template/test_test.rb b/actionpack/test/template/test_test.rb index f32d0b3d42..05a14f3554 100644 --- a/actionpack/test/template/test_test.rb +++ b/actionpack/test/template/test_test.rb @@ -19,32 +19,41 @@ module PeopleHelper end class PeopleHelperTest < ActionView::TestCase - def setup - super - ActionController::Routing::Routes.draw do |map| - map.people 'people', :controller => 'people', :action => 'index' - map.connect ':controller/:action/:id' - end - end - def test_title assert_equal "

Ruby on Rails

", title("Ruby on Rails") end def test_homepage_path - assert_equal "/people", homepage_path + with_test_route_set do + assert_equal "/people", homepage_path + end end def test_homepage_url - assert_equal "http://test.host/people", homepage_url + with_test_route_set do + assert_equal "http://test.host/people", homepage_url + end end def test_link_to_person - person = mock(:name => "David") - person.class.extend ActiveModel::Naming - expects(:mocha_mock_path).with(person).returns("/people/1") - assert_equal 'David', link_to_person(person) + with_test_route_set do + person = mock(:name => "David") + person.class.extend ActiveModel::Naming + expects(:mocha_mock_path).with(person).returns("/people/1") + assert_equal 'David', link_to_person(person) + end end + + private + def with_test_route_set + with_routing do |set| + set.draw do |map| + map.people 'people', :controller => 'people', :action => 'index' + map.connect ':controller/:action/:id' + end + yield + end + end end class CrazyHelperTest < ActionView::TestCase From 86d059002291b4cc320a86fc3702e40375e0b811 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 3 Oct 2009 23:55:35 -0500 Subject: [PATCH 15/61] Only draw default route once --- actionpack/test/abstract_unit.rb | 59 ++++++++++++++++++++------------ 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index 950c85a34b..1214d608a4 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -53,6 +53,43 @@ ORIGINAL_LOCALES = I18n.available_locales.map {|locale| locale.to_s }.sort FIXTURE_LOAD_PATH = File.join(File.dirname(__FILE__), 'fixtures') FIXTURES = Pathname.new(FIXTURE_LOAD_PATH) +module SetupOnce + extend ActiveSupport::Concern + + included do + cattr_accessor :setup_once_block + self.setup_once_block = nil + + setup :run_setup_once + end + + module ClassMethods + def setup_once(&block) + self.setup_once_block = block + end + end + + private + def run_setup_once + if self.setup_once_block + self.setup_once_block.call + self.setup_once_block = nil + end + end +end + +class ActiveSupport::TestCase + include SetupOnce + + # Hold off drawing routes until all the possible controller classes + # have been loaded. + setup_once do + ActionController::Routing::Routes.draw do |map| + map.connect ':controller/:action/:id' + end + end +end + class ActionController::IntegrationTest < ActiveSupport::TestCase def self.build_app(routes = nil) ActionDispatch::MiddlewareStack.new { |middleware| @@ -81,22 +118,6 @@ class ActionController::IntegrationTest < ActiveSupport::TestCase ActionController::Routing.const_set(:Routes, real_routes) if real_routes self.class.app = self.class.build_app end - - setup do - ActionController::Routing::Routes.draw do |map| - map.connect ':controller/:action/:id' - end - end -end - -module ActionView - class TestCase - setup do - ActionController::Routing::Routes.draw do |map| - map.connect ':controller/:action/:id' - end - end - end end # Temporary base class @@ -172,12 +193,6 @@ module ActionController class TestCase include TestProcess - setup do - ActionController::Routing::Routes.draw do |map| - map.connect ':controller/:action/:id' - end - end - def assert_template(options = {}, message = nil) validate_request! From c97c31b096e627480b64403d1460065738941c3e Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sun, 4 Oct 2009 12:45:53 -0500 Subject: [PATCH 16/61] Fix Dispatch.new so passenger works --- actionpack/lib/action_controller/dispatch/dispatcher.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_controller/dispatch/dispatcher.rb b/actionpack/lib/action_controller/dispatch/dispatcher.rb index e04da42637..008fb54715 100644 --- a/actionpack/lib/action_controller/dispatch/dispatcher.rb +++ b/actionpack/lib/action_controller/dispatch/dispatcher.rb @@ -50,7 +50,7 @@ module ActionController def new # DEPRECATE Rails application fallback - Rails.application + Rails.application.new end end end From 49b52cadc2e66c11a025e7719837ae77b3736046 Mon Sep 17 00:00:00 2001 From: Michael Koziarski Date: Mon, 5 Oct 2009 17:23:37 +1300 Subject: [PATCH 17/61] Revert "Fix Dispatch.new so passenger works" as it broke the build This reverts commit c97c31b096e627480b64403d1460065738941c3e. --- actionpack/lib/action_controller/dispatch/dispatcher.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_controller/dispatch/dispatcher.rb b/actionpack/lib/action_controller/dispatch/dispatcher.rb index 008fb54715..e04da42637 100644 --- a/actionpack/lib/action_controller/dispatch/dispatcher.rb +++ b/actionpack/lib/action_controller/dispatch/dispatcher.rb @@ -50,7 +50,7 @@ module ActionController def new # DEPRECATE Rails application fallback - Rails.application.new + Rails.application end end end From 76d823677fced041cc52b5b1b63d67a0da5a17bd Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 5 Oct 2009 09:17:51 -0500 Subject: [PATCH 18/61] Revert "Revert "Fix Dispatch.new so passenger works" as it broke the build" This reverts commit 49b52cadc2e66c11a025e7719837ae77b3736046. --- actionpack/lib/action_controller/dispatch/dispatcher.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_controller/dispatch/dispatcher.rb b/actionpack/lib/action_controller/dispatch/dispatcher.rb index e04da42637..008fb54715 100644 --- a/actionpack/lib/action_controller/dispatch/dispatcher.rb +++ b/actionpack/lib/action_controller/dispatch/dispatcher.rb @@ -50,7 +50,7 @@ module ActionController def new # DEPRECATE Rails application fallback - Rails.application + Rails.application.new end end end From 635aa91224b5743c5736f916db18e80713650b86 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 5 Oct 2009 09:41:08 -0500 Subject: [PATCH 19/61] More robust console test --- railties/test/application/console_test.rb | 52 +++++++++++++++++++++++ railties/test/console_app_test.rb | 43 ------------------- railties/test/isolation/abstract_unit.rb | 4 ++ 3 files changed, 56 insertions(+), 43 deletions(-) create mode 100644 railties/test/application/console_test.rb delete mode 100644 railties/test/console_app_test.rb diff --git a/railties/test/application/console_test.rb b/railties/test/application/console_test.rb new file mode 100644 index 0000000000..e8a4a4e158 --- /dev/null +++ b/railties/test/application/console_test.rb @@ -0,0 +1,52 @@ +require 'isolation/abstract_unit' + +class ConsoleTest < Test::Unit::TestCase + include ActiveSupport::Testing::Isolation + + def setup + build_app + boot_rails + + # Load steps taken from rails/commands/console.rb + require "#{rails_root}/config/environment" + require 'rails/console_app' + require 'rails/console_with_helpers' + end + + def test_app_method_should_return_integration_session + console_session = app + assert_not_nil console_session + assert_instance_of ActionController::Integration::Session, console_session + end + + def test_new_session_should_return_integration_session + session = new_session + assert_not_nil session + assert_instance_of ActionController::Integration::Session, session + end + + def test_reload_should_fire_preparation_callbacks + a = b = c = nil + + # TODO: These should be defined on the initializer + ActionDispatch::Callbacks.to_prepare { a = b = c = 1 } + ActionDispatch::Callbacks.to_prepare { b = c = 2 } + ActionDispatch::Callbacks.to_prepare { c = 3 } + + # Hide Reloading... output + silence_stream(STDOUT) do + reload! + end + + assert_equal 1, a + assert_equal 2, b + assert_equal 3, c + end + + def test_access_to_helpers + assert_not_nil helper + assert_instance_of ActionView::Base, helper + assert_equal 'Once upon a time in a world...', + helper.truncate('Once upon a time in a world far far away') + end +end diff --git a/railties/test/console_app_test.rb b/railties/test/console_app_test.rb deleted file mode 100644 index 1437e6d885..0000000000 --- a/railties/test/console_app_test.rb +++ /dev/null @@ -1,43 +0,0 @@ -require 'abstract_unit' - -require 'action_controller' # console_app uses 'action_controller/integration' - -require 'rails/dispatcher' -require 'rails/console_app' - -module Rails - def self.application - ActionController::Routing::Routes - end -end - -# console_app sets Test::Unit.run to work around the at_exit hook in test/unit, which kills IRB -if Test::Unit.respond_to?(:run=) - Test::Unit.run = false - - class ConsoleAppTest < Test::Unit::TestCase - def test_app_method_should_return_integration_session - assert_nothing_thrown do - console_session = app - assert_not_nil console_session - assert_instance_of ActionController::Integration::Session, - console_session - end - end - - def test_reload_should_fire_preparation_callbacks - a = b = c = nil - - ActionDispatch::Callbacks.to_prepare { a = b = c = 1 } - ActionDispatch::Callbacks.to_prepare { b = c = 2 } - ActionDispatch::Callbacks.to_prepare { c = 3 } - ActionController::Routing::Routes.expects(:reload) - - reload! - - assert_equal 1, a - assert_equal 2, b - assert_equal 3, c - end - end -end diff --git a/railties/test/isolation/abstract_unit.rb b/railties/test/isolation/abstract_unit.rb index 869e8429cf..a55ee6c01d 100644 --- a/railties/test/isolation/abstract_unit.rb +++ b/railties/test/isolation/abstract_unit.rb @@ -30,6 +30,10 @@ module TestHelpers def app_path(*args) tmp_path(*%w[app] + args) end + + def rails_root + app_path + end end module Rack From 444ba150bd344793ebd0bd5efa6efd93fee89a7c Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 5 Oct 2009 09:42:35 -0500 Subject: [PATCH 20/61] Put test in place for deprecated dispatcher --- railties/test/application/load_test.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/railties/test/application/load_test.rb b/railties/test/application/load_test.rb index 5158abdbb4..305cd7f273 100644 --- a/railties/test/application/load_test.rb +++ b/railties/test/application/load_test.rb @@ -43,6 +43,13 @@ module ApplicationTests assert Rails.application.new.is_a?(Rails::Application) end + # Passenger still uses AC::Dispatcher, so we need to + # keep it working for now + test "deprecated ActionController::Dispatcher still works" do + rackup + assert ActionController::Dispatcher.new.is_a?(Rails::Application) + end + test "the config object is available on the application object" do rackup assert_equal 'UTC', Rails.application.config.time_zone From 7de5f69cc6bc9a946afd3a0fa0ec785f9fb94fb5 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 5 Oct 2009 10:01:46 -0500 Subject: [PATCH 21/61] Try to load lib before trying to activate the gem for testing --- railties/test/abstract_unit.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/railties/test/abstract_unit.rb b/railties/test/abstract_unit.rb index 4510e6241c..50c427dc64 100644 --- a/railties/test/abstract_unit.rb +++ b/railties/test/abstract_unit.rb @@ -27,8 +27,12 @@ else end def uses_gem(gem_name, test_name, version = '> 0') - gem gem_name.to_s, version - require gem_name.to_s + begin + require gem_name.to_s + rescue LoadError + gem gem_name.to_s, version + require gem_name.to_s + end yield rescue LoadError $stderr.puts "Skipping #{test_name} tests. `gem install #{gem_name}` and try again." From 20d6938453f439531a13e2ef1fd0905edf56294c Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 5 Oct 2009 10:36:05 -0500 Subject: [PATCH 22/61] Rewrite FCGI handler test --- railties/lib/rails/fcgi_handler.rb | 4 +- railties/test/abstract_unit.rb | 12 ---- .../{ => application}/fcgi_dispatcher_test.rb | 64 +++++++++++++------ 3 files changed, 46 insertions(+), 34 deletions(-) rename railties/test/{ => application}/fcgi_dispatcher_test.rb (88%) diff --git a/railties/lib/rails/fcgi_handler.rb b/railties/lib/rails/fcgi_handler.rb index ef6f3b094c..77dce5f325 100644 --- a/railties/lib/rails/fcgi_handler.rb +++ b/railties/lib/rails/fcgi_handler.rb @@ -216,7 +216,9 @@ class RailsFCGIHandler def restore! $".replace @features - Dispatcher.reset_application! + # TODO: Reloading the application should be the "Application"s + # responsibility + ActionDispatch::Callbacks.new(lambda {}, true) ActionController::Routing::Routes.reload end diff --git a/railties/test/abstract_unit.rb b/railties/test/abstract_unit.rb index 50c427dc64..6c6af0b2bf 100644 --- a/railties/test/abstract_unit.rb +++ b/railties/test/abstract_unit.rb @@ -25,15 +25,3 @@ if defined?(RAILS_ROOT) else RAILS_ROOT = File.dirname(__FILE__) end - -def uses_gem(gem_name, test_name, version = '> 0') - begin - require gem_name.to_s - rescue LoadError - gem gem_name.to_s, version - require gem_name.to_s - end - yield -rescue LoadError - $stderr.puts "Skipping #{test_name} tests. `gem install #{gem_name}` and try again." -end diff --git a/railties/test/fcgi_dispatcher_test.rb b/railties/test/application/fcgi_dispatcher_test.rb similarity index 88% rename from railties/test/fcgi_dispatcher_test.rb rename to railties/test/application/fcgi_dispatcher_test.rb index 4d77a321a0..b8ab9f8996 100644 --- a/railties/test/fcgi_dispatcher_test.rb +++ b/railties/test/application/fcgi_dispatcher_test.rb @@ -1,18 +1,25 @@ -require 'abstract_unit' +require 'isolation/abstract_unit' +require 'mocha' -uses_gem "fcgi", "0.8.7" do +begin -require 'action_controller' -require 'rails/fcgi_handler' - -module Rails - def self.application - ActionController::Routing::Routes - end +begin + require 'fcgi' +rescue LoadError + gem 'fcgi', '0.8.7' + require 'fcgi' end class RailsFCGIHandlerTest < Test::Unit::TestCase + include ActiveSupport::Testing::Isolation + def setup + build_app + boot_rails + + require "#{rails_root}/config/environment" + require 'rails/fcgi_handler' + @log = StringIO.new @handler = RailsFCGIHandler.new(@log) end @@ -87,7 +94,6 @@ class RailsFCGIHandlerTest < Test::Unit::TestCase assert_nil @handler.when_ready end - def test_reload_runs_gc_when_gc_request_period_set @handler.expects(:run_gc!) @handler.expects(:restore!) @@ -111,7 +117,6 @@ class RailsFCGIHandlerTest < Test::Unit::TestCase def test_restore! $".expects(:replace) - Dispatcher.expects(:reset_application!) ActionController::Routing::Routes.expects(:reload) @handler.send(:restore!) end @@ -127,17 +132,24 @@ class RailsFCGIHandlerTest < Test::Unit::TestCase end end - class RailsFCGIHandlerSignalsTest < Test::Unit::TestCase - class ::RailsFCGIHandler - attr_accessor :signal - alias_method :old_gc_countdown, :gc_countdown - def gc_countdown - signal ? Process.kill(signal, $$) : old_gc_countdown - end - end + include ActiveSupport::Testing::Isolation def setup + build_app + boot_rails + + require "#{rails_root}/config/environment" + require 'rails/fcgi_handler' + + ::RailsFCGIHandler.class_eval do + attr_accessor :signal + alias_method :old_gc_countdown, :gc_countdown + def gc_countdown + signal ? Process.kill(signal, $$) : old_gc_countdown + end + end + @log = StringIO.new @handler = RailsFCGIHandler.new(@log) @dispatcher = mock @@ -232,9 +244,16 @@ class RailsFCGIHandlerSignalsTest < Test::Unit::TestCase end end - class RailsFCGIHandlerPeriodicGCTest < Test::Unit::TestCase + include ActiveSupport::Testing::Isolation + def setup + build_app + boot_rails + + require "#{rails_root}/config/environment" + require 'rails/fcgi_handler' + @log = StringIO.new end @@ -265,4 +284,7 @@ class RailsFCGIHandlerPeriodicGCTest < Test::Unit::TestCase assert_nil @handler.when_ready end end -end # uses_gem "fcgi" + +rescue LoadError + $stderr.puts 'Skipping fcgi tests. `gem install fcgi` and try again.' +end From 570f055c44a0b6da973f63689f8fedbef9fe32d3 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 5 Oct 2009 11:16:24 -0500 Subject: [PATCH 23/61] Yank FCGI Handler from core http://github.com/rails/fcgi_handler --- railties/lib/rails/commands/ncgi/listener | 86 ------ railties/lib/rails/fcgi_handler.rb | 241 --------------- .../generators/rails/app/app_generator.rb | 16 - .../app/templates/dispatchers/dispatch.fcgi | 24 -- .../app/templates/dispatchers/dispatch.rb | 10 - .../app/templates/dispatchers/gateway.cgi | 97 ------ railties/lib/rails/tasks/framework.rake | 5 - .../test/application/fcgi_dispatcher_test.rb | 290 ------------------ .../test/generators/app_generator_test.rb | 12 - 9 files changed, 781 deletions(-) delete mode 100755 railties/lib/rails/commands/ncgi/listener delete mode 100644 railties/lib/rails/fcgi_handler.rb delete mode 100755 railties/lib/rails/generators/rails/app/templates/dispatchers/dispatch.fcgi delete mode 100755 railties/lib/rails/generators/rails/app/templates/dispatchers/dispatch.rb delete mode 100755 railties/lib/rails/generators/rails/app/templates/dispatchers/gateway.cgi delete mode 100644 railties/test/application/fcgi_dispatcher_test.rb diff --git a/railties/lib/rails/commands/ncgi/listener b/railties/lib/rails/commands/ncgi/listener deleted file mode 100755 index 7079ef78a6..0000000000 --- a/railties/lib/rails/commands/ncgi/listener +++ /dev/null @@ -1,86 +0,0 @@ -#!/usr/bin/env ruby - -require 'stringio' -require 'fileutils' -require 'fcgi_handler' - -def message(s) - $stderr.puts "listener: #{s}" if ENV && ENV["DEBUG_GATEWAY"] -end - -class RemoteCGI < CGI - attr_accessor :stdinput, :stdoutput, :env_table - def initialize(env_table, input = nil, output = nil) - self.env_table = env_table - self.stdinput = input || StringIO.new - self.stdoutput = output || StringIO.new - super() - end - - def out(stream) # Ignore the requested output stream - super(stdoutput) - end -end - -class Listener - include DRbUndumped - - def initialize(timeout, socket_path) - @socket = File.expand_path(socket_path) - @mutex = Mutex.new - @active = false - @timeout = timeout - - @handler = RailsFCGIHandler.new - @handler.extend DRbUndumped - - message 'opening socket' - DRb.start_service("drbunix:#{@socket}", self) - - message 'entering process loop' - @handler.process! self - end - - def each_cgi(&cgi_block) - @cgi_block = cgi_block - message 'entering idle loop' - loop do - sleep @timeout rescue nil - die! unless @active - @active = false - end - end - - def process(env, input) - message 'received request' - @mutex.synchronize do - @active = true - - message 'creating input stream' - input_stream = StringIO.new(input) - message 'building CGI instance' - cgi = RemoteCGI.new(eval(env), input_stream) - - message 'yielding to fcgi handler' - @cgi_block.call cgi - message 'yield finished -- sending output' - - cgi.stdoutput.seek(0) - output = cgi.stdoutput.read - - return output - end - end - - def die! - message 'shutting down' - DRb.stop_service - FileUtils.rm_f @socket - Kernel.exit 0 - end -end - -socket_path = ARGV.shift -timeout = (ARGV.shift || 90).to_i - -Listener.new(timeout, socket_path) diff --git a/railties/lib/rails/fcgi_handler.rb b/railties/lib/rails/fcgi_handler.rb deleted file mode 100644 index 77dce5f325..0000000000 --- a/railties/lib/rails/fcgi_handler.rb +++ /dev/null @@ -1,241 +0,0 @@ -require 'fcgi' -require 'logger' -require 'rails/dispatcher' -require 'rbconfig' - -class RailsFCGIHandler - SIGNALS = { - 'HUP' => :reload, - 'INT' => :exit_now, - 'TERM' => :exit_now, - 'USR1' => :exit, - 'USR2' => :restart - } - GLOBAL_SIGNALS = SIGNALS.keys - %w(USR1) - - attr_reader :when_ready - - attr_accessor :log_file_path - attr_accessor :gc_request_period - - # Initialize and run the FastCGI instance, passing arguments through to new. - def self.process!(*args, &block) - new(*args, &block).process! - end - - # Initialize the FastCGI instance with the path to a crash log - # detailing unhandled exceptions (default RAILS_ROOT/log/fastcgi.crash.log) - # and the number of requests to process between garbage collection runs - # (default nil for normal GC behavior.) Optionally, pass a block which - # takes this instance as an argument for further configuration. - def initialize(log_file_path = nil, gc_request_period = nil) - self.log_file_path = log_file_path || "#{RAILS_ROOT}/log/fastcgi.crash.log" - self.gc_request_period = gc_request_period - - # Yield for additional configuration. - yield self if block_given? - - # Safely install signal handlers. - install_signal_handlers - - @app = Dispatcher.new - - # Start error timestamp at 11 seconds ago. - @last_error_on = Time.now - 11 - end - - def process!(provider = FCGI) - mark_features! - - dispatcher_log :info, 'starting' - process_each_request provider - dispatcher_log :info, 'stopping gracefully' - - rescue Exception => error - case error - when SystemExit - dispatcher_log :info, 'stopping after explicit exit' - when SignalException - dispatcher_error error, 'stopping after unhandled signal' - else - # Retry if exceptions occur more than 10 seconds apart. - if Time.now - @last_error_on > 10 - @last_error_on = Time.now - dispatcher_error error, 'retrying after unhandled exception' - retry - else - dispatcher_error error, 'stopping after unhandled exception within 10 seconds of the last' - end - end - end - - protected - def process_each_request(provider) - request = nil - - catch :exit do - provider.each do |request| - process_request(request) - - case when_ready - when :reload - reload! - when :restart - close_connection(request) - restart! - when :exit - close_connection(request) - throw :exit - end - end - end - rescue SignalException => signal - raise unless signal.message == 'SIGUSR1' - close_connection(request) - end - - def process_request(request) - @processing, @when_ready = true, nil - gc_countdown - - with_signal_handler 'USR1' do - begin - ::Rack::Handler::FastCGI.serve(request, @app) - rescue SignalException, SystemExit - raise - rescue Exception => error - dispatcher_error error, 'unhandled dispatch error' - end - end - ensure - @processing = false - end - - def logger - @logger ||= Logger.new(@log_file_path) - end - - def dispatcher_log(level, msg) - time_str = Time.now.strftime("%d/%b/%Y:%H:%M:%S") - logger.send(level, "[#{time_str} :: #{$$}] #{msg}") - rescue Exception => log_error # Logger errors - STDERR << "Couldn't write to #{@log_file_path.inspect}: #{msg}\n" - STDERR << " #{log_error.class}: #{log_error.message}\n" - end - - def dispatcher_error(e, msg = "") - error_message = - "Dispatcher failed to catch: #{e} (#{e.class})\n" + - " #{e.backtrace.join("\n ")}\n#{msg}" - dispatcher_log(:error, error_message) - end - - def install_signal_handlers - GLOBAL_SIGNALS.each { |signal| install_signal_handler(signal) } - end - - def install_signal_handler(signal, handler = nil) - if SIGNALS.include?(signal) && self.class.method_defined?(name = "#{SIGNALS[signal]}_handler") - handler ||= method(name).to_proc - - begin - trap(signal, handler) - rescue ArgumentError - dispatcher_log :warn, "Ignoring unsupported signal #{signal}." - end - else - dispatcher_log :warn, "Ignoring unsupported signal #{signal}." - end - end - - def with_signal_handler(signal) - install_signal_handler(signal) - yield - ensure - install_signal_handler(signal, 'DEFAULT') - end - - def exit_now_handler(signal) - dispatcher_log :info, "asked to stop immediately" - exit - end - - def exit_handler(signal) - dispatcher_log :info, "asked to stop ASAP" - if @processing - @when_ready = :exit - else - throw :exit - end - end - - def reload_handler(signal) - dispatcher_log :info, "asked to reload ASAP" - if @processing - @when_ready = :reload - else - reload! - end - end - - def restart_handler(signal) - dispatcher_log :info, "asked to restart ASAP" - if @processing - @when_ready = :restart - else - restart! - end - end - - def restart! - config = ::Config::CONFIG - ruby = File::join(config['bindir'], config['ruby_install_name']) + config['EXEEXT'] - command_line = [ruby, $0, ARGV].flatten.join(' ') - - dispatcher_log :info, "restarted" - - # close resources as they won't be closed by - # the OS when using exec - logger.close rescue nil - Rails.logger.close rescue nil - - exec(command_line) - end - - def reload! - run_gc! if gc_request_period - restore! - @when_ready = nil - dispatcher_log :info, "reloaded" - end - - # Make a note of $" so we can safely reload this instance. - def mark_features! - @features = $".clone - end - - def restore! - $".replace @features - # TODO: Reloading the application should be the "Application"s - # responsibility - ActionDispatch::Callbacks.new(lambda {}, true) - ActionController::Routing::Routes.reload - end - - def run_gc! - @gc_request_countdown = gc_request_period - GC.enable; GC.start; GC.disable - end - - def gc_countdown - if gc_request_period - @gc_request_countdown ||= gc_request_period - @gc_request_countdown -= 1 - run_gc! if @gc_request_countdown <= 0 - end - end - - def close_connection(request) - request.finish if request - end -end diff --git a/railties/lib/rails/generators/rails/app/app_generator.rb b/railties/lib/rails/generators/rails/app/app_generator.rb index b2322f90b4..78b4b057ae 100644 --- a/railties/lib/rails/generators/rails/app/app_generator.rb +++ b/railties/lib/rails/generators/rails/app/app_generator.rb @@ -18,9 +18,6 @@ module Rails::Generators class_option :template, :type => :string, :aliases => "-m", :desc => "Path to an application template (can be a filesystem path or URL)." - class_option :with_dispatchers, :type => :boolean, :aliases => "-D", :default => false, - :desc => "Add CGI/FastCGI/mod_ruby dispatchers code" - class_option :skip_activerecord, :type => :boolean, :aliases => "-O", :default => false, :desc => "Skip ActiveRecord files" @@ -113,19 +110,6 @@ module Rails::Generators directory "public", "public", :recursive => false # Do small steps, so anyone can overwrite it. end - def create_dispatch_files - return unless options[:with_dispatchers] - - template "dispatchers/dispatch.rb", "public/dispatch.rb" - chmod "public/dispatch.rb", 0755, :verbose => false - - template "dispatchers/dispatch.rb", "public/dispatch.cgi" - chmod "public/dispatch.cgi", 0755, :verbose => false - - template "dispatchers/dispatch.fcgi", "public/dispatch.fcgi" - chmod "public/dispatch.fcgi", 0755, :verbose => false - end - def create_public_image_files directory "public/images" end diff --git a/railties/lib/rails/generators/rails/app/templates/dispatchers/dispatch.fcgi b/railties/lib/rails/generators/rails/app/templates/dispatchers/dispatch.fcgi deleted file mode 100755 index f5b3b71875..0000000000 --- a/railties/lib/rails/generators/rails/app/templates/dispatchers/dispatch.fcgi +++ /dev/null @@ -1,24 +0,0 @@ -<%= shebang %> -# -# You may specify the path to the FastCGI crash log (a log of unhandled -# exceptions which forced the FastCGI instance to exit, great for debugging) -# and the number of requests to process before running garbage collection. -# -# By default, the FastCGI crash log is RAILS_ROOT/log/fastcgi.crash.log -# and the GC period is nil (turned off). A reasonable number of requests -# could range from 10-100 depending on the memory footprint of your app. -# -# Example: -# # Default log path, normal GC behavior. -# RailsFCGIHandler.process! -# -# # Default log path, 50 requests between GC. -# RailsFCGIHandler.process! nil, 50 -# -# # Custom log path, normal GC behavior. -# RailsFCGIHandler.process! '/var/log/myapp_fcgi_crash.log' -# -require File.dirname(__FILE__) + "/../config/environment" -require 'fcgi_handler' - -RailsFCGIHandler.process! diff --git a/railties/lib/rails/generators/rails/app/templates/dispatchers/dispatch.rb b/railties/lib/rails/generators/rails/app/templates/dispatchers/dispatch.rb deleted file mode 100755 index 48e888113a..0000000000 --- a/railties/lib/rails/generators/rails/app/templates/dispatchers/dispatch.rb +++ /dev/null @@ -1,10 +0,0 @@ -<%= shebang %> - -require File.dirname(__FILE__) + "/../config/environment" unless defined?(RAILS_ROOT) - -# If you're using RubyGems and mod_ruby, this require should be changed to an absolute path one, like: -# "/usr/local/lib/ruby/gems/1.8/gems/rails-0.8.0/lib/dispatcher" -- otherwise performance is severely impaired -require "dispatcher" - -ADDITIONAL_LOAD_PATHS.reverse.each { |dir| $:.unshift(dir) if File.directory?(dir) } if defined?(Apache::RubyRun) -Dispatcher.dispatch diff --git a/railties/lib/rails/generators/rails/app/templates/dispatchers/gateway.cgi b/railties/lib/rails/generators/rails/app/templates/dispatchers/gateway.cgi deleted file mode 100755 index bdc1055a22..0000000000 --- a/railties/lib/rails/generators/rails/app/templates/dispatchers/gateway.cgi +++ /dev/null @@ -1,97 +0,0 @@ -<%= shebang %> - -require 'drb' - -# This file includes an experimental gateway CGI implementation. It will work -# only on platforms which support both fork and sockets. -# -# To enable it edit public/.htaccess and replace dispatch.cgi with gateway.cgi. -# -# Next, create the directory log/drb_gateway and grant the apache user rw access -# to said directory. -# -# On the next request to your server, the gateway tracker should start up, along -# with a few listener processes. This setup should provide you with much better -# speeds than dispatch.cgi. -# -# Keep in mind that the first request made to the server will be slow, as the -# tracker and listeners will have to load. Also, the tracker and listeners will -# shutdown after a period if inactivity. You can set this value below -- the -# default is 90 seconds. - -TrackerSocket = File.expand_path(File.join(File.dirname(__FILE__), '../log/drb_gateway/tracker.sock')) -DieAfter = 90 # Seconds -Listeners = 3 - -def message(s) - $stderr.puts "gateway.cgi: #{s}" if ENV && ENV["DEBUG_GATEWAY"] -end - -def listener_socket(number) - File.expand_path(File.join(File.dirname(__FILE__), "../log/drb_gateway/listener_#{number}.sock")) -end - -unless File.exist? TrackerSocket - message "Starting tracker and #{Listeners} listeners" - fork do - Process.setsid - STDIN.reopen "/dev/null" - STDOUT.reopen "/dev/null", "a" - - root = File.expand_path(File.dirname(__FILE__) + '/..') - - message "starting tracker" - fork do - ARGV.clear - ARGV << TrackerSocket << Listeners.to_s << DieAfter.to_s - load File.join(root, 'script', 'tracker') - end - - message "starting listeners" - require File.join(root, 'config/environment.rb') - Listeners.times do |number| - fork do - ARGV.clear - ARGV << listener_socket(number) << DieAfter.to_s - load File.join(root, 'script', 'listener') - end - end - end - - message "waiting for tracker and listener to arise..." - ready = false - 10.times do - sleep 0.5 - break if (ready = File.exist?(TrackerSocket) && File.exist?(listener_socket(0))) - end - - if ready - message "tracker and listener are ready" - else - message "Waited 5 seconds, listener and tracker not ready... dropping request" - Kernel.exit 1 - end -end - -DRb.start_service - -message "connecting to tracker" -tracker = DRbObject.new_with_uri("drbunix:#{TrackerSocket}") - -input = $stdin.read -$stdin.close - -env = ENV.inspect - -output = nil -tracker.with_listener do |number| - message "connecting to listener #{number}" - socket = listener_socket(number) - listener = DRbObject.new_with_uri("drbunix:#{socket}") - output = listener.process(env, input) - message "listener #{number} has finished, writing output" -end - -$stdout.write output -$stdout.flush -$stdout.close diff --git a/railties/lib/rails/tasks/framework.rake b/railties/lib/rails/tasks/framework.rake index 17e16f26fd..16dd0af44e 100644 --- a/railties/lib/rails/tasks/framework.rake +++ b/railties/lib/rails/tasks/framework.rake @@ -110,11 +110,6 @@ namespace :rails do invoke_from_app_generator :create_prototype_files end - desc "Generate dispatcher files in RAILS_ROOT/public" - task :generate_dispatchers do - invoke_from_app_generator :create_dispatch_files - end - desc "Add new scripts to the application script/ directory" task :scripts do invoke_from_app_generator :create_script_files diff --git a/railties/test/application/fcgi_dispatcher_test.rb b/railties/test/application/fcgi_dispatcher_test.rb deleted file mode 100644 index b8ab9f8996..0000000000 --- a/railties/test/application/fcgi_dispatcher_test.rb +++ /dev/null @@ -1,290 +0,0 @@ -require 'isolation/abstract_unit' -require 'mocha' - -begin - -begin - require 'fcgi' -rescue LoadError - gem 'fcgi', '0.8.7' - require 'fcgi' -end - -class RailsFCGIHandlerTest < Test::Unit::TestCase - include ActiveSupport::Testing::Isolation - - def setup - build_app - boot_rails - - require "#{rails_root}/config/environment" - require 'rails/fcgi_handler' - - @log = StringIO.new - @handler = RailsFCGIHandler.new(@log) - end - - def test_process_restart - request = mock - FCGI.stubs(:each).yields(request) - - @handler.expects(:process_request).once - @handler.expects(:dispatcher_error).never - - @handler.expects(:when_ready).returns(:restart) - @handler.expects(:close_connection).with(request) - @handler.expects(:reload!).never - @handler.expects(:restart!) - - @handler.process! - end - - def test_process_exit - request = mock - FCGI.stubs(:each).yields(request) - - @handler.expects(:process_request).once - @handler.expects(:dispatcher_error).never - - @handler.expects(:when_ready).returns(:exit) - @handler.expects(:close_connection).with(request) - @handler.expects(:reload!).never - @handler.expects(:restart!).never - - @handler.process! - end - - def test_process_with_system_exit_exception - request = mock - FCGI.stubs(:each).yields(request) - - @handler.expects(:process_request).once.raises(SystemExit) - @handler.stubs(:dispatcher_log) - @handler.expects(:dispatcher_log).with(:info, regexp_matches(/^stopping/)) - @handler.expects(:dispatcher_error).never - - @handler.expects(:when_ready).never - @handler.expects(:close_connection).never - @handler.expects(:reload!).never - @handler.expects(:restart!).never - - @handler.process! - end - - def test_restart_handler_outside_request - @handler.expects(:dispatcher_log).with(:info, "asked to restart ASAP") - @handler.expects(:restart!).once - - @handler.send(:restart_handler, nil) - assert_equal nil, @handler.when_ready - end - - def test_install_signal_handler_should_log_on_bad_signal - @handler.stubs(:trap).raises(ArgumentError) - - @handler.expects(:dispatcher_log).with(:warn, "Ignoring unsupported signal CHEESECAKE.") - @handler.send(:install_signal_handler, "CHEESECAKE", nil) - end - - def test_reload - @handler.expects(:restore!) - @handler.expects(:dispatcher_log).with(:info, "reloaded") - - @handler.send(:reload!) - assert_nil @handler.when_ready - end - - def test_reload_runs_gc_when_gc_request_period_set - @handler.expects(:run_gc!) - @handler.expects(:restore!) - @handler.expects(:dispatcher_log).with(:info, "reloaded") - @handler.gc_request_period = 10 - @handler.send(:reload!) - end - - def test_reload_doesnt_run_gc_if_gc_request_period_isnt_set - @handler.expects(:run_gc!).never - @handler.expects(:restore!) - @handler.expects(:dispatcher_log).with(:info, "reloaded") - @handler.send(:reload!) - end - - def test_restart! - @handler.expects(:dispatcher_log).with(:info, "restarted") - @handler.expects(:exec).returns('restarted') - assert_equal 'restarted', @handler.send(:restart!) - end - - def test_restore! - $".expects(:replace) - ActionController::Routing::Routes.expects(:reload) - @handler.send(:restore!) - end - - def test_uninterrupted_processing - request = mock - FCGI.expects(:each).yields(request) - @handler.expects(:process_request).with(request) - - @handler.process! - - assert_nil @handler.when_ready - end -end - -class RailsFCGIHandlerSignalsTest < Test::Unit::TestCase - include ActiveSupport::Testing::Isolation - - def setup - build_app - boot_rails - - require "#{rails_root}/config/environment" - require 'rails/fcgi_handler' - - ::RailsFCGIHandler.class_eval do - attr_accessor :signal - alias_method :old_gc_countdown, :gc_countdown - def gc_countdown - signal ? Process.kill(signal, $$) : old_gc_countdown - end - end - - @log = StringIO.new - @handler = RailsFCGIHandler.new(@log) - @dispatcher = mock - Dispatcher.stubs(:new).returns(@dispatcher) - end - - def test_interrupted_via_HUP_when_not_in_request - request = mock - FCGI.expects(:each).once.yields(request) - @handler.expects(:signal).times(2).returns('HUP') - - @handler.expects(:reload!).once - @handler.expects(:close_connection).never - @handler.expects(:exit).never - - @handler.process! - assert_equal :reload, @handler.when_ready - end - - def test_interrupted_via_USR1_when_not_in_request - request = mock - FCGI.expects(:each).once.yields(request) - @handler.expects(:signal).times(2).returns('USR1') - @handler.expects(:exit_handler).never - - @handler.expects(:reload!).never - @handler.expects(:close_connection).with(request).once - @handler.expects(:exit).never - - @handler.process! - assert_nil @handler.when_ready - end - - def test_restart_via_USR2_when_in_request - request = mock - FCGI.expects(:each).once.yields(request) - @handler.expects(:signal).times(2).returns('USR2') - @handler.expects(:exit_handler).never - - @handler.expects(:reload!).never - @handler.expects(:close_connection).with(request).once - @handler.expects(:exit).never - @handler.expects(:restart!).once - - @handler.process! - assert_equal :restart, @handler.when_ready - end - - def test_interrupted_via_TERM - request = mock - FCGI.expects(:each).once.yields(request) - ::Rack::Handler::FastCGI.expects(:serve).once.returns('TERM') - - @handler.expects(:reload!).never - @handler.expects(:close_connection).never - - @handler.process! - assert_nil @handler.when_ready - end - - def test_runtime_exception_in_fcgi - error = RuntimeError.new('foo') - FCGI.expects(:each).times(2).raises(error) - @handler.expects(:dispatcher_error).with(error, regexp_matches(/^retrying/)) - @handler.expects(:dispatcher_error).with(error, regexp_matches(/^stopping/)) - @handler.process! - end - - def test_runtime_error_in_dispatcher - request = mock - error = RuntimeError.new('foo') - FCGI.expects(:each).once.yields(request) - ::Rack::Handler::FastCGI.expects(:serve).once.raises(error) - @handler.expects(:dispatcher_error).with(error, regexp_matches(/^unhandled/)) - @handler.process! - end - - def test_signal_exception_in_fcgi - error = SignalException.new('USR2') - FCGI.expects(:each).once.raises(error) - @handler.expects(:dispatcher_error).with(error, regexp_matches(/^stopping/)) - @handler.process! - end - - def test_signal_exception_in_dispatcher - request = mock - error = SignalException.new('USR2') - FCGI.expects(:each).once.yields(request) - ::Rack::Handler::FastCGI.expects(:serve).once.raises(error) - @handler.expects(:dispatcher_error).with(error, regexp_matches(/^stopping/)) - @handler.process! - end -end - -class RailsFCGIHandlerPeriodicGCTest < Test::Unit::TestCase - include ActiveSupport::Testing::Isolation - - def setup - build_app - boot_rails - - require "#{rails_root}/config/environment" - require 'rails/fcgi_handler' - - @log = StringIO.new - end - - def teardown - GC.enable - end - - def test_normal_gc - @handler = RailsFCGIHandler.new(@log) - assert_nil @handler.gc_request_period - - # When GC is enabled, GC.disable disables and returns false. - assert_equal false, GC.disable - end - - def test_periodic_gc - @handler = RailsFCGIHandler.new(@log, 10) - assert_equal 10, @handler.gc_request_period - - request = mock - FCGI.expects(:each).times(10).yields(request) - - @handler.expects(:run_gc!).never - 9.times { @handler.process! } - @handler.expects(:run_gc!).once - @handler.process! - - assert_nil @handler.when_ready - end -end - -rescue LoadError - $stderr.puts 'Skipping fcgi tests. `gem install fcgi` and try again.' -end diff --git a/railties/test/generators/app_generator_test.rb b/railties/test/generators/app_generator_test.rb index afc0585fba..6e46c4ddc0 100644 --- a/railties/test/generators/app_generator_test.rb +++ b/railties/test/generators/app_generator_test.rb @@ -53,18 +53,6 @@ class AppGeneratorTest < GeneratorsTestCase assert_match /Invalid value for \-\-database option/, content end - def test_dispatchers_are_not_added_by_default - run_generator - assert_no_file "public/dispatch.cgi" - assert_no_file "public/dispatch.fcgi" - end - - def test_dispatchers_are_added_if_required - run_generator ["--with-dispatchers"] - assert_file "public/dispatch.cgi" - assert_file "public/dispatch.fcgi" - end - def test_config_database_is_added_by_default run_generator assert_file "config/database.yml", /sqlite3/ From b480da5cd65de966ac14bbdc52b2fae3ffc06547 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 5 Oct 2009 13:58:43 -0500 Subject: [PATCH 24/61] Coerce all out going body parts to Strings --- actionpack/lib/action_dispatch.rb | 1 + .../middleware/string_coercion.rb | 29 ++++++++++++++ actionpack/test/abstract_unit.rb | 1 + .../test/dispatch/string_coercion_test.rb | 40 +++++++++++++++++++ railties/lib/rails/initializer.rb | 1 + 5 files changed, 72 insertions(+) create mode 100644 actionpack/lib/action_dispatch/middleware/string_coercion.rb create mode 100644 actionpack/test/dispatch/string_coercion_test.rb diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb index 38aaa6146e..11cd812695 100644 --- a/actionpack/lib/action_dispatch.rb +++ b/actionpack/lib/action_dispatch.rb @@ -39,6 +39,7 @@ module ActionDispatch autoload :Rescue, 'action_dispatch/middleware/rescue' autoload :ShowExceptions, 'action_dispatch/middleware/show_exceptions' autoload :Static, 'action_dispatch/middleware/static' + autoload :StringCoercion, 'action_dispatch/middleware/string_coercion' autoload :Assertions, 'action_dispatch/testing/assertions' autoload :Integration, 'action_dispatch/testing/integration' diff --git a/actionpack/lib/action_dispatch/middleware/string_coercion.rb b/actionpack/lib/action_dispatch/middleware/string_coercion.rb new file mode 100644 index 0000000000..232e947835 --- /dev/null +++ b/actionpack/lib/action_dispatch/middleware/string_coercion.rb @@ -0,0 +1,29 @@ +module ActionDispatch + class StringCoercion + class UglyBody < ActiveSupport::BasicObject + def initialize(body) + @body = body + end + + def each + @body.each do |part| + yield part.to_s + end + end + + private + def method_missing(*args, &block) + @body.__send__(*args, &block) + end + end + + def initialize(app) + @app = app + end + + def call(env) + status, headers, body = @app.call(env) + [status, headers, UglyBody.new(body)] + end + end +end diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index 1214d608a4..4820f00aa1 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -93,6 +93,7 @@ end class ActionController::IntegrationTest < ActiveSupport::TestCase def self.build_app(routes = nil) ActionDispatch::MiddlewareStack.new { |middleware| + middleware.use "ActionDispatch::StringCoercion" middleware.use "ActionDispatch::ShowExceptions" middleware.use "ActionDispatch::Callbacks" middleware.use "ActionDispatch::ParamsParser" diff --git a/actionpack/test/dispatch/string_coercion_test.rb b/actionpack/test/dispatch/string_coercion_test.rb new file mode 100644 index 0000000000..d79b17b932 --- /dev/null +++ b/actionpack/test/dispatch/string_coercion_test.rb @@ -0,0 +1,40 @@ +require 'abstract_unit' + +class StringCoercionTest < ActiveSupport::TestCase + test "body responds to each" do + original_body = [] + body = ActionDispatch::StringCoercion::UglyBody.new(original_body) + + assert original_body.respond_to?(:each) + assert body.respond_to?(:each) + end + + test "body responds to to_path" do + original_body = [] + def original_body.to_path; end + body = ActionDispatch::StringCoercion::UglyBody.new(original_body) + + assert original_body.respond_to?(:to_path) + assert body.respond_to?(:to_path) + end + + test "body does not responds to to_path" do + original_body = [] + body = ActionDispatch::StringCoercion::UglyBody.new(original_body) + + assert !original_body.respond_to?(:to_path) + assert !body.respond_to?(:to_path) + end + + test "calls to_s on body parts" do + app = lambda { |env| + [200, {'Content-Type' => 'html'}, [1, 2, 3]] + } + app = ActionDispatch::StringCoercion.new(app) + parts = [] + status, headers, body = app.call({}) + body.each { |part| parts << part } + + assert_equal %w( 1 2 3 ), parts + end +end diff --git a/railties/lib/rails/initializer.rb b/railties/lib/rails/initializer.rb index 2d63ac4d39..c2d6e1609f 100644 --- a/railties/lib/rails/initializer.rb +++ b/railties/lib/rails/initializer.rb @@ -271,6 +271,7 @@ module Rails configuration.middleware.use(ActionDispatch::ParamsParser) configuration.middleware.use(::Rack::MethodOverride) configuration.middleware.use(::Rack::Head) + configuration.middleware.use(ActionDispatch::StringCoercion) end end From 9212138ad0a9ae3285a2566300afb7d94344214a Mon Sep 17 00:00:00 2001 From: Jeffrey Hardy Date: Mon, 5 Oct 2009 08:27:54 -0400 Subject: [PATCH 25/61] MessageVerifier#verify raises InvalidSignature if the signature is blank Signed-off-by: Jeremy Kemper --- activesupport/lib/active_support/message_verifier.rb | 2 ++ activesupport/test/message_verifier_test.rb | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/activesupport/lib/active_support/message_verifier.rb b/activesupport/lib/active_support/message_verifier.rb index 74e080a23d..fcdc09ff08 100644 --- a/activesupport/lib/active_support/message_verifier.rb +++ b/activesupport/lib/active_support/message_verifier.rb @@ -26,6 +26,8 @@ module ActiveSupport end def verify(signed_message) + raise InvalidSignature if signed_message.blank? + data, digest = signed_message.split("--") if secure_compare(digest, generate_digest(data)) Marshal.load(ActiveSupport::Base64.decode64(data)) diff --git a/activesupport/test/message_verifier_test.rb b/activesupport/test/message_verifier_test.rb index 4f8837ba4e..e6370bc3db 100644 --- a/activesupport/test/message_verifier_test.rb +++ b/activesupport/test/message_verifier_test.rb @@ -18,6 +18,11 @@ class MessageVerifierTest < Test::Unit::TestCase assert_equal @data, @verifier.verify(message) end + def test_missing_signature_raises + assert_not_verified(nil) + assert_not_verified("") + end + def test_tampered_data_raises data, hash = @verifier.generate(@data).split("--") assert_not_verified("#{data.reverse}--#{hash}") From 126f623711ce421b7b1bbf7e94099403ecaf2d20 Mon Sep 17 00:00:00 2001 From: Michael Koziarski Date: Tue, 6 Oct 2009 15:52:47 +1300 Subject: [PATCH 26/61] don't ignore all bin directories --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index 1c94c4b0f8..da296e7e11 100644 --- a/.gitignore +++ b/.gitignore @@ -30,5 +30,4 @@ railties/guides/output actionpack/bin vendor/gems/ */vendor/gems/ -bin/ railties/tmp From 6361d4234ca7f7c2dcb98e6ed34187d2933b56d7 Mon Sep 17 00:00:00 2001 From: Paul Gillard Date: Tue, 6 Oct 2009 16:25:51 -0500 Subject: [PATCH 27/61] Call initialize_copy when cloning [#3164 state:resolved] Cloned AR objects are now instantiated through initialize_copy rather than new/initialize. This allows AR classes to override initialize_copy in order to implement deep cloning. Signed-off-by: Joshua Peek --- activerecord/lib/active_record/base.rb | 65 +++++++++++++++----------- activerecord/test/cases/base_test.rb | 2 +- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 502fe0442e..150e3fc263 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -2427,6 +2427,29 @@ module ActiveRecord #:nodoc: result end + # Cloned objects have no id assigned and are treated as new records. Note that this is a "shallow" clone + # as it copies the object's attributes only, not its associations. The extent of a "deep" clone is + # application specific and is therefore left to the application to implement according to its need. + def initialize_copy(other) + # Think the assertion which fails if the after_initialize callback goes at the end of the method is wrong. The + # deleted clone method called new which therefore called the after_initialize callback. It then went on to copy + # over the attributes. But if it's copying the attributes afterwards then it hasn't finished initializing right? + # For example in the test suite the topic model's after_initialize method sets the author_email_address to + # test@test.com. I would have thought this would mean that all cloned models would have an author email address + # of test@test.com. However the test_clone test method seems to test that this is not the case. As a result the + # after_initialize callback has to be run *before* the copying of the atrributes rather than afterwards in order + # for all tests to pass. This makes no sense to me. + callback(:after_initialize) if respond_to_without_attributes?(:after_initialize) + cloned_attributes = other.clone_attributes(:read_attribute_before_type_cast) + cloned_attributes.delete(self.class.primary_key) + @attributes = cloned_attributes + clear_aggregation_cache + @attributes_cache = {} + @new_record = true + ensure_proper_type + self.class.send(:scope, :create).each { |att, value| self.send("#{att}=", value) } if self.class.send(:scoped?, :create) + end + # Returns a String, which Action Pack uses for constructing an URL to this # object. The default implementation returns this record's id as a String, # or nil if this record's unsaved. @@ -2555,19 +2578,6 @@ module ActiveRecord #:nodoc: freeze end - # Returns a clone of the record that hasn't been assigned an id yet and - # is treated as a new record. Note that this is a "shallow" clone: - # it copies the object's attributes only, not its associations. - # The extent of a "deep" clone is application-specific and is therefore - # left to the application to implement according to its need. - def clone - attrs = clone_attributes(:read_attribute_before_type_cast) - attrs.delete(self.class.primary_key) - record = self.class.new - record.send :instance_variable_set, '@attributes', attrs - record - end - # Returns an instance of the specified +klass+ with the attributes of the current record. This is mostly useful in relation to # single-table inheritance structures where you want a subclass to appear as the superclass. This can be used along with record # identification in Action Pack to allow, say, Client < Company to do something like render :partial => @client.becomes(Company) @@ -2831,6 +2841,21 @@ module ActiveRecord #:nodoc: "#<#{self.class} #{attributes_as_nice_string}>" end + protected + def clone_attributes(reader_method = :read_attribute, attributes = {}) + self.attribute_names.inject(attributes) do |attrs, name| + attrs[name] = clone_attribute_value(reader_method, name) + attrs + end + end + + def clone_attribute_value(reader_method, attribute_name) + value = send(reader_method, attribute_name) + value.duplicable? ? value.clone : value + rescue TypeError, NoMethodError + value + end + private def create_or_update raise ReadOnlyRecord if readonly? @@ -3093,20 +3118,6 @@ module ActiveRecord #:nodoc: return string unless string.is_a?(String) && string =~ /^---/ YAML::load(string) rescue string end - - def clone_attributes(reader_method = :read_attribute, attributes = {}) - self.attribute_names.inject(attributes) do |attrs, name| - attrs[name] = clone_attribute_value(reader_method, name) - attrs - end - end - - def clone_attribute_value(reader_method, attribute_name) - value = send(reader_method, attribute_name) - value.duplicable? ? value.clone : value - rescue TypeError, NoMethodError - value - end end Base.class_eval do diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 8421a8fb07..1e2d903492 100755 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1352,7 +1352,7 @@ class BasicsTest < ActiveRecord::TestCase cloned_topic.title["a"] = "c" assert_equal "b", topic.title["a"] - #test if attributes set as part of after_initialize are cloned correctly + # test if attributes set as part of after_initialize are cloned correctly assert_equal topic.author_email_address, cloned_topic.author_email_address # test if saved clone object differs from original From e57197a9679d5d7a682fc73f12c9f04e067e85cc Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Tue, 6 Oct 2009 22:36:14 -1000 Subject: [PATCH 28/61] Fix warning spew --- actionpack/lib/action_view/base.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index ec1b07797b..01f33203ed 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -236,7 +236,9 @@ module ActionView #:nodoc: # they are in AC. if controller.class.respond_to?(:_helper_serial) klass = @views[controller.class._helper_serial] ||= Class.new(self) do - Subclasses.const_set(controller.class.name.gsub(/::/, '__'), self) + name = controller.class.name.gsub(/::/, '__') + Subclasses.remove_const(name) if Subclasses.const_defined?(name) + Subclasses.const_set(name, self) if controller.respond_to?(:_helpers) include controller._helpers From 3916f0340e8714d36a64162be793192849a9e51f Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Wed, 7 Oct 2009 00:33:13 -1000 Subject: [PATCH 29/61] Not calling a private method anymore --- actionpack/lib/action_view/base.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 01f33203ed..664cc3b562 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -237,8 +237,11 @@ module ActionView #:nodoc: if controller.class.respond_to?(:_helper_serial) klass = @views[controller.class._helper_serial] ||= Class.new(self) do name = controller.class.name.gsub(/::/, '__') - Subclasses.remove_const(name) if Subclasses.const_defined?(name) - Subclasses.const_set(name, self) + + Subclasses.class_eval do + remove_const(name) if const_defined?(name) + const_set(name, self) + end if controller.respond_to?(:_helpers) include controller._helpers From f8e91bda9c171dd0c31dbea95ba4e9ced4ee2547 Mon Sep 17 00:00:00 2001 From: Sam Pohlenz Date: Wed, 7 Oct 2009 09:05:54 -0500 Subject: [PATCH 30/61] Don't share attribute matchers between classes [#3216 state:resolved] Allows separate models that include ActiveModel::AttributeMethods to use different sets of attribute matchers. Signed-off-by: Joshua Peek --- .../lib/active_model/attribute_methods.rb | 5 ++++- .../test/cases/attribute_methods_test.rb | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 activemodel/test/cases/attribute_methods_test.rb diff --git a/activemodel/lib/active_model/attribute_methods.rb b/activemodel/lib/active_model/attribute_methods.rb index aa35a2726e..26ee9c4315 100644 --- a/activemodel/lib/active_model/attribute_methods.rb +++ b/activemodel/lib/active_model/attribute_methods.rb @@ -1,3 +1,6 @@ +require 'active_support/core_ext/hash/keys' +require 'active_support/core_ext/class/inheritable_attributes' + module ActiveModel class MissingAttributeError < NoMethodError end @@ -219,7 +222,7 @@ module ActiveModel end def attribute_method_matchers #:nodoc: - @@attribute_method_matchers ||= [] + read_inheritable_attribute(:attribute_method_matchers) || write_inheritable_attribute(:attribute_method_matchers, []) end end diff --git a/activemodel/test/cases/attribute_methods_test.rb b/activemodel/test/cases/attribute_methods_test.rb new file mode 100644 index 0000000000..614c4a3645 --- /dev/null +++ b/activemodel/test/cases/attribute_methods_test.rb @@ -0,0 +1,20 @@ +require 'cases/helper' + +class ModelWithAttributes + include ActiveModel::AttributeMethods + + attribute_method_suffix '' +end + +class ModelWithAttributes2 + include ActiveModel::AttributeMethods + + attribute_method_suffix '_test' +end + +class AttributeMethodsTest < ActiveModel::TestCase + test 'unrelated classes should not share attribute method matchers' do + assert_not_equal ModelWithAttributes.send(:attribute_method_matchers), + ModelWithAttributes2.send(:attribute_method_matchers) + end +end From 4df96338ede62da8ae8c188de5454b4b6b186ff8 Mon Sep 17 00:00:00 2001 From: Sam Pohlenz Date: Wed, 7 Oct 2009 09:06:54 -0500 Subject: [PATCH 31/61] Fixed behavior of attribute_methods_generated? [#3220 state:resolved] Signed-off-by: Joshua Peek --- .../lib/active_model/attribute_methods.rb | 2 +- .../test/cases/attribute_methods_test.rb | 26 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/activemodel/lib/active_model/attribute_methods.rb b/activemodel/lib/active_model/attribute_methods.rb index 26ee9c4315..977a101277 100644 --- a/activemodel/lib/active_model/attribute_methods.rb +++ b/activemodel/lib/active_model/attribute_methods.rb @@ -165,6 +165,7 @@ module ActiveModel end end end + @attribute_methods_generated = true end def undefine_attribute_methods @@ -176,7 +177,6 @@ module ActiveModel def generated_attribute_methods #:nodoc: @generated_attribute_methods ||= begin - @attribute_methods_generated = true mod = Module.new include mod mod diff --git a/activemodel/test/cases/attribute_methods_test.rb b/activemodel/test/cases/attribute_methods_test.rb index 614c4a3645..5659dcbc48 100644 --- a/activemodel/test/cases/attribute_methods_test.rb +++ b/activemodel/test/cases/attribute_methods_test.rb @@ -4,6 +4,15 @@ class ModelWithAttributes include ActiveModel::AttributeMethods attribute_method_suffix '' + + def attributes + { :foo => 'value of foo' } + end + +private + def attribute(name) + attributes[name.to_sym] + end end class ModelWithAttributes2 @@ -17,4 +26,21 @@ class AttributeMethodsTest < ActiveModel::TestCase assert_not_equal ModelWithAttributes.send(:attribute_method_matchers), ModelWithAttributes2.send(:attribute_method_matchers) end + + test '#define_attribute_methods generates attribute methods' do + ModelWithAttributes.define_attribute_methods([:foo]) + + assert ModelWithAttributes.attribute_methods_generated? + assert ModelWithAttributes.new.respond_to?(:foo) + assert_equal "value of foo", ModelWithAttributes.new.foo + end + + test '#undefine_attribute_methods removes attribute methods' do + ModelWithAttributes.define_attribute_methods([:foo]) + ModelWithAttributes.undefine_attribute_methods + + assert !ModelWithAttributes.attribute_methods_generated? + assert !ModelWithAttributes.new.respond_to?(:foo) + assert_raises(NoMethodError) { ModelWithAttributes.new.foo } + end end From ff56f3d5e1c69b923625f20a80f25c2eee3bbb35 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Wed, 7 Oct 2009 09:24:51 -0500 Subject: [PATCH 32/61] Rewrite ActiveModel::Lint as a simple TU mixin --- activemodel/lib/active_model/lint.rb | 80 +++++++++++----------------- activemodel/test/cases/lint_test.rb | 40 +++----------- 2 files changed, 39 insertions(+), 81 deletions(-) diff --git a/activemodel/lib/active_model/lint.rb b/activemodel/lib/active_model/lint.rb index ceaa29dc8c..1c2347adbf 100644 --- a/activemodel/lib/active_model/lint.rb +++ b/activemodel/lib/active_model/lint.rb @@ -1,6 +1,6 @@ # You can test whether an object is compliant with the ActiveModel API by -# calling ActiveModel::Lint.test(object). It will emit a Test::Unit -# output that tells you whether your object is fully compliant, or if not, +# including ActiveModel::Lint::Tests in your TestCase. It will included +# tests that tell you whether your object is fully compliant, or if not, # which aspects of the API are not implemented. # # These tests do not attempt to determine the semantic correctness of the @@ -12,36 +12,15 @@ # call to to_model. It is perfectly fine for to_model to return self. module ActiveModel module Lint - def self.test(object, verbosity = 2, output = STDOUT) - require "test/unit" - require "test/unit/ui/console/testrunner" - - test_class = Class.new(::Test::Unit::TestCase) do - include Test - - define_method(:setup) do - assert object.respond_to?(:to_model), "The object should respond_to :to_model" - @object = object.to_model - super - end - end - - ::Test::Unit::UI::Console::TestRunner.new(test_class, verbosity, output).start - end - - module Test - def assert_boolean(name, result) - assert result == true || result == false, "#{name} should be a boolean" - end - + module Tests # valid? # ------ # # Returns a boolean that specifies whether the object is in a valid or invalid # state. def test_valid? - assert @object.respond_to?(:valid?), "The model should respond to valid?" - assert_boolean "valid?", @object.valid? + assert model.respond_to?(:valid?), "The model should respond to valid?" + assert_boolean model.valid?, "valid?" end # new_record? @@ -53,13 +32,13 @@ module ActiveModel # collection. If it is persisted, a form for the object will put PUTed to the # URL for the object. def test_new_record? - assert @object.respond_to?(:new_record?), "The model should respond to new_record?" - assert_boolean "new_record?", @object.new_record? + assert model.respond_to?(:new_record?), "The model should respond to new_record?" + assert_boolean model.new_record?, "new_record?" end def test_destroyed? - assert @object.respond_to?(:destroyed?), "The model should respond to destroyed?" - assert_boolean "destroyed?", @object.destroyed? + assert model.respond_to?(:destroyed?), "The model should respond to destroyed?" + assert_boolean model.destroyed?, "destroyed?" end # errors @@ -67,29 +46,32 @@ module ActiveModel # # Returns an object that has :[] and :full_messages defined on it. See below # for more details. - def setup - assert @object.respond_to?(:errors), "The model should respond to errors" - @errors = @object.errors + + # Returns an Array of Strings that are the errors for the attribute in + # question. If localization is used, the Strings should be localized + # for the current locale. If no error is present, this method should + # return an empty Array. + def test_errors_aref + assert model.respond_to?(:errors), "The model should respond to errors" + assert model.errors[:hello].is_a?(Array), "errors#[] should return an Array" end - # This module tests the #errors object - module Errors - # Returns an Array of Strings that are the errors for the attribute in - # question. If localization is used, the Strings should be localized - # for the current locale. If no error is present, this method should - # return an empty Array. - def test_errors_aref - assert @errors[:hello].is_a?(Array), "errors#[] should return an Array" - end - - # Returns an Array of all error messages for the object. Each message - # should contain information about the field, if applicable. - def test_errors_full_messages - assert @errors.full_messages.is_a?(Array), "errors#full_messages should return an Array" - end + # Returns an Array of all error messages for the object. Each message + # should contain information about the field, if applicable. + def test_errors_full_messages + assert model.respond_to?(:errors), "The model should respond to errors" + assert model.errors.full_messages.is_a?(Array), "errors#full_messages should return an Array" end - include Errors + private + def model + assert @model.respond_to?(:to_model), "The object should respond_to to_model" + @model.to_model + end + + def assert_boolean(result, name) + assert result == true || result == false, "#{name} should be a boolean" + end end end end diff --git a/activemodel/test/cases/lint_test.rb b/activemodel/test/cases/lint_test.rb index ed576a91e2..da7d2112dc 100644 --- a/activemodel/test/cases/lint_test.rb +++ b/activemodel/test/cases/lint_test.rb @@ -1,15 +1,17 @@ require "cases/helper" -class TestLint < ActiveModel::TestCase - class CompliantObject +class LintTest < ActiveModel::TestCase + include ActiveModel::Lint::Tests + + class CompliantModel def to_model self end - + def valid?() true end def new_record?() true end def destroyed?() true end - + def errors obj = Object.new def obj.[](key) [] end @@ -17,34 +19,8 @@ class TestLint < ActiveModel::TestCase obj end end - - def assert_output(object, failures, errors, *test_names) - ActiveModel::Lint.test(object, 3, output = StringIO.new) - regex = %r{#{failures} failures, #{errors} errors} - assert_match regex, output.string - - test_names.each do |test_name| - assert_match test_name, output.string - end - end - - def test_valid - assert_output(CompliantObject.new, 0, 0, /test_valid/) - end - - def test_new_record - assert_output(CompliantObject.new, 0, 0, /test_new_record?/) - end - - def test_destroyed - assert_output(CompliantObject.new, 0, 0, /test_destroyed/) - end - - def test_errors_aref - assert_output(CompliantObject.new, 0, 0, /test_errors_aref/) - end - def test_errors_full_messages - assert_output(CompliantObject.new, 0, 0, /test_errors_aref/) + def setup + @model = CompliantModel.new end end From f27e7ebc0e2a55a268631c78d49a5b70b06ad59a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 6 Oct 2009 19:57:44 -0300 Subject: [PATCH 33/61] Do not ignore .empty_directory files. Signed-off-by: Carl Lerche --- railties/rails.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/railties/rails.gemspec b/railties/rails.gemspec index 659d6727ea..dc66e1efea 100644 --- a/railties/rails.gemspec +++ b/railties/rails.gemspec @@ -18,7 +18,7 @@ Gem::Specification.new do |s| s.rdoc_options << '--exclude' << '.' s.has_rdoc = false - s.files = Dir['CHANGELOG', 'README', 'bin/**/*', 'builtin/**/*', 'guides/**/*', 'lib/**/*'] + s.files = Dir['CHANGELOG', 'README', 'bin/**/*', 'builtin/**/*', 'guides/**/*', 'lib/**/{*,.[a-z]*}'] s.require_path = 'lib' s.bindir = "bin" s.executables = ["rails"] From 9415935902f120a9bac0bfce7129725a0db38ed3 Mon Sep 17 00:00:00 2001 From: Michael Koziarski Date: Thu, 8 Oct 2009 09:31:20 +1300 Subject: [PATCH 34/61] Switch to on-by-default XSS escaping for rails. This consists of: * String#html_safe! a method to mark a string as 'safe' * ActionView::SafeBuffer a string subclass which escapes anything unsafe which is concatenated to it * Calls to String#html_safe! throughout the rails helpers * a 'raw' helper which lets you concatenate trusted HTML from non-safety-aware sources (e.g. presantized strings in the DB) * New ERB implementation based on erubis which uses a SafeBuffer instead of a String Hat tip to Django for the inspiration. --- actionpack/Gemfile | 1 + actionpack/lib/action_view.rb | 6 +- actionpack/lib/action_view/base.rb | 2 +- actionpack/lib/action_view/erb/util.rb | 12 ++- actionpack/lib/action_view/helpers.rb | 2 + .../helpers/active_model_helper.rb | 1 + .../action_view/helpers/asset_tag_helper.rb | 6 +- .../lib/action_view/helpers/capture_helper.rb | 2 +- .../lib/action_view/helpers/date_helper.rb | 6 +- .../lib/action_view/helpers/form_helper.rb | 4 +- .../helpers/form_options_helper.rb | 2 +- .../action_view/helpers/form_tag_helper.rb | 6 +- .../action_view/helpers/prototype_helper.rb | 2 +- .../action_view/helpers/raw_output_helper.rb | 9 ++ .../action_view/helpers/sanitize_helper.rb | 12 ++- .../lib/action_view/helpers/tag_helper.rb | 8 +- .../lib/action_view/helpers/url_helper.rb | 10 +-- actionpack/lib/action_view/render/partials.rb | 2 +- actionpack/lib/action_view/safe_buffer.rb | 28 ++++++ .../lib/action_view/template/handlers/erb.rb | 28 +++++- actionpack/lib/action_view/test_case.rb | 2 +- .../test/controller/output_escaping_test.rb | 19 ++++ actionpack/test/controller/render_test.rb | 2 +- .../test/template/asset_tag_helper_test.rb | 12 +++ actionpack/test/template/erb_util_test.rb | 12 +++ actionpack/test/template/form_helper_test.rb | 2 +- .../test/template/raw_output_helper_test.rb | 21 +++++ actionpack/test/template/render_test.rb | 2 +- .../test/template/sanitize_helper_test.rb | 11 ++- actionpack/test/template/tag_helper_test.rb | 1 + actionpack/test/template/test_case_test.rb | 2 +- actionpack/test/template/url_helper_test.rb | 2 +- actionpack/test/view/safe_buffer_test.rb | 41 +++++++++ .../lib/active_support/core_ext/string.rb | 3 +- .../core_ext/string/output_safety.rb | 43 ++++++++++ .../test/core_ext/string_ext_test.rb | 86 +++++++++++++++++++ 36 files changed, 368 insertions(+), 42 deletions(-) create mode 100644 actionpack/lib/action_view/helpers/raw_output_helper.rb create mode 100644 actionpack/lib/action_view/safe_buffer.rb create mode 100644 actionpack/test/controller/output_escaping_test.rb create mode 100644 actionpack/test/template/raw_output_helper_test.rb create mode 100644 actionpack/test/view/safe_buffer_test.rb create mode 100644 activesupport/lib/active_support/core_ext/string/output_safety.rb diff --git a/actionpack/Gemfile b/actionpack/Gemfile index 7d99e0601b..60d24104d2 100644 --- a/actionpack/Gemfile +++ b/actionpack/Gemfile @@ -4,6 +4,7 @@ gem "rack", "~> 1.0.0" gem "rack-test", "~> 0.5.0" gem "activesupport", "3.0.pre", :vendored_at => rails_root.join("activesupport") gem "activemodel", "3.0.pre", :vendored_at => rails_root.join("activemodel") +gem "erubis", "~> 2.6.0" only :test do gem "mocha" diff --git a/actionpack/lib/action_view.rb b/actionpack/lib/action_view.rb index 3df4f2d6a3..e95e84aeb5 100644 --- a/actionpack/lib/action_view.rb +++ b/actionpack/lib/action_view.rb @@ -44,11 +44,11 @@ module ActionView autoload :TextTemplate, 'action_view/template/text' autoload :Helpers, 'action_view/helpers' autoload :FileSystemResolverWithFallback, 'action_view/template/resolver' + autoload :SafeBuffer, 'action_view/safe_buffer' end -class ERB - autoload :Util, 'action_view/erb/util' -end +require 'action_view/erb/util' + I18n.load_path << "#{File.dirname(__FILE__)}/action_view/locale/en.yml" diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 664cc3b562..5a4e1bee43 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -260,7 +260,7 @@ module ActionView #:nodoc: @assigns = assigns_for_first_render.each { |key, value| instance_variable_set("@#{key}", value) } @controller = controller @helpers = self.class.helpers || Module.new - @_content_for = Hash.new {|h,k| h[k] = "" } + @_content_for = Hash.new {|h,k| h[k] = ActionView::SafeBuffer.new } self.view_paths = view_paths end diff --git a/actionpack/lib/action_view/erb/util.rb b/actionpack/lib/action_view/erb/util.rb index 3c77c5ce76..f767a5e27e 100644 --- a/actionpack/lib/action_view/erb/util.rb +++ b/actionpack/lib/action_view/erb/util.rb @@ -15,9 +15,19 @@ class ERB # puts html_escape("is a > 0 & a < 10?") # # => is a > 0 & a < 10? def html_escape(s) - s.to_s.gsub(/[&"><]/) { |special| HTML_ESCAPE[special] } + s = s.to_s + if s.html_safe? + s + else + s.gsub(/[&"><]/) { |special| HTML_ESCAPE[special] }.html_safe! + end end + alias h html_escape + + module_function :html_escape + module_function :h + # A utility method for escaping HTML entities in JSON strings. # This method is also aliased as j. # diff --git a/actionpack/lib/action_view/helpers.rb b/actionpack/lib/action_view/helpers.rb index 652561f7f8..d63e8602f1 100644 --- a/actionpack/lib/action_view/helpers.rb +++ b/actionpack/lib/action_view/helpers.rb @@ -15,6 +15,7 @@ module ActionView #:nodoc: autoload :JavaScriptHelper, 'action_view/helpers/javascript_helper' autoload :NumberHelper, 'action_view/helpers/number_helper' autoload :PrototypeHelper, 'action_view/helpers/prototype_helper' + autoload :RawOutputHelper, 'action_view/helpers/raw_output_helper' autoload :RecordIdentificationHelper, 'action_view/helpers/record_identification_helper' autoload :RecordTagHelper, 'action_view/helpers/record_tag_helper' autoload :SanitizeHelper, 'action_view/helpers/sanitize_helper' @@ -46,6 +47,7 @@ module ActionView #:nodoc: include JavaScriptHelper include NumberHelper include PrototypeHelper + include RawOutputHelper include RecordIdentificationHelper include RecordTagHelper include SanitizeHelper diff --git a/actionpack/lib/action_view/helpers/active_model_helper.rb b/actionpack/lib/action_view/helpers/active_model_helper.rb index 3e6e62237d..c9231225e1 100644 --- a/actionpack/lib/action_view/helpers/active_model_helper.rb +++ b/actionpack/lib/action_view/helpers/active_model_helper.rb @@ -91,6 +91,7 @@ module ActionView yield contents if block_given? contents << submit_tag(submit_value) contents << '' + contents.html_safe! end # Returns a string containing the error message attached to the +method+ on the +object+ if one exists. diff --git a/actionpack/lib/action_view/helpers/asset_tag_helper.rb b/actionpack/lib/action_view/helpers/asset_tag_helper.rb index 95f00cda39..faa7f2e2e9 100644 --- a/actionpack/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionpack/lib/action_view/helpers/asset_tag_helper.rb @@ -289,7 +289,7 @@ module ActionView else sources = expand_javascript_sources(sources, recursive) ensure_javascript_sources!(sources) if cache - sources.collect { |source| javascript_src_tag(source, options) }.join("\n") + sources.collect { |source| javascript_src_tag(source, options) }.join("\n").html_safe! end end @@ -440,7 +440,7 @@ module ActionView else sources = expand_stylesheet_sources(sources, recursive) ensure_stylesheet_sources!(sources) if cache - sources.collect { |source| stylesheet_tag(source, options) }.join("\n") + sources.collect { |source| stylesheet_tag(source, options) }.join("\n").html_safe! end end @@ -584,7 +584,7 @@ module ActionView if sources.is_a?(Array) content_tag("video", options) do - sources.map { |source| tag("source", :src => source) }.join + sources.map { |source| tag("source", :src => source) }.join.html_safe! end else options[:src] = path_to_video(sources) diff --git a/actionpack/lib/action_view/helpers/capture_helper.rb b/actionpack/lib/action_view/helpers/capture_helper.rb index c90acc5ac2..b62df75dbb 100644 --- a/actionpack/lib/action_view/helpers/capture_helper.rb +++ b/actionpack/lib/action_view/helpers/capture_helper.rb @@ -143,7 +143,7 @@ module ActionView # Defaults to a new empty string. def with_output_buffer(buf = nil) #:nodoc: unless buf - buf = '' + buf = ActionView::SafeBuffer.new buf.force_encoding(output_buffer.encoding) if buf.respond_to?(:force_encoding) end self.output_buffer, old_buffer = buf, output_buffer diff --git a/actionpack/lib/action_view/helpers/date_helper.rb b/actionpack/lib/action_view/helpers/date_helper.rb index 8a7a870b99..4b51dc7856 100644 --- a/actionpack/lib/action_view/helpers/date_helper.rb +++ b/actionpack/lib/action_view/helpers/date_helper.rb @@ -916,15 +916,15 @@ module ActionView class InstanceTag #:nodoc: def to_date_select_tag(options = {}, html_options = {}) - datetime_selector(options, html_options).select_date + datetime_selector(options, html_options).select_date.html_safe! end def to_time_select_tag(options = {}, html_options = {}) - datetime_selector(options, html_options).select_time + datetime_selector(options, html_options).select_time.html_safe! end def to_datetime_select_tag(options = {}, html_options = {}) - datetime_selector(options, html_options).select_datetime + datetime_selector(options, html_options).select_datetime.html_safe! end private diff --git a/actionpack/lib/action_view/helpers/form_helper.rb b/actionpack/lib/action_view/helpers/form_helper.rb index 32b9c4a7dd..c46b39fc23 100644 --- a/actionpack/lib/action_view/helpers/form_helper.rb +++ b/actionpack/lib/action_view/helpers/form_helper.rb @@ -282,7 +282,7 @@ module ActionView concat(form_tag(options.delete(:url) || {}, options.delete(:html) || {})) fields_for(object_name, *(args << options), &proc) - concat('') + concat(''.html_safe!) end def apply_form_for_options!(object_or_array, options) #:nodoc: @@ -809,7 +809,7 @@ module ActionView add_default_name_and_id(options) hidden = tag("input", "name" => options["name"], "type" => "hidden", "value" => options['disabled'] && checked ? checked_value : unchecked_value) checkbox = tag("input", options) - hidden + checkbox + (hidden + checkbox).html_safe! end def to_boolean_select_tag(options = {}) diff --git a/actionpack/lib/action_view/helpers/form_options_helper.rb b/actionpack/lib/action_view/helpers/form_options_helper.rb index 3db5202e7d..935ab5f3e8 100644 --- a/actionpack/lib/action_view/helpers/form_options_helper.rb +++ b/actionpack/lib/action_view/helpers/form_options_helper.rb @@ -296,7 +296,7 @@ module ActionView options << %() end - options_for_select.join("\n") + options_for_select.join("\n").html_safe! end # Returns a string of option tags that have been compiled by iterating over the +collection+ and assigning the diff --git a/actionpack/lib/action_view/helpers/form_tag_helper.rb b/actionpack/lib/action_view/helpers/form_tag_helper.rb index 1d851ecbd7..7688e786b1 100644 --- a/actionpack/lib/action_view/helpers/form_tag_helper.rb +++ b/actionpack/lib/action_view/helpers/form_tag_helper.rb @@ -440,7 +440,7 @@ module ActionView concat(tag(:fieldset, options, true)) concat(content_tag(:legend, legend)) unless legend.blank? concat(content) - concat("") + concat("".html_safe!) end private @@ -467,14 +467,14 @@ module ActionView def form_tag_html(html_options) extra_tags = extra_tags_for_form(html_options) - tag(:form, html_options, true) + extra_tags + (tag(:form, html_options, true) + extra_tags).html_safe! end def form_tag_in_block(html_options, &block) content = capture(&block) concat(form_tag_html(html_options)) concat(content) - concat("") + concat("".html_safe!) end def token_tag diff --git a/actionpack/lib/action_view/helpers/prototype_helper.rb b/actionpack/lib/action_view/helpers/prototype_helper.rb index 03f1dabb4e..8c1f0ad81f 100644 --- a/actionpack/lib/action_view/helpers/prototype_helper.rb +++ b/actionpack/lib/action_view/helpers/prototype_helper.rb @@ -395,7 +395,7 @@ module ActionView concat(form_remote_tag(options)) fields_for(object_name, *(args << options), &proc) - concat('') + concat(''.html_safe!) end alias_method :form_remote_for, :remote_form_for diff --git a/actionpack/lib/action_view/helpers/raw_output_helper.rb b/actionpack/lib/action_view/helpers/raw_output_helper.rb new file mode 100644 index 0000000000..79b0e4ee75 --- /dev/null +++ b/actionpack/lib/action_view/helpers/raw_output_helper.rb @@ -0,0 +1,9 @@ +module ActionView #:nodoc: + module Helpers #:nodoc: + module RawOutputHelper + def raw(stringish) + stringish.to_s.html_safe! + end + end + end +end \ No newline at end of file diff --git a/actionpack/lib/action_view/helpers/sanitize_helper.rb b/actionpack/lib/action_view/helpers/sanitize_helper.rb index d89b955317..69d0d0fb67 100644 --- a/actionpack/lib/action_view/helpers/sanitize_helper.rb +++ b/actionpack/lib/action_view/helpers/sanitize_helper.rb @@ -49,7 +49,11 @@ module ActionView # confuse browsers. # def sanitize(html, options = {}) - self.class.white_list_sanitizer.sanitize(html, options) + returning self.class.white_list_sanitizer.sanitize(html, options) do |sanitized| + if sanitized + sanitized.html_safe! + end + end end # Sanitizes a block of CSS code. Used by +sanitize+ when it comes across a style attribute. @@ -72,7 +76,11 @@ module ActionView # strip_tags("
Welcome to my website!
") # # => Welcome to my website! def strip_tags(html) - self.class.full_sanitizer.sanitize(html) + returning self.class.full_sanitizer.sanitize(html) do |sanitized| + if sanitized + sanitized.html_safe! + end + end end # Strips all link tags from +text+ leaving just the link text. diff --git a/actionpack/lib/action_view/helpers/tag_helper.rb b/actionpack/lib/action_view/helpers/tag_helper.rb index 7fae0f6b8d..ceddbd8cc1 100644 --- a/actionpack/lib/action_view/helpers/tag_helper.rb +++ b/actionpack/lib/action_view/helpers/tag_helper.rb @@ -41,7 +41,7 @@ module ActionView # tag("img", { :src => "open & shut.png" }, false, false) # # => def tag(name, options = nil, open = false, escape = true) - "<#{name}#{tag_options(options, escape) if options}#{open ? ">" : " />"}" + "<#{name}#{tag_options(options, escape) if options}#{open ? ">" : " />"}".html_safe! end # Returns an HTML block tag of type +name+ surrounding the +content+. Add @@ -94,7 +94,7 @@ module ActionView # cdata_section(File.read("hello_world.txt")) # # => def cdata_section(content) - "" + "".html_safe! end # Returns an escaped version of +html+ without affecting existing escaped entities. @@ -128,7 +128,7 @@ module ActionView def content_tag_string(name, content, options, escape = true) tag_options = tag_options(options, escape) if options - "<#{name}#{tag_options}>#{content}" + "<#{name}#{tag_options}>#{content}".html_safe! end def tag_options(options, escape = true) @@ -143,7 +143,7 @@ module ActionView attrs << %(#{key}="#{final_value}") end end - " #{attrs.sort * ' '}" unless attrs.empty? + " #{attrs.sort * ' '}".html_safe! unless attrs.empty? end end end diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb index 204d4d71e1..e651bc17a9 100644 --- a/actionpack/lib/action_view/helpers/url_helper.rb +++ b/actionpack/lib/action_view/helpers/url_helper.rb @@ -93,7 +93,7 @@ module ActionView polymorphic_path(options) end - escape ? escape_once(url) : url + (escape ? escape_once(url) : url).html_safe! end # Creates a link tag of the given +name+ using a URL created by the set @@ -220,7 +220,7 @@ module ActionView if block_given? options = args.first || {} html_options = args.second - concat(link_to(capture(&block), options, html_options)) + concat(link_to(capture(&block), options, html_options).html_safe!) else name = args[0] options = args[1] || {} @@ -238,7 +238,7 @@ module ActionView end href_attr = "href=\"#{url}\"" unless href - "#{name || url}" + "#{ERB::Util.h(name || url)}".html_safe! end end @@ -309,8 +309,8 @@ module ActionView html_options.merge!("type" => "submit", "value" => name) - "
" + - method_tag + tag("input", html_options) + request_token_tag + "
" + ("
" + + method_tag + tag("input", html_options) + request_token_tag + "
").html_safe! end diff --git a/actionpack/lib/action_view/render/partials.rb b/actionpack/lib/action_view/render/partials.rb index 7f10f54d2e..4f60566a09 100644 --- a/actionpack/lib/action_view/render/partials.rb +++ b/actionpack/lib/action_view/render/partials.rb @@ -223,7 +223,7 @@ module ActionView end result = template ? collection_with_template(template) : collection_without_template - result.join(spacer) + result.join(spacer).html_safe! end def collection_with_template(template) diff --git a/actionpack/lib/action_view/safe_buffer.rb b/actionpack/lib/action_view/safe_buffer.rb new file mode 100644 index 0000000000..8ba9cd80d6 --- /dev/null +++ b/actionpack/lib/action_view/safe_buffer.rb @@ -0,0 +1,28 @@ + +module ActionView #:nodoc: + class SafeBuffer < String + def <<(value) + if value.html_safe? + super(value) + else + super(CGI.escapeHTML(value)) + end + end + + def concat(value) + self << value + end + + def html_safe? + true + end + + def html_safe! + self + end + + def to_s + self + end + end +end \ No newline at end of file diff --git a/actionpack/lib/action_view/template/handlers/erb.rb b/actionpack/lib/action_view/template/handlers/erb.rb index aab7baf442..a780ab8d85 100644 --- a/actionpack/lib/action_view/template/handlers/erb.rb +++ b/actionpack/lib/action_view/template/handlers/erb.rb @@ -1,7 +1,31 @@ require 'active_support/core_ext/class/attribute_accessors' +require 'active_support/core_ext/string/output_safety' +require 'erubis' module ActionView module TemplateHandlers + class Erubis < ::Erubis::Eruby + def add_preamble(src) + src << "@output_buffer = ActionView::SafeBuffer.new;" + end + + def add_text(src, text) + src << "@output_buffer << ('" << escape_text(text) << "'.html_safe!);" + end + + def add_expr_literal(src, code) + src << '@output_buffer << ((' << code << ').to_s);' + end + + def add_expr_escaped(src, code) + src << '@output_buffer << ' << escaped_expr(code) << ';' + end + + def add_postamble(src) + src << '@output_buffer.to_s' + end + end + class ERB < TemplateHandler include Compilable @@ -15,11 +39,9 @@ module ActionView self.default_format = Mime::HTML def compile(template) - require 'erb' - magic = $1 if template.source =~ /\A(<%#.*coding[:=]\s*(\S+)\s*-?%>)/ erb = "#{magic}<% __in_erb_template=true %>#{template.source}" - ::ERB.new(erb, nil, erb_trim_mode, '@output_buffer').src + Erubis.new(erb, :trim=>(self.class.erb_trim_mode == "-")).src end end end diff --git a/actionpack/lib/action_view/test_case.rb b/actionpack/lib/action_view/test_case.rb index 441f462bc9..8beda24aba 100644 --- a/actionpack/lib/action_view/test_case.rb +++ b/actionpack/lib/action_view/test_case.rb @@ -55,7 +55,7 @@ module ActionView setup :setup_with_controller def setup_with_controller @controller = TestController.new - @output_buffer = '' + @output_buffer = ActionView::SafeBuffer.new @rendered = '' self.class.send(:include_helper_modules!) diff --git a/actionpack/test/controller/output_escaping_test.rb b/actionpack/test/controller/output_escaping_test.rb new file mode 100644 index 0000000000..7332f3f1e3 --- /dev/null +++ b/actionpack/test/controller/output_escaping_test.rb @@ -0,0 +1,19 @@ +require 'abstract_unit' + +class OutputEscapingTest < ActiveSupport::TestCase + + test "escape_html shouldn't die when passed nil" do + assert ERB::Util.h(nil).blank? + end + + test "escapeHTML should escape strings" do + assert_equal "<>"", ERB::Util.h("<>\"") + end + + test "escapeHTML shouldn't touch explicitly safe strings" do + # TODO this seems easier to compose and reason about, but + # this should be verified + assert_equal "<", ERB::Util.h("<".html_safe!) + end + +end diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index abcc8bf384..2db524ca4b 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -1050,7 +1050,7 @@ class RenderTest < ActionController::TestCase def test_action_talk_to_layout get :action_talk_to_layout - assert_equal "Talking to the layout\nAction was here!", @response.body + assert_equal "Talking to the layout\n\nAction was here!", @response.body end # :addressed: diff --git a/actionpack/test/template/asset_tag_helper_test.rb b/actionpack/test/template/asset_tag_helper_test.rb index 83fc6a282c..d94135b04b 100644 --- a/actionpack/test/template/asset_tag_helper_test.rb +++ b/actionpack/test/template/asset_tag_helper_test.rb @@ -231,6 +231,11 @@ class AssetTagHelperTest < ActionView::TestCase assert_dom_equal(%(\n\n\n\n), javascript_include_tag(:defaults)) end + def test_javascript_include_tag_is_html_safe + assert javascript_include_tag(:defaults).html_safe? + assert javascript_include_tag("prototype").html_safe? + end + def test_register_javascript_include_default ENV["RAILS_ASSET_ID"] = "" ActionView::Helpers::AssetTagHelper::register_javascript_include_default 'bank' @@ -285,6 +290,13 @@ class AssetTagHelperTest < ActionView::TestCase } end + def test_stylesheet_link_tag_is_html_safe + ENV["RAILS_ASSET_ID"] = "" + assert stylesheet_link_tag('dir/file').html_safe? + assert stylesheet_link_tag('dir/other/file', 'dir/file2').html_safe? + assert stylesheet_tag('dir/file', {}).html_safe? + end + def test_custom_stylesheet_expansions ENV["RAILS_ASSET_ID"] = '' ActionView::Helpers::AssetTagHelper::register_stylesheet_expansion :robbery => ["bank", "robber"] diff --git a/actionpack/test/template/erb_util_test.rb b/actionpack/test/template/erb_util_test.rb index 49f51c50c5..fa6b263965 100644 --- a/actionpack/test/template/erb_util_test.rb +++ b/actionpack/test/template/erb_util_test.rb @@ -15,6 +15,18 @@ class ErbUtilTest < Test::Unit::TestCase end end + def test_html_escape_is_html_safe + escaped = h("

") + assert_equal "<p>", escaped + assert escaped.html_safe? + end + + def test_html_escape_passes_html_escpe_unmodified + escaped = h("

".html_safe!) + assert_equal "

", escaped + assert escaped.html_safe? + end + def test_rest_in_ascii (0..127).to_a.map {|int| int.chr }.each do |chr| next if %w(& " < >).include?(chr) diff --git a/actionpack/test/template/form_helper_test.rb b/actionpack/test/template/form_helper_test.rb index 6a08c99619..04c635e770 100644 --- a/actionpack/test/template/form_helper_test.rb +++ b/actionpack/test/template/form_helper_test.rb @@ -974,7 +974,7 @@ class FormHelperTest < ActionView::TestCase (field_helpers - %w(hidden_field)).each do |selector| src = <<-END_SRC def #{selector}(field, *args, &proc) - " " + super + "
" + (" " + super + "
").html_safe! end END_SRC class_eval src, __FILE__, __LINE__ diff --git a/actionpack/test/template/raw_output_helper_test.rb b/actionpack/test/template/raw_output_helper_test.rb new file mode 100644 index 0000000000..598aa5b1d8 --- /dev/null +++ b/actionpack/test/template/raw_output_helper_test.rb @@ -0,0 +1,21 @@ +require 'abstract_unit' +require 'testing_sandbox' + +class RawOutputHelperTest < ActionView::TestCase + tests ActionView::Helpers::RawOutputHelper + include TestingSandbox + + def setup + @string = "hello" + end + + test "raw returns the safe string" do + result = raw(@string) + assert_equal @string, result + assert result.html_safe? + end + + test "raw handles nil values correctly" do + assert_equal "", raw(nil) + end +end \ No newline at end of file diff --git a/actionpack/test/template/render_test.rb b/actionpack/test/template/render_test.rb index 3c192906ae..35c51ca7ea 100644 --- a/actionpack/test/template/render_test.rb +++ b/actionpack/test/template/render_test.rb @@ -229,7 +229,7 @@ module RenderTestCases end def test_render_with_nested_layout - assert_equal %(title\n

column
\n
content
\n), + assert_equal %(title\n\n\n
column
\n
content
\n), @view.render(:file => "test/nested_layout.erb", :layout => "layouts/yield") end diff --git a/actionpack/test/template/sanitize_helper_test.rb b/actionpack/test/template/sanitize_helper_test.rb index f715071bbc..222d4dbf4c 100644 --- a/actionpack/test/template/sanitize_helper_test.rb +++ b/actionpack/test/template/sanitize_helper_test.rb @@ -39,7 +39,16 @@ class SanitizeHelperTest < ActionView::TestCase %{This is a test.\n\n\nIt no longer contains any HTML.\n}, strip_tags( %{This is <b>a <a href="" target="_blank">test</a></b>.\n\n\n\n

It no longer contains any HTML.

\n})) assert_equal "This has a here.", strip_tags("This has a here.") - [nil, '', ' '].each { |blank| assert_equal blank, strip_tags(blank) } + [nil, '', ' '].each do |blank| + stripped = strip_tags(blank) + assert_equal blank, stripped + assert stripped.html_safe? unless blank.nil? + end + assert strip_tags("").html_safe? end def assert_sanitized(text, expected = nil) diff --git a/actionpack/test/template/tag_helper_test.rb b/actionpack/test/template/tag_helper_test.rb index 2aa3d5b5fa..433f6514cf 100644 --- a/actionpack/test/template/tag_helper_test.rb +++ b/actionpack/test/template/tag_helper_test.rb @@ -34,6 +34,7 @@ class TagHelperTest < ActionView::TestCase def test_content_tag assert_equal "Create", content_tag("a", "Create", "href" => "create") + assert content_tag("a", "Create", "href" => "create").html_safe? assert_equal content_tag("a", "Create", "href" => "create"), content_tag("a", "Create", :href => "create") end diff --git a/actionpack/test/template/test_case_test.rb b/actionpack/test/template/test_case_test.rb index 5db42c4d68..ca72c13ffa 100644 --- a/actionpack/test/template/test_case_test.rb +++ b/actionpack/test/template/test_case_test.rb @@ -155,7 +155,7 @@ module ActionView class AssertionsTest < ActionView::TestCase def render_from_helper form_tag('/foo') do - concat render(:text => '
  • foo
') + concat render(:text => '
  • foo
').html_safe! end end helper_method :render_from_helper diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb index ce99482078..7f6ebc56b7 100644 --- a/actionpack/test/template/url_helper_test.rb +++ b/actionpack/test/template/url_helper_test.rb @@ -139,7 +139,7 @@ class UrlHelperTest < ActionView::TestCase end def test_link_tag_with_img - assert_dom_equal "", link_to("", "http://www.example.com") + assert_dom_equal "\"Favicon\"", link_to(image_tag("/favicon.jpg"), "http://www.example.com") end def test_link_with_nil_html_options diff --git a/actionpack/test/view/safe_buffer_test.rb b/actionpack/test/view/safe_buffer_test.rb new file mode 100644 index 0000000000..2236709627 --- /dev/null +++ b/actionpack/test/view/safe_buffer_test.rb @@ -0,0 +1,41 @@ +require 'abstract_unit' + +class SafeBufferTest < ActionView::TestCase + def setup + @buffer = ActionView::SafeBuffer.new + end + + test "Should look like a string" do + assert @buffer.is_a?(String) + assert_equal "", @buffer + end + + test "Should escape a raw string which is passed to them" do + @buffer << "