After commit on create callback not triggered

Hello @janko.

I found that after_commit on: :create callbacks on activerecord models are not working when using them with shrine.

I create a simple model to replicate the issue

class Catalog < ApplicationRecord
  
  after_create     :after_create_callback
  after_update     :after_update_callback
  after_commit     :after_commit_create_callback,  on: :create
  after_commit     :after_commit_update_callback,  on: :update
  
  def after_create_callback
    Rails.logger.debug "After Create Triggered"
  end

  def after_update_callback
    Rails.logger.debug "After Update Triggered"
  end
  
  def after_commit_create_callback
    Rails.logger.debug "After Commit-Create Triggered"
  end

  def after_commit_update_callback
    Rails.logger.debug "After Commit-Update Triggered"
  end

  include FileUploader::Attachment.new(:file)
end

class FileUploader < Shrine
  plugin :instrumentation

  plugin :activerecord, callbacks: true
end

When creating a new model like this

catalog = Catalog.new
catalog.name = "New"
catalog.file = File.new("test.jpg","rb")
catalog.save

The line After Commit-CreateTriggered is never printed, but After Commit-Update Triggered is printed instead.
To notice that the after_create callback do work (but i can’t use it on my use case), is only the after_commit on:create that is not working.

If i set callbacks as false in the activerecord plugin configuration on the attacher
plugin :activerecord, callbacks: false
The after_commit on:create lines are correctly printed, so the issue is only there when having the activerecord plugin callbacks enabled.

So after tracing the code, when having the callbacks:true on the plugin the problem arise on the persist call in activerecord_after_save method of the activerecord plugin, that if i’m not wrong just saves the record without validation.

def activerecord_after_save
  finalize
  persist
end

I know is the persist line because if i remove the persist line and only keep the finalize one, the after_commit on: :create works again.

Do you think there is an alternative to get this working, with the alternative not being removing the after_commit callback on activerecord models?

Thanks in advance!

Are you telling me that, even though you’ve added your after_commit callbacks before Shrine, the Shrine one still gets executed before some of the ones you’ve defined? That sounds like an ActiveRecord bug.

Note that the ActiveRecord::Transactions module which defines the transactional callbacks is pretty complex and riddled with internal state changes, so it’s not surprising if there are bugs. I’m surprised that the Rails core team finds this production-ready :man_shrugging:

Thanks for the quick answer @janko, yes that’s what i see.

But now i was re-checking the rails guide

As i see, non-transactional callbacks are executed in the order defined, but for transactional callbacks (after_commit and after_rollback), the order is reversed.

I check and if i define the uploader at the beggining my after commit callback is correctly triggered on create.

class Catalog < ApplicationRecord
  
  include FileUploader::Attachment.new(:file)

  after_create     :after_create_callback
  after_update     :after_update_callback
  after_commit     :after_commit_create_callback,  on: :create
  after_commit     :after_commit_update_callback,  on: :update
  
  def after_create_callback
    Rails.logger.debug "After Create Triggered"
  end

  def after_update_callback
    Rails.logger.debug "After Update Triggered"
  end
  
  def after_commit_create_callback
    Rails.logger.debug "After Commit-Create Triggered"
  end

  def after_commit_update_callback
    Rails.logger.debug "After Commit-Update Triggered"
  end

end

I didn’t remember that when starting this thread, i think i got confused because the statement written on the shrine activerecord plugin https://shrinerb.com/docs/plugins/activerecord

Active Record currently has a bug with transaction callbacks, so if you have any “after commit” callbacks, make sure to include Shrine’s attachment module after they have all been defined.

So now i’m not sure, is the shrine module supposed to be defined after any other after_commit callback, so the after_commit shrine callback is called first? I mean, is the shrine after_commit callback supposed to be the first triggered?

That statement in the Shrine documentation was probably the result of some ActiveRecord bug in the past which might have been fixed by now. I think it should be removed.

To me it makes more sense that Shrine’s callback is the last, because it triggers a new update, and according to your case that might not work well with other callbacks. We should probably suggest that in the documentation, would you like to submit a PR for that?

Hello @janko. As i saw, the activerecord issue that was referenced on shrine documentation was not fixed but closed.

But as you said i also think that the shrine callback should be the last, so need to be defined first as after_commit are reversed.
Mainly because the shrine callback ends saving the record, so for any callback runnning after that one, changes are “lost” and a is not more a creation but an update.

I’m really not sure how to write the “Caveat” for the plugin. Although I don’t know why someone would need the after_commit shrine callback run before the others, maybe there is some use case that i’m not thinking at. Besides the shrine callback also add a before_save callback, and those are triggered in the opposite order.
So if the uploader is included first to have the after_commit callback run at the latest, the before_save callback would be the first to run.

Maybe something like this could work?

The activerecord_before_save plugin callback is run through a before_save non-transactional activerecord callback.

But the activerecord_after_save and activerecord_after_destroy plugin callbacks are run through an after_commit transactional activerecord callback.

As per activerecord definition, non-transactional callbacks are executed in the order defined, but transactional callbacks the order is reversed.

Keep that in mind when including the module in your activerecord models.

PS: As i mention in the other thread (Store id of attacher as attribute), this reversed callback order was the reason i was always getting false for the changed? method when a new file was attached.