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
This commit is contained in:
Lin Jen-Shin 2016-12-01 16:17:20 +08:00
parent bbcce5f7dc
commit 5747b0d3ed
2 changed files with 41 additions and 7 deletions

View file

@ -52,6 +52,24 @@ 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
table_name = self.class.quoted_table_name
super.where("#{table_name}.#{column_name}" => 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

View file

@ -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