From 1080351437dc43c3ecaa0d494f5ca215f03b1883 Mon Sep 17 00:00:00 2001 From: Bryan Stearns Date: Wed, 30 Sep 2009 23:17:50 -0700 Subject: Add failing test that triggers the stack overflow --- activerecord/test/cases/autosave_association_test.rb | 9 +++++++++ activerecord/test/models/invoice.rb | 4 ++++ activerecord/test/models/line_item.rb | 3 +++ activerecord/test/schema/schema.rb | 10 ++++++++++ 4 files changed, 26 insertions(+) create mode 100644 activerecord/test/models/invoice.rb create mode 100644 activerecord/test/models/line_item.rb diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 116c8fc509..97d75b651b 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -3,6 +3,8 @@ require 'models/bird' require 'models/company' require 'models/customer' require 'models/developer' +require 'models/invoice' +require 'models/line_item' require 'models/order' require 'models/parrot' require 'models/person' @@ -1215,3 +1217,10 @@ class TestAutosaveAssociationValidationMethodsGeneration < ActiveRecord::TestCas assert !@pirate.respond_to?(:validate_associated_records_for_non_validated_parrots) end end + +class TestAutosaveAssociationWithTouch < ActiveRecord::TestCase + def test_autosave_with_touch_should_not_raise_system_stack_error + invoice = Invoice.create + assert_nothing_raised { invoice.line_items.create(:amount => 10) } + end +end diff --git a/activerecord/test/models/invoice.rb b/activerecord/test/models/invoice.rb new file mode 100644 index 0000000000..fc6ef0230e --- /dev/null +++ b/activerecord/test/models/invoice.rb @@ -0,0 +1,4 @@ +class Invoice < ActiveRecord::Base + has_many :line_items, :autosave => true + before_save {|record| record.balance = record.line_items.map(&:amount).sum } +end diff --git a/activerecord/test/models/line_item.rb b/activerecord/test/models/line_item.rb new file mode 100644 index 0000000000..0dd921a300 --- /dev/null +++ b/activerecord/test/models/line_item.rb @@ -0,0 +1,3 @@ +class LineItem < ActiveRecord::Base + belongs_to :invoice, :touch => true +end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 1ec36e7832..bec4291457 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -191,6 +191,11 @@ ActiveRecord::Schema.define do t.string :info end + create_table :invoices, :force => true do |t| + t.integer :balance + t.datetime :updated_at + end + create_table :items, :force => true do |t| t.column :name, :integer end @@ -216,6 +221,11 @@ ActiveRecord::Schema.define do t.integer :version, :null => false, :default => 0 end + create_table :line_items, :force => true do |t| + t.integer :invoice_id + t.integer :amount + end + create_table :lock_without_defaults, :force => true do |t| t.column :lock_version, :integer end -- cgit v1.2.3 From 5193fe9dd730f9bbb72db055f37625fe9558b6ca Mon Sep 17 00:00:00 2001 From: Lawrence Pit Date: Thu, 7 Jan 2010 19:53:15 +0100 Subject: Exclude unchanged records from the collection being considered for autosave. [#2578 state:resolved] Signed-off-by: Eloy Duran --- activerecord/lib/active_record/autosave_association.rb | 6 +++--- activerecord/test/cases/autosave_association_test.rb | 13 ++++--------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index f01d0903cd..741fb2ef40 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -224,10 +224,10 @@ module ActiveRecord def associated_records_to_validate_or_save(association, new_record, autosave) if new_record association - elsif association.loaded? - autosave ? association : association.find_all { |record| record.new_record? } + elsif autosave + association.target.find_all { |record| record.new_record? || record.changed? || record.marked_for_destruction? } else - autosave ? association.target : association.target.find_all { |record| record.new_record? } + association.target.find_all { |record| record.new_record? } end end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 97d75b651b..fd2b0d2d9f 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -701,23 +701,18 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase define_method("test_should_rollback_destructions_if_an_exception_occurred_while_saving_#{association_name}") do 2.times { |i| @pirate.send(association_name).create!(:name => "#{association_name}_#{i}") } - before = @pirate.send(association_name).map { |c| c } + before = @pirate.send(association_name).map { |c| c.mark_for_destruction ; c } - # Stub the save method of the first child to destroy and the second to raise an exception - class << before.first - def save(*args) - super - destroy - end - end + # Stub the destroy method of the the second child to raise an exception class << before.last - def save(*args) + def destroy(*args) super raise 'Oh noes!' end end assert_raise(RuntimeError) { assert !@pirate.save } + assert before.first.frozen? # the first child was indeed destroyed assert_equal before, @pirate.reload.send(association_name) end -- cgit v1.2.3 From f2aacd51405724cdf7cfd36a439c9dbfce16973a Mon Sep 17 00:00:00 2001 From: Eloy Duran Date: Fri, 8 Jan 2010 20:47:49 +0100 Subject: Rollback the transaction when one of the autosave associations fails to save. [#3391 state:resolved] --- .../lib/active_record/autosave_association.rb | 14 +++++-- .../test/cases/autosave_association_test.rb | 45 +++++++++++++++++++++- activerecord/test/models/bird.rb | 6 +++ activerecord/test/models/parrot.rb | 6 +++ activerecord/test/models/pirate.rb | 6 +++ activerecord/test/models/ship.rb | 6 +++ 6 files changed, 78 insertions(+), 5 deletions(-) diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 741fb2ef40..f7119a77cc 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -296,13 +296,15 @@ module ActiveRecord association.destroy(record) elsif autosave != false && (@new_record_before_save || record.new_record?) if autosave - association.send(:insert_record, record, false, false) + saved = association.send(:insert_record, record, false, false) else association.send(:insert_record, record) end elsif autosave - record.save(false) + saved = record.save(false) end + + raise ActiveRecord::Rollback if saved == false end end @@ -329,7 +331,9 @@ module ActiveRecord key = reflection.options[:primary_key] ? send(reflection.options[:primary_key]) : id if autosave != false && (new_record? || association.new_record? || association[reflection.primary_key_name] != key || autosave) association[reflection.primary_key_name] = key - association.save(!autosave) + saved = association.save(!autosave) + raise ActiveRecord::Rollback if !saved && autosave + saved end end end @@ -350,7 +354,7 @@ module ActiveRecord if autosave && association.marked_for_destruction? association.destroy elsif autosave != false - association.save(!autosave) if association.new_record? || autosave + saved = association.save(!autosave) if association.new_record? || autosave if association.updated? association_id = association.send(reflection.options[:primary_key] || :id) @@ -360,6 +364,8 @@ module ActiveRecord self[reflection.options[:foreign_type]] = association.class.base_class.name.to_s end end + + saved if autosave end end end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index fd2b0d2d9f..7be605ed95 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -830,6 +830,18 @@ class TestAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCase end end + def test_should_not_save_and_return_false_if_a_callback_cancelled_saving + pirate = Pirate.new(:catchphrase => 'Arr') + ship = pirate.build_ship(:name => 'The Vile Insanity') + ship.cancel_save_from_callback = true + + assert_no_difference 'Pirate.count' do + assert_no_difference 'Ship.count' do + assert !pirate.save + end + end + end + def test_should_rollback_any_changes_if_an_exception_occurred_while_saving before = [@pirate.catchphrase, @pirate.ship.name] @@ -913,6 +925,18 @@ class TestAutosaveAssociationOnABelongsToAssociation < ActiveRecord::TestCase end end + def test_should_not_save_and_return_false_if_a_callback_cancelled_saving + ship = Ship.new(:name => 'The Vile Insanity') + pirate = ship.build_pirate(:catchphrase => 'Arr') + pirate.cancel_save_from_callback = true + + assert_no_difference 'Ship.count' do + assert_no_difference 'Pirate.count' do + assert !ship.save + end + end + end + def test_should_rollback_any_changes_if_an_exception_occurred_while_saving before = [@ship.pirate.catchphrase, @ship.name] @@ -928,7 +952,6 @@ class TestAutosaveAssociationOnABelongsToAssociation < ActiveRecord::TestCase end assert_raise(RuntimeError) { assert !@ship.save } - # TODO: Why does using reload on @ship looses the associated pirate? assert_equal before, [@ship.pirate.reload.catchphrase, @ship.reload.name] end @@ -1029,6 +1052,26 @@ module AutosaveAssociationOnACollectionAssociationTests end end + def test_should_not_save_and_return_false_if_a_callback_cancelled_saving_in_either_create_or_update + @pirate.catchphrase = 'Changed' + @child_1.name = 'Changed' + @child_1.cancel_save_from_callback = true + + assert !@pirate.save + assert_equal "Don' botharrr talkin' like one, savvy?", @pirate.reload.catchphrase + assert_equal "Posideons Killer", @child_1.reload.name + + new_pirate = Pirate.new(:catchphrase => 'Arr') + new_child = new_pirate.send(@association_name).build(:name => 'Grace OMalley') + new_child.cancel_save_from_callback = true + + assert_no_difference 'Pirate.count' do + assert_no_difference "#{new_child.class.name}.count" do + assert !new_pirate.save + end + end + end + def test_should_rollback_any_changes_if_an_exception_occurred_while_saving before = [@pirate.catchphrase, *@pirate.send(@association_name).map(&:name)] new_names = ['Grace OMalley', 'Privateers Greed'] diff --git a/activerecord/test/models/bird.rb b/activerecord/test/models/bird.rb index 341d2eeffc..e61d48e6a5 100644 --- a/activerecord/test/models/bird.rb +++ b/activerecord/test/models/bird.rb @@ -1,3 +1,9 @@ class Bird < ActiveRecord::Base validates_presence_of :name + + attr_accessor :cancel_save_from_callback + before_save :cancel_save_callback_method, :if => :cancel_save_from_callback + def cancel_save_callback_method + false + end end \ No newline at end of file diff --git a/activerecord/test/models/parrot.rb b/activerecord/test/models/parrot.rb index 4a7ed52636..737ef9131b 100644 --- a/activerecord/test/models/parrot.rb +++ b/activerecord/test/models/parrot.rb @@ -6,6 +6,12 @@ class Parrot < ActiveRecord::Base alias_attribute :title, :name validates_presence_of :name + + attr_accessor :cancel_save_from_callback + before_save :cancel_save_callback_method, :if => :cancel_save_from_callback + def cancel_save_callback_method + false + end end class LiveParrot < Parrot diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb index 88c1634717..f1dbe32c6e 100644 --- a/activerecord/test/models/pirate.rb +++ b/activerecord/test/models/pirate.rb @@ -51,6 +51,12 @@ class Pirate < ActiveRecord::Base attributes.delete('_reject_me_if_new').present? && new_record? end + attr_accessor :cancel_save_from_callback + before_save :cancel_save_callback_method, :if => :cancel_save_from_callback + def cancel_save_callback_method + false + end + private def log_before_add(record) log(record, "before_adding_method") diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb index a96e38ab41..75c792d176 100644 --- a/activerecord/test/models/ship.rb +++ b/activerecord/test/models/ship.rb @@ -9,4 +9,10 @@ class Ship < ActiveRecord::Base accepts_nested_attributes_for :update_only_pirate, :update_only => true validates_presence_of :name + + attr_accessor :cancel_save_from_callback + before_save :cancel_save_callback_method, :if => :cancel_save_from_callback + def cancel_save_callback_method + false + end end -- cgit v1.2.3 From 7f775ef1a92129fcc77079bc8e00c99a75b38a38 Mon Sep 17 00:00:00 2001 From: Eloy Duran Date: Fri, 8 Jan 2010 21:44:06 +0100 Subject: Renamed AssociationReflection #collection_association? to #collection?. --- activerecord/lib/active_record/associations.rb | 6 +++--- activerecord/lib/active_record/autosave_association.rb | 2 +- activerecord/lib/active_record/nested_attributes.rb | 2 +- activerecord/lib/active_record/reflection.rb | 10 +++++----- activerecord/test/cases/reflection_test.rb | 8 ++++---- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 1320f5b624..aceb83044b 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1770,7 +1770,7 @@ module ActiveRecord end def using_limitable_reflections?(reflections) - reflections.collect(&:collection_association?).length.zero? + reflections.collect(&:collection?).length.zero? end def column_aliases(join_dependency) @@ -1843,7 +1843,7 @@ module ActiveRecord case associations when Symbol, String reflection = base.reflections[associations] - if reflection && reflection.collection_association? + if reflection && reflection.collection? records.each { |record| record.send(reflection.name).target.uniq! } end when Array @@ -1857,7 +1857,7 @@ module ActiveRecord parent_records = [] records.each do |record| if descendant = record.send(reflection.name) - if reflection.collection_association? + if reflection.collection? parent_records.concat descendant.target.uniq else parent_records << descendant diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index f7119a77cc..7c4e81a617 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -166,7 +166,7 @@ module ActiveRecord def add_autosave_association_callbacks(reflection) save_method = :"autosave_associated_records_for_#{reflection.name}" validation_method = :"validate_associated_records_for_#{reflection.name}" - collection = reflection.collection_association? + collection = reflection.collection? unless method_defined?(save_method) if collection diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 9038888d22..f79a12a95c 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -238,7 +238,7 @@ module ActiveRecord reflection.options[:autosave] = true add_autosave_association_callbacks(reflection) nested_attributes_options[association_name.to_sym] = options - type = (reflection.collection_association? ? :collection : :one_to_one) + type = (reflection.collection? ? :collection : :one_to_one) # def pirate_attributes=(attributes) # assign_nested_attributes_for_one_to_one_association(:pirate, attributes) diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index d6fad5cf29..32b9a2aa87 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -255,11 +255,11 @@ module ActiveRecord # Returns whether or not this association reflection is for a collection # association. Returns +true+ if the +macro+ is one of +has_many+ or # +has_and_belongs_to_many+, +false+ otherwise. - def collection_association? - if @collection_association.nil? - @collection_association = [:has_many, :has_and_belongs_to_many].include?(macro) + def collection? + if @collection.nil? + @collection = [:has_many, :has_and_belongs_to_many].include?(macro) end - @collection_association + @collection end # Returns whether or not the association should be validated as part of @@ -278,7 +278,7 @@ module ActiveRecord private def derive_class_name class_name = name.to_s.camelize - class_name = class_name.singularize if collection_association? + class_name = class_name.singularize if collection? class_name end diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb index 774ab7aa4c..2c9158aa7b 100644 --- a/activerecord/test/cases/reflection_test.rb +++ b/activerecord/test/cases/reflection_test.rb @@ -198,11 +198,11 @@ class ReflectionTest < ActiveRecord::TestCase end def test_collection_association - assert Pirate.reflect_on_association(:birds).collection_association? - assert Pirate.reflect_on_association(:parrots).collection_association? + assert Pirate.reflect_on_association(:birds).collection? + assert Pirate.reflect_on_association(:parrots).collection? - assert !Pirate.reflect_on_association(:ship).collection_association? - assert !Ship.reflect_on_association(:pirate).collection_association? + assert !Pirate.reflect_on_association(:ship).collection? + assert !Ship.reflect_on_association(:pirate).collection? end def test_default_association_validation -- cgit v1.2.3