Permalink
Browse files

active_record: Quote numeric values compared to string columns.

  • Loading branch information...
1 parent 1b75666 commit a712e08ebe21f6d8653a0e6602df2e0f5d40d9ca @dylanahsmith dylanahsmith committed Feb 6, 2013
@@ -1,5 +1,15 @@
## Rails 4.0.0 (unreleased) ##
+* Quote numeric values being compared to non-numeric columns. Otherwise,
+ in some database, the string column values will be coerced to a numeric
+ allowing 0, 0.0 or false to match any string starting with a non-digit.
+
+ Example:
+
+ App.where(apikey: 0) # => SELECT * FROM users WHERE apikey = '0'
+
+ *Dylan Smith*
+
* Schema dumper supports dumping the enabled database extensions to `schema.rb`
(currently only supported by postgresql).
@@ -25,13 +25,19 @@ def quote(value, column = nil)
when true, false
if column && column.type == :integer
value ? '1' : '0'
+ elsif column && [:text, :string, :binary].include?(column.type)
+ value ? "'1'" : "'0'"
else
value ? quoted_true : quoted_false
end
# BigDecimals need to be put in a non-normalized form and quoted.
when nil then "NULL"
- when BigDecimal then value.to_s('F')
- when Numeric, ActiveSupport::Duration then value.to_s
+ when Numeric, ActiveSupport::Duration
+ value = BigDecimal === value ? value.to_s('F') : value.to_s
+ if column && ![:integer, :float, :decimal].include?(column.type)
+ value = "'#{value}'"
@tenderlove
tenderlove Feb 12, 2013 Ruby on Rails member

This causes a regression, but I'm not sure why.

  def test_string_ids
    mv = Minivan.where(:minivan_id => 1234).first_or_initialize
    assert mv.new_record?
    assert_equal '1234', mv.minivan_id
  end
test_string_ids(PersistencesTest) [test/cases/persistence_test.rb:405]:
Expected: "1234"
  Actual: "'1234'"
@dylanahsmith
dylanahsmith Feb 12, 2013

minivan_id is a string column, so the integer 1234 is quoted in the Arel::Nodes::Equality object of the relations where_values. Relation#where_values_hash is used to determine the attribute values that are used to create the new record.

@awendt
awendt Feb 12, 2013

This is obviously a regression. I've seen 0268b5d, but was any code changed for the test in that commit?

@tenderlove
tenderlove Feb 13, 2013 Ruby on Rails member

@awendt not yet. If I (or someone) can't figure out how to make the test pass, then I'll revert this commit.

@dylanahsmith
dylanahsmith Feb 13, 2013

If we revert, can we just revert the changes to the predicate builder. Because I think the right way to fix this in Arel by calling the connection's quote method with integers while providing the correct column. Alternatives would be parsing the SqlLiteral or storing the unquoted value with the Equality nodes, both of which are inelegant solutions.

@pixeltrix
pixeltrix Feb 13, 2013 Ruby on Rails member

@tenderlove can't we just always quote numeric values for MySQL in ARel? e.g:

module Arel
  module Visitors
    class MySQL < Arel::Visitors::ToSql
      def visit_Bignum 
        quoted(o)
      end

      def visit_Fixnum
        quoted(o)
      end
    end
  end
end

and remove the creation of a Arel::Nodes::SQLLiteral in ActiveRecord::PredicateBuilder#build.

@dylanahsmith
dylanahsmith Feb 13, 2013

@pixeltrix No. Your code will not provide column information, so will still leave numbers as numbers. Using last_column will cause problems because it isn't cleared when building the sql query, so you will end up with something like LIMIT '1' which mysql will complain about.

@pixeltrix
pixeltrix Feb 13, 2013 Ruby on Rails member

MySQL!!!

@dylanahsmith
dylanahsmith Feb 13, 2013

My proposed solution to dealing with this regression is this change to Arel rails/arel#162 and reverting the changes to predicate builder.

@take
take Feb 18, 2013

So after rails/arel#162 is merged, we revert the changes and 0268b5d will be fixed? :O

+ end
+ value
when Date, Time then "'#{quoted_date(value)}'"
when Symbol then "'#{quote_string(value.to_s)}'"
when Class then "'#{value.to_s}'"
@@ -212,8 +212,6 @@ def quote(value, column = nil)
if value.kind_of?(String) && column && column.type == :binary && column.class.respond_to?(:string_to_binary)
s = column.class.string_to_binary(value).unpack("H*")[0]
"x'#{s}'"
- elsif value.kind_of?(BigDecimal)
- value.to_s("F")
else
super
end
@@ -98,6 +98,11 @@ def self.build(attribute, value)
when Class
# FIXME: I think we need to deprecate this behavior
attribute.eq(value.name)
+ when Integer, ActiveSupport::Duration
@defsan
defsan Feb 12, 2013

this causing an error on test environment when you use:
scope :customer1, where(customer_id: 1)
and running
RAILS_ENV=test bundle exec rake db:schema:load
on empty database

@pmarreck
pmarreck Feb 18, 2013

FYI, the workaround for now is to wrap it in a lambda{} (or stabby lambda, depending on your syntax preference)

scope :customer1, -> { where(customer_id: 1) }
@steveklabnik
steveklabnik Feb 18, 2013 Ruby on Rails member

And it's a good idea in general, @defsan, it'll be required as of Rails 4.

@defsan
defsan Feb 19, 2013

@pmarreck thanks, currently we are using same solution, Proc 👍

+ # Arel treats integers as literals, but they should be quoted when compared with strings
+ table = attribute.relation
+ column = table.engine.connection.schema_cache.columns_hash(table.name)[attribute.name.to_s]
+ attribute.eq(Arel::Nodes::SqlLiteral.new(table.engine.connection.quote(value, column)))
else
attribute.eq(value)
end
@@ -122,35 +122,35 @@ def test_quote_false
def test_quote_float
float = 1.2
assert_equal float.to_s, @quoter.quote(float, nil)
- assert_equal float.to_s, @quoter.quote(float, Object.new)
+ assert_equal float.to_s, @quoter.quote(float, FakeColumn.new(:float))
end
def test_quote_fixnum
fixnum = 1
assert_equal fixnum.to_s, @quoter.quote(fixnum, nil)
- assert_equal fixnum.to_s, @quoter.quote(fixnum, Object.new)
+ assert_equal fixnum.to_s, @quoter.quote(fixnum, FakeColumn.new(:integer))
end
def test_quote_bignum
bignum = 1 << 100
assert_equal bignum.to_s, @quoter.quote(bignum, nil)
- assert_equal bignum.to_s, @quoter.quote(bignum, Object.new)
+ assert_equal bignum.to_s, @quoter.quote(bignum, FakeColumn.new(:integer))
end
def test_quote_bigdecimal
bigdec = BigDecimal.new((1 << 100).to_s)
assert_equal bigdec.to_s('F'), @quoter.quote(bigdec, nil)
- assert_equal bigdec.to_s('F'), @quoter.quote(bigdec, Object.new)
+ assert_equal bigdec.to_s('F'), @quoter.quote(bigdec, FakeColumn.new(:decimal))
end
def test_dates_and_times
@quoter.extend(Module.new { def quoted_date(value) 'lol' end })
assert_equal "'lol'", @quoter.quote(Date.today, nil)
- assert_equal "'lol'", @quoter.quote(Date.today, Object.new)
+ assert_equal "'lol'", @quoter.quote(Date.today, FakeColumn.new(:date))
assert_equal "'lol'", @quoter.quote(Time.now, nil)
- assert_equal "'lol'", @quoter.quote(Time.now, Object.new)
+ assert_equal "'lol'", @quoter.quote(Time.now, FakeColumn.new(:time))
assert_equal "'lol'", @quoter.quote(DateTime.now, nil)
- assert_equal "'lol'", @quoter.quote(DateTime.now, Object.new)
+ assert_equal "'lol'", @quoter.quote(DateTime.now, FakeColumn.new(:datetime))
end
def test_crazy_object
@@ -108,5 +108,30 @@ def test_where_with_blank_conditions
assert_equal 4, Edge.where(blank).order("sink_id").to_a.size
end
end
+
+ def test_where_with_integer_for_string_column
+ count = Post.where(:title => 0).count
+ assert_equal 0, count
+ end
+
+ def test_where_with_float_for_string_column
+ count = Post.where(:title => 0.0).count
+ assert_equal 0, count
+ end
+
+ def test_where_with_boolean_for_string_column
+ count = Post.where(:title => false).count
+ assert_equal 0, count
+ end
+
+ def test_where_with_decimal_for_string_column
+ count = Post.where(:title => BigDecimal.new(0)).count
+ assert_equal 0, count
+ end
+
+ def test_where_with_duration_for_string_column
+ count = Post.where(:title => 0.seconds).count
+ assert_equal 0, count
+ end
end
end
@@ -391,19 +391,19 @@ def test_default_scoping_with_threads
def test_default_scope_with_inheritance
wheres = InheritedPoorDeveloperCalledJamis.all.where_values_hash
assert_equal "Jamis", wheres[:name]
- assert_equal 50000, wheres[:salary]
+ assert_equal Arel.sql("50000"), wheres[:salary]
end
def test_default_scope_with_module_includes
wheres = ModuleIncludedPoorDeveloperCalledJamis.all.where_values_hash
assert_equal "Jamis", wheres[:name]
- assert_equal 50000, wheres[:salary]
+ assert_equal Arel.sql("50000"), wheres[:salary]
end
def test_default_scope_with_multiple_calls
wheres = MultiplePoorDeveloperCalledJamis.all.where_values_hash
assert_equal "Jamis", wheres[:name]
- assert_equal 50000, wheres[:salary]
+ assert_equal Arel.sql("50000"), wheres[:salary]
end
def test_scope_overwrites_default
@@ -540,6 +540,8 @@ def create_table(*args, &block)
create_table :price_estimates, :force => true do |t|
t.string :estimate_of_type
t.integer :estimate_of_id
+ t.string :thing_type
+ t.integer :thing_id
t.integer :price
end

1 comment on commit a712e08

@pmarreck

Some thoughts...

  1. Rake tasks that recreate databases clearly need integration test coverage of some sort.
  2. Since "where" called within class scope now triggers a table schema query with this set of changes, you can get around it for now by deferring it by wrapping the arel scope in a lambda.
  3. Apparently, scopes in Rails 4 will all have to be lambdas anyway, so might as well start now! :)
  4. Were the tests added here checked for validity by removing the changes they are ostensibly testing for and making sure at least some of them failed? :)
Please sign in to comment.