after_remove callbacks only run if record was removed. #9346

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants

If a record that does not belong to a HABTM collection is passed in to a collection to be deleted, the after_remove callback is run even though the record was not actually removed. This can cause unexpected behaviour, such as incorrectly decrementing a counter.

The new behaviour still has before_remove called, but after_remove called only if the record was actually removed from the collection.

Example of unexpected behaviour:

class Post < ActiveRecord::Base
  has_and_belongs_to_many :post_collections
end

class PostCollection < ActiveRecord::Base
  has_and_belongs_to_many :posts, after_remove: :decrememnt_posts_count

  def decrement_posts_count
     PostCollection.decrement_counter :posts_count, self.id
  end
end

post = Post.new
post_collection = PostCollection.new
post_collection.posts.delete(post)

# post_collection.posts_count is now -1

Addendum:
The pull request fixes behaviour that may or may not have been what was originally intended. If after_remove is to be called regardless of what happens to the data, this will invalidate the pull request. It seems intuitive however that the after_remove callback should only be run if the record was actually removed from the collection.

@samdalton samdalton after_remove callbacks only run if record was removed.
If a record that does not belong to a collection is
passed in to a collection to be deleted, the after_remove
callback is run even though the record was not actually
removed. This can cause invalid behaviour, such as decrementing
a cache counter.

The new behaviour still has before_remove called, but after_remove
called only if the record was actually removed from the collection.
b4ab206

@frodsan frodsan commented on the diff Feb 26, 2013

.../active_record/associations/collection_association.rb
@@ -468,9 +468,10 @@ def remove_records(existing_records, records, method)
records.each { |record| callback(:before_remove, record) }
delete_records(existing_records, method) if existing_records.any?
- records.each { |record| target.delete(record) }
-
- records.each { |record| callback(:after_remove, record) }
+ records.each do |record|
+ deleted = target.delete(record)
+ callback(:after_remove, record) if deleted
+ end
@frodsan

frodsan Feb 26, 2013

Contributor

Why don't you do this one line instead?

@samdalton

samdalton Feb 26, 2013

I thought it would be doing too much in one line, deleting and running the callback. Made it clearer to split it in 2.

Member

neerajdotname commented Mar 12, 2013

after_remove callback should only be run if the record was actually removed from the collection.

@samdalton by that definition we should also check for if the element is present in the collection. So that we fire before_remove only if the item has a chance of getting removed. What do you think ?

That makes sense to me. It's difficult to know what the original intent was, but from a purely semantic viewpoint it makes sense that only actually removed objects trigger a callback.

On Monday, March 11, 2013 at 7:51 PM, Neeraj Singh wrote:

after_remove callback should only be run if the record was actually removed from the collection.

@samdalton (https://github.com/samdalton) by that definition we should also check for if the element is present in the collection. So that we fire before_remove only if the item has a chance of getting removed. What do you think ?


Reply to this email directly or view it on GitHub (#9346 (comment)).

Member

arthurnn commented Apr 14, 2014

@samdalton is this is a broken behaviour on Rails 4.x ?

Owner

rafaelfranca commented Jan 3, 2015

This will change the existing behavior and people may be relying on it so I don't think it is safe to change it like this.

If we want to change this we need a better upgrade path, like a global option to let people to switch the behavior,

Contributor

kerrizor commented Jan 26, 2015

If we want to change this we need a better upgrade path, like a global option to let people to switch the behavior

That seems "easy enough" - would you council it be the current behavior by default, toggled to use this proposed behavior? @samdalton as the original author are you interested in taking a run at that?

kfrz commented Apr 2, 2017

@samdalton hasn't replied yet but @rafaelfranca if we truly want to add an option for this behavior, I could most certainly take a crack. I'd imagine a call like config.active_record.callback_only_on_removal or something may be appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment