This project is archived and is in readonly mode.

#5331 ✓committed
rafmagana

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

Reported by rafmagana | August 7th, 2010 @ 08:21 PM | in 3.0.2

http://bit.ly/rails3_query_cache_L60

def columns(*)
  if @query_cache_enabled
    @query_cache["SHOW FIELDS FROM #{args.first}"] ||= super
  else
   super
  end
end

Comments and changes to this ticket

  • Rohit Arondekar

    Rohit Arondekar August 8th, 2010 @ 06:13 AM

    • State changed from “new” to “open”
    • Assigned user set to “Santiago Pastorino”
    • Importance changed from “” to “Low”

    How did you find this? Did you encounter an error or were you browsing the code?

    Well it certainly is wrong, so I've provided a patch that adds the args to parameters but I don't know how to write a test for this. Does this even need a test?

  • rafmagana

    rafmagana August 8th, 2010 @ 08:20 AM

    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.

  • rafmagana

    rafmagana August 8th, 2010 @ 08:21 AM

    oops, it should be:

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

    sorry

  • Mark Turner

    Mark Turner August 9th, 2010 @ 09:46 PM

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

  • rafmagana

    rafmagana August 9th, 2010 @ 11:52 PM

    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 :)

  • Santiago Pastorino

    Santiago Pastorino August 11th, 2010 @ 04:48 AM

    • Milestone cleared.

    Hey guys can someone provide a test with this patch?.
    Thanks.

  • Rohit Arondekar

    Rohit Arondekar August 11th, 2010 @ 08:41 AM

    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?

  • Rohit Arondekar

    Rohit Arondekar August 11th, 2010 @ 08:56 AM

    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.

  • rafmagana

    rafmagana August 11th, 2010 @ 04:36 PM

    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.

  • Mark Turner

    Mark Turner August 11th, 2010 @ 06:43 PM

    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.

  • rafmagana

    rafmagana August 11th, 2010 @ 07:15 PM

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

    git show d916c62c connection_adapters/abstract/query_cache.rb

  • Repository

    Repository August 14th, 2010 @ 01:20 AM

    • State changed from “open” to “committed”

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

    [#5331 state:committed] http://github.com/rails/rails/commit/27fb88aa22d7887138ec02f34eeb01...

  • Repository

    Repository August 14th, 2010 @ 01:21 AM

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

    [#5331 state:committed] http://github.com/rails/rails/commit/6373dd466f5c87da51051a4fa427c2...

  • Repository

    Repository August 14th, 2010 @ 12:23 PM

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

    [#5331 state:committed] http://github.com/rails/rails/commit/48c7ad17b0e87d315b68c0075046fd...

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:02 PM

    • Milestone set to 3.0.2

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments