pushing caching and visitors down to the connection

13 files changed, 181 insertions, 213 deletions

diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 3558ae3545..7ba67b8540 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -710,21 +710,21 @@ module ActiveRecord #:nodoc:
       # Returns an array of column objects for the table associated with this class.
       def columns
         if defined?(@primary_key)
-          connection_pool.primary_keys[table_name] ||= primary_key
+          connection.schema_cache.primary_keys[table_name] ||= primary_key
         end
 
-        connection_pool.columns[table_name]
+        connection.schema_cache.columns[table_name]
       end
 
       # Returns a hash of column objects for the table associated with this class.
       def columns_hash
-        connection_pool.columns_hash[table_name]
+        connection.schema_cache.columns_hash[table_name]
       end
 
       # Returns a hash where the keys are column names and the values are
       # default values when instantiating the AR object for this table.
       def column_defaults
-        connection_pool.column_defaults[table_name]
+        connection.schema_cache.column_defaults[table_name]
       end
 
       # Returns an array of column names as strings.
@@ -781,14 +781,14 @@ module ActiveRecord #:nodoc:
       def reset_column_information
         connection.clear_cache!
         undefine_attribute_methods
-        connection_pool.clear_table_cache!(table_name) if table_exists?
+        connection.schema_cache.clear_table_cache!(table_name) if table_exists?
 
         @column_names = @content_columns = @dynamic_methods_hash = @inheritance_column = nil
         @arel_engine = @relation = nil
       end
 
       def clear_cache! # :nodoc:
-        connection_pool.clear_cache!
+        connection.schema_cache.clear!
       end
 
       def attribute_method?(attribute)
@@ -1356,9 +1356,9 @@ MSG
           return nil if condition.blank?
 
           case condition
-            when Array; sanitize_sql_array(condition)
-            when Hash;  sanitize_sql_hash_for_conditions(condition, table_name)
-            else        condition
+          when Array; sanitize_sql_array(condition)
+          when Hash;  sanitize_sql_hash_for_conditions(condition, table_name)
+          else        condition
           end
         end
         alias_method :sanitize_sql, :sanitize_sql_for_conditions
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
index 0ec0576795..f1781f057c 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
@@ -2,6 +2,7 @@ require 'thread'
 require 'monitor'
 require 'set'
 require 'active_support/core_ext/module/synchronization'
+require 'active_support/core_ext/module/deprecation'
 
 module ActiveRecord
   # Raised when a connection could not be obtained within the connection
@@ -59,8 +60,6 @@ module ActiveRecord
     class ConnectionPool
       attr_accessor :automatic_reconnect
       attr_reader :spec, :connections
-      attr_reader :columns, :columns_hash, :primary_keys, :tables
-      attr_reader :column_defaults
 
       # Creates a new ConnectionPool object. +spec+ is a ConnectionSpecification
       # object which describes database connection information (e.g. adapter,
@@ -85,72 +84,7 @@ module ActiveRecord
         @connections         = []
         @checked_out         = []
         @automatic_reconnect = true
-        @tables              = {}
         @visitor             = nil
-
-        @columns     = Hash.new do |h, table_name|
-          h[table_name] = with_connection do |conn|
-
-            # Fetch a list of columns
-            conn.columns(table_name, "#{table_name} Columns").tap do |columns|
-
-              # set primary key information
-              columns.each do |column|
-                column.primary = column.name == primary_keys[table_name]
-              end
-            end
-          end
-        end
-
-        @columns_hash = Hash.new do |h, table_name|
-          h[table_name] = Hash[columns[table_name].map { |col|
-            [col.name, col]
-          }]
-        end
-
-        @column_defaults = Hash.new do |h, table_name|
-          h[table_name] = Hash[columns[table_name].map { |col|
-            [col.name, col.default]
-          }]
-        end
-
-        @primary_keys = Hash.new do |h, table_name|
-          h[table_name] = with_connection do |conn|
-            table_exists?(table_name) ? conn.primary_key(table_name) : 'id'
-          end
-        end
-      end
-
-      # A cached lookup for table existence.
-      def table_exists?(name)
-        return @tables[name] if @tables.key? name
-
-        with_connection do |conn|
-          conn.tables.each { |table| @tables[table] = true }
-          @tables[name] = conn.table_exists?(name) if !@tables.key?(name)
-        end
-
-        @tables[name]
-      end
-
-      # Clears out internal caches:
-      #
-      #   * columns
-      #   * columns_hash
-      #   * tables
-      def clear_cache!
-        @columns.clear
-        @columns_hash.clear
-        @column_defaults.clear
-        @tables.clear
-      end
-
-      # Clear out internal caches for table with +table_name+.
-      def clear_table_cache!(table_name)
-        @columns.delete table_name
-        @columns_hash.delete table_name
-        @column_defaults.delete table_name
-        @primary_keys.delete table_name
       end
 
       # Retrieve the connection associated with the current thread, or call
@@ -227,6 +161,35 @@ module ActiveRecord
         end
       end
 
+      def columns
+        with_connection do |c|
+          c.schema_cache.columns
+        end
+      end
+      deprecate :columns
+
+      def columns_hash
+        with_connection do |c|
+          c.schema_cache.columns_hash
+        end
+      end
+      deprecate :columns_hash
+
+      def primary_keys
+        raise
+        with_connection do |c|
+          c.schema_cache.primary_keys
+        end
+      end
+      deprecate :primary_keys
+
+      def clear_cache!
+        with_connection do |c|
+          c.schema_cache.clear!
+        end
+      end
+      deprecate :clear_cache!
+
       # Return any checked-out connections back to the pool by threads that
       # are no longer alive.
       def clear_stale_cached_connections!
@@ -301,16 +264,7 @@ module ActiveRecord
       private
 
       def new_connection
-        connection = ActiveRecord::Base.send(spec.adapter_method, spec.config)
-
-        # TODO: This is a bit icky, and in the long term we may want to change the method
-        #       signature for connections. Also, if we switch to have one visitor per
-        #       connection (and therefore per thread), we can get rid of the thread-local
-        #       variable in Arel::Visitors::ToSql.
-        @visitor ||= connection.class.visitor_for(self)
-        connection.visitor = @visitor
-
-        connection
+        ActiveRecord::Base.send(spec.adapter_method, spec.config)
       end
 
       def current_connection_id #:nodoc:
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index c47bcfc406..cbb7423480 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -3,6 +3,7 @@ require 'bigdecimal'
 require 'bigdecimal/util'
 require 'active_support/core_ext/benchmark'
 require 'active_support/deprecation'
+require 'active_record/connection_adapters/schema_cache'
 
 module ActiveRecord
   module ConnectionAdapters # :nodoc:
@@ -51,6 +52,7 @@ module ActiveRecord
       define_callbacks :checkout, :checkin
 
       attr_accessor :visitor
+      attr_reader :schema_cache
 
       def initialize(connection, logger = nil) #:nodoc:
         @active = nil
@@ -60,6 +62,7 @@ module ActiveRecord
         @open_transactions = 0
         @instrumenter = ActiveSupport::Notifications.instrumenter
         @visitor = nil
+        @schema_cache = SchemaCache.new self
       end
 
       # Returns a visitor instance for this adaptor, which conforms to the Arel::ToSql interface
diff --git a/activerecord/lib/active_record/connection_adapters/schema_cache.rb b/activerecord/lib/active_record/connection_adapters/schema_cache.rb
new file mode 100644
index 0000000000..b14b37ce89
--- /dev/null
+++ b/activerecord/lib/active_record/connection_adapters/schema_cache.rb
@@ -0,0 +1,72 @@
+module ActiveRecord
+  module ConnectionAdapters
+    class SchemaCache
+      attr_reader :columns, :columns_hash, :primary_keys, :tables
+      attr_reader :column_defaults
+      attr_reader :connection
+
+      def initialize(conn)
+        @connection = conn
+        @tables              = {}
+
+        @columns     = Hash.new do |h, table_name|
+          h[table_name] =
+            # Fetch a list of columns
+            conn.columns(table_name, "#{table_name} Columns").tap do |cs|
+              # set primary key information
+              cs.each do |column|
+                column.primary = column.name == primary_keys[table_name]
+              end
+            end
+        end
+
+        @columns_hash = Hash.new do |h, table_name|
+          h[table_name] = Hash[columns[table_name].map { |col|
+            [col.name, col]
+          }]
+        end
+
+        @column_defaults = Hash.new do |h, table_name|
+          h[table_name] = Hash[columns[table_name].map { |col|
+            [col.name, col.default]
+          }]
+        end
+
+        @primary_keys = Hash.new do |h, table_name|
+          h[table_name] = table_exists?(table_name) ?
+                          conn.primary_key(table_name) : 'id'
+        end
+      end
+
+      # A cached lookup for table existence.
+      def table_exists?(name)
+        return @tables[name] if @tables.key? name
+
+        connection.tables.each { |table| @tables[table] = true }
+        @tables[name] = connection.table_exists?(name) if !@tables.key?(name)
+
+        @tables[name]
+      end
+
+      # Clears out internal caches:
+      #
+      #   * columns
+      #   * columns_hash
+      #   * tables
+      def clear!
+        @columns.clear
+        @columns_hash.clear
+        @column_defaults.clear
+        @tables.clear
+      end
+
+      # Clear out internal caches for table with +table_name+.
+      def clear_table_cache!(table_name)
+        @columns.delete table_name
+        @columns_hash.delete table_name
+        @column_defaults.delete table_name
+        @primary_keys.delete table_name
+      end
+    end
+  end
+end
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
index bc3804b3d9..c11f82a33f 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
@@ -89,10 +89,7 @@ module ActiveRecord
         @statements = StatementPool.new(@connection,
                                         config.fetch(:statement_limit) { 1000 })
         @config = config
-      end
-
-      def self.visitor_for(pool) # :nodoc:
-        Arel::Visitors::SQLite.new(pool)
+        @visitor = Arel::Visitors::SQLite.new self
       end
 
       def adapter_name #:nodoc:
diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb
index af167dc59b..a789f48725 100644
--- a/activerecord/lib/active_record/relation/predicate_builder.rb
+++ b/activerecord/lib/active_record/relation/predicate_builder.rb
@@ -23,7 +23,7 @@ module ActiveRecord
             attribute.in(value.arel.ast)
           when Array, ActiveRecord::Associations::CollectionProxy
             values = value.to_a.map {|x| x.is_a?(ActiveRecord::Base) ? x.id : x}
-            ranges, values = values.partition {|value| value.is_a?(Range) || value.is_a?(Arel::Relation)}
+            ranges, values = values.partition {|v| v.is_a?(Range) || v.is_a?(Arel::Relation)}
 
             array_predicates = ranges.map {|range| attribute.in(range)}
 
diff --git a/activerecord/lib/active_record/session_store.rb b/activerecord/lib/active_record/session_store.rb
index 76c37cc367..e3bbd06f7e 100644
--- a/activerecord/lib/active_record/session_store.rb
+++ b/activerecord/lib/active_record/session_store.rb
@@ -59,12 +59,12 @@ module ActiveRecord
       end
 
       def drop_table!
-        connection_pool.clear_table_cache!(table_name)
+        connection.schema_cache.clear_table_cache!(table_name)
         connection.drop_table table_name
       end
 
       def create_table!
-        connection_pool.clear_table_cache!(table_name)
+        connection.schema_cache.clear_table_cache!(table_name)
         connection.create_table(table_name) do |t|
           t.string session_id_column, :limit => 255
           t.text data_column_name
diff --git a/activerecord/test/active_record/connection_adapters/fake_adapter.rb b/activerecord/test/active_record/connection_adapters/fake_adapter.rb
index 1c2942170e..267ea8bb6b 100644
--- a/activerecord/test/active_record/connection_adapters/fake_adapter.rb
+++ b/activerecord/test/active_record/connection_adapters/fake_adapter.rb
@@ -9,11 +9,16 @@ module ActiveRecord
     class FakeAdapter < AbstractAdapter
       attr_accessor :tables, :primary_keys
 
+      @columns = Hash.new { |h,k| h[k] = [] }
+      class << self
+        attr_reader :columns
+      end
+
       def initialize(connection, logger)
         super
         @tables       = []
         @primary_keys = {}
-        @columns      = Hash.new { |h,k| h[k] = [] }
+        @columns      = self.class.columns
       end
 
       def primary_key(table)
diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb
index 94497e37c7..f1023ed7ef 100644
--- a/activerecord/test/cases/adapter_test.rb
+++ b/activerecord/test/cases/adapter_test.rb
@@ -157,14 +157,4 @@ class AdapterTest < ActiveRecord::TestCase
       end
     end
   end
-
-  def test_deprecated_visitor_for
-    visitor_klass = Class.new(Arel::Visitors::ToSql)
-    Arel::Visitors::VISITORS['fuuu'] = visitor_klass
-    pool = stub(:spec => stub(:config => { :adapter => 'fuuu' }))
-    visitor = assert_deprecated {
-      ActiveRecord::ConnectionAdapters::AbstractAdapter.visitor_for(pool)
-    }
-    assert visitor.is_a?(visitor_klass)
-  end
 end
diff --git a/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb b/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
index eb6f071dc1..156a8b7b14 100644
--- a/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
+++ b/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
@@ -158,6 +158,7 @@ module ActiveRecord
         binary.save!
         assert_equal str, binary.data
 
+      ensure
         DualEncoding.connection.drop_table('dual_encodings')
       end
 
diff --git a/activerecord/test/cases/connection_adapters/schema_cache_test.rb b/activerecord/test/cases/connection_adapters/schema_cache_test.rb
new file mode 100644
index 0000000000..79e842f5e1
--- /dev/null
+++ b/activerecord/test/cases/connection_adapters/schema_cache_test.rb
@@ -0,0 +1,55 @@
+require "cases/helper"
+
+module ActiveRecord
+  module ConnectionAdapters
+    class SchemaCacheTest < ActiveRecord::TestCase
+      def setup
+        connection = ActiveRecord::Base.connection
+        @cache     = SchemaCache.new connection
+
+        if in_memory_db?
+          connection.create_table :posts do |t|
+            t.integer :cololumn
+          end
+        end
+      end
+
+      def test_primary_key
+        assert_equal 'id', @cache.primary_keys['posts']
+      end
+
+      def test_primary_key_for_non_existent_table
+        assert_equal 'id', @cache.primary_keys['omgponies']
+      end
+
+      def test_primary_key_is_set_on_columns
+        posts_columns = @cache.columns_hash['posts']
+        assert posts_columns['id'].primary
+
+        (posts_columns.keys - ['id']).each do |key|
+          assert !posts_columns[key].primary
+        end
+      end
+
+      def test_caches_columns
+        columns = @cache.columns['posts']
+        assert_equal columns, @cache.columns['posts']
+      end
+
+      def test_caches_columns_hash
+        columns_hash = @cache.columns_hash['posts']
+        assert_equal columns_hash, @cache.columns_hash['posts']
+      end
+
+      def test_clearing_column_cache
+        @cache.columns['posts']
+        @cache.columns_hash['posts']
+
+        @cache.clear!
+
+        assert_equal 0, @cache.columns.size
+        assert_equal 0, @cache.columns_hash.size
+      end
+    end
+  end
+end
diff --git a/activerecord/test/cases/connection_pool_test.rb b/activerecord/test/cases/connection_pool_test.rb
index 8a0f453127..1550fa5530 100644
--- a/activerecord/test/cases/connection_pool_test.rb
+++ b/activerecord/test/cases/connection_pool_test.rb
@@ -26,43 +26,6 @@ module ActiveRecord
         assert !@pool.active_connection?
       end
 
-      def test_pool_caches_columns
-        columns = @pool.columns['posts']
-        assert_equal columns, @pool.columns['posts']
-      end
-
-      def test_pool_caches_columns_hash
-        columns_hash = @pool.columns_hash['posts']
-        assert_equal columns_hash, @pool.columns_hash['posts']
-      end
-
-      def test_clearing_column_cache
-        @pool.columns['posts']
-        @pool.columns_hash['posts']
-
-        @pool.clear_cache!
-
-        assert_equal 0, @pool.columns.size
-        assert_equal 0, @pool.columns_hash.size
-      end
-
-      def test_primary_key
-        assert_equal 'id', @pool.primary_keys['posts']
-      end
-
-      def test_primary_key_for_non_existent_table
-        assert_equal 'id', @pool.primary_keys['omgponies']
-      end
-
-      def test_primary_key_is_set_on_columns
-        posts_columns = @pool.columns_hash['posts']
-        assert posts_columns['id'].primary
-
-        (posts_columns.keys - ['id']).each do |key|
-          assert !posts_columns[key].primary
-        end
-      end
-
       def test_clear_stale_cached_connections!
         pool = ConnectionPool.new ActiveRecord::Base.connection_pool.spec
 
diff --git a/activerecord/test/cases/pooled_connections_test.rb b/activerecord/test/cases/pooled_connections_test.rb
index 434b8a677a..bc3dfb1078 100644
--- a/activerecord/test/cases/pooled_connections_test.rb
+++ b/activerecord/test/cases/pooled_connections_test.rb
@@ -66,78 +66,6 @@ class PooledConnectionsTest < ActiveRecord::TestCase
     assert_equal 1, ActiveRecord::Base.connection_pool.connections.size
   end
 
-  def test_pooled_connection_checkin_two
-    checkout_checkin_connections 2, 3
-    assert_equal 3, @connection_count
-    assert_equal 0, @timed_out
-    assert_equal 1, ActiveRecord::Base.connection_pool.connections.size
-  end
-
-  def test_pooled_connection_checkout_existing_first
-    ActiveRecord::Base.establish_connection(@connection.merge({:pool => 1}))
-    conn_pool = ActiveRecord::Base.connection_pool
-    conn = conn_pool.checkout
-    conn_pool.checkin(conn)
-    conn = conn_pool.checkout
-    assert ActiveRecord::ConnectionAdapters::AbstractAdapter === conn
-    conn_pool.checkin(conn)
-  end
-
-  def test_not_connected_defined_connection_returns_false
-    ActiveRecord::Base.establish_connection(@connection)
-    assert ! ActiveRecord::Base.connected?
-  end
-
-  def test_undefined_connection_returns_false
-    old_handler = ActiveRecord::Base.connection_handler
-    ActiveRecord::Base.connection_handler = ActiveRecord::ConnectionAdapters::ConnectionHandler.new
-    assert ! ActiveRecord::Base.connected?
-  ensure
-    ActiveRecord::Base.connection_handler = old_handler
-  end
-
-  def test_connection_config
-    ActiveRecord::Base.establish_connection(@connection)
-    assert_equal @connection, ActiveRecord::Base.connection_config
-  end
-
-  def test_with_connection_nesting_safety
-    ActiveRecord::Base.establish_connection(@connection.merge({:pool => 1, :wait_timeout => 0.1}))
-
-    before_count = Project.count
-
-    add_record('one')
-
-    ActiveRecord::Base.connection.transaction do
-      add_record('two')
-      # Have another thread try to screw up the transaction
-      Thread.new do
-        ActiveRecord::Base.connection.rollback_db_transaction
-        ActiveRecord::Base.connection_pool.release_connection
-      end
-      add_record('three')
-    end
-
-    after_count = Project.count
-    assert_equal 3, after_count - before_count
-  end
-
-  def test_connection_pool_callbacks
-    checked_out, checked_in = false, false
-    ActiveRecord::ConnectionAdapters::AbstractAdapter.class_eval do
-      set_callback(:checkout, :after) { checked_out = true }
-      set_callback(:checkin, :before) { checked_in = true }
-    end
-    @per_test_teardown << proc do
-      ActiveRecord::ConnectionAdapters::AbstractAdapter.class_eval do
-        reset_callbacks :checkout
-        reset_callbacks :checkin
-      end
-    end
-    checkout_checkin_connections 1, 1
-    assert checked_out
-    assert checked_in
-  end
 
   private