Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Update other counter caches on destroy #7706

Merged
merged 1 commit into from Mar 20, 2013

Conversation

Projects
None yet
4 participants
Contributor

iangreenleaf commented Sep 19, 2012

When a model is destroyed by a :dependent => :destroy association, the counter cache is optimized not to update the destroyed parent's counter. However, the optimization is over-zealous and prevents updating of any counter cache from that model, which causes incorrect behavior on models with more than one belongs_to with counter caches.

Contributor

frodsan commented Oct 26, 2012

/cc @jonleighton @tenderlove thoughts?

@frodsan frodsan and 1 other commented on an outdated diff Oct 26, 2012

activerecord/test/models/dog.rb
@@ -1,4 +1,5 @@
class Dog < ActiveRecord::Base
belongs_to :breeder, :class_name => "DogLover", :counter_cache => :bred_dogs_count
belongs_to :trainer, :class_name => "DogLover", :counter_cache => :trained_dogs_count
+ belongs_to :doglover, :foreign_key => :dog_lover_id, :class_name => "DogLover", :counter_cache => true
@frodsan

frodsan Oct 26, 2012

Contributor

Please, use 1.9 hash syntax here. Thanks.

@iangreenleaf

iangreenleaf Oct 26, 2012

Contributor

Done. I updated the immediate neighbors as well, so the new syntax wouldn't look too out-of-place.

@frodsan frodsan commented on an outdated diff Oct 26, 2012

activerecord/test/models/dog_lover.rb
@@ -1,4 +1,5 @@
class DogLover < ActiveRecord::Base
- has_many :trained_dogs, :class_name => "Dog", :foreign_key => :trainer_id
+ has_many :trained_dogs, :class_name => "Dog", :foreign_key => :trainer_id, :dependent => :destroy
@frodsan

frodsan Oct 26, 2012

Contributor

ditto.

@frodsan frodsan commented on an outdated diff Oct 26, 2012

activerecord/test/schema/schema.rb
@@ -230,11 +230,13 @@ def create_table(*args, &block)
create_table :dog_lovers, :force => true do |t|
t.integer :trained_dogs_count, :default => 0
t.integer :bred_dogs_count, :default => 0
+ t.integer :dogs_count, :default => 0
@frodsan

frodsan Oct 26, 2012

Contributor

ditto.

Contributor

iangreenleaf commented Nov 19, 2012

Ping in hopes of reviving this. I'm sitting on a backport of this issue for 3.2 as well that I'd love to get into a release sometime soon.

Contributor

frodsan commented Nov 19, 2012

/cc @carlosantoniodasilva @rafaelfranca thoughts?

@iangreenleaf Please squash your commits and a CHANGELOG entry. Thanks :)

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Nov 19, 2012

.../lib/active_record/associations/builder/belongs_to.rb
@@ -29,7 +30,7 @@ def belongs_to_counter_cache_after_create_for_#{name}
end
def belongs_to_counter_cache_before_destroy_for_#{name}
- unless marked_for_destruction?
+ unless destroyed_by_association && destroyed_by_association.foreign_key.to_sym == #{this_fkey.to_sym.inspect}
@rafaelfranca

rafaelfranca Nov 19, 2012

Owner

Why de we need to inspect here?

@iangreenleaf

iangreenleaf Nov 19, 2012

Contributor

To get the symbol syntax into the string. Most of the time I could simply add : to the front, but :"symbols with spaces" would fail on that.

Contributor

iangreenleaf commented Nov 20, 2012

@frodsan Thanks. Force-pushed a new commit.

Contributor

iangreenleaf commented Jan 4, 2013

Bumping this, hope I can get some eyes on it.

Contributor

iangreenleaf commented Feb 21, 2013

Still hoping to get this merged soon. I'm happy to answer any other questions or feedback if you got em @frodsan @rafaelfranca.

Contributor

iangreenleaf commented Mar 20, 2013

@jeremy could you take a look at this? I noticed you gave another counter_cache bug some love recently.

@jeremy jeremy added a commit that referenced this pull request Mar 20, 2013

@jeremy jeremy Merge pull request #7706 from iangreenleaf/multiple_counter_caches
Update other counter caches on destroy
70d0537

@jeremy jeremy merged commit 70d0537 into rails:master Mar 20, 2013

Contributor

iangreenleaf commented Mar 20, 2013

❤️ ❤️ ❤️

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