Merge remote branch 'eloy/master'

activerecord/lib/active_record/associations.rb

@@ -1770,7 +1770,7 @@ def construct_finder_sql_for_association_limiting(options, join_dependency)
         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 @@ def remove_duplicate_results!(base, records, associations)
             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 @@ def remove_duplicate_results!(base, records, associations)
                   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

activerecord/lib/active_record/autosave_association.rb

@@ -166,7 +166,7 @@ def #{type}(name, options = {})
       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
@@ -224,10 +224,10 @@ def marked_for_destruction?
     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
 
@@ -296,13 +296,15 @@ def save_collection_association(reflection)
               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 @@ def save_has_one_association(reflection)
           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 @@ def save_belongs_to_association(reflection)
         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 @@ def save_belongs_to_association(reflection)
               self[reflection.options[:foreign_type]] = association.class.base_class.name.to_s
             end
           end
+
+          saved if autosave
         end
       end
     end

activerecord/lib/active_record/nested_attributes.rb

@@ -238,7 +238,7 @@ def accepts_nested_attributes_for(*attr_names)
             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)

activerecord/lib/active_record/reflection.rb

@@ -255,11 +255,11 @@ def polymorphic_inverse_of(associated_class)
       # 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 @@ def validate?
       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
 

activerecord/test/cases/autosave_association_test.rb

@@ -3,6 +3,8 @@
 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'
@@ -699,23 +701,18 @@ def save(*args)
 
     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
 
@@ -833,6 +830,18 @@ def test_should_still_raise_an_ActiveRecordRecord_Invalid_exception_if_we_want_t
     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]
 
@@ -916,6 +925,18 @@ def test_should_still_raise_an_ActiveRecordRecord_Invalid_exception_if_we_want_t
     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]
 
@@ -931,7 +952,6 @@ def save(*args)
     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
 
@@ -1032,6 +1052,26 @@ def test_should_allow_to_bypass_validations_on_the_associated_models_on_create
     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']
@@ -1215,3 +1255,10 @@ def setup
     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

activerecord/test/cases/reflection_test.rb

@@ -198,11 +198,11 @@ def test_has_many_through_reflection
   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

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

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

activerecord/test/models/line_item.rb

@@ -0,0 +1,3 @@
+class LineItem < ActiveRecord::Base
+  belongs_to :invoice, :touch => true
+end

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

activerecord/test/models/pirate.rb

@@ -51,6 +51,12 @@ def reject_empty_ships_on_create(attributes)
     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")

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

activerecord/test/schema/schema.rb

@@ -191,6 +191,11 @@ def create_table(*args, &block)
     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 @@ def create_table(*args, &block)
     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