Moved the validation logic to the association reflection and refactored autosave_association.rb a bit.

4 files changed, 70 insertions, 33 deletions

diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index b3e28b9373..f01d0903cd 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -158,46 +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)
+        collection = reflection.collection_association?
 
-        if reflection.collection_association?
-          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/reflection.rb b/activerecord/lib/active_record/reflection.rb
index 96aac96cda..d6fad5cf29 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -262,6 +262,19 @@ module ActiveRecord
         @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
diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb
index a5e2fa96f2..d034bf23a0 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -562,7 +562,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]
@@ -579,7 +579,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 f35bd547c6..774ab7aa4c 100644
--- a/activerecord/test/cases/reflection_test.rb
+++ b/activerecord/test/cases/reflection_test.rb
@@ -9,6 +9,8 @@ require 'models/pirate'
 require 'models/price_estimate'
 
 class ReflectionTest < ActiveRecord::TestCase
+  include ActiveRecord::Reflection
+
   fixtures :topics, :customers, :companies, :subscribers, :price_estimates
 
   def setup
@@ -69,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
     )
 
@@ -109,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)
 
@@ -121,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
@@ -192,10 +194,10 @@ 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?
+  def test_collection_association
     assert Pirate.reflect_on_association(:birds).collection_association?
     assert Pirate.reflect_on_association(:parrots).collection_association?
 
@@ -203,6 +205,35 @@ class ReflectionTest < ActiveRecord::TestCase
     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
     def assert_reflection(klass, association, options)
       assert reflection = klass.reflect_on_association(association)