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

Make AR::Base#touch fire the after_commit and after_rollback callbacks #12031

Closed

Conversation

@airhorns
Copy link
Contributor

@airhorns airhorns commented Aug 26, 2013

I expected

class Product < ActiveRecord::Base
  after_commit :expire_cache

  def expire_cache
    puts "cache expired"
  end

product.touch

to output "cache expired", but it doesn't, and it used to in Rails 3.0. This PR brings back that functionality by making touches call after_commit callbacks, woo!

We relied on after_commit callbacks being fired by touches to do things like expire caches, send documents to our search cluster, bump versions in Redis, etc. When we upgraded to 3.2, we found this problem and had to go and add after_touch callbacks to all those places to get touches to cause the same changes in the system. We make pretty heavy use of touches to bump timestamps up the association chain and whatnot, so they are a principle instigator of attribute change in the system. I think having to add both callbacks to really get a callback fired when stuff changes is sucky because it means client code has to deal with de-duplication if both callbacks fire, client code has to be aware of the semantic differences of when they fire, and before this change there is no way to get called back to when a record's timestamp bump has definitely been committed to the database.

Thanks for any feedback you can give me!

@carlosantoniodasilva this is a follow up to #12026 on top of 4-0-stable, is this what you need?

@arthurnn
Copy link
Member

@arthurnn arthurnn commented Sep 9, 2013

👍

@pftg
pftg reviewed Sep 9, 2013
View changes
activerecord/test/models/owner.rb Outdated
def blocks
@blocks ||= []
end
def on_after_commit(&block)

This comment has been minimized.

@pftg

pftg Sep 9, 2013
Contributor

Need one line before on_after_commit method!

@pftg
Copy link
Contributor

@pftg pftg commented Sep 9, 2013

Please add Changelog

@airhorns
Copy link
Contributor Author

@airhorns airhorns commented Sep 10, 2013

@pftg updated, thanks for the feedback

@ryansch
Copy link

@ryansch ryansch commented Sep 11, 2013

👍

@airhorns
Copy link
Contributor Author

@airhorns airhorns commented Jan 14, 2014

Bump?

@ryansch
Copy link

@ryansch ryansch commented Jan 14, 2014

We've been running this in production via a monkey patch for 4 months. Works as intended.

@airhorns
Copy link
Contributor Author

@airhorns airhorns commented Jan 14, 2014

Us at Shopify too

@carlosantoniodasilva
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva commented Jan 14, 2014

Hey folks, sorry I missed this one, already marked it here to take a look as soon as I find some time here. Thanks!

@arthurnn
Copy link
Member

@arthurnn arthurnn commented Jan 14, 2014

I think we wanna close this PR, and open another one against master... =( , than we backport if needed.

@@ -1465,7 +1465,9 @@ def test_cache_key_changes_when_child_touched
Bulb.create(car: car)

key = car.cache_key
car.bulb.touch
Bulb.transaction do

This comment has been minimized.

@arthurnn

arthurnn Jan 14, 2014
Member

is this necessary?

carlosantoniodasilva added a commit that referenced this pull request Jan 16, 2014
Make AR::Base#touch fire the after_commit and after_rollback callbacks.

Closes #12031.

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/test/cases/integration_test.rb
huoxito added a commit to huoxito/rails that referenced this pull request Mar 14, 2014
As of rails#12031 after_commit and
after_rollback are also executed
mmontossi added a commit to mmontossi/rails that referenced this pull request Oct 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.