ActsAsParanoid / acts_as_paranoid

ActiveRecord plugin allowing you to hide and restore records without actually deleting them.
MIT License
1.47k stars 196 forks source link

Recovery of dependent records fails due to validation error #166

Open thebravoman opened 4 years ago

thebravoman commented 4 years ago

Using Globalized

I have

class Episode < ApplicationRecord
 translates :title

This translated from globalized to

# https://github.com/globalize/globalize/blob/0bfe9470168c19c718e3815881ea73cdbf3ccbeb/lib/globalize/active_record/act_macro.rb
  has_many :translations, :class_name  => translation_class.name,
                                :foreign_key => options[:foreign_key],
                                :dependent   => :destroy,
                                :extend      => HasManyExtensions,
                                :autosave    => options[:autosave],
                                :inverse_of  => :globalized_model

So we have

class Episode < ApplicationRecord
   has_many :translations, :class_name  => translation_class.name,
                                :foreign_key => options[:foreign_key],
                                :dependent   => :destroy,
                                :extend      => HasManyExtensions,
                                :autosave    => options[:autosave],
                                :inverse_of  => :globalized_model

I've added a deleted_at column on Episode::Translations. I've filled it with

class Episode < ApplicationRecord
  acts_as_paranoid
  translates :title

  translation_class.class_eval do
    acts_as_paranoid
  end
end

But when I try to recover an error is thrown "Globalized model must exist". This error is because Episode::Translation expects the record it is attached to it. But it is not because it is not recovered yet and somehow the translation class - Episode::Translation is not acting as a paranoid.

mvz commented 4 years ago

You're saying 'Episode::Translation is not acting as a paranoid'. Do you mean the record gets fully deleted? Can you check whether the translation record exists in the database with deleted_at set?

You may need to recover the translation record first before recovering the main record.

thebravoman commented 4 years ago

When it is deleted it is correctly marked as deleted by setting the deleted_at.

2.6.5 :064 > e.destroy
   (0.1ms)  begin transaction
  Episode::Translation Update (0.4ms)  UPDATE "episode_translations" SET "deleted_at" = ?, "updated_at" = ? WHERE "episode_translations"."id" = ?  [["deleted_at", "2020-06-29 06:18:48.368328"], ["updated_at", "2020-06-29 06:18:48.368712"], ["id", 10]]
  Episode Update All (0.3ms)  UPDATE "episodes" SET deleted_at = '2020-06-29 06:18:48.370711' WHERE "episodes"."deleted_at" IS NULL AND "episodes"."id" = ?  [["id", 10]]
   (11.6ms)  commit transaction

But when it is recovered only the episode is recovered

2.6.5 :065 > e.recover
   (0.1ms)  begin transaction
  Episode Update (0.4ms)  UPDATE "episodes" SET "updated_at" = ?, "deleted_at" = ? WHERE "episodes"."id" = ?  [["updated_at", "2020-06-29 06:18:53.709338"], ["deleted_at", nil], ["id", 10]]
   (13.8ms)  commit transaction
 => true 

Update 1: I am expecting, incorrectly I assumen, to have recover to work as destroy. If destroy is working on the associations, shouldn't recover also work on the associations and recover them?

mvz commented 4 years ago

When do you get the 'Globalized model must exist' error?

thebravoman commented 4 years ago

Does this help:

    ActiveRecord::RecordInvalid:
       Validation failed: Globalized model must exist
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activerecord-6.0.3.2/lib/active_record/validations.rb:80:in `raise_validation_error'
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activerecord-6.0.3.2/lib/active_record/validations.rb:53:in `save!'
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activerecord-6.0.3.2/lib/active_record/transactions.rb:318:in `block in save!'
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activerecord-6.0.3.2/lib/active_record/transactions.rb:375:in `block in with_transaction_returning_status'
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activerecord-6.0.3.2/lib/active_record/connection_adapters/abstract/database_statements.rb:278:in `transaction'
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activerecord-6.0.3.2/lib/active_record/transactions.rb:212:in `transaction'
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activerecord-6.0.3.2/lib/active_record/transactions.rb:366:in `with_transaction_returning_status'
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activerecord-6.0.3.2/lib/active_record/transactions.rb:318:in `save!'
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activerecord-6.0.3.2/lib/active_record/suppressor.rb:48:in `save!'
     # /home/user/projects/acts_as_paranoid/lib/acts_as_paranoid/core.rb:197:in `block (2 levels) in recover'
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activesupport-6.0.3.2/lib/active_support/callbacks.rb:101:in `run_callbacks'
     # /home/user/projects/acts_as_paranoid/lib/acts_as_paranoid/core.rb:188:in `block in recover'
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activerecord-6.0.3.2/lib/active_record/connection_adapters/abstract/database_statements.rb:278:in `transaction'
     # /home/user/.rvm/gems/ruby-2.6.5/gems/activerecord-6.0.3.2/lib/active_record/transactions.rb:212:in `transaction'
mvz commented 4 years ago

No, I need to know what method leads to that error. From your first description I thought it happened when you call recover, but from your comment https://github.com/ActsAsParanoid/acts_as_paranoid/issues/166#issuecomment-650954536 it seems that's not the case. Now your backtrace in https://github.com/ActsAsParanoid/acts_as_paranoid/issues/166#issuecomment-650966424 suggests that it does happen in recover?

thebravoman commented 4 years ago

Here it is the test case

it "#destroy" do
  instance = FactoryBot.create(:episode)
  instance.destroy!

  expect(Episode.exists?(instance.id)).to eq false
  instance.reload
  deleted_instance = Episode.with_deleted.find(instance.id)
  instance.recover!
end

Update 1 I mentioned in the original post that "But when I try to recover an error is thrown "Globalized model must exist". " Sorry if it was not clear. Thanks for the help

mvz commented 4 years ago

I'm curious why you don't see the error in https://github.com/ActsAsParanoid/acts_as_paranoid/issues/166#issuecomment-650954536. Does the error happen with recover! but not with recover ?

thebravoman commented 4 years ago

Yes, You are right. I don't see the error on .recover. Only on recover! (I don't know why).

mvz commented 4 years ago

Ok, then I think I see what happens: ActsAsParanoid tries to recover your translations but their validation fails because they are recovered before the main object. When you just call recover this means the records are just not saved and the code proceeds to save the main record. However, when you call recover!, the validation failure leads to the exception you see.

In this case, it seems, the main record needs to be recovered first. I wonder if that should be made the default.

thebravoman commented 4 years ago

Just to make the example more full - recover is also behaving differently for the rest of the associations. I have two more associations - content_to_content_pictures, and subscriptions accesses.

By using .recover they get recovered like

ContentToContentPicture Update (0.4ms)  UPDATE "content_to_content_pictures" SET "deleted_at" = $1, "updated_at" = $2 WHERE "content_to_content_pictures"."id" = $3  [["deleted_at", nil], ["updated_at", "2020-06-29 07:12:52.929881"], ["id", 758]]

SubscriptionAccess Update (0.6ms)  UPDATE "subscription_accesses" SET "deleted_at" = $1, "updated_at" = $2 WHERE "subscription_accesses"."id" = $3  [["deleted_at", nil], ["updated_at", "2020-06-29 07:13:01.545088"], ["id", 3363]]

But there is no query for episode_translations.

At the end as there are no translations of the episode and I have a validation that it's title can't be blank, I get a "Title can't be blank" error on the episode as the translations are not recovered.

thebravoman commented 4 years ago

This is how I managed to workaround it for the moment. Posting for reference if anybody is searching

module TranslatableSoftdeletable
  extend ActiveSupport::Concern

  included do

    translation_class.class_eval do
      acts_as_paranoid
    end

    before_recover :restore_translations

  end

  def restore_translations
    translations.with_deleted.each do |translation|
      translation.deleted_at = nil
      translation.save(validate: false)
    end
    self.translations.reload
  end

end

and the spec that is working it

require 'rails_helper'

RSpec.describe "TranslatableSoftdeletable", type: :model do

  it "destroys and recovers translations" do 
    instance = FactoryBot.create(:episode)
    instance.destroy
    expect(Episode.exists?(instance.id)).to eq false
    expect(instance.translations.only_deleted.count).to eq 1

    instance.recover
    expect(instance.errors.to_a).to eq []
    instance.reload
    expect(Episode.exists?(instance.id)).to eq true
    expect(instance.translations.count).to eq 1
  end

end
Kounts commented 4 years ago

@thebravoman , thanks for you workaround. We've got the exact same issue : our object Pool has many PoolCountry. A destroyed PoolCountry can be recovered just fine but if I destroy the Pool and wants to recover it, I've got the error in recover_dependent_associations . @alphafoxtrott

mvz commented 3 years ago

This is probably fixed by #227.