From 5cda000bf0f6d85d1a1efedf9fa4d0b6eaf988a1 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 27 Feb 2009 13:50:24 +0100 Subject: Fixed that autosave should validate associations even if master is invalid [#1930 status:committed] --- activerecord/lib/active_record/associations.rb | 120 +---------- .../has_and_belongs_to_many_association.rb | 4 +- .../associations/has_many_association.rb | 4 +- .../associations/has_many_through_association.rb | 4 +- .../lib/active_record/autosave_association.rb | 237 ++++++++++++++++----- 5 files changed, 187 insertions(+), 182 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index e2dc883b1b..b6f87a0486 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -786,11 +786,7 @@ module ActiveRecord # '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] @@ -872,10 +868,10 @@ module ActiveRecord # [:source] # Specifies the source association name used by has_one :through queries. Only use it if the name cannot be # inferred from the association. has_one :favorite, :through => :favorites will look for a - # :favorite on Favorite, unless a :source is given. + # :favorite on Favorite, unless a :source is given. # [:source_type] # Specifies type of the source association used by has_one :through 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] @@ -898,22 +894,9 @@ module ActiveRecord 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 @@ -1006,40 +989,10 @@ module ActiveRecord 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 @@ -1067,8 +1020,6 @@ module ActiveRecord ) end - add_single_associated_validation_callbacks(reflection.name) if options[:validate] == true - configure_dependency_for_belongs_to(reflection) end @@ -1234,9 +1185,6 @@ module ActiveRecord # '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 @@ -1358,70 +1306,6 @@ module ActiveRecord 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? diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb index a5cc3bf091..af9ce3dfb2 100644 --- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb +++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb @@ -28,12 +28,12 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 3348079e9d..a2cbabfe0c 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -56,9 +56,9 @@ module ActiveRecord "#{@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 :dependent option. diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 2eeeb28d1f..d5d188ac2a 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -47,12 +47,12 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 680b41518c..6e09b13150 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -125,79 +125,63 @@ module ActiveRecord # post.author.name = '' # post.save(false) # => true module AutosaveAssociation + ASSOCIATION_TYPES = %w{ has_one belongs_to has_many has_and_belongs_to_many } + def self.included(base) base.class_eval do + base.extend(ClassMethods) alias_method_chain :reload, :autosave_associations - alias_method_chain :save, :autosave_associations - alias_method_chain :save!, :autosave_associations - alias_method_chain :valid?, :autosave_associations - %w{ has_one belongs_to has_many has_and_belongs_to_many }.each do |type| + ASSOCIATION_TYPES.each do |type| base.send("valid_keys_for_#{type}_association") << :autosave end end end - # Saves the parent, self, and any loaded autosave associations. - # In addition, it destroys all children that were marked for destruction - # with mark_for_destruction. - # - # This all happens inside a transaction, _if_ the Transactions module is included into - # ActiveRecord::Base after the AutosaveAssociation module, which it does by default. - def save_with_autosave_associations(perform_validation = true) - returning(save_without_autosave_associations(perform_validation)) do |valid| - if valid - self.class.reflect_on_all_autosave_associations.each do |reflection| - if (association = association_instance_get(reflection.name)) && association.loaded? - if association.is_a?(Array) - association.proxy_target.each do |child| - child.marked_for_destruction? ? child.destroy : child.save(perform_validation) - end - else - association.marked_for_destruction? ? association.destroy : association.save(perform_validation) - end - end + module ClassMethods + private + + # def belongs_to(name, options = {}) + # super + # add_autosave_association_callbacks(reflect_on_association(name)) + # end + ASSOCIATION_TYPES.each do |type| + module_eval %{ + def #{type}(name, options = {}) + super + add_autosave_association_callbacks(reflect_on_association(name)) end - end + } end - end - # Attempts to save the record just like save_with_autosave_associations but - # will raise a RecordInvalid exception instead of returning false if the - # record is not valid. - def save_with_autosave_associations! - if valid_with_autosave_associations? - save_with_autosave_associations(false) || raise(RecordNotSaved) - else - raise RecordInvalid.new(self) - end - end + # Adds a validate and save callback for the association as specified by + # the +reflection+. + 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 - # Returns whether or not the parent, self, and any loaded autosave associations are valid. - def valid_with_autosave_associations? - if valid_without_autosave_associations? - self.class.reflect_on_all_autosave_associations.all? do |reflection| - if (association = association_instance_get(reflection.name)) && association.loaded? - if association.is_a?(Array) - association.proxy_target.all? { |child| autosave_association_valid?(reflection, child) } - else - autosave_association_valid?(reflection, association) - end - else - true # association not loaded yet, so it should be valid + case reflection.macro + when :has_many, :has_and_belongs_to_many + 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 + + define_method(validation_method) { validate_collection_association(reflection) } + else + case reflection.macro + when :has_one + define_method(save_method) { save_has_one_association(reflection) } + after_save save_method + when :belongs_to + define_method(save_method) { save_belongs_to_association(reflection) } + before_save save_method end + define_method(validation_method) { validate_single_association(reflection) } end - else - false # self was not valid - end - end - - # Returns whether or not the association is valid and applies any errors to the parent, self, if it wasn't. - def autosave_association_valid?(reflection, association) - returning(association.valid?) do |valid| - association.errors.each do |attribute, message| - errors.add "#{reflection.name}_#{attribute}", message - end unless valid end end @@ -221,5 +205,142 @@ module ActiveRecord def marked_for_destruction? @marked_for_destruction end + + private + + # Returns the record for an association collection that should be validated + # or saved. If +autosave+ is +false+ only new records will be returned, + # unless the parent is/was a new record itself. + def associated_records_to_validate_or_save(association, new_record, autosave) + if new_record + association + elsif association.loaded? + autosave ? association : association.select { |record| record.new_record? } + else + autosave ? association.target : association.target.select { |record| record.new_record? } + end + end + + # Validate the association if :validate or :autosave is + # turned on for the association specified by +reflection+. + def validate_single_association(reflection) + if reflection.options[:validate] == true || reflection.options[:autosave] == true + if (association = association_instance_get(reflection.name)) && !association.target.nil? + association_valid?(reflection, association) + end + end + end + + # Validate the associated records if :validate or + # :autosave is turned on for the association specified by + # +reflection+. + def validate_collection_association(reflection) + if reflection.options[:validate] != false && association = association_instance_get(reflection.name) + if records = associated_records_to_validate_or_save(association, new_record?, reflection.options[:autosave]) + records.each { |record| association_valid?(reflection, record) } + end + end + end + + # Returns whether or not the association is valid and applies any errors to + # the parent, self, if it wasn't. + def association_valid?(reflection, association) + unless valid = association.valid? + if reflection.options[:autosave] + association.errors.each do |attribute, message| + attribute = "#{reflection.name}_#{attribute}" + errors.add(attribute, message) unless errors.on(attribute) + end + else + errors.add(reflection.name) + end + end + valid + end + + # Is used as a before_save callback to check while saving a collection + # association whether or not the parent was a new record before saving. + def before_save_collection_association + @new_record_before_save = new_record? + true + end + + # Saves any new associated records, or all loaded autosave associations if + # :autosave is enabled on the association. + # + # In addition, it destroys all children that were marked for destruction + # with mark_for_destruction. + # + # This all happens inside a transaction, _if_ the Transactions module is included into + # ActiveRecord::Base after the AutosaveAssociation module, which it does by default. + def save_collection_association(reflection) + if association = association_instance_get(reflection.name) + autosave = reflection.options[:autosave] + + if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave) + records.each do |record| + if autosave && record.marked_for_destruction? + record.destroy + elsif @new_record_before_save || record.new_record? + if autosave + association.send(:insert_record, record, false, false) + else + association.send(:insert_record, record) + end + elsif autosave + record.save(false) + end + end + end + + # reconstruct the SQL queries now that we know the owner's id + association.send(:construct_sql) if association.respond_to?(:construct_sql) + end + end + + # Saves the associated record if it's new or :autosave is enabled + # on the association. + # + # In addition, it will destroy the association if it was marked for + # destruction with mark_for_destruction. + # + # This all happens inside a transaction, _if_ the Transactions module is included into + # ActiveRecord::Base after the AutosaveAssociation module, which it does by default. + def save_has_one_association(reflection) + if association = association_instance_get(reflection.name) + if reflection.options[:autosave] && association.marked_for_destruction? + association.destroy + elsif new_record? || association.new_record? || association[reflection.primary_key_name] != id || reflection.options[:autosave] + association[reflection.primary_key_name] = id + association.save(false) + end + end + end + + # Saves the associated record if it's new or :autosave is enabled + # on the association. + # + # In addition, it will destroy the association if it was marked for + # destruction with mark_for_destruction. + # + # This all happens inside a transaction, _if_ the Transactions module is included into + # ActiveRecord::Base after the AutosaveAssociation module, which it does by default. + def save_belongs_to_association(reflection) + if association = association_instance_get(reflection.name) + if reflection.options[:autosave] && association.marked_for_destruction? + association.destroy + else + association.save(false) if association.new_record? || reflection.options[:autosave] + + if association.updated? + self[reflection.primary_key_name] = association.id + # TODO: Removing this code doesn't seem to matter… + if reflection.options[:polymorphic] + self[reflection.options[:foreign_type]] = association.class.base_class.name.to_s + end + end + end + end + end end end \ No newline at end of file -- cgit v1.2.3