connection.columns doesn't get cached

Matt - Doesn't the query_cache only persist for the life of the request? We want the habtm columns to be cached for the life of the process, (or until reset_column_information is called), similar to how ActiveRecord::Base caches columns. It seems that the alias method chaining of columns in query_cache should just be removed... even if it was working it is redundant unless someone calls connection.columns directly from their code.

Michael - By caching the columns in the "association instance" I assume you mean within the reflection object. This seems to work as intended (caches columns for life of process) and is cleaner then my original solution of putting the cache in the owner class.

module ActiveRecord module Reflection

class AssociationReflection < MacroReflection

  def columns(join_table_name)
    unless defined?(@columns) && @columns
      @columns = klass.connection.columns(join_table_name, "#{join_table_name} Join Table Columns")
    end
    @columns
  end

I'll get a new patch together using this approach.

Luke

That's what I had in mind luke, just make sure it forgets the info when you call reset_column_information, and I'll be happy to apply the patch.

Here is the patch file. The columns are now cached within the reflection instance. You can now call columns and reset_column_information on the the habtm association directly, like this:

developer = Developer.find(1)
devloper.projects.columns
developer.projects.reset_column_information

+        unless defined?(@columns) && @columns
+          @columns = klass.connection.columns(tbl_name, log_msg)
+        end
+        @columns

This is synonymous with @columns ||= klass...

Is there any reason you have the specific defined? test in there?

Good point. New patch attached.

(from [7c147e94e66530a8314bc5d836df412fd749d55b]) Cache columns for has_and_belongs_to_many associations

This avoids repeatedly calling SHOW COLUMNS when the association is queried [#1738 connection.columns doesn't get cached state:committed] http://github.com/rails/rails/co...

Tests are failing on both edge and 2.2 after applying this patch:

http://ci.rubyonrails.org/builds... http://ci.rubyonrails.org/builds...

Looks like the sqlite adapter doesn't make any queries when calling connection.columns, at least from the perspective of assert_queries, which causes the test_caching_of_columns test to fail. I added an 'if current_adapter?(:MysqlAdapter)' around the test, since the only adapter that I know this test works with for sure is the MysqlAdapter.

Checking the build logs, we're seeing pass on MySQL, fail on PostgreSQL, SQLite2, SQLite3, before this patch.

All tests pass after applying this patch, but I'm -1 on it anyhow. If the problem is that the tests aren't testing the right thing, we should fix the tests. If the problem is that the caching works differently depending on adapter, we should have the check in the caching code, not in the test code. This approach is just hiding the issue.

The caching works the same for all adapters, so this is just a test issue. The caching of the habtm columns is very similar in regards to the caching of columns in ActiveRecord::Base. As far as I can tell there aren't any tests to verify that the caching works in ActiveRecord::Base.columns, so this test that I've added is better than nothing! I can look at getting this test to work for the other adapters, and at adding similar tests for ActiveRecord::Base.columns.

@Luke - indeed, there isn't a test. The current code in query_cache.rb doesn't get called at all.

I've got an idea how to rewrite it transparently (using some of the .extend stuff wycats wrote about yesterday) - will post a patch in the next few days.

Attached is a patch that fixes test_caching_of_columns to work for all database adapters. I did an alias_method_chain on connection.columns to count the number of times it is called, and then defined an assert_columns method which I use instead of assert_queries. I also added a test_caching_of_columns to base_test.rb.

Note that these tests do not test the query cache in any way.