From 39b5bfe2394d0a4d479b41ee8d170e0f6c65fd59 Mon Sep 17 00:00:00 2001 From: Alfred Wong Date: Sun, 17 Jun 2012 11:43:31 -0400 Subject: [PATCH 1/3] Specified column type for quote_value When calling quote_value the underlying connection sometimes requires more information about the column to properly return the correct quoted value. I ran into this issue when using optimistic locking in JRuby and the activerecord-jdbcmssql-adapter. In SQLSever 2000, we aren't allowed to insert a integer into a NVARCHAR column type so we need to format it as N'3' if we want to insert into the NVARCHAR type. Unfortuantely, without the column type being passed the connection adapter cannot properly return the correct quote value because it doesn't know to return N'3' or '3'. This patch is fairly straight forward where it just passes in the column type into the quote_value, as it already has the ability to take in the column, so it can properly handle at the connection level. I've added the tests required to make sure that the quote_value method is being passed the column type so that the underlying connection can determine how to quote the value. --- activerecord/CHANGELOG.md | 7 +++++++ .../lib/active_record/locking/optimistic.rb | 2 +- activerecord/test/cases/locking_test.rb | 13 +++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 7e9ec7ad94..8ab9a64244 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,10 @@ +* When using optimisitc locking, `update` whas not passing the column type to `quote_value` + to allow the connection adapter to properly determine how to quote the value. This was + affecting certain databases that use specific colmn types. + Fix #6763 + + *Alfred Wong* + * rescue from all exceptions in `ConnectionManagement#call` Fixes #11497 diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb index 2a7996c4e7..a1501ee453 100644 --- a/activerecord/lib/active_record/locking/optimistic.rb +++ b/activerecord/lib/active_record/locking/optimistic.rb @@ -82,7 +82,7 @@ module ActiveRecord stmt = relation.where( relation.table[self.class.primary_key].eq(id).and( - relation.table[lock_col].eq(self.class.quote_value(previous_lock_value)) + relation.table[lock_col].eq(self.class.quote_value(previous_lock_value, self.class.columns_hash[lock_col])) ) ).arel.compile_update(arel_attributes_with_values_for_update(attribute_names)) diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb index db7d3b80ab..4a72b69848 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -1,4 +1,5 @@ require 'thread' +require 'mocha/setup' require "cases/helper" require 'models/person' require 'models/job' @@ -28,6 +29,18 @@ end class OptimisticLockingTest < ActiveRecord::TestCase fixtures :people, :legacy_things, :references, :string_key_objects, :peoples_treasures + def test_quote_value_passed_lock_col + p1 = Person.find(1) + assert_equal 0, p1.lock_version + + Person.expects(:quote_value).with(0, Person.columns_hash[Person.locking_column]).returns('0').once + + p1.first_name = 'anika2' + p1.save! + + assert_equal 1, p1.lock_version + end + def test_non_integer_lock_existing s1 = StringKeyObject.find("record1") s2 = StringKeyObject.find("record1") From c083dc22dd16c3c2e43bbe1e13e4ee210c2adbc1 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Sat, 20 Jul 2013 20:15:48 -0700 Subject: [PATCH 2/3] Tidy up the "Specified column type for quote_value" changes This includes fixing typos in changelog, removing a deprecated mocha/setup test require, and preferring the `column_for_attribute` accessor over direct access to the columns_hash in the new code. --- activerecord/CHANGELOG.md | 5 +++-- activerecord/lib/active_record/locking/optimistic.rb | 2 +- activerecord/test/cases/locking_test.rb | 1 - 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 8ab9a64244..76174dc398 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,7 +1,8 @@ -* When using optimisitc locking, `update` whas not passing the column type to `quote_value` +* When using optimistic locking, `update` was not passing the column to `quote_value` to allow the connection adapter to properly determine how to quote the value. This was affecting certain databases that use specific colmn types. - Fix #6763 + + Fixes: #6763 *Alfred Wong* diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb index a1501ee453..626fe40103 100644 --- a/activerecord/lib/active_record/locking/optimistic.rb +++ b/activerecord/lib/active_record/locking/optimistic.rb @@ -82,7 +82,7 @@ module ActiveRecord stmt = relation.where( relation.table[self.class.primary_key].eq(id).and( - relation.table[lock_col].eq(self.class.quote_value(previous_lock_value, self.class.columns_hash[lock_col])) + relation.table[lock_col].eq(self.class.quote_value(previous_lock_value, column_for_attribute(lock_col))) ) ).arel.compile_update(arel_attributes_with_values_for_update(attribute_names)) diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb index 4a72b69848..dfa12cb97c 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -1,5 +1,4 @@ require 'thread' -require 'mocha/setup' require "cases/helper" require 'models/person' require 'models/job' From 31a43ebc107fbd50e7e62567e5208a05909ec76c Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Sat, 20 Jul 2013 20:30:35 -0700 Subject: [PATCH 3/3] Don't allow `quote_value` to be called without a column Some adapters require column information to do their job properly. By enforcing the provision of the column for this internal method we ensure that those using adapters that require column information will always get the proper behavior. --- activerecord/CHANGELOG.md | 9 +++++++++ activerecord/lib/active_record/sanitization.rb | 4 ++-- activerecord/test/cases/finder_test.rb | 2 +- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 76174dc398..61dba12b64 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,12 @@ +* Don't allow `quote_value` to be called without a column. + + Some adapters require column information to do their job properly. + By enforcing the provision of the column for this internal method + we ensure that those using adapters that require column information + will always get the proper behavior. + + *Ben Woosley* + * When using optimistic locking, `update` was not passing the column to `quote_value` to allow the connection adapter to properly determine how to quote the value. This was affecting certain databases that use specific colmn types. diff --git a/activerecord/lib/active_record/sanitization.rb b/activerecord/lib/active_record/sanitization.rb index 31e294022f..0b87ab9926 100644 --- a/activerecord/lib/active_record/sanitization.rb +++ b/activerecord/lib/active_record/sanitization.rb @@ -3,8 +3,8 @@ module ActiveRecord extend ActiveSupport::Concern module ClassMethods - def quote_value(value, column = nil) #:nodoc: - connection.quote(value,column) + def quote_value(value, column) #:nodoc: + connection.quote(value, column) end # Used to sanitize objects before they're used in an SQL SELECT statement. Delegates to connection.quote. diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index 6f0de42aef..d8bc0653a1 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -613,7 +613,7 @@ class FinderTest < ActiveRecord::TestCase def test_named_bind_with_postgresql_type_casts l = Proc.new { bind(":a::integer '2009-01-01'::date", :a => '10') } assert_nothing_raised(&l) - assert_equal "#{ActiveRecord::Base.quote_value('10')}::integer '2009-01-01'::date", l.call + assert_equal "#{ActiveRecord::Base.connection.quote('10')}::integer '2009-01-01'::date", l.call end def test_string_sanitation