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

Recovery of dependent records fails due to validation error #166

Open
thebravoman opened this issue Jun 29, 2020 · 13 comments
Open

Recovery of dependent records fails due to validation error #166

thebravoman opened this issue Jun 29, 2020 · 13 comments
Assignees
Labels

Comments

@thebravoman
Copy link

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
Copy link
Contributor

mvz commented Jun 29, 2020

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
Copy link
Author

thebravoman commented Jun 29, 2020

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
Copy link
Contributor

mvz commented Jun 29, 2020

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

@thebravoman
Copy link
Author

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
Copy link
Contributor

mvz commented Jun 29, 2020

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 #166 (comment) it seems that's not the case. Now your backtrace in #166 (comment) suggests that it does happen in recover?

@thebravoman
Copy link
Author

thebravoman commented Jun 29, 2020

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
Copy link
Contributor

mvz commented Jun 29, 2020

I'm curious why you don't see the error in #166 (comment). Does the error happen with recover! but not with recover ?

@thebravoman
Copy link
Author

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

@mvz
Copy link
Contributor

mvz commented Jun 29, 2020

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.

@mvz mvz changed the title "Globalized model must exist" error Recovery of dependent records fails due to validation error Jun 29, 2020
@mvz mvz added the bug label Jun 29, 2020
@mvz mvz self-assigned this Jun 29, 2020
@thebravoman
Copy link
Author

thebravoman commented Jun 29, 2020

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
Copy link
Author

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
Copy link

Kounts commented Aug 28, 2020

@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
Copy link
Contributor

mvz commented Apr 25, 2021

This is probably fixed by #227.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants