Fixes has many through counter-cache bug #8400

Merged
merged 1 commit into from Dec 14, 2012

Conversation

Projects
None yet
6 participants
@matthewrobertson
Contributor

matthewrobertson commented Dec 2, 2012

This commit fixes reported issue #7630 in which counter caches were not being updated properly when replacing has_many_through relationships.

eg if posts have many tags through taggings

and I did:

post = Post.create!
tag = post.tags.create!
tag.posts = []

then it was: post.reload.taggings_count == 1

this pull makes it: post.reload.taggings_count == 0

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 2, 2012

Member

I receive some errors when running all the tests but this could be because the master build is red at the moment (all errors are related to "deprecated_dynamic_methods_test.rb".

Just some cosmetic corrections:

  • Could you add a CHANGELOG entry, which explains the change?
  • Could you trim the commit message subject to <= 72 characters (you can use the body to add more details to the commit message)?
  • You can find more information related to contributions in our guide
Member

senny commented Dec 2, 2012

I receive some errors when running all the tests but this could be because the master build is red at the moment (all errors are related to "deprecated_dynamic_methods_test.rb".

Just some cosmetic corrections:

  • Could you add a CHANGELOG entry, which explains the change?
  • Could you trim the commit message subject to <= 72 characters (you can use the body to add more details to the commit message)?
  • You can find more information related to contributions in our guide
@matthewrobertson

This comment has been minimized.

Show comment Hide comment
@matthewrobertson

matthewrobertson Dec 2, 2012

Contributor

@senny I trimmed the message, and amended the CHANGELOG.

I am also getting 3 failed tests, but those fail on master with and without this change. So I assume they are unrelated.

Contributor

matthewrobertson commented Dec 2, 2012

@senny I trimmed the message, and amended the CHANGELOG.

I am also getting 3 failed tests, but those fail on master with and without this change. So I assume they are unrelated.

@@ -153,6 +153,11 @@ def delete_records(records, method)
delete_through_records(records)
+ if source_reflection.options[:counter_cache]

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Owner

I think you should use has_cached_counter? for this check.

@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Owner

I think you should use has_cached_counter? for this check.

This comment has been minimized.

Show comment Hide comment
@matthewrobertson

matthewrobertson Dec 4, 2012

Contributor

@carlosantoniodasilva I thought so too, but that method just takes a wild stab at determining if a has_many association has a counter cache. In this case we are operating on the belongs_to side, so Reflection#counter_cache_column will be set if there is a counter_cache.

I think there might actually be problems with the way has_cached_counter? works but if there is I don't think it is in the scope of this pull.

@matthewrobertson

matthewrobertson Dec 4, 2012

Contributor

@carlosantoniodasilva I thought so too, but that method just takes a wild stab at determining if a has_many association has a counter cache. In this case we are operating on the belongs_to side, so Reflection#counter_cache_column will be set if there is a counter_cache.

I think there might actually be problems with the way has_cached_counter? works but if there is I don't think it is in the scope of this pull.

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Owner

Alright, I'll check that method later.

@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Owner

Alright, I'll check that method later.

+* Fix counter cache columns not updated when replacing `has_many :through`
+ associations.
+
+ *Matthew Robertson*

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Owner

Please move this entry to the top.

@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Owner

Please move this entry to the top.

This comment has been minimized.

Show comment Hide comment
@matthewrobertson

matthewrobertson Dec 4, 2012

Contributor

done

@@ -153,6 +153,11 @@ def delete_records(records, method)
delete_through_records(records)
+ if source_reflection.options[:counter_cache]
+ counter = source_reflection.counter_cache_column
+ klass.decrement_counter counter, records.map(&:id)

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Owner

Doesn't decrement_counter only decrement by 1? Can you change your test so that it has 2 records before setting the association to [] to see if it passes correctly?

@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Owner

Doesn't decrement_counter only decrement by 1? Can you change your test so that it has 2 records before setting the association to [] to see if it passes correctly?

This comment has been minimized.

Show comment Hide comment
@matthewrobertson

matthewrobertson Dec 4, 2012

Contributor

Decrementing by exactly 1 is what we want to do here. I have amended the specs to prove it 😉

@matthewrobertson

matthewrobertson Dec 4, 2012

Contributor

Decrementing by exactly 1 is what we want to do here. I have amended the specs to prove it 😉

@sandelius

This comment has been minimized.

Show comment Hide comment
@sandelius

sandelius Dec 14, 2012

This is really awesome. I've been waiting for this fix a long time.

This is really awesome. I've been waiting for this fix a long time.

@jeremy

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy Dec 14, 2012

Owner

👍

Owner

jeremy commented Dec 14, 2012

👍

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Dec 14, 2012

Owner
Owner

rafaelfranca commented Dec 14, 2012

Fix for has_many_through counter_cache bug
This commit fixes reported issue #7630 in which counter
caches were not being updated properly when replacing
has_many_through relationships
@matthewrobertson

This comment has been minimized.

Show comment Hide comment
@matthewrobertson

matthewrobertson Dec 14, 2012

Contributor

@rafaelfranca I rebased, should be good to go now.

Contributor

matthewrobertson commented Dec 14, 2012

@rafaelfranca I rebased, should be good to go now.

carlosantoniodasilva added a commit that referenced this pull request Dec 14, 2012

Merge pull request #8400 from matthewrobertson/has-many-through-count…
…er-cache

Fix for has_many_through counter_cache bug

Counter caches were not being updated properly when replacing
has_many_through relationships.

@carlosantoniodasilva carlosantoniodasilva merged commit 0b97ffd into rails:master Dec 14, 2012

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 14, 2012

Owner
@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 14, 2012

Owner

This added the AR gem to the repo, and I committed it without seeing through the github interface (that doesn't make it very apparent) =/. It has been reverted already.

This added the AR gem to the repo, and I committed it without seeing through the github interface (that doesn't make it very apparent) =/. It has been reverted already.

@matthewrobertson

This comment has been minimized.

Show comment Hide comment
@matthewrobertson

matthewrobertson Dec 14, 2012

Contributor

@carlosantoniodasilva sorry about that. I also didn't notice that it was in there. I don't know why it didn't show up on Github...

Contributor

matthewrobertson commented Dec 14, 2012

@carlosantoniodasilva sorry about that. I also didn't notice that it was in there. I don't know why it didn't show up on Github...

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 14, 2012

Owner

@matthewrobertson no problem. It did show up, but since there's no diff, only the file, I missed while scanning the changes, my bad. It'd be way easier to see if the diff states were shown by default as previously.

@matthewrobertson no problem. It did show up, but since there's no diff, only the file, I missed while scanning the changes, my bad. It'd be way easier to see if the diff states were shown by default as previously.

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