Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• 

  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?

  • args_param.diff 958 Bytes

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  rafmagana August 8th, 2010 @ 08:21 AM

  oops, it should be:

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

  sorry

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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 August 11th, 2010 @ 04:48 AM

  • Milestone cleared.

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

• 

  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 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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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 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 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 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 October 15th, 2010 @ 11:02 PM

  • Milestone set to “3.0.2”

  [bulk edit]