Wrap discard methods in a transaction

[thewatts]

NOTE: This PR doesn’t currently include tests (I tested against the provided Ruby script).

Should this code be something you all want in the project, I will by all means add specs 😄

———

This seeks to solve issue: #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

To run this against the failing spec referenced in the issue (with a few minor adjustments of my own):

# 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', github: "thewatts/discard", branch: "nw-transactions"
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.string :text
    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 if text == "RAISE"
  end

  belongs_to :post
end

class BugTest < Minitest::Test
  def test_discard_in_transaction
    post = Post.create!
    post.comments << Comment.create!(text: "KEEP")
    post.comments << Comment.create!(text: "RAISE")

    refute post.comments.any?(&:discarded?)

    begin
      post.discard!
    rescue StandardError => e

    end
    refute post.discarded?
    refute post.comments.any?(&:discarded?)
    assert_empty post.comments.discarded
  end
end

[willcosgrove]

We discussed this on the phone 📞 which is why I left you no notes 😄

[thewatts]

Thanks, @willcosgrove!

[GarrisonD]

Waiting for this MR to get merged soon! 🙂

[jarednorman]

I haven't yet had time to review this to determine if I'm happy with the solution. Feel free to use your the branch directly until I get time. This project merges so few changes that it's unlikely you'll miss out on anything. 🙂

[GarrisonD]

I just wrapped obj.discard in a transaction 🙂 need it once so far

[egorslam]

Faced this issue too. For now, wrapped object.discard in a transaction.

[thewatts]

@jarednorman friendly bump here 😄

[jarednorman]

Aren't AR operations only run in transactions when AR callbacks are present? I don't think we want to wrap every discard in a transaction.

Can you reference how this is done in AR? I'd like to keep the implementation here minimal and in line with roughly how they do it, if we're going to make this change.

[thewatts]

Hey @jarednorman - apologies that I'm just now responding. I missed the fact that you requested changes and responded with a question.

Aren't AR operations only run in transactions when AR callbacks are present? I don't think we want to wrap every discard in a transaction.

If I understand correctly, all persistence operations are run within a transaction. This is to account for situations like failures as the database level (ex: null: false on the field, but the AR model doesn't have a presence validation).

Here's an example:

You can see in the picture that there's a transaction that is started, and then a rollback on the database error.

---

I'm certainly open to dig into how Rails is exactly handling this, though.

[jarednorman]

Hmmm. I'm struggling to find a reference to when AR uses transactions automatically. If all persistence operations are run in a transaction and discard uses the public persistence API for updating records, then we wouldn't have to do anything for them to be in transactions. It must be something more subtle than that.