#3005 :counter_cache not updated if polymorphic => true when assigning association - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#3005 ✓stale

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

Reported by Larry Sprock | August 7th, 2009 @ 07:54 PM

Example:

class Image < ActiveRecord::Base
  belongs_to :resource, :polymorphic => true, :counter_cache => :resource_count
end

class Job < ActiveRecord::Base
  has_many :images, :as => :resource, :dependent => :destroy
end

When assigning an association the counter_cache column does not get updated.

i = Image.create
j = Job.create
j.images << i #does not update
i.resource = j #does not update

The only way to get it to work is to use j.images.create which is bad if you have to create the images before the job or reassign images to a different job. Also works if the association is not polymorphic.

Comments and changes to this ticket

CancelProfileIsBroken September 25th, 2009 @ 12:58 PM
- Tag changed from “2-3-stable, activerecord, association, belongs_to, counter_cache, polymorphic_association” to “2-3-stable, activerecord, association, belongs_to, bugmash, counter_cache, polymorphic_association”
- Assigned user cleared.
You flagged this item as spam.
Robert Rouse September 26th, 2009 @ 03:18 PM
+1 verified.

It can be reproduced with the example given.

I am investigating.
David Trasbo September 26th, 2009 @ 05:34 PM
+1 verified

Reproducible on 2-3-master.
You flagged this item as spam.
Elomar França September 26th, 2009 @ 07:12 PM
+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 :(
You flagged this item as spam.
sr.iniv.t September 27th, 2009 @ 12:42 AM
+1 verified.

I have attached a patch for 2-3-stable which contains the failing test.
- counter_cache_polymorphic_test.patch 3.5 KB
You flagged this item as spam.
Blue Box Stephen September 27th, 2009 @ 04:40 AM
+1 to sr.iniv.t's patch. Applies and generates the described error.
You flagged this item as spam.
hsume2 (Henry) September 27th, 2009 @ 06:20 AM
+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).
- LH3005-counter_cache_not_updating_in_polymorphic_belongs_to.diff 1.7 KB
You flagged this item as spam.
sr.iniv.t September 27th, 2009 @ 07:13 AM
@hsume2: Thank you :-)
You flagged this item as spam.
Elad Meidar September 27th, 2009 @ 07:17 AM
+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 ?
You flagged this item as spam.
hsume2 (Henry) September 27th, 2009 @ 11:15 AM
Made a mistake assigning tagging. I've reattached the fixed test.
- LH3005-counter_cache_not_updating_in_polymorphic_belongs_to2.diff 1.7 KB
You flagged this item as spam.
hsume2 (Henry) September 27th, 2009 @ 06:38 PM
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=?
- LH3005-patch-1-counter_cache_not_updating_in_polymorphic_belongs_to.diff 8.4 KB
- LH3005-patch-1-counter_cache_not_updating_in_polymorphic_belongs_to_2-3-stable.diff 7.9 KB
You flagged this item as spam.
Elad Meidar September 27th, 2009 @ 09:53 PM
+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.
Rizwan Reza February 12th, 2010 @ 12:46 PM
- Tag changed from “2-3-stable, activerecord, association, belongs_to, bugmash, counter_cache, polymorphic_association” to “2-3-stable, activerecord, association, belongs_to, counter_cache, polymorphic_association”
[bulk edit]
Santiago Pastorino February 2nd, 2011 @ 04:44 PM
- State changed from “new” to “open”
- Importance changed from “” to “”
This issue has been automatically marked as stale because it has not been commented on for at least three months.

The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.
Santiago Pastorino February 2nd, 2011 @ 04:44 PM
- State changed from “open” to “stale”
You flagged this item as spam.
Greg Hazel February 3rd, 2011 @ 02:36 PM
No hope of getting this applied to 2.3?