This project is archived and is in readonly mode.

#2146 ✓resolved
Luca Guidi

AutosaveAssociation doesn't run removing association callbacks

Reported by Luca Guidi | March 6th, 2009 @ 01:30 AM


class Person < ActiveRecord::Base
  has_many :emails, :after_remove => :set_preferred_email

  accepts_nested_attributes_for :emails, :allow_destroy => true

  private
    def set_preferred_email
      # ...
    end
end

person.emails.delete(email) # => it run the callback
person.update_attributes :emails_attributes => [{:id => email.id, :_delete => 1}] # => it doesn't run the callback

I attached a patch.

Comments and changes to this ticket

  • Eloy Duran

    Eloy Duran March 6th, 2009 @ 09:13 AM

    • Assigned user set to “Eloy Duran”
    • Tag changed from activerecord, associations, callbacks, nested to activerecord, associations, callbacks, nested, tested
    • Milestone cleared.

    Cool thanks for the patch.

    Although I wonder why the callbacks aren't ran, because surely code like this already exists somewhere in Callbacks (I assume):

    
    def with_reflection_callbacks(record, reflection)
      run_reflection_callbacks(record, reflection.options[:before_remove]) if reflection.options[:before_remove]
      yield
      run_reflection_callbacks(record, reflection.options[:after_remove]) if reflection.options[:after_remove]
    end
    
    def run_reflection_callbacks(record, *callbacks)
      callbacks.each do |callback|
        case callback
        when Proc
          callback.call(self, record)
        when Symbol
          self.send(callback, record)
        end
      end
    end
    
  • Luca Guidi

    Luca Guidi March 6th, 2009 @ 10:15 AM

    Callbacks aren't ran, cause we use only record.destroy. Code similar to this is in AssociationProxy#delete, but it run also unwanted callbacks.

    Just to try, instead of invoke with_reflection_callbacks, use collection.delete(record) and start the test suite. Few tests belonging to Autosave module will fail.

    I guess run_reflection_callbacks could be eliminated in favor of similar evaluation techniques in ActiveSupport.

  • Luca Guidi

    Luca Guidi March 6th, 2009 @ 10:20 AM

    ..And I don't know why if use collection.send(:callback, :before_remove, record) it returns a NoMethodError.

  • Eloy Duran

    Eloy Duran March 6th, 2009 @ 10:23 AM

    Callbacks aren't ran, cause we use only record.destroy. Code similar to this is in AssociationProxy#delete, but it run also unwanted callbacks.

    Oh, I actually thought that #destroy should run callbacks whereas #delete shouldn't. This is also what the docs (2.2) say:

    destroy: Deletes the record in the database and freezes this instance to reflect that no changes should be made (since they can’t be persisted).

    delete: Deletes the record in the database and freezes this instance to reflect that no changes should be made (since they can’t be persisted). Unlike destroy, this method doesn’t run any before_delete and after_delete callbacks, nor will it enforce any association +:dependent+ rules. In addition to deleting this record, any defined before_delete and after_delete callbacks are run, and +:dependent+ rules defined on associations are run.

    Just to try, instead of invoke with_reflection_callbacks, use collection.delete(record) and start the test suite. Few tests belonging to Autosave module will fail.

    Will try.

  • Luca Guidi

    Luca Guidi March 6th, 2009 @ 10:35 AM

    Invoking destroy will run ActiveRecord::Base callbacks, cause record is an instance of that class and doesn't know to belonging to an association.

    AssociationCollection#delete is ok for solving the current issue, but breaks other behaviors from AutosaveAssociation.

    This is the reason I introduced those methods. :)

  • Eloy Duran

    Eloy Duran March 8th, 2009 @ 10:18 PM

    Invoking destroy will run ActiveRecord::Base callbacks, cause record is an instance of that class and doesn't know to belonging to an association.

    Sorry, I don't really understand what you meant by this. Could you please re-explain it?

    Thanks

  • Luca Guidi

    Luca Guidi March 9th, 2009 @ 02:49 PM

    I mean, invoking record.destroy (http://github.com/rails/rails/bl...) delete the record, running the ActiveRecord::Base callbacks only: before_destroy and after_destroy.

    Now, we want also run before/after_remove from an has_many association. To achieve this goal, AssociationProxy#delete (http://github.com/rails/rails/bl...) wraps the destruction in a transaction, and invoking the callbacks.

    Solution 1: It seems to be the nicest solution, because we re-use already existing code, avoiding duplication and also the tests of this patch will pass.

    But, if we use association.delete(record) instead of record.destroy, we probably break something in AutosaveAssociation module (please verify test failures).

    Solution 2: It seems we need to run manually the callbacks: we have an AssociationCollection instance (http://github.com/rails/rails/bl...), so I tried to invoke a private method:

    
    association.send(:callback, :before_remove, record)
    record.destroy
    association.send(:callback, :after_remove, record)
    

    But it raises a NoMethodError, it seems to be an instance of Array, instead.

    Solution 3 (current patch): It seems we need to duplicate the code :( So I wrapped the record destruction in with_reflection_callbacks and added run_reflection_callbacks for callbacks execution.

  • Eloy Duran

    Eloy Duran March 10th, 2009 @ 08:58 AM

    Ah I understand now. The difference is that you don't get callbacks to the parent that a associated records will be removed. Ok, I'll pick this up again when I have some free time this week. Thanks for the feedback.

  • Luca Guidi

    Luca Guidi March 10th, 2009 @ 11:14 AM

    Ok, I solved the NoMethodError problem (solution 2): AssociationCollection#callback should be protected instead of to be private.

    
    association.send(:callback, :before_remove, record)
    record.destroy
    association.send(:callback, :after_remove, record)
    

    Everything works fine, all tests will pass, but I don't like it, and if you think about, this code should be equivalent to association.delete(record). What's the problem there?

    Digging inside the code, I discovered that HasManyAssociation#delete_records deletes the records according to the :dependent option.

    Since for test collections (ie. birds) that option wasn't set, the record will be updated (nullifying the foreign key) instead to be deleted. This is why AutosaveAssociation tests won't pass.

    At this point I create another method similar to delete, called destroy:

    
    def destroy(*records)
      records = flatten_deeper(records)
      records.each { |record| raise_on_type_mismatch(record) }
    
      transaction do
        records.each { |record| callback(:before_remove, record) }
    
        old_records = records.reject { |r| r.new_record? }
        old_records.each { |record| record.destroy }
          
        records.each do |record|
          @target.delete(record)
          callback(:after_remove, record)
        end
      end
    end
    

    The only difference is the record.destroy vs delete_records. But I found another interesting issue: @target.delete(record) will cause AutosaveAssociation test failure too. I don't know why, @target should be a Ruby Array, and that method should only remove an element from that collection.

  • Luca Guidi

    Luca Guidi March 10th, 2009 @ 11:22 AM

    Ok, I changed a little bit the method:

    
    # Destroy +records+ and remove from this association calling +before_remove+
    # and +after_remove+ callbacks.
    #
    # Note this method will always remove records from database ignoring the
    # +:dependent+ option.
    def destroy(*records)
      records = flatten_deeper(records)
      records.each { |record| raise_on_type_mismatch(record) }
    
      transaction do
        records.each { |record| callback(:before_remove, record) }
    
        old_records = records.reject { |r| r.new_record? }
        old_records.each { |record| record.destroy }
    
        records.each do |record|
          callback(:after_remove, record)
        end
      end
    
      load_target
    end
    

    And also refactored destroy_all:

    
    def destroy_all
      destroy(self)
      reset_target!
    end
    

    Now the code Inside AutosaveAssociation is clean:

    
    association.destroy(record)
    

    Of course all tests pass, and there aren't repetitions about callbacks evaluation. I think it's a nice solution now :)

    I attach the new version of the patch.

  • Eloy Duran

    Eloy Duran March 10th, 2009 @ 11:41 AM

    Very good, a few comments:

    • There is now a little bit of repetition in association_collection #delete and #destroy. How about refatoring the common code into a private method which does all but the actual removing of the record and yields the collection back to #delete or #destroy which then do the specific stuff they need to do.
    • I would like to see some tests for the habtm case as well (pirate.parrots).

    Thanks

  • Luca Guidi
  • Eloy Duran

    Eloy Duran March 10th, 2009 @ 02:01 PM

    • State changed from “new” to “open”

    Looks great!

    I'll try to find some time tonight to apply and check it out.

  • Eloy Duran

    Eloy Duran March 10th, 2009 @ 06:30 PM

    • State changed from “open” to “verified”

    I applied and test your patch and it works as expected.

    I then moved the tests to the AutosaveAssociation tests, because that's actually where it happens, not in NestedAttributes and refactored it a bit.

    The attached patch contains the patch by Luca and mine.

    One note I have though, is that there are no tests for AssociationCollection::destroy. It is tested in, the tests I moved to the AutosaveAssociation tests, so for now it doesn't matter. But I would like it if you could add create another patch in the near future (no 2.3 blocker) which adds tests like the ones for AssociationCollection::delete (hint: disable the delete code to see which tests fail and look for the ones that you'd want to duplicate for destroy)

    Because if for some reason at some point the AutosaveAssociation tests where to be removed, there should be tests for the AssociationCollection::destroy behaviour.

    For now, let's get it applied! Thanks :)

  • Eloy Duran

    Eloy Duran March 10th, 2009 @ 06:31 PM

    • Title changed from “Nested attributes doesn't run removing association callbacks” to “AutosaveAssociation doesn't run removing association callbacks”
  • Eloy Duran
  • Luca Guidi
  • Eloy Duran
  • Eloy Duran

    Eloy Duran March 12th, 2009 @ 10:21 AM

    • Assigned user changed from “Eloy Duran” to “Pratik”
  • Eloy Duran
  • Repository

    Repository March 12th, 2009 @ 03:26 PM

    • State changed from “verified” to “resolved”

    (from [47bdf3bf40ec17e1f8ca1c0e3d7f697d0c4cd1bf]) Ensure AutosaveAssociation runs remove callbacks [#2146 state:resolved]

    Signed-off-by: Eloy Duran eloy.de.enige@gmail.com Signed-off-by: Pratik Naik pratiknaik@gmail.com http://github.com/rails/rails/co...

  • Martin Andert

    Martin Andert March 16th, 2009 @ 08:03 AM

    I found some unexpected behavior dealing with has_many :through associations. Given the following:

    
    class Post < ActiveRecord::Base
      has_many :taggings
      has_many :tags, :through => :taggings
    end
    
    post = Post.first
    tag = post.tags.first
    

    Calling post.tags.delete(tag) removes the tagging from the database and leaves the tag intact.

    But calling post.tags.destroy(tag) removes the tag from the database (not the tagging).

    IMO, I don't expect destroy to delete the record from the database, only the through_association item.

  • Martin Andert
  • Eloy Duran

    Eloy Duran March 16th, 2009 @ 09:55 AM

    Calling post.tags.delete(tag) removes the tagging from the database and leaves the tag intact. But calling post.tags.destroy(tag) removes the tag from the database (not the tagging). IMO, I don't expect destroy to delete the record from the database, only the through_association item.

    Well destroy should #delete the record from the database, in fact that is it's difference to #delete. However, I do agree that it should remove the tagging if that's how #delete works.

    @ Luca: Will you look into this?

    PS: It's probably better to move any further discussion into a new ticket as this ticket is about a different topic and already resolved. (I also closed you're older ticket #1843)

    Thanks

  • Luca Guidi

    Luca Guidi March 16th, 2009 @ 10:35 AM

    Ok, moved the discussion at #2251

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages