From a2a6a83a21e30892f6be81d13af633607007d1d1 Mon Sep 17 00:00:00 2001 From: Aymeric Le Dorze Date: Wed, 1 Aug 2018 13:57:56 +0200 Subject: [PATCH 1/2] Fix many callback problems such as callbacks not being called or being called too many times. --- lib/acts_as_paranoid/core.rb | 2 + test/test_core.rb | 104 +++++++++++++++++++++++++++-------- 2 files changed, 82 insertions(+), 24 deletions(-) diff --git a/lib/acts_as_paranoid/core.rb b/lib/acts_as_paranoid/core.rb index a932fd0..87838e9 100644 --- a/lib/acts_as_paranoid/core.rb +++ b/lib/acts_as_paranoid/core.rb @@ -158,6 +158,8 @@ def destroy_fully! decrement_counters_on_associations end + @_trigger_destroy_callback = true + @destroyed = true freeze end diff --git a/test/test_core.rb b/test/test_core.rb index c041245..60f72c7 100644 --- a/test/test_core.rb +++ b/test/test_core.rb @@ -123,31 +123,32 @@ class ParanoidWithCallback < ActiveRecord::Base before_recover :call_me_before_recover after_recover :call_me_after_recover - def initialize(*attrs) - @called_before_destroy = false - @called_after_destroy = false - @called_after_commit_on_destroy = false - super(*attrs) - end + set_callback :initialize, lambda { + @called_before_destroy = 0 + @called_after_destroy = 0 + @called_after_commit_on_destroy = 0 + @called_before_recover = 0 + @called_after_recover = 0 + } def call_me_before_destroy - @called_before_destroy = true + @called_before_destroy += 1 end def call_me_after_destroy - @called_after_destroy = true + @called_after_destroy += 1 end def call_me_after_commit_on_destroy - @called_after_commit_on_destroy = true + @called_after_commit_on_destroy += 1 end def call_me_before_recover - @called_before_recover = true + @called_before_recover += 1 end def call_me_after_recover - @called_after_recover = true + @called_after_recover += 1 end end @@ -779,21 +780,32 @@ def test_paranoid_destroy_callbacks @paranoid_with_callback.destroy end - assert @paranoid_with_callback.called_before_destroy - assert @paranoid_with_callback.called_after_destroy - assert @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy + end + + def test_paranoid_destroy_destroy_callbacks + @paranoid_with_callback = ParanoidWithCallback.first + ParanoidWithCallback.transaction do + @paranoid_with_callback.destroy.destroy + end + + assert_equal 2, @paranoid_with_callback.called_before_destroy + assert_equal 2, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy end def test_hard_destroy_callbacks @paranoid_with_callback = ParanoidWithCallback.first ParanoidWithCallback.transaction do - @paranoid_with_callback.destroy! + @paranoid_with_callback.destroy_fully! end - assert @paranoid_with_callback.called_before_destroy - assert @paranoid_with_callback.called_after_destroy - assert @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy end def test_recovery_callbacks @@ -802,22 +814,66 @@ def test_recovery_callbacks ParanoidWithCallback.transaction do @paranoid_with_callback.destroy - assert_nil @paranoid_with_callback.called_before_recover - assert_nil @paranoid_with_callback.called_after_recover + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 0, @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 0, @paranoid_with_callback.called_before_recover + assert_equal 0, @paranoid_with_callback.called_after_recover @paranoid_with_callback.recover end - assert @paranoid_with_callback.called_before_recover - assert @paranoid_with_callback.called_after_recover + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_includes 0..1, @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 1, @paranoid_with_callback.called_before_recover + assert_equal 1, @paranoid_with_callback.called_after_recover + end + + def test_recovery_callbacks_with_2_transactions + @paranoid_with_callback = ParanoidWithCallback.first + + ParanoidWithCallback.transaction do + @paranoid_with_callback.destroy + end + + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 0, @paranoid_with_callback.called_before_recover + assert_equal 0, @paranoid_with_callback.called_after_recover + + ParanoidWithCallback.transaction do + @paranoid_with_callback.recover + end + + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 1, @paranoid_with_callback.called_before_recover + assert_equal 1, @paranoid_with_callback.called_after_recover + end + + def test_paranoid_destroy_with_update_callbacks + @paranoid_with_callback = ParanoidWithCallback.first + ParanoidWithCallback.transaction do + @paranoid_with_callback.destroy + end + ParanoidWithCallback.transaction do + @paranoid_with_callback.update(name: "still paranoid") + end + + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy end def test_recovery_callbacks_without_destroy @paranoid_with_callback = ParanoidWithCallback.first @paranoid_with_callback.recover - assert_nil @paranoid_with_callback.called_before_recover - assert_nil @paranoid_with_callback.called_after_recover + assert_equal 0, @paranoid_with_callback.called_before_recover + assert_equal 0, @paranoid_with_callback.called_after_recover end def test_delete_by_multiple_id_is_paranoid From 735fef5f9da721528ac71ea3923c4ed9e14b8b43 Mon Sep 17 00:00:00 2001 From: Aymeric Le Dorze Date: Wed, 1 Aug 2018 14:29:47 +0200 Subject: [PATCH 2/2] Added a test with active_storage (issue #103) --- .gitignore | 1 + Appraisals | 6 ++ gemfiles/active_record_61.gemfile | 2 + gemfiles/active_record_70.gemfile | 2 + gemfiles/active_record_71.gemfile | 2 + test/fixtures/hello.txt | 1 + test/test_active_storage.rb | 130 ++++++++++++++++++++++++++++++ 7 files changed, 144 insertions(+) create mode 100644 test/fixtures/hello.txt create mode 100644 test/test_active_storage.rb diff --git a/.gitignore b/.gitignore index 3743258..93305a5 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ gemfiles/*.lock .ruby-version coverage/ log/ +test/tmp diff --git a/Appraisals b/Appraisals index 6b0c351..d9368cb 100644 --- a/Appraisals +++ b/Appraisals @@ -1,7 +1,9 @@ # frozen_string_literal: true appraise "active_record_61" do + gem "activejob", "~> 6.1.0", require: "active_job" gem "activerecord", "~> 6.1.0", require: "active_record" + gem "activestorage", "~> 6.1.0" gem "activesupport", "~> 6.1.0", require: "active_support" group :development do @@ -10,7 +12,9 @@ appraise "active_record_61" do end appraise "active_record_70" do + gem "activejob", "~> 7.0.0", require: "active_job" gem "activerecord", "~> 7.0.0", require: "active_record" + gem "activestorage", "~> 7.0.0" gem "activesupport", "~> 7.0.0", require: "active_support" group :development do @@ -19,7 +23,9 @@ appraise "active_record_70" do end appraise "active_record_71" do + gem "activejob", "~> 7.1.0", require: "active_job" gem "activerecord", "~> 7.1.0", require: "active_record" + gem "activestorage", "~> 7.1.0" gem "activesupport", "~> 7.1.0", require: "active_support" group :development do diff --git a/gemfiles/active_record_61.gemfile b/gemfiles/active_record_61.gemfile index 2360820..90651ce 100644 --- a/gemfiles/active_record_61.gemfile +++ b/gemfiles/active_record_61.gemfile @@ -4,7 +4,9 @@ source "https://rubygems.org" +gem "activejob", "~> 6.1.0", require: "active_job" gem "activerecord", "~> 6.1.0", require: "active_record" +gem "activestorage", "~> 6.1.0" gem "activesupport", "~> 6.1.0", require: "active_support" group :development do diff --git a/gemfiles/active_record_70.gemfile b/gemfiles/active_record_70.gemfile index a34c913..99f4a3c 100644 --- a/gemfiles/active_record_70.gemfile +++ b/gemfiles/active_record_70.gemfile @@ -4,7 +4,9 @@ source "https://rubygems.org" +gem "activejob", "~> 7.0.0", require: "active_job" gem "activerecord", "~> 7.0.0", require: "active_record" +gem "activestorage", "~> 7.0.0" gem "activesupport", "~> 7.0.0", require: "active_support" group :development do diff --git a/gemfiles/active_record_71.gemfile b/gemfiles/active_record_71.gemfile index e55e308..a5984b0 100644 --- a/gemfiles/active_record_71.gemfile +++ b/gemfiles/active_record_71.gemfile @@ -4,7 +4,9 @@ source "https://rubygems.org" +gem "activejob", "~> 7.1.0", require: "active_job" gem "activerecord", "~> 7.1.0", require: "active_record" +gem "activestorage", "~> 7.1.0" gem "activesupport", "~> 7.1.0", require: "active_support" group :development do diff --git a/test/fixtures/hello.txt b/test/fixtures/hello.txt new file mode 100644 index 0000000..5dd01c1 --- /dev/null +++ b/test/fixtures/hello.txt @@ -0,0 +1 @@ +Hello, world! \ No newline at end of file diff --git a/test/test_active_storage.rb b/test/test_active_storage.rb new file mode 100644 index 0000000..8c8cb40 --- /dev/null +++ b/test/test_active_storage.rb @@ -0,0 +1,130 @@ +# frozen_string_literal: true + +require "test_helper" + +# load ActiveStorage +require "global_id" +ActiveRecord::Base.include(GlobalID::Identification) +GlobalID.app = "ActsAsParanoid" + +require "active_job" +ActiveJob::Base.queue_adapter = :test +ActiveJob::Base.logger = Logger.new(File::NULL) + +require "active_support/cache" + +require "active_storage" +require "active_storage/attached" +require "active_storage/service/disk_service" +require "active_storage/reflection" +ActiveRecord::Base.include(ActiveStorage::Reflection::ActiveRecordExtensions) +ActiveRecord::Reflection.singleton_class.prepend(ActiveStorage::Reflection::ReflectionExtension) +ActiveRecord::Base.include(ActiveStorage::Attached::Model) + +require "#{Gem.loaded_specs['activestorage'].full_gem_path}/app/models/active_storage/record" +module ActiveStorage + class Blob < Record + end +end +Dir.glob("#{Gem.loaded_specs['activestorage'].full_gem_path}/app/models/active_storage/blob/*").sort.each do |f| + require f +end +$LOAD_PATH << "#{Gem.loaded_specs['activestorage'].full_gem_path}/app/models/" +Dir.glob("#{Gem.loaded_specs['activestorage'].full_gem_path}/app/models/active_storage/*").sort.each do |f| + require f +end +if ActiveStorage::Blob.respond_to?(:services=) + require "active_storage/service/disk_service" + ActiveStorage::Blob.services = { + "test" => ActiveStorage::Service::DiskService.build(name: "test", configurator: nil, root: "test/tmp") + } +end +if File.exist?("#{Gem.loaded_specs['activestorage'].full_gem_path}/app/jobs/active_storage/base_job.rb") + require "#{Gem.loaded_specs['activestorage'].full_gem_path}/app/jobs/active_storage/base_job" +end +Dir.glob("#{Gem.loaded_specs['activestorage'].full_gem_path}/app/jobs/active_storage/*").sort.each do |f| + require f +end +ActiveStorage::Blob.service = ActiveStorage::Service::DiskService.new(root: "test/tmp") + +class ParanoidActiveStorageTest < ActiveSupport::TestCase + self.file_fixture_path = "test/fixtures" + + class ParanoidActiveStorage < ActiveRecord::Base + acts_as_paranoid + + has_one_attached :main_file + has_many_attached :files + has_one_attached :undependent_main_file, dependent: false + has_many_attached :undependent_files, dependent: false + end + + def setup_db + ActiveRecord::Schema.define(version: 1) do + create_table :active_storage_blobs do |t| + t.string :key, null: false + t.string :filename, null: false + t.string :content_type + t.text :metadata + t.string :service_name + t.bigint :byte_size, null: false + t.string :checksum, null: false + t.datetime :created_at, null: false + t.index [:key], name: "index_active_storage_blobs_on_key", unique: true + end + + create_table :active_storage_attachments do |t| + t.string :name, null: false + t.references :record, null: false, polymorphic: true, index: false + t.references :blob, null: false + t.datetime :created_at, null: false + t.index [:record_type, :record_id, :name, :blob_id], name: "index_active_storage_attachments_uniqueness", unique: true + end + + create_table :active_storage_variant_records do |t| + t.belongs_to :blob, null: false, index: false + t.string :variation_digest, null: false + t.index [:blob_id, :variation_digest], name: "index_active_storage_variant_records_uniqueness", unique: true + end + + create_table :paranoid_active_storages do |t| + t.datetime :deleted_at + timestamps t + end + end + end + + def clean_active_storage_attachments + Dir.glob("test/tmp/*").each do |f| + FileUtils.rm_r(f) + end + end + + def create_file_blob(filename: "hello.txt", content_type: "text/plain", metadata: nil) + args = { io: file_fixture(filename).open, filename: filename, content_type: content_type, metadata: metadata, service_name: "test" } + if ActiveStorage::Blob.respond_to?(:create_and_upload!) + ActiveStorage::Blob.create_and_upload!(**args) + else + ActiveStorage::Blob.create_after_upload!(**args) + end + end + + def setup + setup_db + end + + def teardown + super + clean_active_storage_attachments + end + + def test_paranoid_active_storage + pt = ParanoidActiveStorage.create({ + main_file: create_file_blob, + files: [create_file_blob, create_file_blob], + undependent_main_file: create_file_blob, + undependent_files: [create_file_blob, create_file_blob] + }) + pt.destroy + end +end