Define autosave association validation methods only when needed. [#3161 state:resolved]

Signed-off-by: Eloy Duran <eloy.de.enige@gmail.com>

4 files changed, 132 insertions, 7 deletions

diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index bfae5933ea..7b5bd33863 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -158,7 +158,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}"
-        validate validation_method
+        force_validation = (reflection.options[:validate] == true || reflection.options[:autosave] == true)
 
         case reflection.macro
         when :has_many, :has_and_belongs_to_many
@@ -169,7 +169,10 @@ module ActiveRecord
           after_create save_method
           after_update save_method
 
-          define_method(validation_method) { validate_collection_association(reflection) }
+          if force_validation || (reflection.macro == :has_many && reflection.options[:validate] != false)
+            define_method(validation_method) { validate_collection_association(reflection) }
+            validate validation_method
+          end
         else
           case reflection.macro
           when :has_one
@@ -179,7 +182,11 @@ module ActiveRecord
             define_method(save_method) { save_belongs_to_association(reflection) }
             before_save save_method
           end
-          define_method(validation_method) { validate_single_association(reflection) }
+
+          if force_validation
+            define_method(validation_method) { validate_single_association(reflection) }
+            validate validation_method
+          end
         end
       end
     end
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index 29b199cf04..9164701601 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -1056,3 +1056,118 @@ class TestAutosaveAssociationOnAHasAndBelongsToManyAssociation < ActiveRecord::T
 
   include AutosaveAssociationOnACollectionAssociationTests
 end
+
+class TestAutosaveAssociationValidationsOnAHasManyAssocication < ActiveRecord::TestCase
+  self.use_transactional_fixtures = false
+
+  def setup
+    @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?")
+    @pirate.birds.create(:name => 'cookoo')
+  end
+
+  test "should automatically validate associations" do
+    assert @pirate.valid?
+    @pirate.birds.each { |bird| bird.name = '' }
+
+    assert !@pirate.valid?
+  end
+end
+
+class TestAutosaveAssociationValidationsOnAHasOneAssocication < ActiveRecord::TestCase
+  self.use_transactional_fixtures = false
+
+  def setup
+    @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?")
+    @pirate.create_ship(:name => 'titanic')
+  end
+
+  test "should automatically validate associations with :validate => true" do
+    assert @pirate.valid?
+    @pirate.ship.name = ''
+    assert !@pirate.valid?
+  end
+
+  test "should not automatically validate associations without :validate => true" do
+    assert @pirate.valid?
+    @pirate.non_validated_ship.name = ''
+    assert @pirate.valid?
+  end
+end
+
+class TestAutosaveAssociationValidationsOnABelongsToAssocication < ActiveRecord::TestCase
+  self.use_transactional_fixtures = false
+
+  def setup
+    @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?")
+  end
+
+  test "should automatically validate associations with :validate => true" do
+    assert @pirate.valid?
+    @pirate.parrot = Parrot.new(:name => '')
+    assert !@pirate.valid?
+  end
+
+  test "should not automatically validate associations without :validate => true" do
+    assert @pirate.valid?
+    @pirate.non_validated_parrot = Parrot.new(:name => '')
+    assert @pirate.valid?
+  end
+end
+
+class TestAutosaveAssociationValidationsOnAHABTMAssocication < ActiveRecord::TestCase
+  self.use_transactional_fixtures = false
+
+  def setup
+    @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?")
+  end
+
+  test "should automatically validate associations with :validate => true" do
+    assert @pirate.valid?
+    @pirate.parrots = [ Parrot.new(:name => 'popuga') ]
+    @pirate.parrots.each { |parrot| parrot.name = '' }
+    assert !@pirate.valid?
+  end
+
+  test "should not automatically validate associations without :validate => true" do
+    assert @pirate.valid?
+    @pirate.non_validated_parrots = [ Parrot.new(:name => 'popuga') ]
+    @pirate.non_validated_parrots.each { |parrot| parrot.name = '' }
+    assert @pirate.valid?
+  end
+end
+
+class TestAutosaveAssociationValidationMethodsGeneration < ActiveRecord::TestCase
+  self.use_transactional_fixtures = false
+
+  def setup
+    @pirate = Pirate.new
+  end
+
+  test "should generate validation methods for has_many associations" do
+    assert @pirate.respond_to?(:validate_associated_records_for_birds)
+  end
+
+  test "should generate validation methods for has_one associations with :validate => true" do
+    assert @pirate.respond_to?(:validate_associated_records_for_ship)
+  end
+
+  test "should not generate validation methods for has_one associations without :validate => true" do
+    assert !@pirate.respond_to?(:validate_associated_records_for_non_validated_ship)
+  end
+
+  test "should generate validation methods for belongs_to associations with :validate => true" do
+    assert @pirate.respond_to?(:validate_associated_records_for_parrot)
+  end
+
+  test "should not generate validation methods for belongs_to associations without :validate => true" do
+    assert !@pirate.respond_to?(:validate_associated_records_for_non_validated_parrot)
+  end
+
+  test "should generate validation methods for HABTM associations with :validate => true" do
+    assert @pirate.respond_to?(:validate_associated_records_for_parrots)
+  end
+
+  test "should not generate validation methods for HABTM associations without :validate => true" do
+    assert !@pirate.respond_to?(:validate_associated_records_for_non_validated_parrots)
+  end
+end
diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb
index acf53fce8b..3d7c4bc48a 100644
--- a/activerecord/test/models/pirate.rb
+++ b/activerecord/test/models/pirate.rb
@@ -1,6 +1,8 @@
 class Pirate < ActiveRecord::Base
-  belongs_to :parrot
-  has_and_belongs_to_many :parrots
+  belongs_to :parrot, :validate => true
+  belongs_to :non_validated_parrot, :class_name => 'Parrot'
+  has_and_belongs_to_many :parrots, :validate => true
+  has_and_belongs_to_many :non_validated_parrots, :class_name => 'Parrot'
   has_and_belongs_to_many :parrots_with_method_callbacks, :class_name => "Parrot",
     :before_add    => :log_before_add,
     :after_add     => :log_after_add,
@@ -16,7 +18,8 @@ class Pirate < ActiveRecord::Base
   has_many :treasure_estimates, :through => :treasures, :source => :price_estimates
 
   # These both have :autosave enabled because accepts_nested_attributes_for is used on them.
-  has_one :ship
+  has_one :ship, :validate => true
+  has_one :non_validated_ship, :class_name => 'Ship'
   has_many :birds
   has_many :birds_with_method_callbacks, :class_name => "Bird",
     :before_add    => :log_before_add,
diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb
index 06759d64b8..d0df951622 100644
--- a/activerecord/test/models/ship.rb
+++ b/activerecord/test/models/ship.rb
@@ -1,7 +1,7 @@
 class Ship < ActiveRecord::Base
   self.record_timestamps = false
 
-  belongs_to :pirate
+  belongs_to :pirate, :validate => true
   has_many :parts, :class_name => 'ShipPart', :autosave => true
 
   accepts_nested_attributes_for :pirate, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }