Fix counter cache behaviour on rails 4.2 #192

sergey-alekseev · 2014-12-14T09:46:32Z

Gist

The PR adds Rails 4.2 compatibility and particularly addresses counter cache changes in rails/rails#14735 and rails/rails#14765.

Details

The introduced test test_counter_cache_column_update_on_destroy with counter cache update on #destroy would break with Rails 4.2. The reason is Rails 4.2 changes introduced in this commit Make counter cache decrementation on destroy idempotent. Try and compare:

# On the commit 'Make counter cache decrementation on destroy idempotent'
# test_counter_cache_column_update_on_destroy fails
gem 'rails', github: 'rails', ref: '655a3667aa99ae3f7e68024b3971b5783d6396bf'

Another introduced test test_counter_cache_column_update_on_really_destroy with counter cache update on #really_destroy! would break with Rails 4.1. It starts to succeed with the commit mentioned above. Try and compare with the commit done right before the Make counter cache decrementation on destroy idempotent:

# Right before the commit 'Make counter cache decrementation on destroy idempotent'
# test_counter_cache_column_update_on_really_destroy fails
gem 'rails', github: 'rails', ref: 'dd063f6ef436b5e6a594e70eeb50532a09ef7a57'

Notes

Apart of the fix I felt free to refactor the code a bit, remove 3 annoying deprecation warnings and add 1 deprecation warning (helpful for future compatibility).

Not sure whether #restore should increment counter cache back. In my current application I need it to be the same behaviour as it's now – don't increment counter cache after restore. So I leave it as is. Let me know if you need this behaviour, I'd include the commit to this or separate PR. The test for #restore is simple:

assert_equal 0, parent_model_with_counter_cache_column.reload.related_models_count
related_model.restore
assert_equal 1, parent_model_with_counter_cache_column.reload.related_models_count

Another proposal: probably it would be good to bump a minor version with Rails 4.2 support. I've noticed you didn't bumb it for Rails 4.1.

BTW love the gem! ❤️ It does a good job.

sergey-alekseev · 2014-12-14T11:01:03Z

Pushed the fix for test_counter_cache_column_update_on_destroy.

This is a workaround actually. Since ActiveRecord::CounterCache.each_counter_cached_associations is private. We shouldn’t use it. Proposals for the proper fix are welcome.

The reason the previous version stopped working is affected_rows returns 0 at active_record/counter_cache.rb#L142. If you follow the method call you’ll find that it’s called at active_record/persistence.rb#L486. Probably we’d better override the return value in ActiveRecord::Relation.delete_all at active_record/relation.rb#L481.

sergey-alekseev · 2014-12-14T14:44:47Z

I've made test_counter_cache_column_update_on_really_destroy to succeed.

This is another workaround actually. My goal is Rails 4.2 support and it works well with Rails 4.2. I’m
leaving this bug not fixed for Rails 4.1 for the time being. We should fix it later though. Proposals for the proper fix are welcome.

pomartel · 2014-12-26T23:51:08Z

👍 I just tested it with one of my app and the fix works for me. I would also vote to fix incrementing the counter cache on a record restore.

sergey-alekseev · 2014-12-27T09:10:11Z

Thanks @pomartel for testing.
Just a note that if you want to use this fix until it's merged you can add this line to your Gemfile: gem 'paranoia', github: 'sergey-alekseev/paranoia', branch: 'switch-to-rails-4-2'.

jhawthorn · 2015-01-18T23:52:29Z

@sergey-alekseev Thanks very much for this work. I've merged all the deprecation fixes, but I need more time to review before merging the counter cache fixes.

I want to avoid anyone experiencing different behaviour when switching from rails 4.1 to 4.2. The conditional test cases suggest that is probably the case here.

sergey-alekseev · 2015-01-18T23:54:34Z

@jhawthorn let me know if you need my help. Good to see some changes were merged, even after more than a month.

thibaudgg · 2015-02-16T07:53:38Z

👍

thibaudgg · 2015-02-16T09:03:40Z

@sergey-alekseev could you rebase your PR on master please, for some reason paranoia_destroyed? is not defined in your fork. Thanks!

http://edgeguides.rubyonrails.org/4_2_release_notes.html#active-record-d eprecations > Deprecated passing Active Record objects to .find or .exists?. Call id on the objects first. Commits in Rails: rails/rails@d92ae6c 0bd270 rails/rails@d35f003 9596f7 rails/rails@c0609dd 356b35

https://github.com/radar/paranoia/blame/1b051ae78793f3b62297b5adbaa9c1d0 0e6726e0/README.md#L154

…che column update

This is a workaround actually. Since `ActiveRecord::CounterCache.each_counter_cached_associations` is private. We shouldn’t use it. Proposals for the proper fix are welcome. The reason the previous version stopped working is `affected_rows` returns `0` at [active_record/counter_cache.rb#L142](https://github.com/rails/rails/blo b/ef99d4cd3ecc58a8c1484740b2fb5447dbda23ab/activerecord/lib/active_recor d/counter_cache.rb#L142). If you follow the method call you’ll find that it’s called at [active_record/persistence.rb#L486](https://github.com/rails/rails/blob/ ef99d4cd3ecc58a8c1484740b2fb5447dbda23ab/activerecord/lib/active_record/ persistence.rb#L486). Probably we’d better override the return value in `ActiveRecord::Relation.delete_all` at [active_record/relation.rb#L481](https://github.com/rails/rails/blob/ef9 9d4cd3ecc58a8c1484740b2fb5447dbda23ab/activerecord/lib/active_record/rel ation.rb#L481).

My goal is Rails 4.2 support and it works well with Rails 4.2. I’m leaving this bug not fixed for Rails 4.1 for the time being.

This will allow to detect counter cached columns changes in callbacks like `after_destroy` or `after_commit on: :destroy`.

sergey-alekseev · 2015-02-17T11:07:15Z

@thibaudgg rebased. paranoia_destroyed? was not defined because my PR was created 2 months ago. paranoia_destroyed? has been committed later 2b0db84.

You may want to fix the last commented test. Authors seems doesn't have interest merging this PR. And the old not rebased branch addresses my needs pretty well. No need to upgrade for me.

thibaudgg · 2015-02-17T12:42:54Z

Hey @radar, what's the status on that PR?
It would be great to have paranoia working fine with Rails 4.2 and counter_cache feature, do you plan to fix it directly on master?

Thanks for your work on this gem, much appreciated!

thibaudgg · 2015-02-17T12:45:35Z

@sergey-alekseev thanks for the rebase!

radar · 2015-02-17T20:53:46Z

Which PR?

On 17 Feb 2015, at 23:42, Thibaud Guillaume-Gentil [email protected] wrote:

Hey @radar, what's the status on that PR?
It would be great to have paranoia working fine with Rails 4.2 and counter_cache feature, do you plan to fix it directly on master?

Thanks for your work on this gem, much appreciated!

—
Reply to this email directly or view it on GitHub.

sergey-alekseev · 2015-02-17T21:17:18Z

@radar this one I suppose.

thibaudgg · 2015-02-18T07:05:09Z

Yep, this one: #192!

jhawthorn · 2015-02-18T08:24:02Z

test/paranoia_test.rb

+    assert_equal 1, parent_model_with_counter_cache_column.reload.related_models_count
+    related_model.destroy
+    assert_equal 0, parent_model_with_counter_cache_column.reload.related_models_count
+    # related_model.restore


Does this work? Why is it commented out?

I asked about restore in the Notes of the PR's description. Personally I don't need such a behaviour. So removing in sergey-alekseev@d252fab.

jhawthorn · 2015-02-18T08:25:26Z

test/paranoia_test.rb

+    related_model.destroy
+
+    assert related_model.instance_variable_get(:@after_destroy_callback_called)
+    # assert related_model.instance_variable_get(:@after_commit_on_destroy_callback_called)


Why is it commented out?

As I wrote here:

You may want to fix the last commented test.

I don't know why it stopped working with the new changes introduced during last 2 months. Your help would be appreciated.

I asked about `restore` in the [Notes](rubysherpas#192 (comment)) of the PR's description. Personally I don't need such a behaviour. So removing.

This has been removed and should no longer exist. rubysherpas#192 (comment)

sergey-alekseev · 2015-02-18T09:46:41Z

@thibaudgg np

ghost · 2015-02-23T15:36:17Z

When can we expect this merged?

radar · 2015-02-23T21:14:49Z

When can I expect people to be patient and to realise we have lives outside of open source?

On 24 Feb 2015, at 02:36, Anthony Collini [email protected] wrote:

When can we expect this merged?

—
Reply to this email directly or view it on GitHub.

ghost · 2015-02-23T21:59:51Z

Woah. Just a question since it seems to be in a good place for merging and there hasn't been any activity in about a week. The reason I ask is because none of my counter caches are working on production. Thanks.

radar · 2015-02-23T23:42:53Z

I will review this when I have time.

Currently time is very limited as I get up at 6:30 every morning to get to work on time at 8:30. Then I work until 5:30 and get the train home for 20 minutes, where I work on my Rails 4 in Action book. There are no seats on the train in the morning to work on the book. After that, I don't do any more work unless my wife is at music class, which happens only on Tuesday nights. I would rather spend time with her than work. Weekends are out because of reasons I outlined in my last reply, and if I do have spare time on the weekend then it's going to go into Rails 4 in Action or just, you know, not working above my limits and burning out like I did at the end of 2009 quite horribly. It took months to recover from that properly and I dread that happening again.

All my spare time is going to work on Rails 4 in Action. Maintaining paranoia (or any of my other gems) is not a priority for me right now. When I get a spare moment, I will review this PR myself and then merge it in. For the time being, your patience is appreciated. If you are super keen to use this patch in production then use the fork until I have time to merge.

Counter caches on Rails 4.2 are broken anyway according to this comment from a user of Forem. Fix is supposed to be coming in 4.2.2. I don't know if this will/won't affect this PR.

So my questions to you @anthonycollini is: have you reviewed this pull request? Does it look shippable to you? Can you try using it in production and see if it does indeed fix your problem?

Thanks for your assistance and prompting.

sergey-alekseev · 2015-02-24T13:34:58Z

@radar thanks for sharing. We appreciate your work on the open source projects and contributions to Rails! Personally I'm inspired by people like you.

allaire · 2015-03-16T17:29:32Z

@sergey-alekseev We are currently using your branch version in our Gemfile while this get merged, thanks for your work!

sergey-alekseev · 2015-03-16T17:31:57Z

@allaire my pleasure. I use my branch as well. :)

ghost · 2015-03-17T22:14:40Z

I am also using this on production and it is working fine.

pomartel · 2015-03-18T00:26:23Z

Same here!

radar · 2015-03-18T03:46:48Z

That's enough verification. Thanks everyone :)

Fix counter cache behaviour on rails 4.2

radar · 2015-03-18T03:46:57Z

Thanks @sergey-alekseev for the patience on this PR.

thibaudgg · 2015-03-18T06:38:01Z

Thanks @sergey-alekseev, thanks @radar!

sergey-alekseev · 2015-03-18T06:39:59Z

My pleasure. Thanks for merging.

joelbarker2011 · 2015-03-21T00:44:30Z

Thanks for the patch, @sergey-alekseev. I'm also using your before_restore code from Stack Overflow. In your OP on this thread, you said

Not sure whether #restore should increment counter cache back. In my current application I need it to be the same behaviour as it's now – don't increment counter cache after restore.

May I ask in what kind of situation you need the present behavior? In my current project, I'm using parent.children_count as a faster and easier cached value for parent.children.count (which is the whole purpose of the "counter cache", as I understand it). So it seems to me that the before_restore functionality should be the default. Having parent.children_count != parent.children.count seems terribly confusing.

(Sorry if I'm hijacking the thread; it seemed the most appropriate place to ask.)

sergey-alekseev changed the title ~~Switch to rails 4 2~~ Switch to rails 4.2 Dec 14, 2014

sergey-alekseev force-pushed the switch-to-rails-4-2 branch from 1ec686e to 4890ffa Compare December 14, 2014 14:44

sergey-alekseev force-pushed the switch-to-rails-4-2 branch 2 times, most recently from c51cf6e to df67172 Compare December 14, 2014 19:37

aaronjensen mentioned this pull request Jan 15, 2015

counter_cache does not decrement when destroying a child record in rails 4.2 #202

Closed

sergey-alekseev added 7 commits February 17, 2015 14:00

apply actual naming to restore's parameter name

b8c9466

https://github.com/radar/paranoia/blame/1b051ae78793f3b62297b5adbaa9c1d0 0e6726e0/README.md#L154

introduce tests for #destroy and #really_destroy! with counter ca…

5b29d8b

…che column update

DRY records creation

29d26d1

skip test_counter_cache_column_update_on_really_destroy for Rails 4.1

3a48a05

My goal is Rails 4.2 support and it works well with Rails 4.2. I’m leaving this bug not fixed for Rails 4.1 for the time being.

move Rails 4.2 counter cached associations destroy inside callbacks

46d9970

This will allow to detect counter cached columns changes in callbacks like `after_destroy` or `after_commit on: :destroy`.

sergey-alekseev force-pushed the switch-to-rails-4-2 branch from dfcdde1 to 46d9970 Compare February 17, 2015 11:01

comment the red test

5fef21d

jhawthorn reviewed Feb 18, 2015
View reviewed changes

jhawthorn changed the title ~~Switch to rails 4.2~~ Fix counter cache behaviour on rails 4.2 Feb 18, 2015

sergey-alekseev added 2 commits February 18, 2015 12:39

✂️

d252fab

I asked about `restore` in the [Notes](rubysherpas#192 (comment)) of the PR's description. Personally I don't need such a behaviour. So removing.

✂️

e3a8f93

This has been removed and should no longer exist. rubysherpas#192 (comment)

radar added a commit that referenced this pull request Mar 18, 2015

Merge pull request #192 from sergey-alekseev/switch-to-rails-4-2

a37652f

Fix counter cache behaviour on rails 4.2

radar merged commit a37652f into rubysherpas:rails4 Mar 18, 2015

sergey-alekseev mentioned this pull request Mar 22, 2015

#restore should increment counter cache back (vote with 👍) #220

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix counter cache behaviour on rails 4.2 #192

Fix counter cache behaviour on rails 4.2 #192

sergey-alekseev commented Dec 14, 2014

sergey-alekseev commented Dec 14, 2014

sergey-alekseev commented Dec 14, 2014

pomartel commented Dec 26, 2014

sergey-alekseev commented Dec 27, 2014

jhawthorn commented Jan 18, 2015

sergey-alekseev commented Jan 18, 2015

thibaudgg commented Feb 16, 2015

thibaudgg commented Feb 16, 2015

sergey-alekseev commented Feb 17, 2015

thibaudgg commented Feb 17, 2015

thibaudgg commented Feb 17, 2015

radar commented Feb 17, 2015

sergey-alekseev commented Feb 17, 2015

thibaudgg commented Feb 18, 2015

jhawthorn Feb 18, 2015

sergey-alekseev Feb 18, 2015

jhawthorn Feb 18, 2015

sergey-alekseev Feb 18, 2015

sergey-alekseev commented Feb 18, 2015

ghost commented Feb 23, 2015

radar commented Feb 23, 2015

ghost commented Feb 23, 2015

radar commented Feb 23, 2015

sergey-alekseev commented Feb 24, 2015

allaire commented Mar 16, 2015

sergey-alekseev commented Mar 16, 2015

ghost commented Mar 17, 2015

pomartel commented Mar 18, 2015

radar commented Mar 18, 2015

radar commented Mar 18, 2015

thibaudgg commented Mar 18, 2015

sergey-alekseev commented Mar 18, 2015

joelbarker2011 commented Mar 21, 2015

Fix counter cache behaviour on rails 4.2 #192

Fix counter cache behaviour on rails 4.2 #192

Conversation

sergey-alekseev commented Dec 14, 2014

Gist

Details

Notes

sergey-alekseev commented Dec 14, 2014

sergey-alekseev commented Dec 14, 2014

pomartel commented Dec 26, 2014

sergey-alekseev commented Dec 27, 2014

jhawthorn commented Jan 18, 2015

sergey-alekseev commented Jan 18, 2015

thibaudgg commented Feb 16, 2015

thibaudgg commented Feb 16, 2015

sergey-alekseev commented Feb 17, 2015

thibaudgg commented Feb 17, 2015

thibaudgg commented Feb 17, 2015

radar commented Feb 17, 2015

sergey-alekseev commented Feb 17, 2015

thibaudgg commented Feb 18, 2015

jhawthorn Feb 18, 2015

Choose a reason for hiding this comment

sergey-alekseev Feb 18, 2015

Choose a reason for hiding this comment

jhawthorn Feb 18, 2015

Choose a reason for hiding this comment

sergey-alekseev Feb 18, 2015

Choose a reason for hiding this comment

sergey-alekseev commented Feb 18, 2015

ghost commented Feb 23, 2015

radar commented Feb 23, 2015

ghost commented Feb 23, 2015

radar commented Feb 23, 2015

sergey-alekseev commented Feb 24, 2015

allaire commented Mar 16, 2015

sergey-alekseev commented Mar 16, 2015

ghost commented Mar 17, 2015

pomartel commented Mar 18, 2015

radar commented Mar 18, 2015

radar commented Mar 18, 2015

thibaudgg commented Mar 18, 2015

sergey-alekseev commented Mar 18, 2015

joelbarker2011 commented Mar 21, 2015