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

Fix counter cache with double destroy, really_destroy, and restore #390

Merged
merged 1 commit into from
Mar 9, 2017

Conversation

excid3
Copy link
Contributor

@excid3 excid3 commented Mar 7, 2017

There's several cases where counter caches get out of sync with soft deletes. This patch gives the ability to disable counter caches for certain events so that the counter caches automatically reflect the correct count.

  1. If you destroy a record multiple times, it will decrement the count multiple times.
  2. If you soft destroy a record and then permanently destroy it, it will decrement multiple times.
  3. Restore does not increment the count.

This checks to see if a record has already been soft deleted when doing a destroy or really_destroy! and then disables the counter cache decrement if it was already soft deleted.

It also adds an increment to the restore method which is the same as the CounterCache create inside ActiveRecord.

Tested this on both Rails 4.2.8 and 5.0.2 and everything looks to be working correctly.

@BenMorganIO
Copy link
Collaborator

BenMorganIO commented Mar 7, 2017

Looks like Rails 4.1 has become the bane...

NoMethodError: super: no superclass method `each_counter_cached_associations' for #<ParanoidModelWithHasOne:0x00000002628278>

lib/paranoia.rb Outdated
@@ -161,6 +169,11 @@ def really_destroy!
end
end

def each_counter_cached_associations
return [] if @_disable_counter_cache
super
Copy link
Collaborator

@BenMorganIO BenMorganIO Mar 7, 2017

Choose a reason for hiding this comment

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

We could probably update this to:

def each_counter_cached_associations
  !@_disable_counter_cache && defined?(super) ? super : []
end

And add a comment that the defined? super is for Rails 4.1 so that we remember to remove it when we (eventually) drop support for it.

@excid3
Copy link
Contributor Author

excid3 commented Mar 7, 2017

Yeah unfortunately Rails 4.1 doesn't have the same methods. It looks like this got moved from associations to the CounterCache.

I might be able to tweak that method for Rails 4.1 to be compatible with the ways 4.2+ handles it. Gonna take a look at that today.

@excid3
Copy link
Contributor Author

excid3 commented Mar 9, 2017

@BenMorganIO so far the only decent solution I have for this is trying to skip the anonymous callbacks that are added for counter caches in Rails 4.1. It's pretty nasty trying to skip the callbacks though.

filter = self.class._destroy_callbacks.find{ |cb| cb.kind == :before && cb.name == :destroy && cb.raw_filter.source_location.first.include?("belongs_to") }.filter
self.class.skip_callback :destroy, :before, filter, if: ->{ @_disable_counter_cache }

Unfortunately, this is even tougher because if you include Paranoia before the counter cache, this callback won't exist yet to modify... fun fun.

@BenMorganIO
Copy link
Collaborator

@excid3 standard style is usually to include Paranoia the very top of their model since it's using a default_scope. Otherwise, all macros are supposed to be at the very bottom. (Yet, that's the style I adhere to, most people probably don't.)

The Rails 4.1 problem is difficult. Can you just an if statement checking if the Rails version is 4.2 or higher? Once Rails 5.1 is out, we can drop support for 4.1.

@excid3
Copy link
Contributor Author

excid3 commented Mar 9, 2017

Yep agreed, I also include at the top as well.

I'm good with that. I've spent a good bit of time on here for 4.1 and with 5.1 around the corner, this is probably the best way to handle it. Let you know when I push up the changes for 4.1 then. 👍

@excid3
Copy link
Contributor Author

excid3 commented Mar 9, 2017

@BenMorganIO alrighty, all done and passed. 🤘

@BenMorganIO BenMorganIO merged commit 25d2118 into rubysherpas:core Mar 9, 2017
@BenMorganIO
Copy link
Collaborator

@excid3 #393

@excid3
Copy link
Contributor Author

excid3 commented Mar 25, 2017

Awesome, thanks for the heads up @BenMorganIO 👍

karunkumar1ly pushed a commit to edcast/paranoia that referenced this pull request Feb 6, 2024
…sociations

This `defined?(super)` check was introduced for Rails 4.1 support
rubysherpas#390 (comment)
but we've stopped supporting Rails 4.1 already
rubysherpas#393 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants