:counter_cache not updated if polymorphic => true when assigning association

+1 verified.

It can be reproduced with the example given.

I am investigating.

+1 verified

Reproducible on 2-3-master.

+1, verified.

Can be reproduced in 2-3-stable and master, but I still lack the knowledge about the rails internals to create a failing test :(

+1 verified.

I have attached a patch for 2-3-stable which contains the failing test.

+1 to sr.iniv.t's patch. Applies and generates the described error.

+1, verified on 2-3-stable and master.

Good job on the test! However, the Tagging and Post models already encompass the belongs to polymorphic association. Also, the first part replicates testing done elsewhere. I've attached a patch, placing the failing test in join_model_test.rb instead (using the Tagging and Post models).

@hsume2: Thank you :-)

+1, verified on 2-3-stable and master.

I was going through the flow of this process and i came up with this

1. There is not test that checks counter_cache changes with the append (<<) operand on has_many relation.
   
2. in association_collection.rb, the append (<<) method points to add_record_to_target_with_callbacks that fires the :before_add and :after_add on the target and not the *_create callbacks that fire AR::Base.update_counters.

Pratik wrote about those callbacks, maybe adding an option to the reflection, to run update_counters on after_add ?

Made a mistake assigning tagging. I've reattached the fixed test.

1. There is not test that checks counter_cache changes with the append (<<) operand on has_many relation.

I agree, counter_cache updating is unbalanced, especially since the same counter can be updated from both sides of the association.

2. in association_collection.rb, the append (<<) method points to add_record_to_target_with_callbacks that fires the :before_add and :after_add on the target and not the *_create callbacks that fire AR::Base.update_counters.

It's a good start. Adding an :after_add call back to increment the counter fixed the problem as expected. But decrementing the counter on tagging.destroy doesn't work. :before_remove is never called, because it's assigned to the :has_many association and tagging uses belongs_to (which doesn't support :before_remove).

This got me thinking: to strive for more balanced (omni-directional) counter cache updating.

Here is a possible implementation:

1. adding :before_add, :after_add, :before_remove, :after_remove callbacks on ActiveRecord::Base instead.
   
2. modify #add_counter_cache_callbacks to use #after_add, #before_remove instead of #after_create, #before_destroy, respectively.
   
3. modify #create, #update, and #destroy callbacks to run add, add, and remove callbacks, respectively.
   
4. finally, deal with some edge cases:
   

• should only increment if there are changes on record, specifically
• should only increment if size of target collection changed
• decrement as usual

I've attached a patch to that effect (different patches for master and 2-3-stable). It fixes the :counter_cache using :add and :remove callbacks.

Notes:

• collection_ids=[...] will still fail to update counter_cache, because collection_ids= uses #delete and no callbacks will get called. That is expected behavior with #delete, but for collection_ids=?

+1 on solution idea, patches apply cleanly on 2-3-stable and master, i believe the implementation itself is mostly right.

@Henry, as for the delete thing, i think that any change to the collection should update the counter, consistency is the key in this scope.

No hope of getting this applied to 2.3?