Fixes has many through counter-cache bug #8400

Merged
merged 1 commit into from Dec 14, 2012
Jump to file or symbol
Failed to load files and symbols.
+21 −0
Diff settings

Always

Just for now

@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* Fix counter cache columns not updated when replacing `has_many :through`
+ associations.
+
+ *Matthew Robertson*

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Member

Please move this entry to the top.

@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Member

Please move this entry to the top.

This comment has been minimized.

@matthewrobertson

matthewrobertson Dec 4, 2012

Contributor

done

+
* Recognize migrations placed in directories containing numbers and 'rb'.
Fix #8492
Binary file not shown.
@@ -153,6 +153,11 @@ def delete_records(records, method)
delete_through_records(records)
+ if source_reflection.options[:counter_cache]

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Member

I think you should use has_cached_counter? for this check.

@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Member

I think you should use has_cached_counter? for this check.

This comment has been minimized.

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

@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Member

Alright, I'll check that method later.

@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Member

Alright, I'll check that method later.

+ counter = source_reflection.counter_cache_column
+ klass.decrement_counter counter, records.map(&:id)

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Member

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

Member

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.

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

+ end
+
if through_reflection.macro == :has_many && update_through_counter?(method)
update_counter(-count, through_reflection)
end
@@ -330,6 +330,17 @@ def test_update_counter_caches_on_delete_with_dependent_nullify
end
end
+ def test_update_counter_caches_on_replace_association
+ post = posts(:welcome)
+ tag = post.tags.create!(:name => 'doomed')
+ tag.tagged_posts << posts(:thinking)
+
+ tag.tagged_posts = []
+ post.reload
+
+ assert_equal(post.taggings.count, post.taggings_count)
+ end
+
def test_replace_association
assert_queries(4){posts(:welcome);people(:david);people(:michael); posts(:welcome).people(true)}