The "columns" method in ActiveRecord::ConnectionAdapters::QueryCache module calls an inexistent "args" argument

Hey Rohit, I was browsing the code because I was writing a post in my blog about the splat operator (http://bit.ly/raflabs_foo_splat), so I was looking for code to show the readers real-world usages.

I didn't provide any patch because I hadn't time to run the tests, I only tried to find out if there were any tests on this but I only wasn't able to find this:

Task.cache do
  assert Task.connection.query_cache_enabled
  Task.find(1)
  ...
end

which doesn't seem to call the "columns" method so it doesn't fail.

Thanks.

oops, it should be:

...but I only was able to find this:

sorry

I'm having a hard time figuring out if the .columns is even used correctly in the current implementation.

Hey Mark, I am not sure, I was doing some testing on that and I wasn't able to make Rails to call that method in any way. Look:

Task.cache do
  # calls ActiveRecord::Base#columns which calls SQLiteAdapter#columns
  Topic.columns

  # calls SQLiteAdapter#columns directly
  Topic.connection.columns :topics
  ActiveRecord::Base.connection.columns :topics
end

So, it never calls the QueryCache#columns method but:

Task.cache do
  Task.find(1) # => calls QueryCache#select_all
end

The QueryCache#columns method is defined just below the QueryCache#select_all and now I'm confused :)

QueryCache is included in AbstractAdapter which forms the base to all the DB adapters. I took a look at the columns method in mySQL and SQLite3 (defined in SQLite adapter) adapters and none of them call super.

If you call super in the columns method of mySQL or SQLite adapters then the error is raised —

NameError: undefined local variable or method `args' for #<ActiveRecord::ConnectionAdapters::SQLite3Adapter:0x00000002664980>
    /home/rohit/projects/rails/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:62:in `columns'

I haven't looked deeper but it seems to me that caching won't be available unless you call super inside the columns method. How can this be checked?

Since at present none of the adapters are calling super inside the supplied colums method, the error is not caught. So if the correct implementation is right then how can this method be tested? Or more importantly is this implementation right?

What is happening is something like the following —

ruby-1.9.2-rc1 > module QueryCache
ruby-1.9.2-rc1 ?>  def columns(*)
ruby-1.9.2-rc1 ?>    puts args.inspect
ruby-1.9.2-rc1 ?>  end
ruby-1.9.2-rc1 ?>end
 => nil 

ruby-1.9.2-rc1 > class AdapterBase
ruby-1.9.2-rc1 ?>  include QueryCache
ruby-1.9.2-rc1 ?>end
 => AdapterBase 

ruby-1.9.2-rc1 > class Sqlite < AdapterBase
ruby-1.9.2-rc1 ?>  def columns(sql, arg_b)
ruby-1.9.2-rc1 ?>    puts "hello"
ruby-1.9.2-rc1 ?>  end
ruby-1.9.2-rc1 ?>end
 => nil 

ruby-1.9.2-rc1 > class Mysql < AdapterBase
ruby-1.9.2-rc1 ?>  def columns(sql, arg_b)
ruby-1.9.2-rc1 ?>    super
ruby-1.9.2-rc1 ?>    puts "hello"
ruby-1.9.2-rc1 ?>  end
ruby-1.9.2-rc1 ?>end
 => nil 

ruby-1.9.2-rc1 > Sqlite.new.columns(1 ,2)
hello
 => nil 

ruby-1.9.2-rc1 > Mysql.new.columns(1, 2)
NameError: undefined local variable or method `args' for #<Mysql:0x00000001e41348>
    from (irb):3:in `columns'
    from (irb):19:in `columns'
    from (irb):24
    from /home/rohit/.rvm/rubies/ruby-1.9.2-rc1/bin/irb:17:in `<main>'

Which means that either the adapters are not calling super when they are supposed to or they shouldn't call them, meaning that the AbstractAdapter and its modules don't have proper test coverage.

Maybe I'm wrong, but as far as I understand, the caching of the columns names is always[?] done by the ActiveRecord::Base.columns method via @columns:

http://bit.ly/9NxQY6 < Line 677 of ActiveRecord::Base

If we do

User.find(1)

the ActiveRecord::Base#find method hits the ActiveRecord::Base.columns and caches the columns in @columns, the second time we call ActiveRecord::Base#find the columns are already cached so it doesn't hit connection.columns, so it wouldn't matter if we call super in the adapters' column method, at least not the subsequent times.

Of course, the caching is done anyways, but not by the QueryCache#columns method, I'm not saying this is wrong, just that it doesn't work as I'd expect.

Yeah I was just forming the same explanations in my head before posting it here.

My thoughts:
1. The QueryCache#columns method is wrong and should be fixed with the above patch, its clear what the method is trying to do, its a typo.
2. It appears that the current implementation of QueryCache#columns is not used by the connection adapters. Even if we looked at using super, each adapter has a slightly different implementation of columns and might need some refactoring.

Maybe we should ask Yehuda how this actually works or should work:

git show d916c62c connection_adapters/abstract/query_cache.rb

(from [6373dd466f5c87da51051a4fa427c222c962e46b]) This method is actually not used, it's implemented on the concrete adapters

[#5331 The "columns" method in ActiveRecord::ConnectionAdapters::QueryCache module c... state:committed] http://github.com/rails/rails/commit/6373dd466f5c87da51051a4fa427c2...

(from [48c7ad17b0e87d315b68c0075046fd29e8802e93]) This method is actually not used, it's implemented on the concrete adapters

[#5331 The "columns" method in ActiveRecord::ConnectionAdapters::QueryCache module c... state:committed] http://github.com/rails/rails/commit/48c7ad17b0e87d315b68c0075046fd...