Rails Ruby on Rails

Comments and changes to this ticket

• 

  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
  

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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 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.

• You flagged this item as spam.
  

  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 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

• You flagged this item as spam.
  

  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 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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

  • 0001-Make-sure-autosaved-associations-run-callbacks.patch 7.4 KB
• 

  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

• You flagged this item as spam.
  

  Luca Guidi March 10th, 2009 @ 01:54 PM

  Ok, refactored and added tests for habtm.

  • 0001-Make-sure-autosaved-association-run-callbacks.patch 10.3 KB
• 

  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 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 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 March 10th, 2009 @ 06:32 PM

  Arg, and the patch…

  • make_sure_autosave_association_runs_callbacks.diff 15.8 KB

• You flagged this item as spam.
  

  Luca Guidi March 11th, 2009 @ 11:35 AM

  Ok, added basic tests for AssociationCollection#destroy.

  • 0003-Adding-basic-tests-for-AssociationCollection-destroy.patch 4.6 KB
• 

  Eloy Duran March 11th, 2009 @ 09:29 PM

  Thanks Luca.

• 

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

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

  Eloy Duran March 12th, 2009 @ 03:13 PM

  Normalized patch.

  • make_sure_autosave_association_runs_callbacks.diff 13.8 KB
• 

  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...

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  Martin Andert March 16th, 2009 @ 09:10 AM

  Failing test attached.

  • failing_test.diff 1.2 KB
• 

  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

• You flagged this item as spam.
  

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

  Ok, moved the discussion at #2251