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
This commit is contained in:
commit
14545f46af
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue