diff options
Diffstat (limited to 'activerecord')
-rw-r--r-- | activerecord/CHANGELOG | 4 | ||||
-rw-r--r-- | activerecord/MIT-LICENSE | 2 | ||||
-rw-r--r-- | activerecord/lib/active_record.rb | 2 | ||||
-rwxr-xr-x | activerecord/lib/active_record/associations.rb | 4 | ||||
-rw-r--r-- | activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb | 12 | ||||
-rw-r--r-- | activerecord/lib/active_record/callbacks.rb | 5 | ||||
-rw-r--r-- | activerecord/lib/active_record/dirty.rb | 4 | ||||
-rw-r--r-- | activerecord/lib/active_record/reflection.rb | 8 | ||||
-rw-r--r-- | activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb | 11 | ||||
-rw-r--r-- | activerecord/test/cases/associations/has_many_associations_test.rb | 3 | ||||
-rw-r--r-- | activerecord/test/cases/callbacks_test.rb | 44 | ||||
-rw-r--r-- | activerecord/test/cases/dirty_test.rb | 26 | ||||
-rw-r--r-- | activerecord/test/cases/helper.rb | 2 | ||||
-rw-r--r-- | activerecord/test/cases/json_serialization_test.rb | 2 | ||||
-rw-r--r-- | activerecord/test/schema/schema.rb | 1 |
15 files changed, 112 insertions, 18 deletions
diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index fbc71cddf1..507e37ac3b 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,9 @@ *2.3.0/3.0* +* Make after_save callbacks fire only if the record was successfully saved. #1735 [Michael Lovitt] + + Previously the callbacks would fire if a before_save cancelled saving. + * Support nested transactions using database savepoints. #383 [Jonathan Viney, Hongli Lai] * Added dynamic scopes ala dynamic finders #1648 [Yaroslav Markin] diff --git a/activerecord/MIT-LICENSE b/activerecord/MIT-LICENSE index 93be57f683..e6df48772f 100644 --- a/activerecord/MIT-LICENSE +++ b/activerecord/MIT-LICENSE @@ -1,4 +1,4 @@ -Copyright (c) 2004-2008 David Heinemeier Hansson +Copyright (c) 2004-2009 David Heinemeier Hansson Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index 390c005785..e1265b7e1e 100644 --- a/activerecord/lib/active_record.rb +++ b/activerecord/lib/active_record.rb @@ -1,5 +1,5 @@ #-- -# Copyright (c) 2004-2008 David Heinemeier Hansson +# Copyright (c) 2004-2009 David Heinemeier Hansson # # Permission is hereby granted, free of charge, to any person obtaining # a copy of this software and associated documentation files (the diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 86616abf52..8b51a38f48 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1531,14 +1531,14 @@ module ActiveRecord association = send(reflection.name) association.destroy unless association.nil? end - before_destroy method_name + after_destroy method_name when :delete method_name = "belongs_to_dependent_delete_for_#{reflection.name}".to_sym define_method(method_name) do association = send(reflection.name) association.delete unless association.nil? end - before_destroy method_name + after_destroy method_name else raise ArgumentError, "The :dependent option expects either :destroy or :delete (#{reflection.options[:dependent].inspect})" end diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb index 3d689098b5..a5cc3bf091 100644 --- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb +++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb @@ -9,6 +9,14 @@ module ActiveRecord create_record(attributes) { |record| insert_record(record, true) } end + def columns + @reflection.columns(@reflection.options[:join_table], "#{@reflection.options[:join_table]} Columns") + end + + def reset_column_information + @reflection.reset_column_information + end + protected def construct_find_options!(options) options[:joins] = @join_sql @@ -32,8 +40,6 @@ module ActiveRecord if @reflection.options[:insert_sql] @owner.connection.insert(interpolate_sql(@reflection.options[:insert_sql], record)) else - columns = @owner.connection.columns(@reflection.options[:join_table], "#{@reflection.options[:join_table]} Columns") - attributes = columns.inject({}) do |attrs, column| case column.name.to_s when @reflection.primary_key_name.to_s @@ -103,7 +109,7 @@ module ActiveRecord # clause has been explicitly defined. Otherwise you can get broken records back, if, for example, the join column also has # an id column. This will then overwrite the id column of the records coming back. def finding_with_ambiguous_select?(select_clause) - !select_clause && @owner.connection.columns(@reflection.options[:join_table], "Join Table Columns").size != 2 + !select_clause && columns.size != 2 end private diff --git a/activerecord/lib/active_record/callbacks.rb b/activerecord/lib/active_record/callbacks.rb index dc33f8c2c7..88958f4583 100644 --- a/activerecord/lib/active_record/callbacks.rb +++ b/activerecord/lib/active_record/callbacks.rb @@ -219,8 +219,9 @@ module ActiveRecord def after_save() end def create_or_update_with_callbacks #:nodoc: return false if callback(:before_save) == false - result = create_or_update_without_callbacks - callback(:after_save) + if result = create_or_update_without_callbacks + callback(:after_save) + end result end private :create_or_update_with_callbacks diff --git a/activerecord/lib/active_record/dirty.rb b/activerecord/lib/active_record/dirty.rb index 4c899f58e5..4a2510aa63 100644 --- a/activerecord/lib/active_record/dirty.rb +++ b/activerecord/lib/active_record/dirty.rb @@ -151,8 +151,8 @@ module ActiveRecord def field_changed?(attr, old, value) if column = column_for_attribute(attr) - if column.type == :integer && column.null && (old.nil? || old == 0) && value.blank? - # For nullable integer columns, NULL gets stored in database for blank (i.e. '') values. + if column.number? && column.null && (old.nil? || old == 0) && value.blank? + # For nullable numeric columns, NULL gets stored in database for blank (i.e. '') values. # Hence we don't record it as a change if the value changes from nil to ''. # If an old value of 0 is set to '' we want this to get changed to nil as otherwise it'll # be typecast back to 0 (''.to_i => 0) diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index dbff4f24d6..1937abdc83 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -198,6 +198,14 @@ module ActiveRecord end end + def columns(tbl_name, log_msg) + @columns ||= klass.connection.columns(tbl_name, log_msg) + end + + def reset_column_information + @columns = nil + end + def check_validity! end diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb index 2f08e09d43..1e3b423471 100644 --- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb @@ -775,4 +775,15 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase end end end + + def test_caching_of_columns + david = Developer.find(1) + # clear cache possibly created by other tests + david.projects.reset_column_information + assert_queries(0) { david.projects.columns; david.projects.columns } + # and again to verify that reset_column_information clears the cache correctly + david.projects.reset_column_information + assert_queries(0) { david.projects.columns; david.projects.columns } + 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 a5ae5cd8ec..428fb50013 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -698,7 +698,8 @@ class HasManyAssociationsTest < ActiveRecord::TestCase authors(:david).destroy end - assert_equal [author_address.id], AuthorAddress.destroyed_author_address_ids[authors(:david).id] + assert_equal nil, AuthorAddress.find_by_id(authors(:david).author_address_id) + assert_equal nil, AuthorAddress.find_by_id(authors(:david).author_address_extra_id) end def test_invalid_belongs_to_dependent_option_raises_exception diff --git a/activerecord/test/cases/callbacks_test.rb b/activerecord/test/cases/callbacks_test.rb index 11830a2ef6..33b1ea034d 100644 --- a/activerecord/test/cases/callbacks_test.rb +++ b/activerecord/test/cases/callbacks_test.rb @@ -121,9 +121,19 @@ end class CallbackCancellationDeveloper < ActiveRecord::Base set_table_name 'developers' - def before_create - false - end + + attr_reader :after_save_called, :after_create_called, :after_update_called, :after_destroy_called + attr_accessor :cancel_before_save, :cancel_before_create, :cancel_before_update, :cancel_before_destroy + + def before_save; !@cancel_before_save; end + def before_create; !@cancel_before_create; end + def before_update; !@cancel_before_update; end + def before_destroy; !@cancel_before_destroy; end + + def after_save; @after_save_called = true; end + def after_update; @after_update_called = true; end + def after_create; @after_create_called = true; end + def after_destroy; @after_destroy_called = true; end end class CallbacksTest < ActiveRecord::TestCase @@ -349,19 +359,47 @@ class CallbacksTest < ActiveRecord::TestCase assert !david.valid? assert !david.save assert_raises(ActiveRecord::RecordInvalid) { david.save! } + + someone = CallbackCancellationDeveloper.find(1) + someone.cancel_before_save = true + assert someone.valid? + assert !someone.save + assert_save_callbacks_not_called(someone) end def test_before_create_returning_false someone = CallbackCancellationDeveloper.new + someone.cancel_before_create = true assert someone.valid? assert !someone.save + assert_save_callbacks_not_called(someone) + end + + def test_before_update_returning_false + someone = CallbackCancellationDeveloper.find(1) + someone.cancel_before_update = true + assert someone.valid? + assert !someone.save + assert_save_callbacks_not_called(someone) end def test_before_destroy_returning_false david = ImmutableDeveloper.find(1) assert !david.destroy assert_not_nil ImmutableDeveloper.find_by_id(1) + + someone = CallbackCancellationDeveloper.find(1) + someone.cancel_before_destroy = true + assert !someone.destroy + assert !someone.after_destroy_called + end + + def assert_save_callbacks_not_called(someone) + assert !someone.after_save_called + assert !someone.after_create_called + assert !someone.after_update_called end + private :assert_save_callbacks_not_called def test_zzz_callback_returning_false # must be run last since we modify CallbackDeveloper david = CallbackDeveloper.find(1) diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb index 10cdbdc622..1c9e281cc0 100644 --- a/activerecord/test/cases/dirty_test.rb +++ b/activerecord/test/cases/dirty_test.rb @@ -21,6 +21,10 @@ private end end +class NumericData < ActiveRecord::Base + self.table_name = 'numeric_data' +end + class DirtyTest < ActiveRecord::TestCase def test_attribute_changes # New record - no changes. @@ -58,7 +62,7 @@ class DirtyTest < ActiveRecord::TestCase assert_equal parrot.name_change, parrot.title_change end - def test_nullable_integer_not_marked_as_changed_if_new_value_is_blank + def test_nullable_number_not_marked_as_changed_if_new_value_is_blank pirate = Pirate.new ["", nil].each do |value| @@ -68,6 +72,26 @@ class DirtyTest < ActiveRecord::TestCase end end + def test_nullable_decimal_not_marked_as_changed_if_new_value_is_blank + numeric_data = NumericData.new + + ["", nil].each do |value| + numeric_data.bank_balance = value + assert !numeric_data.bank_balance_changed? + assert_nil numeric_data.bank_balance_change + end + end + + def test_nullable_float_not_marked_as_changed_if_new_value_is_blank + numeric_data = NumericData.new + + ["", nil].each do |value| + numeric_data.temperature = value + assert !numeric_data.temperature_changed? + assert_nil numeric_data.temperature_change + end + end + def test_nullable_integer_zero_to_string_zero_not_marked_as_changed pirate = Pirate.new pirate.parrot_id = 0 diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index ccd04b67f6..24ce35e2e2 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -34,7 +34,7 @@ rescue LoadError end ActiveRecord::Base.connection.class.class_eval do - IGNORED_SQL = [/^PRAGMA/, /^SELECT currval/, /^SELECT CAST/, /^SELECT @@IDENTITY/, /^SELECT @@ROWCOUNT/, /^SAVEPOINT/, /^ROLLBACK TO SAVEPOINT/, /^RELEASE SAVEPOINT/] + IGNORED_SQL = [/^PRAGMA/, /^SELECT currval/, /^SELECT CAST/, /^SELECT @@IDENTITY/, /^SELECT @@ROWCOUNT/, /^SAVEPOINT/, /^ROLLBACK TO SAVEPOINT/, /^RELEASE SAVEPOINT/, /SHOW FIELDS/] def execute_with_query_record(sql, name = nil, &block) $queries_executed ||= [] diff --git a/activerecord/test/cases/json_serialization_test.rb b/activerecord/test/cases/json_serialization_test.rb index 3446e5e7f0..975acde096 100644 --- a/activerecord/test/cases/json_serialization_test.rb +++ b/activerecord/test/cases/json_serialization_test.rb @@ -200,6 +200,6 @@ class DatabaseConnectedJsonEncodingTest < ActiveRecord::TestCase 2 => @mary } - assert_equal %({1: {"name": "David"}}), authors_hash.to_json(:only => [1, :name]) + assert_equal %({"1": {"name": "David"}}), authors_hash.to_json(:only => [1, :name]) end end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 8199cb8fc7..094932d375 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -252,6 +252,7 @@ ActiveRecord::Schema.define do t.decimal :world_population, :precision => 10, :scale => 0 t.decimal :my_house_population, :precision => 2, :scale => 0 t.decimal :decimal_number_with_default, :precision => 3, :scale => 2, :default => 2.78 + t.float :temperature end create_table :orders, :force => true do |t| |