Skip to content

Commit

Permalink
Fixed that autosave should validate associations even if master is in…
Browse files Browse the repository at this point in the history 
…valid [#1930 status:committed]
  • Loading branch information
@dhh
dhh committed Feb 27, 2009
1 parent dec91a2 commit 5cda000
Show file tree
Hide file tree
Showing 9 changed files with 629 additions and 535 deletions.
120 changes: 2 additions & 118 deletions activerecord/lib/active_record/associations.rb
View file
Expand Up @@ -786,11 +786,7 @@ module ClassMethods
# 'ORDER BY p.first_name'
def has_many(association_id, options = {}, &extension)
reflection = create_has_many_reflection(association_id, options, &extension)

configure_dependency_for_has_many(reflection)

add_multiple_associated_validation_callbacks(reflection.name) unless options[:validate] == false
add_multiple_associated_save_callbacks(reflection.name)
add_association_callbacks(reflection.name, reflection.options)

if options[:through]
Expand Down Expand Up @@ -872,10 +868,10 @@ def has_many(association_id, options = {}, &extension)
# [:source]
# Specifies the source association name used by <tt>has_one :through</tt> queries. Only use it if the name cannot be
# inferred from the association. <tt>has_one :favorite, :through => :favorites</tt> will look for a
# <tt>:favorite</tt> on Favorite, unless a <tt>:source</tt> is given.
# <tt>:favorite</tt> on Favorite, unless a <tt>:source</tt> is given.
# [:source_type]
# Specifies type of the source association used by <tt>has_one :through</tt> queries where the source
# association is a polymorphic +belongs_to+.
# association is a polymorphic +belongs_to+.
# [:readonly]
# If true, the associated object is readonly through the association.
# [:validate]
Expand All @@ -898,22 +894,9 @@ def has_one(association_id, options = {})
association_accessor_methods(reflection, ActiveRecord::Associations::HasOneThroughAssociation)
else
reflection = create_has_one_reflection(association_id, options)

method_name = "has_one_after_save_for_#{reflection.name}".to_sym
define_method(method_name) do
association = association_instance_get(reflection.name)
if association && (new_record? || association.new_record? || association[reflection.primary_key_name] != id)
association[reflection.primary_key_name] = id
association.save(true)
end
end
after_save method_name

add_single_associated_validation_callbacks(reflection.name) if options[:validate] == true
association_accessor_methods(reflection, HasOneAssociation)
association_constructor_method(:build, reflection, HasOneAssociation)
association_constructor_method(:create, reflection, HasOneAssociation)

configure_dependency_for_has_one(reflection)
end
end
Expand Down Expand Up @@ -1006,40 +989,10 @@ def belongs_to(association_id, options = {})

if reflection.options[:polymorphic]
association_accessor_methods(reflection, BelongsToPolymorphicAssociation)

method_name = "polymorphic_belongs_to_before_save_for_#{reflection.name}".to_sym
define_method(method_name) do
association = association_instance_get(reflection.name)
if association && association.target
if association.new_record?
association.save(true)
end

if association.updated?
self[reflection.primary_key_name] = association.id
self[reflection.options[:foreign_type]] = association.class.base_class.name.to_s
end
end
end
before_save method_name
else
association_accessor_methods(reflection, BelongsToAssociation)
association_constructor_method(:build, reflection, BelongsToAssociation)
association_constructor_method(:create, reflection, BelongsToAssociation)

method_name = "belongs_to_before_save_for_#{reflection.name}".to_sym
define_method(method_name) do
if association = association_instance_get(reflection.name)
if association.new_record?
association.save(true)
end

if association.updated?
self[reflection.primary_key_name] = association.id
end
end
end
before_save method_name
end

# Create the callbacks to update counter cache
Expand Down Expand Up @@ -1067,8 +1020,6 @@ def belongs_to(association_id, options = {})
)
end

add_single_associated_validation_callbacks(reflection.name) if options[:validate] == true

configure_dependency_for_belongs_to(reflection)
end

Expand Down Expand Up @@ -1234,9 +1185,6 @@ def belongs_to(association_id, options = {})
# 'DELETE FROM developers_projects WHERE active=1 AND developer_id = #{id} AND project_id = #{record.id}'
def has_and_belongs_to_many(association_id, options = {}, &extension)
reflection = create_has_and_belongs_to_many_reflection(association_id, options, &extension)

add_multiple_associated_validation_callbacks(reflection.name) unless options[:validate] == false
add_multiple_associated_save_callbacks(reflection.name)
collection_accessor_methods(reflection, HasAndBelongsToManyAssociation)

# Don't use a before_destroy callback since users' before_destroy
Expand Down Expand Up @@ -1358,70 +1306,6 @@ def collection_accessor_methods(reflection, association_proxy_class, writer = tr
end
end

def add_single_associated_validation_callbacks(association_name)
method_name = "validate_associated_records_for_#{association_name}".to_sym
define_method(method_name) do
if association = association_instance_get(association_name)
errors.add association_name unless association.target.nil? || association.valid?
end
end

validate method_name
end

def add_multiple_associated_validation_callbacks(association_name)
method_name = "validate_associated_records_for_#{association_name}".to_sym
define_method(method_name) do
association = association_instance_get(association_name)

if association
if new_record?
association
elsif association.loaded?
association.select { |record| record.new_record? }
else
association.target.select { |record| record.new_record? }
end.each do |record|
errors.add association_name unless record.valid?
end
end
end

validate method_name
end

def add_multiple_associated_save_callbacks(association_name)
method_name = "before_save_associated_records_for_#{association_name}".to_sym
define_method(method_name) do
@new_record_before_save = new_record?
true
end
before_save method_name

method_name = "after_create_or_update_associated_records_for_#{association_name}".to_sym
define_method(method_name) do
association = association_instance_get(association_name)

records_to_save = if @new_record_before_save
association
elsif association && association.loaded?
association.select { |record| record.new_record? }
elsif association && !association.loaded?
association.target.select { |record| record.new_record? }
else
[]
end
records_to_save.each { |record| association.send(:insert_record, record) } unless records_to_save.blank?

# reconstruct the SQL queries now that we know the owner's id
association.send(:construct_sql) if association.respond_to?(:construct_sql)
end

# Doesn't use after_save as that would save associations added in after_create/after_update twice
after_create method_name
after_update method_name
end

def association_constructor_method(constructor, reflection, association_proxy_class)
define_method("#{constructor}_#{reflection.name}") do |*params|
attributees = params.first unless params.empty?
Expand Down
4 changes: 2 additions & 2 deletions activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
View file
Expand Up @@ -28,12 +28,12 @@ def count_records
load_target.size
end

def insert_record(record, force=true)
def insert_record(record, force = true, validate = true)
if record.new_record?
if force
record.save!
else
return false unless record.save
return false unless record.save(validate)
end
end

Expand Down
4 changes: 2 additions & 2 deletions activerecord/lib/active_record/associations/has_many_association.rb
View file
Expand Up @@ -56,9 +56,9 @@ def cached_counter_attribute_name
"#{@reflection.name}_count"
end

def insert_record(record)
def insert_record(record, force = false, validate = true)
set_belongs_to_association_for(record)
record.save
force ? record.save! : record.save(validate)
end

# Deletes the records according to the <tt>:dependent</tt> option.
Expand Down
4 changes: 2 additions & 2 deletions activerecord/lib/active_record/associations/has_many_through_association.rb
View file
Expand Up @@ -47,12 +47,12 @@ def construct_find_options!(options)
options[:include] = @reflection.source_reflection.options[:include] if options[:include].nil?
end

def insert_record(record, force=true)
def insert_record(record, force = true, validate = true)
if record.new_record?
if force
record.save!
else
return false unless record.save
return false unless record.save(validate)
end
end
through_reflection = @reflection.through_reflection
Expand Down

5 comments on commit 5cda000

@alloy
Copy link
Contributor

@alloy alloy commented on 5cda000 Feb 27, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This commit does quite some more than the commit message explains.
I basically moved all code related to auto validating and saving associations that existed prior to AutosaveAssociation into AutosaveAssociation.

It should not change any existing API.

@kovyrin
Copy link
Contributor

@kovyrin kovyrin commented on 5cda000 Sep 7, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see one problem here. For models with many (really many) associations this code would generate tons of dummy validation methods that would call those validate_collection_association/validate_single_association. In our project with hundreds of highly-relational models (some models have 50+ associations) this causes the code to work significantly slower (tests execution time x 1.5).

I've created a patch here, that would simply skip validation methods generation for associations, that weren't specifically requested to be validating ones (:validate => true) and are not :has_many ones. The only problem I see is that this patch would break David's tests that assume validation to be done for every association (which is not correct according to official API docs where only has_many association is auto-validated by default).

@kovyrin
Copy link
Contributor

@kovyrin kovyrin commented on 5cda000 Sep 7, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, to clarify my previous comment: this patch changes the API (it causes all associations to be auto-validating even though docs say only has_many should be) and it hurts performance (in some cases pretty noticeably).

I'm wondering if I should open a ticket and attach my patch with fixed code and tests (not sure what is the official procedure in such a cases).

@alloy
Copy link
Contributor

@alloy alloy commented on 5cda000 Sep 7, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case I accidentally changed the behavior I would love a ticket with the explanation and your patch! The branch I'm pulling in patches on is http://github.com/alloy/rails/commits/2-3-nested_attributes_and_autosave, so you might want to make sure your patch will work with that branch and I will afterwards rebase it to the latest 2.3 revision. Thanks!

@kovyrin
Copy link
Contributor

@kovyrin kovyrin commented on 5cda000 Sep 7, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://rails.lighthouseapp.com/projects/8994/tickets/3161-patch-autosaveassociation-generates-unneeded-validations - here is the patch with my comments and tests

Please sign in to comment.