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

Run callbacks inside of the same transaction #77

Open
jeffblake opened this issue Dec 9, 2020 · 5 comments
Open

Run callbacks inside of the same transaction #77

jeffblake opened this issue Dec 9, 2020 · 5 comments

Comments

@jeffblake
Copy link

Describe the bug

Currently the callbacks such as after_discard are run outside of the discarded model's transaction. This can lead to unexpected behavior where, for example, the model is discarded but its' associations discarded in the after_discard callback are not.

@jarednorman
Copy link
Collaborator

Currently the callbacks such as after_discard are run outside of the discarded model's transaction.

Perhaps I don't understand the context where you're seeing this behaviour. There's not normally a transaction involved, but if you wrap that in a transaction they'll happen inside the transaction as expected, unless I'm missing something.

Mind providing a clearer example to demonstrate the behaviour you think should be changed?

@jeffblake
Copy link
Author

Here's a test case that should pass. So if any errors are raised in the callbacks, database transaction should be rolled back so that nothing is discarded.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activerecord", "6.1.0"
  gem "sqlite3"
  gem 'discard'
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.datetime :discarded_at
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
    t.datetime :discarded_at
  end
end

class Post < ActiveRecord::Base
  include Discard::Model

  has_many :comments

  after_discard do
    comments.discard_all
  end
end

class Comment < ActiveRecord::Base
  include Discard::Model

  before_discard do
    raise StandardError
  end

  belongs_to :post
end

class BugTest < Minitest::Test
  def test_discard_in_transaction
    post = Post.create!
    post.comments << Comment.create!

    begin
      post.discard!
    rescue StandardError => e

    end
    refute post.discarded?
    assert_empty post.comments.discarded
  end
end

@jarednorman
Copy link
Collaborator

Thank you! I understand now. I wasn't thinking about the implicit transaction when you have callbacks.

I will have to look at how complex that behaviour would be to add (and how ActiveRecord handles it) to consider whether it makes sense for this gem. This gem is considered feature complete, but at the same time keeping the functionality in line with the existing Rails behaviour does make sense. Thanks for pointing this out!

@mdavidn
Copy link

mdavidn commented Dec 29, 2021

Rails implements this behavior in a concern, ActiveRecord::Transactions. It overrides each of the standard writers (#save, #destroy, etc) and wraps each in a transaction.

@tak9-n
Copy link

tak9-n commented Feb 7, 2022

How about you execute after_discard's callbacks on after_save when you detect changing discarded_at column?
I resolved the issue in my project in this way as a monkey patch.
By the way, I agree with jeffblake's

This can lead to unexpected behavior.

I think the issue mislead this gem's user. Because, the active record's callbacks are in transaction except of after_commit. The users will think after_discard and before_discard must be in transaction as same as the active record's callbacks act.

thewatts added a commit to thewatts/discard that referenced this issue May 5, 2022
This seeks to solve issue: jhawthorn#77

Given the situation where a `before/after` callback fails, particularly
with related records, we shouldn't still discard records.

Note: This is leveraging a private API that exists in
[ActiveRecord::Transactions](https://github.com/rails/rails/blob/cd2949d2d936daa89b7e23f816f1c004aee85461/activerecord/lib/active_record/transactions.rb#L345)
thewatts added a commit to thewatts/discard that referenced this issue May 5, 2022
This seeks to solve issue: jhawthorn#77

Given the situation where a `before/after` callback fails, particularly
with related records, we shouldn't still discard records.

Note: This is leveraging a private API that exists in
[ActiveRecord::Transactions](https://github.com/rails/rails/blob/cd2949d2d936daa89b7e23f816f1c004aee85461/activerecord/lib/active_record/transactions.rb#L345)
thewatts added a commit to thewatts/discard that referenced this issue May 5, 2022
This seeks to solve issue: jhawthorn#77

Given the situation where a `before/after` callback fails, particularly
with related records, we shouldn't still discard records.

Note: This is leveraging a private API that exists in
[ActiveRecord::Transactions](https://github.com/rails/rails/blob/cd2949d2d936daa89b7e23f816f1c004aee85461/activerecord/lib/active_record/transactions.rb#L345)
thewatts added a commit to thewatts/discard that referenced this issue May 5, 2022
This seeks to solve issue: jhawthorn#77

Given the situation where a `before/after` callback fails, particularly
with related records, we shouldn't still discard records.

Note: This is leveraging a private API that exists in
[ActiveRecord::Transactions](https://github.com/rails/rails/blob/cd2949d2d936daa89b7e23f816f1c004aee85461/activerecord/lib/active_record/transactions.rb#L345)

Co-authored-by: Will Cosgrove <[email protected]>
ryansch pushed a commit to detaso/discard that referenced this issue Sep 19, 2024
This seeks to solve issue: jhawthorn#77

Given the situation where a `before/after` callback fails, particularly
with related records, we shouldn't still discard records.

Note: This is leveraging a private API that exists in
[ActiveRecord::Transactions](https://github.com/rails/rails/blob/cd2949d2d936daa89b7e23f816f1c004aee85461/activerecord/lib/active_record/transactions.rb#L345)

Co-authored-by: Will Cosgrove <[email protected]>
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

No branches or pull requests

4 participants