Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes has many through counter-cache bug #8400

Conversation

matthewrobertson
Copy link
Contributor

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
Copy link
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
Copy link
Contributor Author

@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]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should use has_cached_counter? for this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'll check that method later.

@tobidelius
Copy link

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

@jeremy
Copy link
Member

jeremy commented Dec 14, 2012

👍

@rafaelfranca
Copy link
Member

@matthewrobertson this needs a rebase.

@carlosantoniodasilva :shipit:

This commit fixes reported issue rails#7630 in which counter
caches were not being updated properly when replacing
has_many_through relationships
@matthewrobertson
Copy link
Contributor Author

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

carlosantoniodasilva added a commit that referenced this pull request Dec 14, 2012
…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
Copy link
Member

@matthewrobertson thanks!

@carlosantoniodasilva
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants