Merge branch 'dm-lock-version-null' into 'master'

Support both 0 and NULL lock_versions

See merge request gitlab-org/gitlab-ce!24050

4 files changed, 55 insertions, 10 deletions

diff --git a/config/initializers/active_record_avoid_type_casting_in_uniqueness_validator.rb b/config/initializers/active_record_avoid_type_casting_in_uniqueness_validator.rb
index 3e765469995..228ced32188 100644
--- a/config/initializers/active_record_avoid_type_casting_in_uniqueness_validator.rb
+++ b/config/initializers/active_record_avoid_type_casting_in_uniqueness_validator.rb
@@ -20,7 +20,7 @@
 #
 # This bug was fixed in Rails 5.1 by https://github.com/rails/rails/pull/24745/commits/aa062318c451512035c10898a1af95943b1a3803
 
-if Rails.version.start_with?("5.1")
+if Rails.gem_version >= Gem::Version.new("5.1")
   raise "Remove this monkey patch: #{__FILE__}"
 end
 
diff --git a/config/initializers/active_record_locking.rb b/config/initializers/active_record_locking.rb
index bfe41e6029a..1bd1a12e4b7 100644
--- a/config/initializers/active_record_locking.rb
+++ b/config/initializers/active_record_locking.rb
@@ -1,7 +1,10 @@
 # rubocop:disable Lint/RescueException
 
-# Remove this monkey-patch when all lock_version values are converted from NULLs to zeros.
-# See https://gitlab.com/gitlab-org/gitlab-ce/issues/25228
+# Remove this monkey patch when we move to Rails 5.1, because the bug has been fixed in https://github.com/rails/rails/pull/26050.
+if Rails.gem_version >= Gem::Version.new("5.1")
+  raise "Remove this monkey patch: #{__FILE__}"
+end
+
 module ActiveRecord
   module Locking
     module Optimistic
@@ -16,12 +19,7 @@ module ActiveRecord
         return 0 if attribute_names.empty?
 
         lock_col = self.class.locking_column
-
         previous_lock_value = send(lock_col).to_i
-
-        # This line is added as a patch
-        previous_lock_value = nil if previous_lock_value == '0' || previous_lock_value == 0
-
         increment_lock
 
         attribute_names += [lock_col]
@@ -32,7 +30,8 @@ module ActiveRecord
 
           affected_rows = relation.where(
             self.class.primary_key => id,
-            lock_col => previous_lock_value
+            # Patched because when `lock_version` is read as `0`, it may actually be `NULL` in the DB.
+            lock_col => previous_lock_value == 0 ? [nil, 0] : previous_lock_value
           ).update_all(
             attributes_for_update(attribute_names).map do |name|
               [name, _read_attribute(name)]
diff --git a/spec/initializers/active_record_locking_spec.rb b/spec/initializers/active_record_locking_spec.rb
new file mode 100644
index 00000000000..5a16aef78e6
--- /dev/null
+++ b/spec/initializers/active_record_locking_spec.rb
@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'ActiveRecord locking' do
+  let(:issue) { create(:issue) }
+
+  shared_examples 'locked model' do
+    before do
+      issue.update_column(:lock_version, start_lock_version)
+    end
+
+    it 'can be updated' do
+      issue.update(title: "New title")
+
+      expect(issue.reload.lock_version).to eq(new_lock_version)
+    end
+
+    it 'can be deleted' do
+      expect { issue.destroy }.to change { Issue.count }.by(-1)
+    end
+  end
+
+  context 'when lock_version is NULL' do
+    let(:start_lock_version) { nil }
+    let(:new_lock_version) { 1 }
+
+    it_behaves_like 'locked model'
+  end
+
+  context 'when lock_version is 0' do
+    let(:start_lock_version) { 0 }
+    let(:new_lock_version) { 1 }
+
+    it_behaves_like 'locked model'
+  end
+
+  context 'when lock_version is 1' do
+    let(:start_lock_version) { 1 }
+    let(:new_lock_version) { 2 }
+
+    it_behaves_like 'locked model'
+  end
+end
diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb
index 56e2a405bcd..9d65ac15213 100644
--- a/spec/services/ci/register_job_service_spec.rb
+++ b/spec/services/ci/register_job_service_spec.rb
@@ -244,7 +244,9 @@ module Ci
 
       context 'when first build is stalled' do
         before do
-          pending_job.update(lock_version: 0)
+          allow_any_instance_of(Ci::RegisterJobService).to receive(:assign_runner!).and_call_original
+          allow_any_instance_of(Ci::RegisterJobService).to receive(:assign_runner!)
+            .with(pending_job, anything).and_raise(ActiveRecord::StaleObjectError)
         end
 
         subject { described_class.new(specific_runner).execute }