Commit c1b27a5f authored by Stan Hu's avatar Stan Hu Committed by Alejandro Rodríguez

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
parent 01d77f62
...@@ -52,6 +52,23 @@ module ActiveRecord ...@@ -52,6 +52,23 @@ module ActiveRecord
raise raise
end end
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 end
end end
...@@ -7,15 +7,21 @@ describe Projects::DestroyService, services: true do ...@@ -7,15 +7,21 @@ describe Projects::DestroyService, services: true do
let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") } let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") }
let!(:async) { false } # execute or async_execute 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 context 'Sidekiq inline' do
before do before do
# Run sidekiq immediatly to check that renamed repository will be removed # Run sidekiq immediatly to check that renamed repository will be removed
Sidekiq::Testing.inline! { destroy_project(project, user, {}) } Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
end end
it { expect(Project.all).not_to include(project) } it_behaves_like 'deleting the project'
it { expect(Dir.exist?(path)).to be_falsey }
it { expect(Dir.exist?(remove_path)).to be_falsey }
end end
context 'Sidekiq fake' do context 'Sidekiq fake' do
...@@ -38,13 +44,23 @@ describe Projects::DestroyService, services: true do ...@@ -38,13 +44,23 @@ describe Projects::DestroyService, services: true do
Sidekiq::Testing.inline! { destroy_project(project, user, {}) } Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
end end
it 'deletes the project' do it_behaves_like 'deleting the project'
expect(Project.all).not_to include(project) end
expect(Dir.exist?(path)).to be_falsey
expect(Dir.exist?(remove_path)).to be_falsey 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
end end
it_behaves_like 'deleting the project'
end
context 'container registry' do context 'container registry' do
before do before do
stub_container_registry_config(enabled: true) stub_container_registry_config(enabled: true)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment