Merge remote branch 'eloy/master'

6 files changed, 140 insertions, 82 deletions

diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index b2ae447d5e..1320f5b624 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.reject { |r| [ :belongs_to, :has_one ].include?(r.macro) }.length.zero?
+          reflections.collect(&:collection_association?).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 && [:has_many, :has_and_belongs_to_many].include?(reflection.macro)
+                if reflection && reflection.collection_association?
                   records.each { |record| record.send(reflection.name).target.uniq! }
                 end
               when Array
@@ -1853,12 +1853,11 @@ module ActiveRecord
               when Hash
                 associations.keys.each do |name|
                   reflection = base.reflections[name]
-                  is_collection = [:has_many, :has_and_belongs_to_many].include?(reflection.macro)
 
                   parent_records = []
                   records.each do |record|
                     if descendant = record.send(reflection.name)
-                      if is_collection
+                      if reflection.collection_association?
                         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 98ab64537e..f01d0903cd 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -158,47 +158,39 @@ module ActiveRecord
       #
       # For performance reasons, we don't check whether to validate at runtime,
       # but instead only define the method and callback when needed. However,
-      # this can change, for instance, when using nested attributes. Since we
-      # don't want the callbacks to get defined multiple times, there are
-      # guards that check if the save or validation methods have already been
-      # defined before actually defining them.
+      # this can change, for instance, when using nested attributes, which is
+      # called _after_ the association has been defined. Since we don't want
+      # the callbacks to get defined multiple times, there are guards that
+      # check if the save or validation methods have already been defined
+      # before actually defining them.
       def add_autosave_association_callbacks(reflection)
-        save_method = "autosave_associated_records_for_#{reflection.name}"
-        validation_method = "validate_associated_records_for_#{reflection.name}"
-        force_validation = (reflection.options[:validate] == true || reflection.options[:autosave] == true)
+        save_method = :"autosave_associated_records_for_#{reflection.name}"
+        validation_method = :"validate_associated_records_for_#{reflection.name}"
+        collection = reflection.collection_association?
 
-        case reflection.macro
-        when :has_many, :has_and_belongs_to_many
-          unless method_defined?(save_method)
+        unless method_defined?(save_method)
+          if collection
             before_save :before_save_collection_association
 
             define_method(save_method) { save_collection_association(reflection) }
             # Doesn't use after_save as that would save associations added in after_create/after_update twice
             after_create save_method
             after_update save_method
-          end
-
-          if !method_defined?(validation_method) &&
-              (force_validation || (reflection.macro == :has_many && reflection.options[:validate] != false))
-            define_method(validation_method) { validate_collection_association(reflection) }
-            validate validation_method
-          end
-        else
-          unless method_defined?(save_method)
-            case reflection.macro
-            when :has_one
+          else
+            if reflection.macro == :has_one
               define_method(save_method) { save_has_one_association(reflection) }
               after_save save_method
-            when :belongs_to
+            else
               define_method(save_method) { save_belongs_to_association(reflection) }
               before_save save_method
             end
           end
+        end
 
-          if !method_defined?(validation_method) && force_validation
-            define_method(validation_method) { validate_single_association(reflection) }
-            validate validation_method
-          end
+        if reflection.validate? && !method_defined?(validation_method)
+          method = (collection ? :validate_collection_association : :validate_single_association)
+          define_method(validation_method) { send(method, reflection) }
+          validate validation_method
         end
       end
     end
diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb
index ff3a51d5c0..9038888d22 100644
--- a/activerecord/lib/active_record/nested_attributes.rb
+++ b/activerecord/lib/active_record/nested_attributes.rb
@@ -188,6 +188,8 @@ module ActiveRecord
     # the parent model is saved. This happens inside the transaction initiated
     # by the parents save method. See ActiveRecord::AutosaveAssociation.
     module ClassMethods
+      REJECT_ALL_BLANK_PROC = proc { |attributes| attributes.all? { |_, value| value.blank? } }
+
       # Defines an attributes writer for the specified association(s). If you
       # are using <tt>attr_protected</tt> or <tt>attr_accessible</tt>, then you
       # will need to add the attribute writer to the allowed list.
@@ -229,23 +231,14 @@ module ActiveRecord
         options = { :allow_destroy => false, :update_only => false }
         options.update(attr_names.extract_options!)
         options.assert_valid_keys(:allow_destroy, :reject_if, :limit, :update_only)
+        options[:reject_if] = REJECT_ALL_BLANK_PROC if options[:reject_if] == :all_blank
 
         attr_names.each do |association_name|
           if reflection = reflect_on_association(association_name)
-            type = case reflection.macro
-            when :has_one, :belongs_to
-              :one_to_one
-            when :has_many, :has_and_belongs_to_many
-              :collection
-            end
-
             reflection.options[:autosave] = true
             add_autosave_association_callbacks(reflection)
-            self.nested_attributes_options[association_name.to_sym] = options
-
-            if options[:reject_if] == :all_blank
-              self.nested_attributes_options[association_name.to_sym][:reject_if] = proc { |attributes| attributes.all? {|k,v| v.blank?} }
-            end
+            nested_attributes_options[association_name.to_sym] = options
+            type = (reflection.collection_association? ? :collection : :one_to_one)
 
             # def pirate_attributes=(attributes)
             #   assign_nested_attributes_for_one_to_one_association(:pirate, attributes)
@@ -271,21 +264,11 @@ module ActiveRecord
       marked_for_destruction?
     end
 
-    # Deal with deprecated _delete.
-    #
-    def _delete #:nodoc:
-      ActiveSupport::Deprecation.warn "_delete is deprecated in nested attributes. Use _destroy instead."
-      _destroy
-    end
-
     private
 
     # Attribute hash keys that should not be assigned as normal attributes.
     # These hash keys are nested attributes implementation details.
-    #
-    # TODO Remove _delete from UNASSIGNABLE_KEYS when deprecation warning are
-    # removed.
-    UNASSIGNABLE_KEYS = %w( id _destroy _delete )
+    UNASSIGNABLE_KEYS = %w( id _destroy )
 
     # Assigns the given attributes to the association.
     #
@@ -298,13 +281,17 @@ module ActiveRecord
     # update_only is true, and a <tt>:_destroy</tt> key set to a truthy value,
     # then the existing record will be marked for destruction.
     def assign_nested_attributes_for_one_to_one_association(association_name, attributes)
-      options = self.nested_attributes_options[association_name]
+      options = nested_attributes_options[association_name]
       attributes = attributes.with_indifferent_access
       check_existing_record = (options[:update_only] || !attributes['id'].blank?)
 
       if check_existing_record && (record = send(association_name)) &&
           (options[:update_only] || record.id.to_s == attributes['id'].to_s)
         assign_to_or_mark_for_destruction(record, attributes, options[:allow_destroy])
+
+      elsif attributes['id']
+        raise_nested_attributes_record_not_found(association_name, attributes['id'])
+
       elsif !reject_new_record?(association_name, attributes)
         method = "build_#{association_name}"
         if respond_to?(method)
@@ -343,7 +330,7 @@ module ActiveRecord
     #     { :id => '2', :_destroy => true }
     #   ])
     def assign_nested_attributes_for_collection_association(association_name, attributes_collection)
-      options = self.nested_attributes_options[association_name]
+      options = nested_attributes_options[association_name]
 
       unless attributes_collection.is_a?(Hash) || attributes_collection.is_a?(Array)
         raise ArgumentError, "Hash or Array expected, got #{attributes_collection.class.name} (#{attributes_collection.inspect})"
@@ -366,6 +353,8 @@ module ActiveRecord
           end
         elsif existing_record = send(association_name).detect { |record| record.id.to_s == attributes['id'].to_s }
           assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
+        else
+          raise_nested_attributes_record_not_found(association_name, attributes['id'])
         end
       end
     end
@@ -382,8 +371,7 @@ module ActiveRecord
 
     # Determines if a hash contains a truthy _destroy key.
     def has_destroy_flag?(hash)
-      ConnectionAdapters::Column.value_to_boolean(hash['_destroy']) ||
-      ConnectionAdapters::Column.value_to_boolean(hash['_delete']) # TODO Remove after deprecation.
+      ConnectionAdapters::Column.value_to_boolean(hash['_destroy'])
     end
 
     # Determines if a new record should be build by checking for
@@ -394,14 +382,17 @@ module ActiveRecord
     end
 
     def call_reject_if(association_name, attributes)
-      callback = self.nested_attributes_options[association_name][:reject_if]
-
-      case callback
+      case callback = nested_attributes_options[association_name][:reject_if]
       when Symbol
         method(callback).arity == 0 ? send(callback) : send(callback, attributes)
       when Proc
-        callback.try(:call, attributes)
+        callback.call(attributes)
       end
     end
+
+    def raise_nested_attributes_record_not_found(association_name, record_id)
+      reflection = self.class.reflect_on_association(association_name)
+      raise RecordNotFound, "Couldn't find #{reflection.klass.name} with ID=#{record_id} for #{self.class.name} with ID=#{id}"
+    end
   end
 end
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index b751c9ad68..d6fad5cf29 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -252,10 +252,33 @@ module ActiveRecord
         end
       end
 
+      # 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)
+        end
+        @collection_association
+      end
+
+      # Returns whether or not the association should be validated as part of
+      # the parent's validation.
+      #
+      # Unless you explicitely disable validation with
+      # <tt>:validate => false</tt>, it will take place when:
+      #
+      # * you explicitely enable validation; <tt>:validate => true</tt>
+      # * you use autosave; <tt>:autosave => true</tt>
+      # * the association is a +has_many+ association
+      def validate?
+        !options[:validate].nil? ? options[:validate] : (options[:autosave] == true || macro == :has_many)
+      end
+
       private
         def derive_class_name
           class_name = name.to_s.camelize
-          class_name = class_name.singularize if [ :has_many, :has_and_belongs_to_many ].include?(macro)
+          class_name = class_name.singularize if collection_association?
           class_name
         end
 
diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb
index 8891282915..7ca9c416cb 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -34,7 +34,10 @@ class TestNestedAttributesInGeneral < ActiveRecord::TestCase
   end
 
   def test_should_add_a_proc_to_nested_attributes_options
-    [:parrots, :birds, :birds_with_reject_all_blank].each do |name|
+    assert_equal ActiveRecord::NestedAttributes::ClassMethods::REJECT_ALL_BLANK_PROC,
+                 Pirate.nested_attributes_options[:birds_with_reject_all_blank][:reject_if]
+    
+    [:parrots, :birds].each do |name|
       assert_instance_of Proc, Pirate.nested_attributes_options[name][:reject_if]
     end
   end
@@ -79,12 +82,6 @@ class TestNestedAttributesInGeneral < ActiveRecord::TestCase
     assert ship._destroy
   end
 
-  def test_underscore_delete_is_deprecated
-    ActiveSupport::Deprecation.expects(:warn)
-    ship = Ship.create!(:name => 'Nights Dirty Lightning')
-    ship._delete
-  end
-
   def test_reject_if_method_without_arguments
     Pirate.accepts_nested_attributes_for :ship, :reject_if => :new_record?
 
@@ -176,6 +173,12 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase
     assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name
   end
 
+  def test_should_raise_RecordNotFound_if_an_id_is_given_but_doesnt_return_a_record
+    assert_raise_with_message ActiveRecord::RecordNotFound, "Couldn't find Ship with ID=1234567890 for Pirate with ID=#{@pirate.id}" do
+      @pirate.ship_attributes = { :id => 1234567890 }
+    end
+  end
+
   def test_should_take_a_hash_with_string_keys_and_update_the_associated_model
     @pirate.reload.ship_attributes = { 'id' => @ship.id, 'name' => 'Davy Jones Gold Dagger' }
 
@@ -269,6 +272,8 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase
 end
 
 class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
+  include AssertRaiseWithMessage
+
   def setup
     @ship = Ship.new(:name => 'Nights Dirty Lightning')
     @pirate = @ship.build_pirate(:catchphrase => 'Aye')
@@ -323,6 +328,12 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
     assert_equal 'Arr', @ship.pirate.catchphrase
   end
 
+  def test_should_raise_RecordNotFound_if_an_id_is_given_but_doesnt_return_a_record
+    assert_raise_with_message ActiveRecord::RecordNotFound, "Couldn't find Pirate with ID=1234567890 for Ship with ID=#{@ship.id}" do
+      @ship.pirate_attributes = { :id => 1234567890 }
+    end
+  end
+
   def test_should_take_a_hash_with_string_keys_and_update_the_associated_model
     @ship.reload.pirate_attributes = { 'id' => @pirate.id, 'catchphrase' => 'Arr' }
 
@@ -384,10 +395,6 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
     assert Ship.reflect_on_association(:pirate).options[:autosave]
   end
 
-  def test_should_accept_update_only_option
-    @ship.update_attribute(:update_only_pirate_attributes, { :id => @pirate.ship.id, :catchphrase => 'Arr' })
-  end
-
   def test_should_create_new_model_when_nothing_is_there_and_update_only_is_true
     @pirate.delete
     assert_difference('Pirate.count', 1) do
@@ -460,6 +467,12 @@ module NestedAttributesOnACollectionAssociationTests
     assert_equal ['Grace OMalley', 'Privateers Greed'], [@child_1.name, @child_2.name]
   end
 
+  def test_should_raise_RecordNotFound_if_an_id_is_given_but_doesnt_return_a_record
+    assert_raise_with_message ActiveRecord::RecordNotFound, "Couldn't find #{@child_1.class.name} with ID=1234567890 for Pirate with ID=#{@pirate.id}" do
+      @pirate.attributes = { association_getter => [{ :id => 1234567890 }] }
+    end
+  end
+
   def test_should_automatically_build_new_associated_models_for_each_entry_in_a_hash_where_the_id_is_missing
     @pirate.send(@association_name).destroy_all
     @pirate.reload.attributes = {
@@ -565,7 +578,7 @@ module NestedAttributesOnACollectionAssociationTests
     assert Pirate.reflect_on_association(@association_name).options[:autosave]
   end
 
-  def test_validate_presence_of_parent__works_with_inverse_of
+  def test_validate_presence_of_parent_works_with_inverse_of
     Man.accepts_nested_attributes_for(:interests)
     assert_equal :man, Man.reflect_on_association(:interests).options[:inverse_of]
     assert_equal :interests, Interest.reflect_on_association(:man).options[:inverse_of]
@@ -582,7 +595,7 @@ module NestedAttributesOnACollectionAssociationTests
     end
   end
 
-  def test_validate_presence_of_parent__fails_without_inverse_of
+  def test_validate_presence_of_parent_fails_without_inverse_of
     Man.accepts_nested_attributes_for(:interests)
     Man.reflect_on_association(:interests).options.delete(:inverse_of)
     Interest.reflect_on_association(:man).options.delete(:inverse_of)
diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb
index 211cf1d449..774ab7aa4c 100644
--- a/activerecord/test/cases/reflection_test.rb
+++ b/activerecord/test/cases/reflection_test.rb
@@ -4,10 +4,13 @@ require 'models/customer'
 require 'models/company'
 require 'models/company_in_module'
 require 'models/subscriber'
+require 'models/ship'
 require 'models/pirate'
 require 'models/price_estimate'
 
 class ReflectionTest < ActiveRecord::TestCase
+  include ActiveRecord::Reflection
+
   fixtures :topics, :customers, :companies, :subscribers, :price_estimates
 
   def setup
@@ -68,22 +71,22 @@ class ReflectionTest < ActiveRecord::TestCase
   end
 
   def test_reflection_klass_for_nested_class_name
-    reflection = ActiveRecord::Reflection::MacroReflection.new(nil, nil, { :class_name => 'MyApplication::Business::Company' }, nil)
+    reflection = MacroReflection.new(nil, nil, { :class_name => 'MyApplication::Business::Company' }, nil)
     assert_nothing_raised do
       assert_equal MyApplication::Business::Company, reflection.klass
     end
   end
 
   def test_aggregation_reflection
-    reflection_for_address = ActiveRecord::Reflection::AggregateReflection.new(
+    reflection_for_address = AggregateReflection.new(
       :composed_of, :address, { :mapping => [ %w(address_street street), %w(address_city city), %w(address_country country) ] }, Customer
     )
 
-    reflection_for_balance = ActiveRecord::Reflection::AggregateReflection.new(
+    reflection_for_balance = AggregateReflection.new(
       :composed_of, :balance, { :class_name => "Money", :mapping => %w(balance amount) }, Customer
     )
 
-    reflection_for_gps_location = ActiveRecord::Reflection::AggregateReflection.new(
+    reflection_for_gps_location = AggregateReflection.new(
       :composed_of, :gps_location, { }, Customer
     )
 
@@ -108,7 +111,7 @@ class ReflectionTest < ActiveRecord::TestCase
   end
 
   def test_has_many_reflection
-    reflection_for_clients = ActiveRecord::Reflection::AssociationReflection.new(:has_many, :clients, { :order => "id", :dependent => :destroy }, Firm)
+    reflection_for_clients = AssociationReflection.new(:has_many, :clients, { :order => "id", :dependent => :destroy }, Firm)
 
     assert_equal reflection_for_clients, Firm.reflect_on_association(:clients)
 
@@ -120,7 +123,7 @@ class ReflectionTest < ActiveRecord::TestCase
   end
 
   def test_has_one_reflection
-    reflection_for_account = ActiveRecord::Reflection::AssociationReflection.new(:has_one, :account, { :foreign_key => "firm_id", :dependent => :destroy }, Firm)
+    reflection_for_account = AssociationReflection.new(:has_one, :account, { :foreign_key => "firm_id", :dependent => :destroy }, Firm)
     assert_equal reflection_for_account, Firm.reflect_on_association(:account)
 
     assert_equal Account, Firm.reflect_on_association(:account).klass
@@ -191,7 +194,44 @@ class ReflectionTest < ActiveRecord::TestCase
   end
 
   def test_has_many_through_reflection
-    assert_kind_of ActiveRecord::Reflection::ThroughReflection, Subscriber.reflect_on_association(:books)
+    assert_kind_of ThroughReflection, Subscriber.reflect_on_association(:books)
+  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(:ship).collection_association?
+    assert !Ship.reflect_on_association(:pirate).collection_association?
+  end
+
+  def test_default_association_validation
+    assert AssociationReflection.new(:has_many, :clients, {}, Firm).validate?
+
+    assert !AssociationReflection.new(:has_one, :client, {}, Firm).validate?
+    assert !AssociationReflection.new(:belongs_to, :client, {}, Firm).validate?
+    assert !AssociationReflection.new(:has_and_belongs_to_many, :clients, {}, Firm).validate?
+  end
+
+  def test_always_validate_association_if_explicit
+    assert AssociationReflection.new(:has_one, :client, { :validate => true }, Firm).validate?
+    assert AssociationReflection.new(:belongs_to, :client, { :validate => true }, Firm).validate?
+    assert AssociationReflection.new(:has_many, :clients, { :validate => true }, Firm).validate?
+    assert AssociationReflection.new(:has_and_belongs_to_many, :clients, { :validate => true }, Firm).validate?
+  end
+
+  def test_validate_association_if_autosave
+    assert AssociationReflection.new(:has_one, :client, { :autosave => true }, Firm).validate?
+    assert AssociationReflection.new(:belongs_to, :client, { :autosave => true }, Firm).validate?
+    assert AssociationReflection.new(:has_many, :clients, { :autosave => true }, Firm).validate?
+    assert AssociationReflection.new(:has_and_belongs_to_many, :clients, { :autosave => true }, Firm).validate?
+  end
+
+  def test_never_validate_association_if_explicit
+    assert !AssociationReflection.new(:has_one, :client, { :autosave => true, :validate => false }, Firm).validate?
+    assert !AssociationReflection.new(:belongs_to, :client, { :autosave => true, :validate => false }, Firm).validate?
+    assert !AssociationReflection.new(:has_many, :clients, { :autosave => true, :validate => false }, Firm).validate?
+    assert !AssociationReflection.new(:has_and_belongs_to_many, :clients, { :autosave => true, :validate => false }, Firm).validate?
   end
 
   private