This project is archived and is in readonly mode.
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 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 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 August 8th, 2010 @ 08:21 AM
oops, it should be:
...but I only was able to find this:
sorry
-
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 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.
-
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 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 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...
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>
People watching this ticket
Attachments
Tags
Referenced by
- 5331 The "columns" method in ActiveRecord::ConnectionAdapters::QueryCache module calls an inexistent "args" argument [#5331 state:committed] http://github.com/rails/rails/co...
- 5331 The "columns" method in ActiveRecord::ConnectionAdapters::QueryCache module calls an inexistent "args" argument [#5331 state:committed] http://github.com/rails/rails/co...
- 5331 The "columns" method in ActiveRecord::ConnectionAdapters::QueryCache module calls an inexistent "args" argument [#5331 state:committed] http://github.com/rails/rails/co...