From 5ac470b9cd3008cf85ee97fa0f68f7ac63b5cd84 Mon Sep 17 00:00:00 2001 From: Aymeric Le Dorze Date: Wed, 1 Aug 2018 13:57:56 +0200 Subject: [PATCH 1/3] Fix many callback problems such as callbacks not being called or being called too many times. --- lib/acts_as_paranoid/core.rb | 31 +++++++++++ test/test_core.rb | 104 +++++++++++++++++++++++++++-------- 2 files changed, 111 insertions(+), 24 deletions(-) diff --git a/lib/acts_as_paranoid/core.rb b/lib/acts_as_paranoid/core.rb index eaa0038..e315b29 100644 --- a/lib/acts_as_paranoid/core.rb +++ b/lib/acts_as_paranoid/core.rb @@ -4,6 +4,12 @@ module ActsAsParanoid module Core def self.included(base) base.extend ClassMethods + if ActiveRecord::VERSION::MAJOR == 5 + base.send(:alias_method, :committed_without_paranoid!, :committed!) + base.send(:alias_method, :committed!, :committed_with_paranoid!) + base.send(:alias_method, :remember_transaction_record_state_without_paranoid, :remember_transaction_record_state) + base.send(:alias_method, :remember_transaction_record_state, :remember_transaction_record_state_with_paranoid) + end end module ClassMethods @@ -156,6 +162,8 @@ def destroy_fully! decrement_counters_on_associations end + @_trigger_destroy_callback = true + @destroyed = true freeze end @@ -242,6 +250,16 @@ def deleted_fully? alias destroyed_fully? deleted_fully? + if ActiveRecord::VERSION::MAJOR == 5 + def committed_with_paranoid!(should_run_callbacks: true) + committed_without_paranoid!(should_run_callbacks: should_run_callbacks) + ensure + if defined?(@_trigger_destroy_callback) && !@_trigger_destroy_callback.nil? && !@destroyed && deleted? + @_trigger_destroy_callback = nil + end + end + end + private def recover_dependent_associations(deleted_value, options) @@ -327,5 +345,18 @@ def stale_paranoid_value self.paranoid_value = self.class.delete_now_value clear_attribute_changes([self.class.paranoid_column]) end + + if ActiveRecord::VERSION::MAJOR == 5 + def remember_transaction_record_state_with_paranoid + remember_transaction_record_state_without_paranoid + remember_trigger_destroy_callback_before_last_commit + end + + def remember_trigger_destroy_callback_before_last_commit + return unless _committed_already_called && defined?(@_trigger_destroy_callback) && !@_trigger_destroy_callback.nil? && !@destroyed && deleted? + + @_trigger_destroy_callback = nil + end + end end end diff --git a/test/test_core.rb b/test/test_core.rb index 64cd798..68c6558 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 @@ -740,21 +741,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 @@ -763,22 +775,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 a32759dff87caa93fd0edb8075176422343f947b Mon Sep 17 00:00:00 2001 From: Aymeric Le Dorze Date: Wed, 1 Aug 2018 14:29:47 +0200 Subject: [PATCH 2/3] Added a test with active_storage (issue #103) --- .gitignore | 1 + Appraisals | 8 ++ gemfiles/active_record_52.gemfile | 2 + gemfiles/active_record_60.gemfile | 2 + gemfiles/active_record_61.gemfile | 2 + gemfiles/active_record_70.gemfile | 2 + test/fixtures/hello.txt | 1 + test/test_active_storage.rb | 148 ++++++++++++++++++++++++++++++ 8 files changed, 166 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 e917b41..d4f9c90 100644 --- a/Appraisals +++ b/Appraisals @@ -1,12 +1,16 @@ # frozen_string_literal: true appraise "active_record_52" do + gem "activejob", "~> 5.2.0", require: "active_job" gem "activerecord", "~> 5.2.0", require: "active_record" + gem "activestorage", "~> 5.2.0" gem "activesupport", "~> 5.2.0", require: "active_support" end appraise "active_record_60" do + gem "activejob", "~> 6.0.0", require: "active_job" gem "activerecord", "~> 6.0.0", require: "active_record" + gem "activestorage", "~> 6.0.0" gem "activesupport", "~> 6.0.0", require: "active_support" group :development do @@ -15,7 +19,9 @@ appraise "active_record_60" do end 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 @@ -24,7 +30,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 diff --git a/gemfiles/active_record_52.gemfile b/gemfiles/active_record_52.gemfile index c38d68a..bbdfdbd 100644 --- a/gemfiles/active_record_52.gemfile +++ b/gemfiles/active_record_52.gemfile @@ -4,7 +4,9 @@ source "https://rubygems.org" +gem "activejob", "~> 5.2.0", require: "active_job" gem "activerecord", "~> 5.2.0", require: "active_record" +gem "activestorage", "~> 5.2.0" gem "activesupport", "~> 5.2.0", require: "active_support" group :development do diff --git a/gemfiles/active_record_60.gemfile b/gemfiles/active_record_60.gemfile index 1fad559..9f7c75f 100644 --- a/gemfiles/active_record_60.gemfile +++ b/gemfiles/active_record_60.gemfile @@ -4,7 +4,9 @@ source "https://rubygems.org" +gem "activejob", "~> 6.0.0", require: "active_job" gem "activerecord", "~> 6.0.0", require: "active_record" +gem "activestorage", "~> 6.0.0" gem "activesupport", "~> 6.0.0", require: "active_support" group :development do diff --git a/gemfiles/active_record_61.gemfile b/gemfiles/active_record_61.gemfile index cdfd423..297bcd5 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 b52e808..74aaa49 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/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..370d756 --- /dev/null +++ b/test/test_active_storage.rb @@ -0,0 +1,148 @@ +# 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 + +require "active_support/cache" + +require "active_storage" +require "active_storage/attached" +require "active_storage/service/disk_service" +if ActiveRecord::VERSION::MAJOR >= 6 + require "active_storage/reflection" + ActiveRecord::Base.include(ActiveStorage::Reflection::ActiveRecordExtensions) + ActiveRecord::Reflection.singleton_class.prepend(ActiveStorage::Reflection::ReflectionExtension) + ActiveRecord::Base.include(ActiveStorage::Attached::Model) + + if ActiveRecord::VERSION::MAJOR == 6 && ActiveRecord::VERSION::MINOR.zero? + module Rails + def self.autoloaders + Object.new.tap do |o| + def o.zeitwerk_enabled? + false + end + end + end + end + else + 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 + end +else + ActiveRecord::Base.extend(ActiveStorage::Attached::Macros) +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 } + if ActiveRecord::VERSION::MAJOR > 6 || (ActiveRecord::VERSION::MAJOR == 6 && ActiveRecord::VERSION::MINOR >= 1) + args[:service_name] = "test" + end + 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 From 34409312caa6a4d4b5c656b353f1fbbd9f3ea72b Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 26 Dec 2022 05:05:21 +0000 Subject: [PATCH 3/3] Update simplecov requirement from ~> 0.21.2 to ~> 0.22.0 Updates the requirements on [simplecov](https://github.com/simplecov-ruby/simplecov) to permit the latest version. - [Release notes](https://github.com/simplecov-ruby/simplecov/releases) - [Changelog](https://github.com/simplecov-ruby/simplecov/blob/main/CHANGELOG.md) - [Commits](https://github.com/simplecov-ruby/simplecov/compare/v0.21.2...v0.22.0) --- updated-dependencies: - dependency-name: simplecov dependency-type: direct:development ... Signed-off-by: dependabot[bot] --- acts_as_paranoid.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acts_as_paranoid.gemspec b/acts_as_paranoid.gemspec index 78a58d0..1931334 100644 --- a/acts_as_paranoid.gemspec +++ b/acts_as_paranoid.gemspec @@ -32,5 +32,5 @@ Gem::Specification.new do |spec| spec.add_development_dependency "rdoc", "~> 6.3" spec.add_development_dependency "rubocop", "~> 1.25" spec.add_development_dependency "rubocop-minitest", "~> 0.19.0" - spec.add_development_dependency "simplecov", "~> 0.21.2" + spec.add_development_dependency "simplecov", "~> 0.22.0" end