Don't quote ID's as Arel will quote them -- follow same conventions as the delete method.

5 files changed, 52 insertions, 2 deletions

diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb
index 9a31675782..cdedcde0eb 100644
--- a/activerecord/lib/active_record/locking/optimistic.rb
+++ b/activerecord/lib/active_record/locking/optimistic.rb
@@ -94,7 +94,7 @@ module ActiveRecord
             relation = self.class.unscoped
 
             stmt = relation.where(
-              relation.table[self.class.primary_key].eq(quoted_id).and(
+              relation.table[self.class.primary_key].eq(id).and(
                 relation.table[lock_col].eq(quote_value(previous_lock_value))
               )
             ).arel.compile_update(arel_attributes_values(false, false, attribute_names))
diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb
index 636a709924..61baa55027 100644
--- a/activerecord/test/cases/locking_test.rb
+++ b/activerecord/test/cases/locking_test.rb
@@ -5,6 +5,7 @@ require 'models/job'
 require 'models/reader'
 require 'models/legacy_thing'
 require 'models/reference'
+require 'models/string_key_object'
 
 class LockWithoutDefault < ActiveRecord::Base; end
 
@@ -18,7 +19,40 @@ class ReadonlyFirstNamePerson < Person
 end
 
 class OptimisticLockingTest < ActiveRecord::TestCase
-  fixtures :people, :legacy_things, :references
+  fixtures :people, :legacy_things, :references, :string_key_objects
+
+  def test_non_integer_lock_existing
+    s1 = StringKeyObject.find("record1")
+    s2 = StringKeyObject.find("record1")
+    assert_equal 0, s1.lock_version
+    assert_equal 0, s2.lock_version
+
+    s1.name = 'updated record'
+    s1.save!
+    assert_equal 1, s1.lock_version
+    assert_equal 0, s2.lock_version
+
+    s2.name = 'doubly updated record'
+    assert_raise(ActiveRecord::StaleObjectError) { s2.save! }
+  end
+
+  def test_non_integer_lock_destroy
+    s1 = StringKeyObject.find("record1")
+    s2 = StringKeyObject.find("record1")
+    assert_equal 0, s1.lock_version
+    assert_equal 0, s2.lock_version
+
+    s1.name = 'updated record'
+    s1.save!
+    assert_equal 1, s1.lock_version
+    assert_equal 0, s2.lock_version
+    assert_raise(ActiveRecord::StaleObjectError) { s2.destroy }
+
+    assert s1.destroy
+    assert s1.frozen?
+    assert s1.destroyed?
+    assert_raises(ActiveRecord::RecordNotFound) { StringKeyObject.find("record1") }
+  end
 
   def test_lock_existing
     p1 = Person.find(1)
diff --git a/activerecord/test/fixtures/string_key_objects.yml b/activerecord/test/fixtures/string_key_objects.yml
new file mode 100644
index 0000000000..fa1299915b
--- /dev/null
+++ b/activerecord/test/fixtures/string_key_objects.yml
@@ -0,0 +1,7 @@
+first:
+  id: record1
+  name: first record
+
+second:
+  id: record2
+  name: second record
diff --git a/activerecord/test/models/string_key_object.rb b/activerecord/test/models/string_key_object.rb
new file mode 100644
index 0000000000..f8d4c6e0e4
--- /dev/null
+++ b/activerecord/test/models/string_key_object.rb
@@ -0,0 +1,3 @@
+class StringKeyObject < ActiveRecord::Base
+  set_primary_key :id
+end
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index 9479242e4f..c20304c0e2 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -543,6 +543,12 @@ ActiveRecord::Schema.define do
     t.string :sponsorable_type
   end
 
+  create_table :string_key_objects, :id => false, :primary_key => :id, :force => true do |t|
+    t.string     :id
+    t.string     :name
+    t.integer    :lock_version, :null => false, :default => 0
+  end
+
   create_table :students, :force => true do |t|
     t.string :name
   end