From 06165856196ac17b87163d146abea46019b17032 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 10 Jan 2011 11:36:23 -0800 Subject: [PATCH 01/42] use SQLite3::VERSION rather than the deprecated class --- activerecord/test/cases/query_cache_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/test/cases/query_cache_test.rb b/activerecord/test/cases/query_cache_test.rb index 33916c4e46..53aefc7b58 100644 --- a/activerecord/test/cases/query_cache_test.rb +++ b/activerecord/test/cases/query_cache_test.rb @@ -63,7 +63,7 @@ class QueryCacheTest < ActiveRecord::TestCase # Oracle adapter returns count() as Fixnum or Float if current_adapter?(:OracleAdapter) assert_kind_of Numeric, Task.connection.select_value("SELECT count(*) AS count_all FROM tasks") - elsif current_adapter?(:SQLite3Adapter) && SQLite3::Version::VERSION > '1.2.5' || current_adapter?(:Mysql2Adapter) || current_adapter?(:MysqlAdapter) + elsif current_adapter?(:SQLite3Adapter) && SQLite3::VERSION > '1.2.5' || current_adapter?(:Mysql2Adapter) || current_adapter?(:MysqlAdapter) # Future versions of the sqlite3 adapter will return numeric assert_instance_of Fixnum, Task.connection.select_value("SELECT count(*) AS count_all FROM tasks") From f4f4964ce0a05cac38bbff3b308ac558228bad29 Mon Sep 17 00:00:00 2001 From: Raimonds Simanovskis Date: Mon, 10 Jan 2011 18:55:32 +0200 Subject: [PATCH 02/42] Always return decimal average of integer fields In previous version if database adapter (e.g. SQLite and Oracle) returned non-String calculated values then type_cast_using_column converted decimal average value of intefer field to integer value. Now operation parameter is always checked to decide which conversion of calculated value should be done. --- .../lib/active_record/relation/calculations.rb | 14 +++++--------- activerecord/test/cases/calculations_test.rb | 5 +++++ 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index e9e451ec5c..b75a65e3ca 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -282,15 +282,11 @@ module ActiveRecord end def type_cast_calculated_value(value, column, operation = nil) - if value.is_a?(String) || value.nil? - case operation - when 'count' then value.to_i - when 'sum' then type_cast_using_column(value || '0', column) - when 'average' then value.try(:to_d) - else type_cast_using_column(value, column) - end - else - type_cast_using_column(value, column) + case operation + when 'count' then value.to_i + when 'sum' then type_cast_using_column(value || '0', column) + when 'average' then value.try(:to_d) + else type_cast_using_column(value, column) end end diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 5cb8485b4b..644c9cb528 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -23,6 +23,11 @@ class CalculationsTest < ActiveRecord::TestCase assert_equal 53.0, value end + def test_should_return_decimal_average_of_integer_field + value = Account.average(:id) + assert_equal 3.5, value + end + def test_should_return_nil_as_average assert_nil NumericData.average(:bank_balance) end From 1cc556dc5263bbbe5845b9a7abde9c84dffa7fb0 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 10 Jan 2011 15:43:21 -0800 Subject: [PATCH 03/42] adding to_d to BigDecimal --- .../lib/active_support/core_ext/big_decimal/conversions.rb | 4 ++++ .../test/core_ext/{bigdecimal.rb => bigdecimal_test.rb} | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-) rename activesupport/test/core_ext/{bigdecimal.rb => bigdecimal_test.rb} (83%) diff --git a/activesupport/lib/active_support/core_ext/big_decimal/conversions.rb b/activesupport/lib/active_support/core_ext/big_decimal/conversions.rb index f7f03f4d95..3720dbb8b8 100644 --- a/activesupport/lib/active_support/core_ext/big_decimal/conversions.rb +++ b/activesupport/lib/active_support/core_ext/big_decimal/conversions.rb @@ -18,6 +18,10 @@ class BigDecimal end end + def to_d + self + end + DEFAULT_STRING_FORMAT = 'F' def to_formatted_s(format = DEFAULT_STRING_FORMAT) _original_to_s(format) diff --git a/activesupport/test/core_ext/bigdecimal.rb b/activesupport/test/core_ext/bigdecimal_test.rb similarity index 83% rename from activesupport/test/core_ext/bigdecimal.rb rename to activesupport/test/core_ext/bigdecimal_test.rb index 9faad9146f..0272f564de 100644 --- a/activesupport/test/core_ext/bigdecimal.rb +++ b/activesupport/test/core_ext/bigdecimal_test.rb @@ -7,4 +7,9 @@ class BigDecimalTest < Test::Unit::TestCase assert_equal("--- .NaN\n", BigDecimal.new('NaN').to_yaml) assert_equal("--- -.Inf\n", BigDecimal.new('-Infinity').to_yaml) end -end \ No newline at end of file + + def test_to_d + bd = BigDecimal.new '10' + assert_equal bd, bd.to_d + end +end From a60ea742226f09dc566ad5d9a0b465c5d5db9687 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 10 Jan 2011 17:30:40 -0800 Subject: [PATCH 04/42] only use one array when collecting split up queries --- activerecord/lib/active_record/association_preload.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index 5aad2b4558..dc15b2f4c3 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -415,7 +415,7 @@ module ActiveRecord in_clause_length = connection.in_clause_length || ids.size records = [] ids.each_slice(in_clause_length) do |some_ids| - records += yield(some_ids) + records.concat yield(some_ids) end records end From 7d4d7457301faa85aa32b5ae29e13976e828954f Mon Sep 17 00:00:00 2001 From: Ernie Miller Date: Thu, 6 Jan 2011 20:06:29 -0500 Subject: [PATCH 05/42] Fix polymorphic belongs_to associationproxy raising errors when loading target. --- .../associations/belongs_to_polymorphic_association.rb | 5 +++++ .../cases/associations/belongs_to_associations_test.rb | 9 +++++++++ activerecord/test/models/sponsor.rb | 2 ++ 3 files changed, 16 insertions(+) diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb index 4f67b02d00..cae35fe0d0 100644 --- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb @@ -4,6 +4,11 @@ module ActiveRecord class BelongsToPolymorphicAssociation < BelongsToAssociation #:nodoc: private + def conditions + @conditions ||= interpolate_sql(target_klass.send(:sanitize_sql, @reflection.options[:conditions])) if @reflection.options[:conditions] + end + alias :sql_conditions :conditions + def replace_keys(record) super @owner[@reflection.foreign_type] = record && record.class.base_class.name diff --git a/activerecord/test/cases/associations/belongs_to_associations_test.rb b/activerecord/test/cases/associations/belongs_to_associations_test.rb index 1cb29a0fa1..4c4891dcaf 100644 --- a/activerecord/test/cases/associations/belongs_to_associations_test.rb +++ b/activerecord/test/cases/associations/belongs_to_associations_test.rb @@ -146,6 +146,15 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase assert_not_nil Company.find(3).firm_with_condition, "Microsoft should have a firm" end + def test_with_polymorphic_and_condition + sponsor = Sponsor.create + member = Member.create :name => "Bert" + sponsor.sponsorable = member + + assert_equal member, sponsor.sponsorable + assert_nil sponsor.sponsorable_with_conditions + end + def test_with_select assert_equal Company.find(2).firm_with_select.attributes.size, 1 assert_equal Company.find(2, :include => :firm_with_select ).firm_with_select.attributes.size, 1 diff --git a/activerecord/test/models/sponsor.rb b/activerecord/test/models/sponsor.rb index 7e5a1dc38b..aa4a3638fd 100644 --- a/activerecord/test/models/sponsor.rb +++ b/activerecord/test/models/sponsor.rb @@ -2,4 +2,6 @@ class Sponsor < ActiveRecord::Base belongs_to :sponsor_club, :class_name => "Club", :foreign_key => "club_id" belongs_to :sponsorable, :polymorphic => true belongs_to :thing, :polymorphic => true, :foreign_type => :sponsorable_type, :foreign_key => :sponsorable_id + belongs_to :sponsorable_with_conditions, :polymorphic => true, + :foreign_type => 'sponsorable_type', :foreign_key => 'sponsorable_id', :conditions => {:name => 'Ernie'} end From a61e3acef25d8fb86275d41a9d0d1ba163d7e0bb Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Tue, 11 Jan 2011 18:45:34 -0200 Subject: [PATCH 06/42] html_safe.to_str makes no sense --- actionpack/lib/action_controller/caching/fragments.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_controller/caching/fragments.rb b/actionpack/lib/action_controller/caching/fragments.rb index 88f973e62d..0be04b70a1 100644 --- a/actionpack/lib/action_controller/caching/fragments.rb +++ b/actionpack/lib/action_controller/caching/fragments.rb @@ -59,7 +59,7 @@ module ActionController #:nodoc: key = fragment_cache_key(key) instrument_fragment_cache :write_fragment, key do - content = content.html_safe.to_str if content.respond_to?(:html_safe) + content = content.to_str cache_store.write(key, content, options) end content From f6b71dea15435ea91e4db27ddf657ff840fd3a72 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 11 Jan 2011 13:38:17 -0800 Subject: [PATCH 07/42] avoid splatting arrays by using concat --- activerecord/lib/active_record/association_preload.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index dc15b2f4c3..bb7ddfcae9 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -126,7 +126,7 @@ module ActiveRecord parent_records.each do |parent_record| association_proxy = parent_record.send(reflection_name) association_proxy.loaded - association_proxy.target.push(*Array.wrap(associated_record)) + association_proxy.target.concat(Array.wrap(associated_record)) association_proxy.send(:set_inverse_instance, associated_record) end end @@ -139,8 +139,8 @@ module ActiveRecord def set_association_collection_records(id_to_record_map, reflection_name, associated_records, key) associated_records.each do |associated_record| - mapped_records = id_to_record_map[associated_record[key].to_s] - add_preloaded_records_to_collection(mapped_records, reflection_name, associated_record) + parent_records = id_to_record_map[associated_record[key].to_s] + add_preloaded_records_to_collection(parent_records, reflection_name, associated_record) end end From 1390a443289dbe4bfed63bfd3192c8c6a29fd833 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 7 Jan 2011 15:28:47 +0000 Subject: [PATCH 08/42] Correctly indent the bullet points under 'One-to-one associations', so that the lines are not broken in the generated rdoc html --- .../lib/active_record/associations.rb | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 3e7c9a370d..7659bd2d37 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -332,26 +332,26 @@ module ActiveRecord # === One-to-one associations # # * Assigning an object to a +has_one+ association automatically saves that object and - # the object being replaced (if there is one), in order to update their primary - # keys - except if the parent object is unsaved (new_record? == true). + # the object being replaced (if there is one), in order to update their primary + # keys - except if the parent object is unsaved (new_record? == true). # * If either of these saves fail (due to one of the objects being invalid) the assignment - # statement returns +false+ and the assignment is cancelled. + # statement returns +false+ and the assignment is cancelled. # * If you wish to assign an object to a +has_one+ association without saving it, - # use the association.build method (documented below). + # use the association.build method (documented below). # * Assigning an object to a +belongs_to+ association does not save the object, since - # the foreign key field belongs on the parent. It does not save the parent either. + # the foreign key field belongs on the parent. It does not save the parent either. # # === Collections # # * Adding an object to a collection (+has_many+ or +has_and_belongs_to_many+) automatically - # saves that object, except if the parent object (the owner of the collection) is not yet - # stored in the database. + # saves that object, except if the parent object (the owner of the collection) is not yet + # stored in the database. # * If saving any of the objects being added to a collection (via push or similar) - # fails, then push returns +false+. + # fails, then push returns +false+. # * You can add an object to a collection without automatically saving it by using the - # collection.build method (documented below). + # collection.build method (documented below). # * All unsaved (new_record? == true) members of the collection are automatically - # saved when the parent is saved. + # saved when the parent is saved. # # === Association callbacks # From 00dc8f77a2f9962d1bcd6267c63632bb39e6c547 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 7 Jan 2011 15:31:01 +0000 Subject: [PATCH 09/42] For a singular association, it should be build_association, rather than association.build (as association may be nil) --- activerecord/lib/active_record/associations.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 7659bd2d37..5f29f7c6f2 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -337,7 +337,7 @@ module ActiveRecord # * If either of these saves fail (due to one of the objects being invalid) the assignment # statement returns +false+ and the assignment is cancelled. # * If you wish to assign an object to a +has_one+ association without saving it, - # use the association.build method (documented below). + # use the build_association method (documented below). # * Assigning an object to a +belongs_to+ association does not save the object, since # the foreign key field belongs on the parent. It does not save the parent either. # From 15adcc3927b96328f3375aedd7d829f5e582854c Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 7 Jan 2011 15:34:57 +0000 Subject: [PATCH 10/42] Remove incorrect documentation about build_assoc on has_one. This is proven, for example, by test_successful_build_association in has_one_associations_test.rb --- activerecord/lib/active_record/associations.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 5f29f7c6f2..d26519e7d5 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1021,8 +1021,7 @@ module ActiveRecord # [build_association(attributes = {})] # Returns a new object of the associated type that has been instantiated # with +attributes+ and linked to this object through a foreign key, but has not - # yet been saved. Note: This ONLY works if an association already exists. - # It will NOT work if the association is +nil+. + # yet been saved. # [create_association(attributes = {})] # Returns a new object of the associated type that has been instantiated # with +attributes+, linked to this object through a foreign key, and that From 665880c0809b563d3afaeadd073f5d0b6289a42e Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 7 Jan 2011 18:27:33 +0000 Subject: [PATCH 11/42] Return value is irrelevant here as the RHS of the assignment is always returned by methods ending in '=' --- activerecord/lib/active_record/associations.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index d26519e7d5..8907c9c487 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1493,7 +1493,6 @@ module ActiveRecord end association.replace(record) - association.target.nil? ? nil : association end redefine_method("set_#{reflection.name}_target") do |target| From c6e10b0f600a56e962ff7d1614c50fca20630379 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 7 Jan 2011 19:12:37 +0000 Subject: [PATCH 12/42] has_one should always remove the old record (properly), even if not saving the new record, so we don't get the database into a pickle --- .../associations/has_one_association.rb | 2 +- .../associations/has_one_associations_test.rb | 35 +++++++++++++------ activerecord/test/fixtures/ships.yml | 1 + activerecord/test/models/pirate.rb | 4 +++ 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index 739bb919c5..9135c60009 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -20,7 +20,7 @@ module ActiveRecord load_target if @target && @target != record - remove_target(save && @reflection.options[:dependent]) + remove_target(@reflection.options[:dependent]) end if record diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index 8203534a37..f004d46c98 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -2,9 +2,11 @@ require "cases/helper" require 'models/developer' require 'models/project' require 'models/company' +require 'models/ship' +require 'models/pirate' class HasOneAssociationsTest < ActiveRecord::TestCase - fixtures :accounts, :companies, :developers, :projects, :developers_projects + fixtures :accounts, :companies, :developers, :projects, :developers_projects, :ships, :pirates def setup Account.destroyed_account_ids.clear @@ -164,15 +166,6 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_equal account, firm.account end - def test_build_association_twice_without_saving_affects_nothing - count_of_account = Account.count - firm = Firm.find(:first) - firm.build_account("credit_limit" => 1000) - firm.build_account("credit_limit" => 2000) - - assert_equal count_of_account, Account.count - end - def test_create_association firm = Firm.create(:name => "GlobalMegaCorp") account = firm.create_account(:credit_limit => 1000) @@ -293,4 +286,26 @@ class HasOneAssociationsTest < ActiveRecord::TestCase new_account = companies(:first_firm).build_account(:firm_name => 'Account') assert_equal new_account.firm_name, "Account" end + + def test_creation_failure_without_dependent_option + pirate = pirates(:blackbeard) + orig_ship = pirate.ship.target + + assert_equal ships(:black_pearl), orig_ship + new_ship = pirate.create_ship + assert_not_equal ships(:black_pearl), new_ship + assert_equal new_ship, pirate.ship + assert new_ship.new_record? + assert_nil orig_ship.pirate_id + assert !orig_ship.changed? # check it was saved + end + + def test_creation_failure_with_dependent_option + pirate = pirates(:blackbeard).becomes(DestructivePirate) + orig_ship = pirate.dependent_ship.target + + new_ship = pirate.create_dependent_ship + assert new_ship.new_record? + assert orig_ship.destroyed? + end end diff --git a/activerecord/test/fixtures/ships.yml b/activerecord/test/fixtures/ships.yml index 137055aad1..df914262b3 100644 --- a/activerecord/test/fixtures/ships.yml +++ b/activerecord/test/fixtures/ships.yml @@ -1,5 +1,6 @@ black_pearl: name: "Black Pearl" + pirate: blackbeard interceptor: id: 2 name: "Interceptor" diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb index f2c45053e7..b0490f754e 100644 --- a/activerecord/test/models/pirate.rb +++ b/activerecord/test/models/pirate.rb @@ -78,3 +78,7 @@ class Pirate < ActiveRecord::Base ship_log << "#{callback}_#{record.class.name.downcase}_#{record.id || ''}" end end + +class DestructivePirate < Pirate + has_one :dependent_ship, :class_name => 'Ship', :foreign_key => :pirate_id, :dependent => :destroy +end From 80df74bf515124c9db85d4a670989ae5cc28c3ec Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 8 Jan 2011 18:33:33 +0000 Subject: [PATCH 13/42] Enable the sqlite3 in-memory test connection to work --- activerecord/test/cases/helper.rb | 16 +++++++++++----- activerecord/test/cases/locking_test.rb | 4 ++-- .../test/cases/pooled_connections_test.rb | 2 +- activerecord/test/cases/unconnected_test.rb | 1 + .../native_sqlite3/in_memory_connection.rb | 19 ++++++++++--------- 5 files changed, 25 insertions(+), 17 deletions(-) diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index f9bbc5299b..51104d9cf5 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -26,6 +26,11 @@ def current_adapter?(*types) end end +def in_memory_db? + current_adapter?(:SQLiteAdapter) && + ActiveRecord::Base.connection_pool.spec.config[:database] == ":memory:" +end + def with_env_tz(new_tz = 'US/Eastern') old_tz, ENV['TZ'] = ENV['TZ'], new_tz yield @@ -100,11 +105,11 @@ class ActiveSupport::TestCase end end -# silence verbose schema loading -original_stdout = $stdout -$stdout = StringIO.new +def load_schema + # silence verbose schema loading + original_stdout = $stdout + $stdout = StringIO.new -begin adapter_name = ActiveRecord::Base.connection.adapter_name.downcase adapter_specific_schema_file = SCHEMA_ROOT + "/#{adapter_name}_specific_schema.rb" @@ -117,6 +122,8 @@ ensure $stdout = original_stdout end +load_schema + class << Time unless method_defined? :now_before_time_travel alias_method :now_before_time_travel, :now @@ -133,4 +140,3 @@ class << Time @now = nil end end - diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb index f9678cb0c5..71667defa7 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -252,13 +252,13 @@ end # TODO: The Sybase, and OpenBase adapters currently have no support for pessimistic locking -unless current_adapter?(:SybaseAdapter, :OpenBaseAdapter) +unless current_adapter?(:SybaseAdapter, :OpenBaseAdapter) || in_memory_db? class PessimisticLockingTest < ActiveRecord::TestCase self.use_transactional_fixtures = false fixtures :people, :readers def setup - Person.connection_pool.clear_reloadable_connections! + Person.connection_pool.clear_reloadable_connections # Avoid introspection queries during tests. Person.columns; Reader.columns end diff --git a/activerecord/test/cases/pooled_connections_test.rb b/activerecord/test/cases/pooled_connections_test.rb index de5fa140ba..6269437b14 100644 --- a/activerecord/test/cases/pooled_connections_test.rb +++ b/activerecord/test/cases/pooled_connections_test.rb @@ -137,4 +137,4 @@ class PooledConnectionsTest < ActiveRecord::TestCase def add_record(name) ActiveRecord::Base.connection_pool.with_connection { Project.create! :name => name } end -end unless %w(FrontBase).include? ActiveRecord::Base.connection.adapter_name +end unless current_adapter?(:FrontBase) || in_memory_db? diff --git a/activerecord/test/cases/unconnected_test.rb b/activerecord/test/cases/unconnected_test.rb index 23ad10f3f9..e82ca3f93d 100644 --- a/activerecord/test/cases/unconnected_test.rb +++ b/activerecord/test/cases/unconnected_test.rb @@ -14,6 +14,7 @@ class TestUnconnectedAdapter < ActiveRecord::TestCase def teardown @underlying = nil ActiveRecord::Base.establish_connection(@specification) + load_schema if in_memory_db? end def test_connection_no_longer_established diff --git a/activerecord/test/connections/native_sqlite3/in_memory_connection.rb b/activerecord/test/connections/native_sqlite3/in_memory_connection.rb index 6aba9719bb..14e10900d1 100644 --- a/activerecord/test/connections/native_sqlite3/in_memory_connection.rb +++ b/activerecord/test/connections/native_sqlite3/in_memory_connection.rb @@ -1,4 +1,8 @@ -print "Using native SQLite3\n" +# This file connects to an in-memory SQLite3 database, which is a very fast way to run the tests. +# The downside is that disconnect from the database results in the database effectively being +# wiped. For this reason, pooled_connections_test.rb is disabled when using an in-memory database. + +print "Using native SQLite3 (in memory)\n" require_dependency 'models/course' require 'logger' ActiveRecord::Base.logger = Logger.new("debug.log") @@ -6,13 +10,10 @@ ActiveRecord::Base.logger = Logger.new("debug.log") class SqliteError < StandardError end -def make_connection(clazz, db_definitions_file) - clazz.establish_connection(:adapter => 'sqlite3', :database => ':memory:') - File.read(SCHEMA_ROOT + "/#{db_definitions_file}").split(';').each do |command| - clazz.connection.execute(command) unless command.strip.empty? - end +def make_connection(clazz) + ActiveRecord::Base.configurations = { clazz.name => { :adapter => 'sqlite3', :database => ':memory:' } } + clazz.establish_connection(clazz.name) end -make_connection(ActiveRecord::Base, 'sqlite.sql') -make_connection(Course, 'sqlite2.sql') -load(SCHEMA_ROOT + "/schema.rb") +make_connection(ActiveRecord::Base) +make_connection(Course) From c47c54140246f4e5b49376ae9c408c85968ed6c3 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 8 Jan 2011 18:36:30 +0000 Subject: [PATCH 14/42] Have a separate test connection directory for sqlite3 in-memory so that the tests can be run without having to specifically rename the connection file (which then causes git to pick up the changes) --- activerecord/Rakefile | 2 +- .../connection.rb} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename activerecord/test/connections/{native_sqlite3/in_memory_connection.rb => native_sqlite3_mem/connection.rb} (100%) diff --git a/activerecord/Rakefile b/activerecord/Rakefile index f9b77c1799..064734f8e2 100755 --- a/activerecord/Rakefile +++ b/activerecord/Rakefile @@ -41,7 +41,7 @@ namespace :test do end end -%w( mysql mysql2 postgresql sqlite3 firebird db2 oracle sybase openbase frontbase jdbcmysql jdbcpostgresql jdbcsqlite3 jdbcderby jdbch2 jdbchsqldb ).each do |adapter| +%w( mysql mysql2 postgresql sqlite3 sqlite3_mem firebird db2 oracle sybase openbase frontbase jdbcmysql jdbcpostgresql jdbcsqlite3 jdbcderby jdbch2 jdbchsqldb ).each do |adapter| Rake::TestTask.new("test_#{adapter}") { |t| connection_path = "test/connections/#{adapter =~ /jdbc/ ? 'jdbc' : 'native'}_#{adapter}" adapter_short = adapter == 'db2' ? adapter : adapter[/^[a-z0-9]+/] diff --git a/activerecord/test/connections/native_sqlite3/in_memory_connection.rb b/activerecord/test/connections/native_sqlite3_mem/connection.rb similarity index 100% rename from activerecord/test/connections/native_sqlite3/in_memory_connection.rb rename to activerecord/test/connections/native_sqlite3_mem/connection.rb From 1bc71ed9607ba88c21c14b670a4308c8549f3941 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 8 Jan 2011 19:39:40 +0000 Subject: [PATCH 15/42] When assigning a has_one, if the existing record fails to be removed from the association, raise an error --- .../associations/has_one_association.rb | 16 +++++++++++----- .../associations/has_one_associations_test.rb | 12 ++++++++++++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index 9135c60009..a4332a0511 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -20,7 +20,7 @@ module ActiveRecord load_target if @target && @target != record - remove_target(@reflection.options[:dependent]) + remove_target!(@reflection.options[:dependent]) end if record @@ -55,13 +55,19 @@ module ActiveRecord record end - def remove_target(method) - case method - when :delete, :destroy + def remove_target!(method) + if [:delete, :destroy].include?(method) @target.send(method) else @target[@reflection.foreign_key] = nil - @target.save if @target.persisted? && @owner.persisted? + + if @target.persisted? && @owner.persisted? + unless @target.save + @target[@reflection.foreign_key] = @target.send("#{@reflection.foreign_key}_was") + raise RecordNotSaved, "Failed to remove the existing associated #{@reflection.name}. " + + "The record failed to save when after its foreign key was set to nil." + end + end end end end diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index f004d46c98..46459df7c5 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -308,4 +308,16 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert new_ship.new_record? assert orig_ship.destroyed? end + + def test_replacement_failure_due_to_existing_record_should_raise_error + pirate = pirates(:blackbeard) + pirate.ship.name = nil + + assert !pirate.ship.valid? + assert_raise(ActiveRecord::RecordNotSaved) do + pirate.ship = ships(:interceptor) + end + assert_equal ships(:black_pearl), pirate.ship + assert_equal pirate.id, pirate.ship.pirate_id + end end From 7f7b480098fa780dd76b4c1243230d74be67b3ca Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 8 Jan 2011 19:59:30 +0000 Subject: [PATCH 16/42] When assigning a has_one, if the new record fails to save, raise an error --- .../associations/has_one_association.rb | 6 ++-- .../associations/has_one_associations_test.rb | 28 +++++++++---------- .../test/cases/autosave_association_test.rb | 4 +-- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index a4332a0511..693c80163a 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -32,9 +32,9 @@ module ActiveRecord loaded if @owner.persisted? && record && save - record.save && self - else - record && self + unless record.save + raise RecordNotSaved, "Failed to save the new associated #{@reflection.name}." + end end end diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index 46459df7c5..b9719fa983 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -93,18 +93,18 @@ class HasOneAssociationsTest < ActiveRecord::TestCase def test_nullification_on_association_change firm = companies(:rails_core) old_account_id = firm.account.id - firm.account = Account.new + firm.account = Account.new(:credit_limit => 5) # account is dependent with nullify, therefore its firm_id should be nil assert_nil Account.find(old_account_id).firm_id end def test_association_change_calls_delete - companies(:first_firm).deletable_account = Account.new + companies(:first_firm).deletable_account = Account.new(:credit_limit => 5) assert_equal [], Account.destroyed_account_ids[companies(:first_firm).id] end def test_association_change_calls_destroy - companies(:first_firm).account = Account.new + companies(:first_firm).account = Account.new(:credit_limit => 5) assert_equal [companies(:first_firm).id], Account.destroyed_account_ids[companies(:first_firm).id] end @@ -182,17 +182,6 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_equal account, firm.account end - def test_failing_build_association - firm = Firm.new("name" => "GlobalMegaCorp") - firm.save - - firm.account = account = Account.new - assert_equal account, firm.account - assert !account.save - assert_equal account, firm.account - assert_equal ["can't be empty"], account.errors["credit_limit"] - end - def test_create firm = Firm.new("name" => "GlobalMegaCorp") firm.save @@ -320,4 +309,15 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_equal ships(:black_pearl), pirate.ship assert_equal pirate.id, pirate.ship.pirate_id end + + def test_replacement_failure_due_to_new_record_should_raise_error + pirate = pirates(:blackbeard) + new_ship = Ship.new + + assert_raise(ActiveRecord::RecordNotSaved) do + pirate.ship = new_ship + end + assert_equal new_ship, pirate.ship + assert_equal pirate.id, new_ship.pirate_id + end end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 71fd3fd836..2e43a20a73 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -90,7 +90,7 @@ class TestDefaultAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCas firm = Firm.find(:first) assert firm.valid? - firm.account = Account.new + firm.build_account assert !firm.account.valid? assert !firm.valid? @@ -102,7 +102,7 @@ class TestDefaultAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCas firm = Firm.find(:first) assert firm.valid? - firm.unvalidated_account = Account.new + firm.build_unvalidated_account assert !firm.unvalidated_account.valid? assert firm.valid? From 29452abb846c74299a23d510b394768fe6dec47c Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 15:01:04 +0000 Subject: [PATCH 17/42] SQLite3 has supported savepoints since version 3.6.8, we should use this! --- .../connection_adapters/abstract_adapter.rb | 4 ++-- .../connection_adapters/sqlite_adapter.rb | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 0282493219..5ff5813699 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -77,8 +77,8 @@ module ActiveRecord false end - # Does this adapter support savepoints? PostgreSQL and MySQL do, SQLite - # does not. + # Does this adapter support savepoints? PostgreSQL and MySQL do, + # SQLite < 3.6.8 does not. def supports_savepoints? false end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb index bf599a95f7..b04383d5bf 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb @@ -62,6 +62,10 @@ module ActiveRecord sqlite_version >= '2.0.0' end + def supports_savepoints? + sqlite_version >= '3.6.8' + end + # Returns +true+ when the connection adapter supports prepared statement # caching, otherwise returns +false+ def supports_statement_cache? @@ -189,6 +193,18 @@ module ActiveRecord exec_query(sql, name).rows end + def create_savepoint + execute("SAVEPOINT #{current_savepoint_name}") + end + + def rollback_to_savepoint + execute("ROLLBACK TO SAVEPOINT #{current_savepoint_name}") + end + + def release_savepoint + execute("RELEASE SAVEPOINT #{current_savepoint_name}") + end + def begin_db_transaction #:nodoc: @connection.transaction end From 4e19ec566c9132b85fdf0ff13a328238a6aca591 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 15:52:41 +0000 Subject: [PATCH 18/42] In a number of places in the tests, we only need to turn off transactional fixtures when the DB does not support savepoints. This speeds the test run up by about 8-9% on my computer, when running rake test_sqlite3_mem :) --- .../cases/associations/join_model_test.rb | 3 +- .../test/cases/autosave_association_test.rb | 20 +++---- activerecord/test/cases/helper.rb | 4 ++ activerecord/test/cases/locking_test.rb | 54 ++++++++++--------- activerecord/test/cases/migration_test.rb | 3 +- activerecord/test/cases/multiple_db_test.rb | 2 +- .../test/cases/nested_attributes_test.rb | 4 +- .../test/cases/session_store/session_test.rb | 2 +- activerecord/test/cases/unconnected_test.rb | 2 +- 9 files changed, 51 insertions(+), 43 deletions(-) diff --git a/activerecord/test/cases/associations/join_model_test.rb b/activerecord/test/cases/associations/join_model_test.rb index 7e9c190f42..512a6d3ef8 100644 --- a/activerecord/test/cases/associations/join_model_test.rb +++ b/activerecord/test/cases/associations/join_model_test.rb @@ -13,7 +13,8 @@ require 'models/book' require 'models/citation' class AssociationsJoinModelTest < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? + fixtures :posts, :authors, :categories, :categorizations, :comments, :tags, :taggings, :author_favorites, :vertices, :items, :books, # Reload edges table from fixtures as otherwise repeated test was failing :edges diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 2e43a20a73..11c0c5b0ef 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -572,7 +572,7 @@ class TestDefaultAutosaveAssociationOnNewRecord < ActiveRecord::TestCase end class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?") @@ -797,7 +797,7 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase end class TestAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?") @@ -917,7 +917,7 @@ class TestAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCase end class TestAutosaveAssociationOnABelongsToAssociation < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @ship = Ship.create(:name => 'Nights Dirty Lightning') @@ -1164,7 +1164,7 @@ module AutosaveAssociationOnACollectionAssociationTests end class TestAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @association_name = :birds @@ -1178,7 +1178,7 @@ class TestAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCase end class TestAutosaveAssociationOnAHasAndBelongsToManyAssociation < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @association_name = :parrots @@ -1193,7 +1193,7 @@ class TestAutosaveAssociationOnAHasAndBelongsToManyAssociation < ActiveRecord::T end class TestAutosaveAssociationValidationsOnAHasManyAssociation < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?") @@ -1209,7 +1209,7 @@ class TestAutosaveAssociationValidationsOnAHasManyAssociation < ActiveRecord::Te end class TestAutosaveAssociationValidationsOnAHasOneAssociation < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?") @@ -1230,7 +1230,7 @@ class TestAutosaveAssociationValidationsOnAHasOneAssociation < ActiveRecord::Tes end class TestAutosaveAssociationValidationsOnABelongsToAssociation < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?") @@ -1250,7 +1250,7 @@ class TestAutosaveAssociationValidationsOnABelongsToAssociation < ActiveRecord:: end class TestAutosaveAssociationValidationsOnAHABTMAssociation < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?") @@ -1272,7 +1272,7 @@ class TestAutosaveAssociationValidationsOnAHABTMAssociation < ActiveRecord::Test end class TestAutosaveAssociationValidationMethodsGeneration < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @pirate = Pirate.new diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index 51104d9cf5..2cc993b6ed 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -31,6 +31,10 @@ def in_memory_db? ActiveRecord::Base.connection_pool.spec.config[:database] == ":memory:" end +def supports_savepoints? + ActiveRecord::Base.connection.supports_savepoints? +end + def with_env_tz(new_tz = 'US/Eastern') old_tz, ENV['TZ'] = ENV['TZ'], new_tz yield diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb index 71667defa7..c5a204b335 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -19,11 +19,6 @@ end class OptimisticLockingTest < ActiveRecord::TestCase fixtures :people, :legacy_things, :references - # need to disable transactional fixtures, because otherwise the sqlite3 - # adapter (at least) chokes when we try and change the schema in the middle - # of a test (see test_increment_counter_*). - self.use_transactional_fixtures = false - def test_lock_existing p1 = Person.find(1) p2 = Person.find(1) @@ -152,6 +147,33 @@ class OptimisticLockingTest < ActiveRecord::TestCase assert_equal "unchangeable name", p.first_name end + def test_quote_table_name + ref = references(:michael_magician) + ref.favourite = !ref.favourite + assert ref.save + end + + # Useful for partial updates, don't only update the lock_version if there + # is nothing else being updated. + def test_update_without_attributes_does_not_only_update_lock_version + assert_nothing_raised do + p1 = Person.create!(:first_name => 'anika') + lock_version = p1.lock_version + p1.save + p1.reload + assert_equal lock_version, p1.lock_version + end + end +end + +class OptimisticLockingWithSchemaChangeTest < ActiveRecord::TestCase + fixtures :people, :legacy_things, :references + + # need to disable transactional fixtures, because otherwise the sqlite3 + # adapter (at least) chokes when we try and change the schema in the middle + # of a test (see test_increment_counter_*). + self.use_transactional_fixtures = false + { :lock_version => Person, :custom_lock_version => LegacyThing }.each do |name, model| define_method("test_increment_counter_updates_#{name}") do counter_test model, 1 do |id| @@ -198,24 +220,6 @@ class OptimisticLockingTest < ActiveRecord::TestCase assert_raises(ActiveRecord::RecordNotFound) { LegacyThing.find(t.id) } end - def test_quote_table_name - ref = references(:michael_magician) - ref.favourite = !ref.favourite - assert ref.save - end - - # Useful for partial updates, don't only update the lock_version if there - # is nothing else being updated. - def test_update_without_attributes_does_not_only_update_lock_version - assert_nothing_raised do - p1 = Person.create!(:first_name => 'anika') - lock_version = p1.lock_version - p1.save - p1.reload - assert_equal lock_version, p1.lock_version - end - end - private def add_counter_column_to(model, col='test_count') @@ -254,11 +258,11 @@ end unless current_adapter?(:SybaseAdapter, :OpenBaseAdapter) || in_memory_db? class PessimisticLockingTest < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? fixtures :people, :readers def setup - Person.connection_pool.clear_reloadable_connections + Person.connection_pool.clear_reloadable_connections! # Avoid introspection queries during tests. Person.columns; Reader.columns end diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 1a65045ded..a5a9965c3a 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -14,7 +14,7 @@ if ActiveRecord::Base.connection.supports_migrations? class Reminder < ActiveRecord::Base; end class ActiveRecord::Migration - class < "My baby takes tha mornin' train!") @@ -899,7 +899,7 @@ class TestHasOneAutosaveAssociationWhichItselfHasAutosaveAssociations < ActiveRe end class TestHasManyAutosaveAssociationWhichItselfHasAutosaveAssociations < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @ship = Ship.create!(:name => "The good ship Dollypop") diff --git a/activerecord/test/cases/session_store/session_test.rb b/activerecord/test/cases/session_store/session_test.rb index 6f1c170a0c..f906bda8c3 100644 --- a/activerecord/test/cases/session_store/session_test.rb +++ b/activerecord/test/cases/session_store/session_test.rb @@ -5,7 +5,7 @@ require 'active_record/session_store' module ActiveRecord class SessionStore class SessionTest < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? && ActiveRecord::Base.connection.supports_ddl_transactions? def setup super diff --git a/activerecord/test/cases/unconnected_test.rb b/activerecord/test/cases/unconnected_test.rb index e82ca3f93d..f85fb4e5da 100644 --- a/activerecord/test/cases/unconnected_test.rb +++ b/activerecord/test/cases/unconnected_test.rb @@ -4,7 +4,7 @@ class TestRecord < ActiveRecord::Base end class TestUnconnectedAdapter < ActiveRecord::TestCase - self.use_transactional_fixtures = false + self.use_transactional_fixtures = false unless supports_savepoints? def setup @underlying = ActiveRecord::Base.connection From 1d6e2184283d15d20ed3102ca462d905e5efa73d Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 16:06:14 +0000 Subject: [PATCH 19/42] When assigning a has_one, if anything fails, the assignment should be rolled back entirely --- .../associations/has_one_association.rb | 42 ++++++++++--------- .../associations/has_one_associations_test.rb | 7 +++- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index 693c80163a..f60547c22c 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -19,23 +19,25 @@ module ActiveRecord raise_on_type_mismatch(record) unless record.nil? load_target - if @target && @target != record - remove_target!(@reflection.options[:dependent]) - end + @reflection.klass.transaction do + if @target && @target != record + remove_target!(@reflection.options[:dependent]) + end - if record - set_owner_attributes(record) - set_inverse_instance(record) + if record + set_inverse_instance(record) + set_owner_attributes(record) + + if @owner.persisted? && save && !record.save + nullify_owner_attributes(record) + set_owner_attributes(@target) + raise RecordNotSaved, "Failed to save the new associated #{@reflection.name}." + end + end end @target = record loaded - - if @owner.persisted? && record && save - unless record.save - raise RecordNotSaved, "Failed to save the new associated #{@reflection.name}." - end - end end private @@ -59,17 +61,19 @@ module ActiveRecord if [:delete, :destroy].include?(method) @target.send(method) else - @target[@reflection.foreign_key] = nil + nullify_owner_attributes(@target) - if @target.persisted? && @owner.persisted? - unless @target.save - @target[@reflection.foreign_key] = @target.send("#{@reflection.foreign_key}_was") - raise RecordNotSaved, "Failed to remove the existing associated #{@reflection.name}. " + - "The record failed to save when after its foreign key was set to nil." - end + if @target.persisted? && @owner.persisted? && !@target.save + set_owner_attributes(@target) + raise RecordNotSaved, "Failed to remove the existing associated #{@reflection.name}. " + + "The record failed to save when after its foreign key was set to nil." end end end + + def nullify_owner_attributes(record) + record[@reflection.foreign_key] = nil + end end end end diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index b9719fa983..925b76b901 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -6,6 +6,7 @@ require 'models/ship' require 'models/pirate' class HasOneAssociationsTest < ActiveRecord::TestCase + self.use_transactional_fixtures = false unless supports_savepoints? fixtures :accounts, :companies, :developers, :projects, :developers_projects, :ships, :pirates def setup @@ -317,7 +318,9 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_raise(ActiveRecord::RecordNotSaved) do pirate.ship = new_ship end - assert_equal new_ship, pirate.ship - assert_equal pirate.id, new_ship.pirate_id + assert_equal ships(:black_pearl), pirate.ship + assert_equal pirate.id, pirate.ship.pirate_id + assert_equal pirate.id, ships(:black_pearl).reload.pirate_id + assert_nil new_ship.pirate_id end end From 6055bbedaa4b7b4bb2377ac87147196eebb2edc1 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 16:34:23 +0000 Subject: [PATCH 20/42] Raise ActiveRecord::RecordNotSaved if an AssociationCollection fails to be replaced --- .../associations/association_collection.rb | 6 +++++- .../associations/has_many_associations_test.rb | 13 +++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index daec8493ac..3d8a23fdca 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -314,7 +314,11 @@ module ActiveRecord transaction do delete(@target - other_array) - concat(other_array - @target) + + unless concat(other_array - @target) + raise RecordNotSaved, "Failed to replace #{@reflection.name} because one or more of the " + "new records could not be saved." + end end end diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 2b7ad3642a..1ce91d7211 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -975,6 +975,19 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert !firm.clients.include?(:first_client) end + def test_replace_failure + firm = companies(:first_firm) + account = Account.new + orig_accounts = firm.accounts.to_a + + assert !account.valid? + assert !orig_accounts.empty? + assert_raise ActiveRecord::RecordNotSaved do + firm.accounts = [account] + end + assert_equal orig_accounts, firm.accounts + end + def test_get_ids assert_equal [companies(:first_client).id, companies(:second_client).id], companies(:first_firm).client_ids end From 9086b02ba5b0bd3e956097c7edbd0f337c614801 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 16:42:30 +0000 Subject: [PATCH 21/42] Document the recent changes to association assignment --- activerecord/lib/active_record/associations.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 8907c9c487..3f9762383b 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -332,12 +332,14 @@ module ActiveRecord # === One-to-one associations # # * Assigning an object to a +has_one+ association automatically saves that object and - # the object being replaced (if there is one), in order to update their primary + # the object being replaced (if there is one), in order to update their foreign # keys - except if the parent object is unsaved (new_record? == true). - # * If either of these saves fail (due to one of the objects being invalid) the assignment - # statement returns +false+ and the assignment is cancelled. + # * If either of these saves fail (due to one of the objects being invalid), an + # ActiveRecord::RecordNotSaved exception is raised and the assignment is + # cancelled. # * If you wish to assign an object to a +has_one+ association without saving it, - # use the build_association method (documented below). + # use the build_association method (documented below). The object being + # replaced will still be saved to update its foreign key. # * Assigning an object to a +belongs_to+ association does not save the object, since # the foreign key field belongs on the parent. It does not save the parent either. # @@ -348,6 +350,9 @@ module ActiveRecord # stored in the database. # * If saving any of the objects being added to a collection (via push or similar) # fails, then push returns +false+. + # * If saving fails while replacing the collection (via association=), an + # ActiveRecord::RecordNotSaved exception is raised and the assignment is + # cancelled. # * You can add an object to a collection without automatically saving it by using the # collection.build method (documented below). # * All unsaved (new_record? == true) members of the collection are automatically From 4754018272d576590693e5418936db23c7d13172 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 16:45:22 +0000 Subject: [PATCH 22/42] find_target can be inherited --- .../active_record/associations/has_one_through_association.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/activerecord/lib/active_record/associations/has_one_through_association.rb b/activerecord/lib/active_record/associations/has_one_through_association.rb index 11fa40a5c4..ab8543e4af 100644 --- a/activerecord/lib/active_record/associations/has_one_through_association.rb +++ b/activerecord/lib/active_record/associations/has_one_through_association.rb @@ -31,10 +31,6 @@ module ActiveRecord end end end - - def find_target - scoped.first - end end end end From 3b797c8c8681c8f4619bce39d3f042244fe002d9 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 17:13:05 +0000 Subject: [PATCH 23/42] DRY up the code which instantiates the association proxy --- .../lib/active_record/associations.rb | 59 ++++++++----------- activerecord/lib/active_record/reflection.rb | 25 ++++++++ .../validations/uniqueness_validation_test.rb | 13 ---- activerecord/test/models/reply.rb | 7 +++ activerecord/test/models/topic.rb | 4 ++ 5 files changed, 60 insertions(+), 48 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 3f9762383b..20f22f8d1c 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -137,6 +137,22 @@ module ActiveRecord # :nodoc: attr_reader :association_cache + protected + + # Returns the proxy for the given association name, instantiating it if it doesn't + # already exist + def association_proxy(name) + association = association_instance_get(name) + + if association.nil? + reflection = self.class.reflect_on_association(name) + association = reflection.proxy_class.new(self, reflection) + association_instance_set(name, association) + end + + association + end + private # Returns the specified association instance if it responds to :loaded?, nil otherwise. def association_instance_get(name) @@ -1468,12 +1484,7 @@ module ActiveRecord def association_accessor_methods(reflection, association_proxy_class) redefine_method(reflection.name) do |*params| force_reload = params.first unless params.empty? - association = association_instance_get(reflection.name) - - if association.nil? - association = association_proxy_class.new(self, reflection) - association_instance_set(reflection.name, association) - end + association = association_proxy(reflection.name) if force_reload reflection.klass.uncached { association.reload } @@ -1485,38 +1496,26 @@ module ActiveRecord end redefine_method("loaded_#{reflection.name}?") do - association = association_instance_get(reflection.name) + association = association_proxy(reflection.name) association && association.loaded? end redefine_method("#{reflection.name}=") do |record| - association = association_instance_get(reflection.name) - - if association.nil? - association = association_proxy_class.new(self, reflection) - association_instance_set(reflection.name, association) - end - - association.replace(record) + association_proxy(reflection.name).replace(record) end redefine_method("set_#{reflection.name}_target") do |target| - association = association_proxy_class.new(self, reflection) + association = association_proxy(reflection.name) association.target = target association.loaded - association_instance_set(reflection.name, association) + association end end def collection_reader_method(reflection, association_proxy_class) redefine_method(reflection.name) do |*params| force_reload = params.first unless params.empty? - association = association_instance_get(reflection.name) - - unless association - association = association_proxy_class.new(self, reflection) - association_instance_set(reflection.name, association) - end + association = association_proxy(reflection.name) if force_reload reflection.klass.uncached { association.reload } @@ -1541,10 +1540,7 @@ module ActiveRecord if writer redefine_method("#{reflection.name}=") do |new_value| - # Loads proxy class instance (defined in collection_reader_method) if not already loaded - association = send(reflection.name) - association.replace(new_value) - association + association_proxy(reflection.name).replace(new_value) end redefine_method("#{reflection.name.to_s.singularize}_ids=") do |new_value| @@ -1559,14 +1555,7 @@ module ActiveRecord def association_constructor_method(constructor, reflection, association_proxy_class) redefine_method("#{constructor}_#{reflection.name}") do |*params| attributes = params.first unless params.empty? - association = association_instance_get(reflection.name) - - unless association - association = association_proxy_class.new(self, reflection) - association_instance_set(reflection.name, association) - end - - association.send(constructor, attributes) + association_proxy(reflection.name).send(constructor, attributes) end end diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 937efe395f..ceeb0ec39d 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -313,6 +313,31 @@ module ActiveRecord macro == :belongs_to end + def proxy_class + case macro + when :belongs_to + if options[:polymorphic] + Associations::BelongsToPolymorphicAssociation + else + Associations::BelongsToAssociation + end + when :has_and_belongs_to_many + Associations::HasAndBelongsToManyAssociation + when :has_many + if options[:through] + Associations::HasManyThroughAssociation + else + Associations::HasManyAssociation + end + when :has_one + if options[:through] + Associations::HasOneThroughAssociation + else + Associations::HasOneAssociation + end + end + end + private def derive_class_name class_name = name.to_s.camelize diff --git a/activerecord/test/cases/validations/uniqueness_validation_test.rb b/activerecord/test/cases/validations/uniqueness_validation_test.rb index 679d67553b..b4f3dd034c 100644 --- a/activerecord/test/cases/validations/uniqueness_validation_test.rb +++ b/activerecord/test/cases/validations/uniqueness_validation_test.rb @@ -6,19 +6,6 @@ require 'models/warehouse_thing' require 'models/guid' require 'models/event' -# The following methods in Topic are used in test_conditional_validation_* -class Topic - has_many :unique_replies, :dependent => :destroy, :foreign_key => "parent_id" - has_many :silly_unique_replies, :dependent => :destroy, :foreign_key => "parent_id" -end - -class UniqueReply < Reply - validates_uniqueness_of :content, :scope => 'parent_id' -end - -class SillyUniqueReply < UniqueReply -end - class Wizard < ActiveRecord::Base self.abstract_class = true diff --git a/activerecord/test/models/reply.rb b/activerecord/test/models/reply.rb index 110d540120..6adfe0ae3c 100644 --- a/activerecord/test/models/reply.rb +++ b/activerecord/test/models/reply.rb @@ -10,6 +10,13 @@ class Reply < Topic attr_accessible :title, :author_name, :author_email_address, :written_on, :content, :last_read, :parent_title end +class UniqueReply < Reply + validates_uniqueness_of :content, :scope => 'parent_id' +end + +class SillyUniqueReply < UniqueReply +end + class WrongReply < Reply validate :errors_on_empty_content validate :title_is_wrong_create, :on => :create diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb index 6496f36f7e..6440dbe8ab 100644 --- a/activerecord/test/models/topic.rb +++ b/activerecord/test/models/topic.rb @@ -45,6 +45,10 @@ class Topic < ActiveRecord::Base has_many :replies, :dependent => :destroy, :foreign_key => "parent_id" has_many :replies_with_primary_key, :class_name => "Reply", :dependent => :destroy, :primary_key => "title", :foreign_key => "parent_title" + + has_many :unique_replies, :dependent => :destroy, :foreign_key => "parent_id" + has_many :silly_unique_replies, :dependent => :destroy, :foreign_key => "parent_id" + serialize :content before_create :default_written_on From 42b2e4f85bef64b7aa1382e96c79db1d4f318a56 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 18:33:51 +0000 Subject: [PATCH 24/42] We can use the association_proxy method directly in HasOneThroughAssociation now --- .../associations/association_proxy.rb | 35 ++++++++++--------- .../has_one_through_association.rb | 5 ++- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb index 294e1cab50..5fbda0bd3d 100644 --- a/activerecord/lib/active_record/associations/association_proxy.rb +++ b/activerecord/lib/active_record/associations/association_proxy.rb @@ -259,23 +259,6 @@ module ActiveRecord end end - private - # Forwards any missing method call to the \target. - def method_missing(method, *args) - if load_target - unless @target.respond_to?(method) - message = "undefined method `#{method.to_s}' for \"#{@target}\":#{@target.class.to_s}" - raise NoMethodError, message - end - - if block_given? - @target.send(method, *args) { |*block_args| yield(*block_args) } - else - @target.send(method, *args) - end - end - end - # Loads the \target if needed and returns it. # # This method is abstract in the sense that it relies on +find_target+, @@ -299,6 +282,24 @@ module ActiveRecord reset end + private + + # Forwards any missing method call to the \target. + def method_missing(method, *args) + if load_target + unless @target.respond_to?(method) + message = "undefined method `#{method.to_s}' for \"#{@target}\":#{@target.class.to_s}" + raise NoMethodError, message + end + + if block_given? + @target.send(method, *args) { |*block_args| yield(*block_args) } + else + @target.send(method, *args) + end + end + end + # Should be true if there is a foreign key present on the @owner which # references the target. This is used to determine whether we can load # the target if the @owner is currently a new record (and therefore diff --git a/activerecord/lib/active_record/associations/has_one_through_association.rb b/activerecord/lib/active_record/associations/has_one_through_association.rb index ab8543e4af..59a704b7bf 100644 --- a/activerecord/lib/active_record/associations/has_one_through_association.rb +++ b/activerecord/lib/active_record/associations/has_one_through_association.rb @@ -13,9 +13,8 @@ module ActiveRecord private def create_through_record(new_value) - proxy = @owner.send(@reflection.through_reflection.name) || - @owner.send(:association_instance_get, @reflection.through_reflection.name) - record = proxy.target + proxy = @owner.send(:association_proxy, @reflection.through_reflection.name) + record = proxy.send(:load_target) if record && !new_value record.destroy From 681ab53ba15f9fc95c8a91e50bb0138aa66967b2 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 18:37:01 +0000 Subject: [PATCH 25/42] Get rid of set_association_target and association_loaded? as the parts of the code that need that can now just use association_proxy(:name).loaded?/target= --- .../lib/active_record/association_preload.rb | 19 +++++++++++++------ .../lib/active_record/associations.rb | 12 ------------ .../associations/association_proxy.rb | 3 ++- .../class_methods/join_dependency.rb | 3 ++- .../has_one_through_associations_test.rb | 2 +- 5 files changed, 18 insertions(+), 21 deletions(-) diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index bb7ddfcae9..4c517e3339 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -133,7 +133,7 @@ module ActiveRecord def add_preloaded_record_to_collection(parent_records, reflection_name, associated_record) parent_records.each do |parent_record| - parent_record.send("set_#{reflection_name}_target", associated_record) + parent_record.send(:association_proxy, reflection_name).target = associated_record end end @@ -158,14 +158,17 @@ module ActiveRecord seen_keys[seen_key] = true mapped_records = id_to_record_map[seen_key] mapped_records.each do |mapped_record| - association_proxy = mapped_record.send("set_#{reflection_name}_target", associated_record) + association_proxy = mapped_record.send(:association_proxy, reflection_name) + association_proxy.target = associated_record association_proxy.send(:set_inverse_instance, associated_record) end end id_to_record_map.each do |id, records| next if seen_keys.include?(id.to_s) - records.each {|record| record.send("set_#{reflection_name}_target", nil) } + records.each do |record| + record.send(:association_proxy, reflection_name).target = nil + end end end @@ -232,10 +235,14 @@ module ActiveRecord end def preload_has_one_association(records, reflection, preload_options={}) - return if records.first.send("loaded_#{reflection.name}?") + return if records.first.send(:association_proxy, reflection.name).loaded? id_to_record_map, ids = construct_id_map(records, reflection.options[:primary_key]) options = reflection.options - records.each {|record| record.send("set_#{reflection.name}_target", nil)} + + records.each do |record| + record.send(:association_proxy, reflection.name).target = nil + end + if options[:through] through_records = preload_through_records(records, reflection, options[:through]) @@ -317,7 +324,7 @@ module ActiveRecord end def preload_belongs_to_association(records, reflection, preload_options={}) - return if records.first.send("loaded_#{reflection.name}?") + return if records.first.send(:association_proxy, reflection.name).loaded? options = reflection.options klasses_and_ids = {} diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 20f22f8d1c..ba5d048f4f 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1495,21 +1495,9 @@ module ActiveRecord association.target.nil? ? nil : association end - redefine_method("loaded_#{reflection.name}?") do - association = association_proxy(reflection.name) - association && association.loaded? - end - redefine_method("#{reflection.name}=") do |record| association_proxy(reflection.name).replace(record) end - - redefine_method("set_#{reflection.name}_target") do |target| - association = association_proxy(reflection.name) - association.target = target - association.loaded - association - end end def collection_reader_method(reflection, association_proxy_class) diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb index 5fbda0bd3d..844d30c3f5 100644 --- a/activerecord/lib/active_record/associations/association_proxy.rb +++ b/activerecord/lib/active_record/associations/association_proxy.rb @@ -213,7 +213,8 @@ module ActiveRecord # Set the inverse association, if possible def set_inverse_instance(record) if record && invertible_for?(record) - record.send("set_#{inverse_reflection_for(record).name}_target", @owner) + inverse = record.send(:association_proxy, inverse_reflection_for(record).name) + inverse.target = @owner end end diff --git a/activerecord/lib/active_record/associations/class_methods/join_dependency.rb b/activerecord/lib/active_record/associations/class_methods/join_dependency.rb index 6263c4e3b0..cb3edafab1 100644 --- a/activerecord/lib/active_record/associations/class_methods/join_dependency.rb +++ b/activerecord/lib/active_record/associations/class_methods/join_dependency.rb @@ -223,7 +223,8 @@ module ActiveRecord end def set_target_and_inverse(join_part, association, record) - association_proxy = record.send("set_#{join_part.reflection.name}_target", association) + association_proxy = record.send(:association_proxy, join_part.reflection.name) + association_proxy.target = association association_proxy.send(:set_inverse_instance, association) end end diff --git a/activerecord/test/cases/associations/has_one_through_associations_test.rb b/activerecord/test/cases/associations/has_one_through_associations_test.rb index 0afbef5c87..91d3025468 100644 --- a/activerecord/test/cases/associations/has_one_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_through_associations_test.rb @@ -197,7 +197,7 @@ class HasOneThroughAssociationsTest < ActiveRecord::TestCase MemberDetail.find(:all, :include => :member_type) end @new_detail = @member_details[0] - assert @new_detail.loaded_member_type? + assert @new_detail.send(:association_proxy, :member_type).loaded? assert_not_nil assert_no_queries { @new_detail.member_type } end From f4a88e810f66bfde5e6148cf7c276c2d4087f7ca Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 18:41:59 +0000 Subject: [PATCH 26/42] It's not necessary to pass the association proxy class around now --- .../lib/active_record/associations.rb | 37 ++++++++----------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index ba5d048f4f..45b68d5956 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1019,12 +1019,7 @@ module ActiveRecord reflection = create_has_many_reflection(association_id, options, &extension) configure_dependency_for_has_many(reflection) add_association_callbacks(reflection.name, reflection.options) - - if options[:through] - collection_accessor_methods(reflection, HasManyThroughAssociation) - else - collection_accessor_methods(reflection, HasManyAssociation) - end + collection_accessor_methods(reflection) end # Specifies a one-to-one association with another class. This method should only be used @@ -1135,14 +1130,13 @@ module ActiveRecord def has_one(association_id, options = {}) if options[:through] reflection = create_has_one_through_reflection(association_id, options) - association_accessor_methods(reflection, ActiveRecord::Associations::HasOneThroughAssociation) else reflection = create_has_one_reflection(association_id, options) - association_accessor_methods(reflection, HasOneAssociation) - association_constructor_method(:build, reflection, HasOneAssociation) - association_constructor_method(:create, reflection, HasOneAssociation) + association_constructor_method(:build, reflection) + association_constructor_method(:create, reflection) configure_dependency_for_has_one(reflection) end + association_accessor_methods(reflection) end # Specifies a one-to-one association with another class. This method should only be used @@ -1259,12 +1253,11 @@ module ActiveRecord def belongs_to(association_id, options = {}) reflection = create_belongs_to_reflection(association_id, options) - if reflection.options[:polymorphic] - association_accessor_methods(reflection, BelongsToPolymorphicAssociation) - else - association_accessor_methods(reflection, BelongsToAssociation) - association_constructor_method(:build, reflection, BelongsToAssociation) - association_constructor_method(:create, reflection, BelongsToAssociation) + association_accessor_methods(reflection) + + unless reflection.options[:polymorphic] + association_constructor_method(:build, reflection) + association_constructor_method(:create, reflection) end add_counter_cache_callbacks(reflection) if options[:counter_cache] @@ -1449,7 +1442,7 @@ module ActiveRecord # 'DELETE FROM developers_projects WHERE active=1 AND developer_id = #{id} AND project_id = #{record.id}' def has_and_belongs_to_many(association_id, options = {}, &extension) reflection = create_has_and_belongs_to_many_reflection(association_id, options, &extension) - collection_accessor_methods(reflection, HasAndBelongsToManyAssociation) + collection_accessor_methods(reflection) # Don't use a before_destroy callback since users' before_destroy # callbacks will be executed after the association is wiped out. @@ -1481,7 +1474,7 @@ module ActiveRecord table_name_prefix + join_table + table_name_suffix end - def association_accessor_methods(reflection, association_proxy_class) + def association_accessor_methods(reflection) redefine_method(reflection.name) do |*params| force_reload = params.first unless params.empty? association = association_proxy(reflection.name) @@ -1500,7 +1493,7 @@ module ActiveRecord end end - def collection_reader_method(reflection, association_proxy_class) + def collection_reader_method(reflection) redefine_method(reflection.name) do |*params| force_reload = params.first unless params.empty? association = association_proxy(reflection.name) @@ -1523,8 +1516,8 @@ module ActiveRecord end end - def collection_accessor_methods(reflection, association_proxy_class, writer = true) - collection_reader_method(reflection, association_proxy_class) + def collection_accessor_methods(reflection, writer = true) + collection_reader_method(reflection) if writer redefine_method("#{reflection.name}=") do |new_value| @@ -1540,7 +1533,7 @@ module ActiveRecord end end - def association_constructor_method(constructor, reflection, association_proxy_class) + def association_constructor_method(constructor, reflection) redefine_method("#{constructor}_#{reflection.name}") do |*params| attributes = params.first unless params.empty? association_proxy(reflection.name).send(constructor, attributes) From d88caa6e4a8f0f64601fc8bf07b61b682263d712 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 18:49:32 +0000 Subject: [PATCH 27/42] Refactor the code for singular association constructors. This will allow me to define a create_association! method in a minute. --- .../lib/active_record/associations.rb | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 45b68d5956..2e0f54e505 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1132,8 +1132,7 @@ module ActiveRecord reflection = create_has_one_through_reflection(association_id, options) else reflection = create_has_one_reflection(association_id, options) - association_constructor_method(:build, reflection) - association_constructor_method(:create, reflection) + association_constructor_methods(reflection) configure_dependency_for_has_one(reflection) end association_accessor_methods(reflection) @@ -1256,8 +1255,7 @@ module ActiveRecord association_accessor_methods(reflection) unless reflection.options[:polymorphic] - association_constructor_method(:build, reflection) - association_constructor_method(:create, reflection) + association_constructor_methods(reflection) end add_counter_cache_callbacks(reflection) if options[:counter_cache] @@ -1533,10 +1531,17 @@ module ActiveRecord end end - def association_constructor_method(constructor, reflection) - redefine_method("#{constructor}_#{reflection.name}") do |*params| - attributes = params.first unless params.empty? - association_proxy(reflection.name).send(constructor, attributes) + def association_constructor_methods(reflection) + constructors = { + "build_#{reflection.name}" => "build", + "create_#{reflection.name}" => "create" + } + + constructors.each do |name, proxy_name| + redefine_method(name) do |*params| + attributes = params.first unless params.empty? + association_proxy(reflection.name).send(proxy_name, attributes) + end end end From 552df9b933e05a3c1d2508c316f1f2bd240accc5 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 9 Jan 2011 19:09:51 +0000 Subject: [PATCH 28/42] Support for create_association! for has_one associations --- activerecord/lib/active_record/associations.rb | 1 + .../associations/has_one_association.rb | 5 ++++- .../associations/has_one_associations_test.rb | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 2e0f54e505..a03d1bbb06 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1536,6 +1536,7 @@ module ActiveRecord "build_#{reflection.name}" => "build", "create_#{reflection.name}" => "create" } + constructors["create_#{reflection.name}!"] = "create!" if reflection.macro == :has_one constructors.each do |name, proxy_name| redefine_method(name) do |*params| diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index f60547c22c..c29ab8dcec 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -7,7 +7,7 @@ module ActiveRecord end def create!(attributes = {}) - new_record(:create_association!, attributes) + build(attributes).tap { |record| record.save! } end def build(attributes = {}) @@ -51,6 +51,9 @@ module ActiveRecord alias creation_attributes construct_owner_attributes + # The reason that the save param for replace is false, if for create (not just build), + # is because the setting of the foreign keys is actually handled by the scoping, and + # so they are set straight away and do not need to be updated within replace. def new_record(method, attributes) record = scoped.scoping { @reflection.send(method, attributes) } replace(record, false) diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index 925b76b901..d9b6694dd8 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -173,6 +173,24 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_equal account, firm.reload.account end + def test_create_association_with_bang + firm = Firm.create(:name => "GlobalMegaCorp") + account = firm.create_account!(:credit_limit => 1000) + assert_equal account, firm.reload.account + end + + def test_create_association_with_bang_failing + firm = Firm.create(:name => "GlobalMegaCorp") + assert_raise ActiveRecord::RecordInvalid do + firm.create_account! + end + account = firm.account + assert_not_nil account + account.credit_limit = 5 + account.save + assert_equal account, firm.reload.account + end + def test_build firm = Firm.new("name" => "GlobalMegaCorp") firm.save From af96018c9171a9021f915ec63bd0baf4cf5a8e39 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 11 Jan 2011 20:11:28 +0000 Subject: [PATCH 29/42] test_with_polymorphic_and_condition works without the conditions methods in BelongsToPolymorphicAssociation because the conditions are added straight to the association_scope as of a few days ago --- .../associations/belongs_to_polymorphic_association.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb index cae35fe0d0..4f67b02d00 100644 --- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb @@ -4,11 +4,6 @@ module ActiveRecord class BelongsToPolymorphicAssociation < BelongsToAssociation #:nodoc: private - def conditions - @conditions ||= interpolate_sql(target_klass.send(:sanitize_sql, @reflection.options[:conditions])) if @reflection.options[:conditions] - end - alias :sql_conditions :conditions - def replace_keys(record) super @owner[@reflection.foreign_type] = record && record.class.base_class.name From 8c71e8b18f0e589b5413a8e6b6d7336a5506c977 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 11 Jan 2011 15:16:09 -0800 Subject: [PATCH 30/42] lazily instantiate AR objects in order to avoid NoMethodErrors --- .../lib/active_record/association_preload.rb | 22 ++++++++++++------- activerecord/lib/active_record/base.rb | 19 ++++++++-------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index 4c517e3339..68aaff175a 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -137,9 +137,9 @@ module ActiveRecord end end - def set_association_collection_records(id_to_record_map, reflection_name, associated_records, key) + def set_association_collection_records(id_to_parent_map, reflection_name, associated_records, key) associated_records.each do |associated_record| - parent_records = id_to_record_map[associated_record[key].to_s] + parent_records = id_to_parent_map[associated_record[key].to_s] add_preloaded_records_to_collection(parent_records, reflection_name, associated_record) end end @@ -199,7 +199,6 @@ module ActiveRecord right = Arel::Table.new(options[:join_table]).alias('t0') - join_condition = left[reflection.klass.primary_key].eq( right[reflection.association_foreign_key]) @@ -221,17 +220,24 @@ module ActiveRecord custom_conditions = append_conditions(reflection, preload_options) - all_associated_records = associated_records(ids) do |some_ids| + klass = associated_records_proxy.klass + + associated_records(ids) { |some_ids| method = in_or_equal(some_ids) conditions = right[reflection.foreign_key].send(*method) conditions = custom_conditions.inject(conditions) do |ast, cond| ast.and cond end - associated_records_proxy.where(conditions).to_a - end - - set_association_collection_records(id_to_record_map, reflection.name, all_associated_records, 'the_parent_record_id') + relation = associated_records_proxy.where(conditions) + klass.connection.select_all(relation.arel.to_sql, 'SQL', relation.bind_values) + }.map! { |row| + parent_records = id_to_record_map[row['the_parent_record_id'].to_s] + associated_record = klass.instantiate row + add_preloaded_records_to_collection( + parent_records, reflection.name, associated_record) + associated_record + } end def preload_has_one_association(records, reflection, preload_options={}) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 1079094bbf..a5b71ba280 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -880,6 +880,16 @@ module ActiveRecord #:nodoc: record end + + # Finder methods must instantiate through this method to work with the + # single-table inheritance model that makes it possible to create + # objects of different types from the same table. + def instantiate(record) # :nodoc: + model = find_sti_class(record[inheritance_column]).allocate + model.init_with('attributes' => record) + model + end + private def relation #:nodoc: @@ -892,15 +902,6 @@ module ActiveRecord #:nodoc: end end - # Finder methods must instantiate through this method to work with the - # single-table inheritance model that makes it possible to create - # objects of different types from the same table. - def instantiate(record) - model = find_sti_class(record[inheritance_column]).allocate - model.init_with('attributes' => record) - model - end - def find_sti_class(type_name) if type_name.blank? || !columns_hash.include?(inheritance_column) self From 5696d948ed0eb9cf755e843564f80801ea864ee2 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 11 Jan 2011 15:29:35 -0800 Subject: [PATCH 31/42] kill unused variable warnings --- activerecord/lib/active_record/schema_dumper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/schema_dumper.rb b/activerecord/lib/active_record/schema_dumper.rb index e30b481fe1..a893c0ad85 100644 --- a/activerecord/lib/active_record/schema_dumper.rb +++ b/activerecord/lib/active_record/schema_dumper.rb @@ -83,7 +83,7 @@ HEADER # first dump primary key column if @connection.respond_to?(:pk_and_sequence_for) - pk, pk_seq = @connection.pk_and_sequence_for(table) + pk, _ = @connection.pk_and_sequence_for(table) elsif @connection.respond_to?(:primary_key) pk = @connection.primary_key(table) end From fcd8925f236b391d562dc504bcd901f501140c11 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 11 Jan 2011 15:39:26 -0800 Subject: [PATCH 32/42] use underlying _read_attribute method rather than causing NoMethodErrors --- activerecord/lib/active_record/base.rb | 2 +- activerecord/test/cases/associations/join_model_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index a5b71ba280..dde52269d4 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1578,7 +1578,7 @@ MSG # Returns true if the specified +attribute+ has been set by the user or by a database load and is neither # nil nor empty? (the latter only applies to objects that respond to empty?, most notably Strings). def attribute_present?(attribute) - !read_attribute(attribute).blank? + !_read_attribute(attribute).blank? end # Returns the column object for the named attribute. diff --git a/activerecord/test/cases/associations/join_model_test.rb b/activerecord/test/cases/associations/join_model_test.rb index 512a6d3ef8..c50fcd3f33 100644 --- a/activerecord/test/cases/associations/join_model_test.rb +++ b/activerecord/test/cases/associations/join_model_test.rb @@ -523,7 +523,7 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase def test_has_many_through_collection_size_uses_counter_cache_if_it_exists author = authors(:david) - author.stubs(:read_attribute).with('comments_count').returns(100) + author.stubs(:_read_attribute).with('comments_count').returns(100) assert_equal 100, author.comments.size assert !author.comments.loaded? end From f8700038afdaea80cad34a4fca005e1ef068b53e Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 11 Jan 2011 17:57:02 -0800 Subject: [PATCH 33/42] adding a test for no method error --- .../associations/association_proxy.rb | 16 ++---- .../associations/association_proxy_test.rb | 52 +++++++++++++++++++ 2 files changed, 57 insertions(+), 11 deletions(-) create mode 100644 activerecord/test/cases/associations/association_proxy_test.rb diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb index 844d30c3f5..e4a449d4f4 100644 --- a/activerecord/lib/active_record/associations/association_proxy.rb +++ b/activerecord/lib/active_record/associations/association_proxy.rb @@ -286,19 +286,13 @@ module ActiveRecord private # Forwards any missing method call to the \target. - def method_missing(method, *args) + def method_missing(method, *args, &block) if load_target - unless @target.respond_to?(method) - message = "undefined method `#{method.to_s}' for \"#{@target}\":#{@target.class.to_s}" - raise NoMethodError, message - end - - if block_given? - @target.send(method, *args) { |*block_args| yield(*block_args) } - else - @target.send(method, *args) - end + return super unless @target.respond_to?(method) + @target.send(method, *args, &block) end + rescue NoMethodError => e + raise e, e.message.sub(/ for #<.*$/, " via proxy for #{@target}") end # Should be true if there is a foreign key present on the @owner which diff --git a/activerecord/test/cases/associations/association_proxy_test.rb b/activerecord/test/cases/associations/association_proxy_test.rb new file mode 100644 index 0000000000..55d8da4c4e --- /dev/null +++ b/activerecord/test/cases/associations/association_proxy_test.rb @@ -0,0 +1,52 @@ +require "cases/helper" + +module ActiveRecord + module Associations + class AsssociationProxyTest < ActiveRecord::TestCase + class FakeOwner + attr_accessor :new_record + alias :new_record? :new_record + + def initialize + @new_record = false + end + end + + class FakeReflection < Struct.new(:options, :klass) + def initialize options = {}, klass = nil + super + end + + def check_validity! + true + end + end + + class FakeTarget + end + + class FakeTargetProxy < AssociationProxy + def association_scope + true + end + + def find_target + FakeTarget.new + end + end + + def test_method_missing_error + reflection = FakeReflection.new({}, Object.new) + owner = FakeOwner.new + proxy = FakeTargetProxy.new(owner, reflection) + + exception = assert_raises(NoMethodError) do + proxy.omg + end + + assert_match('omg', exception.message) + assert_match(FakeTarget.name, exception.message) + end + end + end +end From 6062d434f174c84a18a121b025dcf4f11d6689f1 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Wed, 12 Jan 2011 12:14:00 -0200 Subject: [PATCH 34/42] Allow view in AV::TestCase to access it's controller helpers methods --- actionpack/lib/action_view/test_case.rb | 1 + actionpack/test/template/test_case_test.rb | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/actionpack/lib/action_view/test_case.rb b/actionpack/lib/action_view/test_case.rb index 4026f7a40e..72d17143f5 100644 --- a/actionpack/lib/action_view/test_case.rb +++ b/actionpack/lib/action_view/test_case.rb @@ -157,6 +157,7 @@ module ActionView def view @view ||= begin view = ActionView::Base.new(ActionController::Base.view_paths, {}, @controller) + view.singleton_class.send :include, @controller._helpers view.singleton_class.send :include, _helpers view.singleton_class.send :include, @controller._routes.url_helpers view.singleton_class.send :delegate, :alert, :notice, :to => "request.flash" diff --git a/actionpack/test/template/test_case_test.rb b/actionpack/test/template/test_case_test.rb index a745999622..203515d508 100644 --- a/actionpack/test/template/test_case_test.rb +++ b/actionpack/test/template/test_case_test.rb @@ -116,6 +116,27 @@ module ActionView end end + class ControllerHelperMethod < ActionView::TestCase + module SomeHelper + def some_method + render :partial => 'test/from_helper' + end + end + + helper SomeHelper + + test "can call a helper method defined on the current controller from a helper" do + @controller.singleton_class.class_eval <<-EOF, __FILE__, __LINE__ + 1 + def render_from_helper + 'controller_helper_method' + end + EOF + @controller.class.helper_method :render_from_helper + + assert_equal 'controller_helper_method', some_method + end + end + class AssignsTest < ActionView::TestCase setup do ActiveSupport::Deprecation.stubs(:warn) From de5852f1a694f912544a40c98812c5d2bcfbc5a6 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Wed, 12 Jan 2011 12:36:14 -0200 Subject: [PATCH 35/42] CI should run isolated tests --- ci/ci_build.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/ci/ci_build.rb b/ci/ci_build.rb index fd884b3b2e..964e2d4eb8 100755 --- a/ci/ci_build.rb +++ b/ci/ci_build.rb @@ -35,7 +35,7 @@ cd "#{root_dir}/activesupport" do puts "[CruiseControl] Building Active Support" puts build_results[:activesupport] = rake 'test' - # build_results[:activesupport_isolated] = rake 'test:isolated' + build_results[:activesupport_isolated] = rake 'test:isolated' end system "sudo rm -R #{root_dir}/railties/tmp" @@ -51,7 +51,7 @@ cd "#{root_dir}/actionpack" do puts "[CruiseControl] Building Action Pack" puts build_results[:actionpack] = rake 'test' - # build_results[:actionpack_isolated] = rake 'test:isolated' + build_results[:actionpack_isolated] = rake 'test:isolated' end cd "#{root_dir}/actionmailer" do @@ -59,7 +59,7 @@ cd "#{root_dir}/actionmailer" do puts "[CruiseControl] Building Action Mailer" puts build_results[:actionmailer] = rake 'test' - # build_results[:actionmailer_isolated] = rake 'test:isolated' + build_results[:actionmailer_isolated] = rake 'test:isolated' end cd "#{root_dir}/activemodel" do @@ -67,7 +67,7 @@ cd "#{root_dir}/activemodel" do puts "[CruiseControl] Building Active Model" puts build_results[:activemodel] = rake 'test' - # build_results[:activemodel_isolated] = rake 'test:isolated' + build_results[:activemodel_isolated] = rake 'test:isolated' end rm_f "#{root_dir}/activeresource/debug.log" @@ -76,7 +76,7 @@ cd "#{root_dir}/activeresource" do puts "[CruiseControl] Building Active Resource" puts build_results[:activeresource] = rake 'test' - # build_results[:activeresource_isolated] = rake 'test:isolated' + build_results[:activeresource_isolated] = rake 'test:isolated' end rm_f "#{root_dir}/activerecord/debug.log" @@ -85,7 +85,7 @@ cd "#{root_dir}/activerecord" do puts "[CruiseControl] Building Active Record with MySQL" puts build_results[:activerecord_mysql] = rake 'mysql:rebuild_databases', 'mysql:test' - # build_results[:activerecord_mysql_isolated] = rake 'mysql:rebuild_databases', 'mysql:isolated_test' + build_results[:activerecord_mysql_isolated] = rake 'mysql:rebuild_databases', 'mysql:isolated_test' end cd "#{root_dir}/activerecord" do @@ -93,7 +93,7 @@ cd "#{root_dir}/activerecord" do puts "[CruiseControl] Building Active Record with MySQL2" puts build_results[:activerecord_mysql2] = rake 'mysql:rebuild_databases', 'mysql2:test' - # build_results[:activerecord_mysql2_isolated] = rake 'mysql:rebuild_databases', 'mysql2:isolated_test' + build_results[:activerecord_mysql2_isolated] = rake 'mysql:rebuild_databases', 'mysql2:isolated_test' end cd "#{root_dir}/activerecord" do @@ -101,7 +101,7 @@ cd "#{root_dir}/activerecord" do puts "[CruiseControl] Building Active Record with PostgreSQL" puts build_results[:activerecord_postgresql8] = rake 'postgresql:rebuild_databases', 'postgresql:test' - # build_results[:activerecord_postgresql8_isolated] = rake 'postgresql:rebuild_databases', 'postgresql:isolated_test' + build_results[:activerecord_postgresql8_isolated] = rake 'postgresql:rebuild_databases', 'postgresql:isolated_test' end cd "#{root_dir}/activerecord" do @@ -109,7 +109,7 @@ cd "#{root_dir}/activerecord" do puts "[CruiseControl] Building Active Record with SQLite 3" puts build_results[:activerecord_sqlite3] = rake 'sqlite3:test' - # build_results[:activerecord_sqlite3_isolated] = rake 'sqlite3:isolated_test' + build_results[:activerecord_sqlite3_isolated] = rake 'sqlite3:isolated_test' end From de18b85969b7a5457536df905c76078f032a9cda Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Wed, 12 Jan 2011 15:16:55 +0100 Subject: [PATCH 36/42] In AS, only inflector/methods is need in proxy_wrappers.rb, as well as date, date_time, and time conversions.rb. This fixes an issue when requiring json and AS saying that i18n is also required. Signed-off-by: Santiago Pastorino --- activesupport/lib/active_support/core_ext/date/conversions.rb | 2 +- .../lib/active_support/core_ext/date_time/conversions.rb | 2 +- activesupport/lib/active_support/core_ext/time/conversions.rb | 2 +- activesupport/lib/active_support/deprecation/proxy_wrappers.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/activesupport/lib/active_support/core_ext/date/conversions.rb b/activesupport/lib/active_support/core_ext/date/conversions.rb index 092f936961..06d868a3b0 100644 --- a/activesupport/lib/active_support/core_ext/date/conversions.rb +++ b/activesupport/lib/active_support/core_ext/date/conversions.rb @@ -1,5 +1,5 @@ require 'date' -require 'active_support/inflector' +require 'active_support/inflector/methods' require 'active_support/core_ext/date/zones' class Date diff --git a/activesupport/lib/active_support/core_ext/date_time/conversions.rb b/activesupport/lib/active_support/core_ext/date_time/conversions.rb index 8e267c76c4..029b8c41b4 100644 --- a/activesupport/lib/active_support/core_ext/date_time/conversions.rb +++ b/activesupport/lib/active_support/core_ext/date_time/conversions.rb @@ -1,4 +1,4 @@ -require 'active_support/inflector' +require 'active_support/inflector/methods' require 'active_support/core_ext/time/conversions' require 'active_support/core_ext/date_time/calculations' require 'active_support/values/time_zone' diff --git a/activesupport/lib/active_support/core_ext/time/conversions.rb b/activesupport/lib/active_support/core_ext/time/conversions.rb index d4ae3131ec..49ac18d245 100644 --- a/activesupport/lib/active_support/core_ext/time/conversions.rb +++ b/activesupport/lib/active_support/core_ext/time/conversions.rb @@ -1,4 +1,4 @@ -require 'active_support/inflector' +require 'active_support/inflector/methods' require 'active_support/core_ext/time/publicize_conversion_methods' require 'active_support/values/time_zone' diff --git a/activesupport/lib/active_support/deprecation/proxy_wrappers.rb b/activesupport/lib/active_support/deprecation/proxy_wrappers.rb index 970536a594..a65fcafb44 100644 --- a/activesupport/lib/active_support/deprecation/proxy_wrappers.rb +++ b/activesupport/lib/active_support/deprecation/proxy_wrappers.rb @@ -1,4 +1,4 @@ -require 'active_support/inflector' +require 'active_support/inflector/methods' module ActiveSupport module Deprecation From 199e220e885277dad5e222bf98f00a67b2e6674e Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Wed, 12 Jan 2011 15:18:21 +0100 Subject: [PATCH 37/42] Fixed various isolated test missing requires within AS. Signed-off-by: Santiago Pastorino --- activesupport/test/core_ext/bigdecimal_test.rb | 6 ++++-- activesupport/test/core_ext/hash_ext_test.rb | 1 + activesupport/test/json/encoding_test.rb | 1 + 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/activesupport/test/core_ext/bigdecimal_test.rb b/activesupport/test/core_ext/bigdecimal_test.rb index 0272f564de..d592973d7a 100644 --- a/activesupport/test/core_ext/bigdecimal_test.rb +++ b/activesupport/test/core_ext/bigdecimal_test.rb @@ -1,10 +1,12 @@ require 'abstract_unit' +require 'bigdecimal' +require 'active_support/core_ext/big_decimal' class BigDecimalTest < Test::Unit::TestCase def test_to_yaml assert_equal("--- 100000.30020320320000000000000000000000000000001\n", BigDecimal.new('100000.30020320320000000000000000000000000000001').to_yaml) - assert_equal("--- .Inf\n", BigDecimal.new('Infinity').to_yaml) - assert_equal("--- .NaN\n", BigDecimal.new('NaN').to_yaml) + assert_equal("--- .Inf\n", BigDecimal.new('Infinity').to_yaml) + assert_equal("--- .NaN\n", BigDecimal.new('NaN').to_yaml) assert_equal("--- -.Inf\n", BigDecimal.new('-Infinity').to_yaml) end diff --git a/activesupport/test/core_ext/hash_ext_test.rb b/activesupport/test/core_ext/hash_ext_test.rb index 74223dd7f2..a0479d45ac 100644 --- a/activesupport/test/core_ext/hash_ext_test.rb +++ b/activesupport/test/core_ext/hash_ext_test.rb @@ -4,6 +4,7 @@ require 'bigdecimal' require 'active_support/core_ext/string/access' require 'active_support/ordered_hash' require 'active_support/core_ext/object/conversions' +require 'active_support/inflections' class HashExtTest < Test::Unit::TestCase class IndifferentHash < HashWithIndifferentAccess diff --git a/activesupport/test/json/encoding_test.rb b/activesupport/test/json/encoding_test.rb index e0494de6e4..7469ae70fd 100644 --- a/activesupport/test/json/encoding_test.rb +++ b/activesupport/test/json/encoding_test.rb @@ -1,5 +1,6 @@ # encoding: utf-8 require 'abstract_unit' +require 'active_support/core_ext/string/inflections' require 'active_support/json' class TestJSONEncoding < Test::Unit::TestCase From 6ddabaa90c512abf4d319056ce296fa75216623e Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 12 Jan 2011 09:46:56 -0800 Subject: [PATCH 38/42] sorry, the CI cannot lie to us anymore --- railties/Rakefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/railties/Rakefile b/railties/Rakefile index 1b469543cc..26fa0bf6a5 100755 --- a/railties/Rakefile +++ b/railties/Rakefile @@ -18,7 +18,7 @@ namespace :test do Dir["test/#{dir}/*_test.rb"].each do |file| next true if file.include?("fixtures") ruby = File.join(*RbConfig::CONFIG.values_at('bindir', 'RUBY_INSTALL_NAME')) - system(ruby, '-Itest', "-I#{File.dirname(__FILE__)}/../activesupport/lib", file) + sh(ruby, '-Itest', "-I#{File.dirname(__FILE__)}/../activesupport/lib", file) end end end From daada51d1051948c3e6de963a90c4b5bb5bf142b Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Wed, 12 Jan 2011 16:44:56 -0200 Subject: [PATCH 39/42] Reuse the view_context from the controller, this make the test environment more similar to the code applications uses --- actionpack/lib/action_view/test_case.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/actionpack/lib/action_view/test_case.rb b/actionpack/lib/action_view/test_case.rb index 72d17143f5..d4f16d4b36 100644 --- a/actionpack/lib/action_view/test_case.rb +++ b/actionpack/lib/action_view/test_case.rb @@ -156,11 +156,8 @@ module ActionView # The instance of ActionView::Base that is used by +render+. def view @view ||= begin - view = ActionView::Base.new(ActionController::Base.view_paths, {}, @controller) - view.singleton_class.send :include, @controller._helpers + view = @controller.view_context view.singleton_class.send :include, _helpers - view.singleton_class.send :include, @controller._routes.url_helpers - view.singleton_class.send :delegate, :alert, :notice, :to => "request.flash" view.extend(Locals) view.locals = self.locals view.output_buffer = self.output_buffer From 1d9c555297c15b6d5212e65c1afec718e043ce45 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 12 Jan 2011 10:01:31 -0800 Subject: [PATCH 40/42] reraising should be in the rescue block --- actionpack/lib/action_dispatch/middleware/show_exceptions.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb index 936fd548b9..11739e08d9 100644 --- a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb @@ -53,8 +53,8 @@ module ActionDispatch exception = ActionController::RoutingError.new("No route matches #{env['PATH_INFO'].inspect}") end rescue Exception => exception + raise exception if env['action_dispatch.show_exceptions'] == false end - raise exception if env['action_dispatch.show_exceptions'] == false exception ? render_exception(env, exception) : [status, headers, body] end From 16ae08fff0f16b3f0eb7757e01aabb02b257bd35 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 12 Jan 2011 10:28:21 -0800 Subject: [PATCH 41/42] use raise to create exceptions and to set the backtrace --- actionpack/lib/action_dispatch/middleware/show_exceptions.rb | 2 +- actionpack/lib/action_dispatch/routing/route_set.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb index 11739e08d9..dbe3206808 100644 --- a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb @@ -50,7 +50,7 @@ module ActionDispatch # Only this middleware cares about RoutingError. So, let's just raise # it here. if headers['X-Cascade'] == 'pass' - exception = ActionController::RoutingError.new("No route matches #{env['PATH_INFO'].inspect}") + raise ActionController::RoutingError, "No route matches #{env['PATH_INFO'].inspect}" end rescue Exception => exception raise exception if env['action_dispatch.show_exceptions'] == false diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 03bfe178e5..683fa19380 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -450,7 +450,7 @@ module ActionDispatch end def raise_routing_error - raise ActionController::RoutingError.new("No route matches #{options.inspect}") + raise ActionController::RoutingError, "No route matches #{options.inspect}" end def different_controller? From 31293ba9d33f5b5f62ee760a548b4f5438183f22 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 12 Jan 2011 10:44:35 -0800 Subject: [PATCH 42/42] remove locales external to the system before assertion --- railties/test/railties/shared_tests.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/railties/test/railties/shared_tests.rb b/railties/test/railties/shared_tests.rb index a162bdb66f..3eb79d57c8 100644 --- a/railties/test/railties/shared_tests.rb +++ b/railties/test/railties/shared_tests.rb @@ -294,7 +294,7 @@ YAML boot_rails - expected = %W( + expected_locales = %W( #{RAILS_FRAMEWORK_ROOT}/activesupport/lib/active_support/locale/en.yml #{RAILS_FRAMEWORK_ROOT}/activemodel/lib/active_model/locale/en.yml #{RAILS_FRAMEWORK_ROOT}/activerecord/lib/active_record/locale/en.yml @@ -304,11 +304,11 @@ YAML #{app_path}/app/locales/en.yml ).map { |path| File.expand_path(path) } - actual = I18n.load_path.map { |path| File.expand_path(path) }.find_all do |p| - p =~ /^#{RAILS_FRAMEWORK_ROOT}/ || p =~ /^#{@plugin.path}/ || p =~ /^#{app_path}/ - end + actual_locales = I18n.load_path.map { |path| + File.expand_path(path) + } & expected_locales # remove locales external to Rails - assert_equal expected, actual + assert_equal expected_locales, actual_locales assert_equal "2", I18n.t(:foo) assert_equal "1", I18n.t(:bar)