shopify / activerecord-rescue_from_duplicate Goto Github PK

Since the code would be in the same place, any interest in turning ActiveRecord::InvalidForeignKey errors into validation errors? Same risks apply, I think, as ActiveRecord::RecordNotUnique.

We use validations like these to handle when the application validation fails, e.g. we have a presence validation on the required belongs to, before the record is saved, the relation is deleted, so now the save raise an FK error, which we should probably turn into an active record error (and re-raise if inside of a transaction)

As @matthewd pointed out, PG exceptions offer an API to get metadata about the exception, notably the concerned columns: http://www.rubydoc.info/gems/pg/PG/Result#error_field-instance_method

It might be worth investigating.

Not sure where the change in Rails is, but looking at the test suite, I suspect it just wasn't run in a while?

tl;dr

  # @raise ActiveRecord::RecordNotSaved
  def create_or_update(...)
    super
  rescue ActiveRecord::StatementInvalid, ActiveRecord::RecordNotUnique => exception
    raise unless handle_unicity_error(exception)
    false
  end

but

  # @raise ActiveRecord::RecordNotUnique
  def save(...)
    super
  rescue ActiveRecord::StatementInvalid, ActiveRecord::RecordNotUnique => exception
    raise unless handle_unicity_error(exception)
    false
  end

The RecordNotSaved does have #record so it's still possible to get the errors out.

Running on Ruby 3.0.3 MRI on OSX M1 using rvm fwiw

backtrace

.bundle/vendor/ruby/3.0.0/gems/activerecord-7.0.3/lib/active_record/persistence.rb:648:in `save!'",
.bundle/vendor/ruby/3.0.0/gems/activerecord-7.0.3/lib/active_record/validations.rb:53:in `save!'",
.bundle/vendor/ruby/3.0.0/gems/activerecord-7.0.3/lib/active_record/transactions.rb:302:in `block in save!'",
.bundle/vendor/ruby/3.0.0/gems/activerecord-7.0.3/lib/active_record/transactions.rb:354:in `block in with_transaction_returning_status'",
.bundle/vendor/ruby/3.0.0/gems/activerecord-7.0.3/lib/active_record/connection_adapters/abstract/transaction.rb:319:in `block in within_new_transaction'",
.bundle/vendor/ruby/3.0.0/gems/activesupport-7.0.3/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'",
.bundle/vendor/ruby/3.0.0/gems/activesupport-7.0.3/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'",
.bundle/vendor/ruby/3.0.0/gems/activesupport-7.0.3/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'",
.bundle/vendor/ruby/3.0.0/gems/activesupport-7.0.3/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'",
.bundle/vendor/ruby/3.0.0/gems/activerecord-7.0.3/lib/active_record/connection_adapters/abstract/transaction.rb:317:in `within_new_transaction'",
.bundle/vendor/ruby/3.0.0/gems/activerecord-7.0.3/lib/active_record/connection_adapters/abstract/database_statements.rb:316:in `transaction'",
.bundle/vendor/ruby/3.0.0/gems/activerecord-7.0.3/lib/active_record/transactions.rb:350:in `with_transaction_returning_status'",
.bundle/vendor/ruby/3.0.0/gems/activerecord-7.0.3/lib/active_record/transactions.rb:302:in `save!'",
.bundle/vendor/ruby/3.0.0/gems/activerecord-7.0.3/lib/active_record/suppressor.rb:54:in `save!'",
.bundle/vendor/ruby/3.0.0/gems/factory_bot-6.2.0/lib/factory_bot/evaluation.rb:18:in `create'",

where in ar persistence

    def save!(**options, &block)
      create_or_update(**options, &block) || raise(RecordNotSaved.new("Failed to save the record", self))
    end

In Postgres, if you

1. have a transaction open
2. something which invokes non bang save/update on a rescued class
3. rescue a ActiveRecord::RecordNotUnique / PG::UniqueViolation
4. the transaction fails to get rolled back and you wind up with a ActiveRecord::StatementInvalid / PG::InFailedSqlTransaction

The resolution, it seems to me, is the check if we're already in a transaction before the lifecycle of save/update starts its own transaction-returning-status.

For example:

let's assert we mix in to instance accessors: rescue_from_duplicate_handled_unicity_exception and rescue_from_duplicate_is_inside_transaction

and we prepend both save and update with something like self.rescue_from_duplicate_is_inside_transaction = ::AfterCommitEverywhere.in_transaction?(db_connection) if rescue_from_duplicate_is_inside_transaction.nil? (then call super)

then in the create_or_update we set rescue_from_duplicate_handled_unicity_exception to false

then at the end of handle_unicity_error we set rescue_from_duplicate_handled_unicity_exception to the rescued exception

then if in save if the call to super returned false, and we have a rescue_from_duplicate_handled_unicity_exception, then call something like re_raise_uniqueness_error_if_in_transaction, which checks if rescue_from_duplicate_is_inside_transaction is true and we have a rescue_from_duplicate_handled_unicity_exception, and if ::AfterCommitEverywhere.in_transaction?, and then re-raises the rescue_from_duplicate_handled_unicity_exception

On the one hand, this is is a bit of chanting and make the save raise
On the other hand, it prevents failing a transaction without a rolback.
On the other hand, you should never call a non bang persisistence method inside of a transaction.
On the other hand, sometimes things happen, and it can be hard to trace back to where they went bad, and it's a shame we could have done something about it.

(I have some code which I've tested with this flow which seems to handle the scenario which I'd be happy to submit.)

Hi 👋🏼

I read in the README that this gem will throw a RecordNotSaved exception on create!. I would like to understand why is that the case instead of returning a RecordInvalid and where is this in the code of this gem (I cannot seem to find any references to RecordNotSaved besides tests).

I have an issue where a service is concurrently trying to create two instances of the same model and I got the feeling this gem is the one responsible for throwing the RecordNotSaved exception I'm getting 👀

Thanks!

I have two classes, A and B, and a relation C between them. Essentially, an instance of B is a subset of As. Thus, I want to make sure that each B contains at most one of each A. This translates to a validation rule for C:

validates :a_id, uniqueness: { scope: [:b_id], rescue_from_duplicate: true, message: "already exists" }

Now, when I create a new instance of B that contains duplicate a_ids, save fails (as it should), but my B object does not have any errors. Instead, only the second C instance with the same a_id has an error. I would expect to see an error on the main B object.

How to properly test rescue_from_duplicate?

And on another note:

add_index :table, "fk_id, lower(code)", unique: true, algorithm: :concurrently

will trigger an error when calling missing indexes:

irb(main):001:0> RescueFromDuplicate.missing_unique_indexes
Traceback (most recent call last):
        1: from (irb):1
NoMethodError (undefined method `map' for "fk_id, lower((code)"::text)":String)

Considering two models Parent and Child where Child has a uniqueness constraint using rescue_from_duplicate. When we instantiate Parent with a Child (not saved yet) that is not unique, there are two possible outcomes:

Scenario 1

The uniqueness validation fails because the other identical record is already visible in the db to the current process. Parent doesn't get saved and #save returns false.

Scenario 2

The uniqueness validation passes but saving fails through a race condition and rescue_from_duplicate rescues ActiveRecord::RecordNotUnique. Parent#save still succeeds and returns true but the Child is not saved. For the caller of Parent#save it looks like both were saved correctly.

I think the right solution is to rollback the Parent transaction (including any other related models) but I have no idea if that's possible.

@gmalette @byroot