Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Moved the validation logic to the association reflection and refactor…

…ed autosave_association.rb a bit.
  • Loading branch information...
commit 6d056c7175d1f146684235ab0155f63bf03d534d 1 parent b3bd101
@alloy alloy authored
View
39 activerecord/lib/active_record/autosave_association.rb
@@ -159,46 +159,39 @@ def #{type}(name, options = {})
#
# 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
View
13 activerecord/lib/active_record/reflection.rb
@@ -264,6 +264,19 @@ def collection_association?
@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
View
4 activerecord/test/cases/nested_attributes_test.rb
@@ -561,7 +561,7 @@ def test_should_automatically_enable_autosave_on_the_association
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]
@@ -578,7 +578,7 @@ def test_validate_presence_of_parent__works_with_inverse_of
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)
View
47 activerecord/test/cases/reflection_test.rb
@@ -8,6 +8,8 @@
require 'models/pirate'
class ReflectionTest < ActiveRecord::TestCase
+ include ActiveRecord::Reflection
+
fixtures :topics, :customers, :companies, :subscribers
def setup
@@ -63,22 +65,22 @@ def test_integer_columns
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
)
@@ -103,7 +105,7 @@ def test_reflect_on_all_autosave_associations
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)
@@ -115,7 +117,7 @@ def test_has_many_reflection
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
@@ -182,10 +184,10 @@ def test_reflection_should_not_raise_error_when_compared_to_other_object
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?
@@ -193,6 +195,35 @@ def test_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
def assert_reflection(klass, association, options)
assert reflection = klass.reflect_on_association(association)
Please sign in to comment.
Something went wrong with that request. Please try again.