Merge branch 'fix-optimistic-locking-for-destroy' into 'master'

Make deleting with optimistic locking respect NULL

Make deleting with optimistic locking respect NULL

For now deleting with optimistic locking is broken when
lock_version is still NULL, because Rails would try to
delete with `lock_version = 0` while in the database
the column is still `NULL`.

The monkey patches would force Rails just pass whatever
in the column, and stop Rails from casting `NULL` into `0`
when the value is read from database.

Closes #24766

See merge request !7867

2 files changed, 40 insertions, 7 deletions

diff --git a/config/initializers/ar_monkey_patch.rb b/config/initializers/ar_monkey_patch.rb
index 0da584626ee..6979f4641b0 100644
--- a/config/initializers/ar_monkey_patch.rb
+++ b/config/initializers/ar_monkey_patch.rb
@@ -52,6 +52,23 @@ module ActiveRecord
           raise
         end
       end
+
+      # This is patched because we need it to query `lock_version IS NULL`
+      # rather than `lock_version = 0` whenever lock_version is NULL.
+      def relation_for_destroy
+        return super unless locking_enabled?
+
+        column_name = self.class.locking_column
+        super.where(self.class.arel_table[column_name].eq(self[column_name]))
+      end
+    end
+
+    # This is patched because we want `lock_version` default to `NULL`
+    # rather than `0`
+    class LockingType < SimpleDelegator
+      def type_cast_from_database(value)
+        super
+      end
     end
   end
 end
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index 7dcd03496bb..90771825f5c 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -7,15 +7,21 @@ describe Projects::DestroyService, services: true do
   let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") }
   let!(:async) { false } # execute or async_execute
 
+  shared_examples 'deleting the project' do
+    it 'deletes the project' do
+      expect(Project.all).not_to include(project)
+      expect(Dir.exist?(path)).to be_falsey
+      expect(Dir.exist?(remove_path)).to be_falsey
+    end
+  end
+
   context 'Sidekiq inline' do
     before do
       # Run sidekiq immediatly to check that renamed repository will be removed
       Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
     end
 
-    it { expect(Project.all).not_to include(project) }
-    it { expect(Dir.exist?(path)).to be_falsey }
-    it { expect(Dir.exist?(remove_path)).to be_falsey }
+    it_behaves_like 'deleting the project'
   end
 
   context 'Sidekiq fake' do
@@ -38,11 +44,21 @@ describe Projects::DestroyService, services: true do
       Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
     end
 
-    it 'deletes the project' do
-      expect(Project.all).not_to include(project)
-      expect(Dir.exist?(path)).to be_falsey
-      expect(Dir.exist?(remove_path)).to be_falsey
+    it_behaves_like 'deleting the project'
+  end
+
+  context 'delete with pipeline' do # which has optimistic locking
+    let!(:pipeline) { create(:ci_pipeline, project: project) }
+
+    before do
+      expect(project).to receive(:destroy!).and_call_original
+
+      perform_enqueued_jobs do
+        destroy_project(project, user, {})
+      end
     end
+
+    it_behaves_like 'deleting the project'
   end
 
   context 'container registry' do