AutosaveAssociation doesn't run removing association callbacks

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.

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

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.

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

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

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.

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.

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.

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.

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

Ok, refactored and added tests for habtm.

Arg, and the patch…

Ok, added basic tests for AssociationCollection#destroy.

Thanks Luca.

Normalized patch.

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.

Failing test attached.

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

Ok, moved the discussion at #2251