Rails core - Jan 2007 - Lack of column quoting in SQL when using :conditions and token substitution

Good evening list,

The following subtlety(?!) of the under the hood SQL magic of  
ActiveRecord has, on many occasions, made me waste hours staring  
blankly at SQL exceptions while wrongly blaming the ruby SQL  
bindings. Ahem.

Anyway, let us assume that I have a Post model with some attributes  
whose names are reserved keywords of my preferred database flavour (I  
know, but just bare with me... It is technically allowed with proper  
quoting. Only DB tables must not be named after keywords I believe).

All the following "find with conditions" will fail very nicely, thank
you. But for the non-hardcore-DBA type person the reason might be  
difficult to spot, as there are tons of keywords that will cause it.

 >> Post.find(:all, :conditions => ["when = ?", Time.now])
 >> Post.find(:all, :conditions => ["distinct = ?",
"boom"])
 >> Post.find(:all, :conditions => ["create = ?",
"boom"])
 >> Post.find(:all, :conditions => ["column = ?",
"boom"])

--
    ActiveRecord::StatementInvalid: Mysql::Error: You have an error  
in your SQL syntax; check the manual that corresponds to your MySQL  
server version for the right syntax to use near ''create =
''boom'')'' at
line 1: SELECT * FROM posts WHERE (create = ''boom'')
--

This happens of course because the column names are not escaped as  
per the correct database requirements.

My first reaction was to write and submit a documentation patch (and  
the reason why I posted this here and not on rails-talk) to clearly  
state, ''Please escape everything if your DB needs it to''.

Having given this some thought I was wondering if the following  
solution has any merit, or am I barking up the wrong tree?

        def escape_model_columns(statement)
           columns.collect(&:name).each do |column|
             re = Regexp.new("\\b#{column}")
             statement.gsub!(re) { |match|  
connection.quote_column_name(column) }
           end
         end

And then call escape_model_columns from three places: The
''else'' case
clause of AR::Base.sanitize_sql and in the end of the two  
AR::Base.replace_*_bind_variables methods.

Is it worth pursuing? I can imagine situations where one DB doesn''t  
care about my dodgy unescaped keywords (db2?), but when I move to  
another one, all hell breaks loose.

Shall I write tests? A patch? Tighten up the Regexp?

-christos
(aka fetaphunk)


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

Good evening list,

The following subtlety(?!) of the under the hood SQL magic of
ActiveRecord has, on many occasions, made me waste hours staring
blankly at SQL exceptions while wrongly blaming the ruby SQL bindings.
Ahem.

Anyway, let us assume that I have a Post model with some attributes
whose names are reserved keywords of my preferred database flavour (I
know, but just bare with me... It is technically allowed with proper
quoting. Only DB tables must not be named after keywords I believe).

All the following "find with conditions" will fail very nicely, thank
you. But for the non-hardcore-DBA type person the reason might be
difficult to spot, as there are tons of keywords that will cause it.
>> Post.find(:all, :conditions => ["when = ?", Time.now])
>> Post.find(:all, :conditions => ["distinct = ?",
"boom"])
>> Post.find(:all, :conditions => ["create = ?",
"boom"])
>> Post.find(:all, :conditions => ["column = ?",
"boom"])
--
   ActiveRecord::StatementInvalid: Mysql::Error: You have an error in
your SQL syntax; check the manual that corresponds to your MySQL
server version for the right syntax to use near ''create =
''boom'')'' at
line 1: SELECT * FROM posts WHERE (create = ''boom'')
--

This happens of course because the column names are not escaped as per
the correct database requirements.

My first reaction was to write and submit a documentation patch (and
the reason why I posted this here and not on rails-talk) to clearly
state, ''Please escape everything if your DB needs it to''.

Having given this some thought I was wondering if the following
solution has any merit, or am I barking up the wrong tree?

       def escape_model_columns(statement)
          columns.collect(&:name).each do |column|
            re = Regexp.new("\\b#{column}")
            statement.gsub!(re) { |match| connection.quote_column_name(column) }
          end
        end

And then call escape_model_columns from three places: The
''else'' case
clause of AR::Base.sanitize_sql and in the end of the two
AR::Base.replace_*_bind_variables methods.

Is it worth pursuing? I can imagine situations where one DB doesn''t
care about my dodgy unescaped keywords (db2?), but when I move to
another one, all hell breaks loose.

Shall I write tests? A patch? Tighten up the Regexp?

-christos

--~--~---------~--~----~------------~-------~--~----~
 You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 1/6/07, Christos Zisopoulos <christos@moonbasezero.com>
wrote:
>
> Shall I write tests? A patch? Tighten up the Regexp?

I don''t think you should go with a regexp. AR finder supports an
alternate
syntax for what you did above:

  :conditions => { :distinct => "boom", :create =>
"boom" }

When you pass it a hash, it escapes column names.

Guessing what could be a column name inside a query fragment string is, IMO,
not something AR should do.

--~--~---------~--~----~------------~-------~--~----~
 You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 6 Jan 2007, at 16:16, Mislav Marohni  wrote:
> On 1/6/07, Christos Zisopoulos <christos@moonbasezero.com> wrote:
>> Shall I write tests? A patch? Tighten up the Regexp?
> I don''t think you should go with a regexp. AR finder supports an  
> alternate syntax for what you did above:
> :conditions => { :distinct => "boom", :create =>
"boom" }
> When you pass it a hash, it escapes column names.
Yeah, I use the above syntax for simple queries. Unfortunately it is  
not as useful as the SQL fragment as it only performs a Boolean AND  
with the arguments.
> Guessing what could be a column name inside a query fragment string  
> is, IMO, not something AR should do.
Fair enough. While I was writing my patch I couldn''t think of an SQL  
fragment complicated enough to break the RegExp. Then again, my  
applications so far have only required simple SQL.

Still, I think that the documentation could a bit more explicit.

Something like, "Be aware that AR, will pass the SQL fragment as is  
to underlying database, after variable substitution. As such, you  
should take care to quote column names in the fragment so that they  
don''t clash with database keywords, producing a syntax error".

Or, at the very least, the examples could use a properly quoted SQL  
fragment, and maybe explain why it is thus.

-christos
--~--~---------~--~----~------------~-------~--~----~
 You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---