Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Regression in 3.2.5: collection.clear does not call after_remove callback #6609

Closed
jplang opened this Issue · 9 comments

4 participants

jplang Erich Menge Jon Leighton emassip
jplang

In previous Rails versions (in Rails 2.3.x and up to Rails 3.2.3 at least), calling collection.clear for a HABTM association used to trigger after_remove callbacks. It's no longer the case with Rails 3.2.5.

class Developer < ActiveRecord::Base
  has_and_belongs_to_many :projects
end

class Project < ActiveRecord::Base
  has_and_belongs_to_many :developers, :after_add => :developer_added, :after_remove => :developer_removed

  def developer_added(developer)
    puts "#{developer.name} added"
  end

  def developer_removed(developer)
    puts "#{developer.name} removed"
  end
end

p = Project.create!(:name => 'Test')
d = Developer.create!(:name => 'John')
p.developers << d
p.developers.clear

With Rails 3.2.3, will output:
John added
John removed

With Rails 3.2.5, will output:
John added

In both cases, the association is cleared.

jplang

Also applies to :has_many association as well.

jplang

Callbacks are called as expected when using p.developers = [] instead of p.developers.clear.

Erich Menge

Verified, git bisect reveals b98d1e2 causes the change in behavior.

It is possible the previous behavior should have never been expected, and that the model was being loaded when it shouldn't have been on a #clear. Perhaps @jonleighton would know better.

Erich Menge

From the documentation: There is also a clear method which is the same as delete_all, so it would appear this behavior was wrong in 3.2.3, and is now correct, since delete_all should not load the records.

jplang

I don't think that 3.2.5 implements the expected behaviour. A test actually exists for that: test_has_and_belongs_to_many_remove_callback_on_clear.
https://github.com/rails/rails/blob/master/activerecord/test/cases/associations/callbacks_test.rb#L132

Erich Menge

@jplang Okay, it looks like delete may be intended to work a bit differently for HABTM. Let's see what Jon Leighton has to say regarding it.

Jon Leighton jonleighton was assigned
Jon Leighton
Collaborator

Damn. Thanks for the report. I will look into it.

emassip

Isn't this issue bad enough to trigger an early 3.2.6 release?

Jon Leighton jonleighton closed this issue from a commit
Jon Leighton jonleighton Revert "Perf: Don't load the association for #delete_all."
This reverts commit b98d1e2.

Closes #6609

Conflicts:

	activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
959fb8e
Jon Leighton
Collaborator

I have reverted the commit which introduced this regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.