tables_in_string matches literal string containing dot

Improved the regexp (and test) to filter out all string literals. (I think...)

I've just run across this issue. I created a test case which shows it off, using searchlogic, because I initially assumed the error was in there:

http://gist.github.com/439071

I'll give xpmatteo's patch a try, see if that fixes my test case.

Yep, works for me. +1

Dodgy monkey patch for anyone else who stumbles across this:

ActiveRecord::Associations::ClassMethods.module_eval do
  def tables_in_string(string)
    return [] if string.blank?
    string.gsub(/'(\\'|[^'])*'|"(\\"|[^"])*"/, '').scan(/([a-zA-Z_][\.\w]+).?\./).flatten
  end
end

The patch creates other issues. tables_in_string needs to return actual tables names:

  def tables_in_string(string)
    return [] if string.blank?
    string.gsub(/'(.*)'/) {|s| s.gsub('.', '_')}.scan(/([\.a-zA-Z_]+).?\./).flatten
  end

Simon's regex above is not sufficient unfortunately.

Two problems:

1. The first gsub's pattern is greedy, so if you have multiple literals table names can get missed

example
bad2 = "((((((LOWER(events.name) LIKE '%ga.fs%')))) AND ((((((LOWER(venues.name) LIKE '%ga.fs%')))) AND (events.delete_flag = 0)) AND (events.date >= curdate()))"

bad2.gsub(/'(.*)'/) {|s| s.gsub('.', '')}.scan(/([.a-zA-Z]+).?./).flatten => ["events", "events", "events"]

Here it lost the venues table.

1. This only works for literals enclosed by single quotes. Literals could also be enclosed by double quotes.

Here is my proposed updated regex:

string.gsub(/(['"])(.*?)(\1)/) {|s| s.gsub('.', '')}.scan(/([.a-zA-Z]+).?./).flatten

I'll be ginning up a patch against 2.3.10 and 3.0.3 shortly

Here are a couple of patches against master and 2-3-stable.

It implements the regex I mentioned above.

+1

Looks good. I'm glad one of the tests included the tablename.columnname case as it looks like the current regexp cares about that.

Good catch, Andrew. Worked perfectly for me.

+1 (Adding comment, this time.) My project got bit by this problem--I think this patch addresses the problem better than the previous patch.

state:resolved

After talking to some of the guys over on the core mailing list, this tables_in string was a temporary hack in the move to preloading. It will probably be coming out altogether in

In Rails 3, use .preload(:bars) rather than include.

In Rails 2, we'll just have to stick with the monkey patch method.

Here's what I've come up with that seems to work with all the stuff people have mentioned.

ActiveRecord::Associations::ClassMethods.module_eval do
  def tables_in_string(string)
    return [] if string.blank?
    literals_without_dots(string).scan(/([\.a-zA-Z_]+).?\./).flatten
  end

  def literals_without_dots(string) 
    string.gsub(/(['"])(([^\\]|(\\.))*?)(\1)/) {|s| s.gsub('.', '_')} 
  end
end

As Andrew says, I'm loathe to climb further down the rabbit hole of trying to parse SQL with regexps. It's destined to fail.

If you're bitten by this in 2-3-stable apps you can either monkey patch in an initializer, or use will's patch in #3683 Add :preload option to find