This project is archived and is in readonly mode.

#3005 ✓stale
Larry Sprock

: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

    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.
  • Robert Rouse

    Robert Rouse September 26th, 2009 @ 03:18 PM

    +1 verified.

    It can be reproduced with the example given.

    I am investigating.

  • David Trasbo

    David Trasbo September 26th, 2009 @ 05:34 PM

    +1 verified

    Reproducible on 2-3-master.

  • Elomar França

    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 :(

  • sr.iniv.t

    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.

  • Blue Box Stephen

    Blue Box Stephen September 27th, 2009 @ 04:40 AM

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

  • hsume2 (Henry)

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

  • sr.iniv.t
  • Elad Meidar

    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 ?

  • hsume2 (Henry)

    hsume2 (Henry) September 27th, 2009 @ 11:15 AM

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

  • hsume2 (Henry)

    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=?
  • Elad Meidar

    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

    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
  • Santiago Pastorino

    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

    Santiago Pastorino February 2nd, 2011 @ 04:44 PM

    • State changed from “open” to “stale”
  • Greg Hazel

    Greg Hazel February 3rd, 2011 @ 02:36 PM

    No hope of getting this applied to 2.3?

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages