Improve error handling on update optimistic lock version to nil

If attempting to update the lock version column to nil/NULL
raise a more explicit message (which would otherwise raise a
`NoMethodError` on the attempt to increment).

Author: stevegeek

activerecord/lib/active_record/locking/optimistic.rb

@@ -101,6 +101,13 @@ def _update_row(attribute_names, attempted_action = "update")
             attribute_names = attribute_names.dup if attribute_names.frozen?
             attribute_names << locking_column
 
+            if self[locking_column].nil?
+              raise(<<-MSG.squish)
+                For optimistic locking, '#{locking_column}' should not be set to `nil`/`NULL`.
+                Are you missing a default value or validation on '#{locking_column}'?
+              MSG
+            end
+
             self[locking_column] += 1
 
             affected_rows = self.class._update_record(

activerecord/test/cases/locking_test.rb

@@ -305,6 +305,43 @@ def test_touch_existing_lock_without_default_should_work_with_null_in_the_databa
     assert_equal ["lock_version", "updated_at"], t1.saved_changes.keys.sort
   end
 
+  def test_update_lock_version_to_nil_without_validation_or_constraint_raises_error
+    t1 = LockWithoutDefault.create!(title: "title1")
+    error = assert_raises(RuntimeError) { t1.update(lock_version: nil) }
+
+    assert_match(locking_column_nil_error_message, error.message)
+
+    error = assert_raises(RuntimeError) { t1.update!(lock_version: nil) }
+
+    assert_match(locking_column_nil_error_message, error.message)
+  end
+
+  def test_update_lock_version_to_nil_without_validation_raises
+    person = Person.find(1)
+    error = assert_raises(RuntimeError) { person.update(lock_version: nil) }
+
+    assert_match(locking_column_nil_error_message, error.message)
+
+    error = assert_raises(RuntimeError) { person.update!(lock_version: nil) }
+
+    assert_match(locking_column_nil_error_message, error.message)
+  end
+
+  def test_update_lock_version_to_nil_with_validation_does_not_raise_runtime_lock_version_error
+    person = LockVersionValidatedPerson.find(1)
+    assert_nothing_raised { person.update(lock_version: nil) }
+
+    assert_equal ["is not a number"], person.errors[:lock_version]
+  end
+
+  def test_update_bang_lock_version_to_nil_with_validation_does_not_raise_runtime_lock_version_error
+    error = assert_raises(ActiveRecord::RecordInvalid) do
+      LockVersionValidatedPerson.find(1).update!(lock_version: nil)
+    end
+
+    assert_equal ["is not a number"], error.record.errors[:lock_version]
+  end
+
   def test_touch_stale_object_with_lock_without_default
     t1 = LockWithoutDefault.create!(title: "title1")
     stale_object = LockWithoutDefault.find(t1.id)
@@ -557,6 +594,11 @@ def test_yaml_dumping_with_lock_column
 
     assert_equal t1.attributes, t2.attributes
   end
+
+  private
+    def locking_column_nil_error_message
+      /'lock_version' should not be set to `nil`/
+    end
 end
 
 class OptimisticLockingWithSchemaChangeTest < ActiveRecord::TestCase

activerecord/test/models/person.rb

@@ -148,3 +148,8 @@ class SerializedPerson < ActiveRecord::Base
 
   serialize :insures, coder: Insure
 end
+
+class LockVersionValidatedPerson < ActiveRecord::Base
+  self.table_name = "people"
+  validates :lock_version, numericality: { only_integer: true }, allow_nil: false
+end