Move database path handling from BaseConfig into the SQLite adapter (#215)

In #204 and #214 we discussed that "path" methods don't belong in
`DatabaseConfigurations::BaseConfig` because the concept of a file path
is SQLite-specific.

This PR moves path concerns out of BaseConfig and into the SQLite
adapter.

There are a few other notable changes in here, too, though:

- I've removed the adapter delegation layer, and instead the BaseConfig
and TenantConfig classes will create the adapter on demand and memoize
it, which should mean fewer objects are being allocated
- Dropped the unnecessary `db_config` argument to `acquire_ready_lock`

As I noted in the comment in `database_adapters/sqlite.rb`, I'm not
entirely satisfied with how we've extracted the SQLite-specific
behavior, because some methods feel like they should be in a
database-specific subclass of BaseConfig, and the rest feel like they
should be in the Rails database connection adapter. So I think we should
continue to refactor this as the opportunity arises!

cc @andrewmarkle

Author: flavorjones

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### Changed
 
 - The return value from an Active Record model `#cache_key` has changed from `users/1?tenant=foo` to `foo/users/1`. For existing applications, this will invalidate any relevant cache entries. #187 @miguelmarcondesf
+- Renamed `ActiveRecord::Tenanted::DatabaseTasks.tenanted_config` to `.base_config`.
 
 
 ### Improved

lib/active_record/tenanted/database_adapter.rb

@@ -8,35 +8,7 @@ class DatabaseAdapter # :nodoc:
       }.freeze
 
       class << self
-        def create_database(db_config)
-          adapter_for(db_config).create_database
-        end
-
-        def drop_database(db_config)
-          adapter_for(db_config).drop_database
-        end
-
-        def database_exist?(db_config)
-          adapter_for(db_config).database_exist?
-        end
-
-        def database_ready?(db_config)
-          adapter_for(db_config).database_ready?
-        end
-
-        def acquire_ready_lock(db_config, &block)
-          adapter_for(db_config).acquire_ready_lock(db_config, &block)
-        end
-
-        def tenant_databases(db_config)
-          adapter_for(db_config).tenant_databases
-        end
-
-        def validate_tenant_name(db_config, tenant_name)
-          adapter_for(db_config).validate_tenant_name(tenant_name)
-        end
-
-        def adapter_for(db_config)
+        def new(db_config)
           adapter_class_name = ADAPTERS[db_config.adapter]
 
           if adapter_class_name.nil?

lib/active_record/tenanted/database_adapters/sqlite.rb

@@ -2,18 +2,46 @@
 
 module ActiveRecord
   module Tenanted
-    module DatabaseAdapters
-      class SQLite # :nodoc:
+    module DatabaseAdapters # :nodoc:
+      #
+      #  TODO: This still feels to me like it's not _quite_ right. I think we could further refactor this by:
+      #
+      #  1. Moving tenant_databases and validate_tenant_name to BaseConfig, and subclassing it for
+      #     each database
+      #  2. Moving create_database, drop_database, database_exist?, database_ready?,
+      #     acquire_ready_lock, ensure_database_directory_exists, and database_path to the SQLite
+      #     connection adapter, possibly into Rails
+      #  3. Moving test_workerize and path_for to be SQLite connection adapter class methods,
+      #     possibly into Rails
+      #
+      class SQLite
+        attr_reader :db_config
+
         def initialize(db_config)
           @db_config = db_config
         end
 
-        def create_database
-          # Ensure the directory exists
-          database_dir = File.dirname(database_path)
-          FileUtils.mkdir_p(database_dir) unless File.directory?(database_dir)
+        def tenant_databases
+          glob = path_for(db_config.database_for("*"))
+          scanner = Regexp.new(path_for(db_config.database_for("(.+)")))
+
+          Dir.glob(glob).filter_map do |path|
+            result = path.scan(scanner).flatten.first
+            if result.nil?
+              Rails.logger.warn "ActiveRecord::Tenanted: Cannot parse tenant name from filename #{path.inspect}"
+            end
+            result
+          end
+        end
+
+        def validate_tenant_name(tenant_name)
+          if tenant_name.match?(%r{[/'"`]})
+            raise BadTenantNameError, "Tenant name contains an invalid character: #{tenant_name.inspect}"
+          end
+        end
 
-          # Create the SQLite database file
+        def create_database
+          ensure_database_directory_exists
           FileUtils.touch(database_path)
         end
 
@@ -32,35 +60,45 @@ def database_ready?
           File.exist?(database_path) && !ActiveRecord::Tenanted::Mutex::Ready.locked?(database_path)
         end
 
-        def tenant_databases
-          glob = db_config.database_path_for("*")
-          scanner = Regexp.new(db_config.database_path_for("(.+)"))
+        def acquire_ready_lock(&block)
+          ActiveRecord::Tenanted::Mutex::Ready.lock(database_path, &block)
+        end
 
-          Dir.glob(glob).filter_map do |path|
-            result = path.scan(scanner).flatten.first
-            if result.nil?
-              Rails.logger.warn "ActiveRecord::Tenanted: Cannot parse tenant name from filename #{path.inspect}"
-            end
-            result
+        def ensure_database_directory_exists
+          return unless database_path
+
+          database_dir = File.dirname(database_path)
+          unless File.directory?(database_dir)
+            FileUtils.mkdir_p(database_dir)
           end
         end
 
-        def acquire_ready_lock(db_config, &block)
-          ActiveRecord::Tenanted::Mutex::Ready.lock(database_path, &block)
+        def database_path
+          path_for(db_config.database)
         end
 
-        def validate_tenant_name(tenant_name)
-          if tenant_name.match?(%r{[/'"`]})
-            raise BadTenantNameError, "Tenant name contains an invalid character: #{tenant_name.inspect}"
+        def test_workerize(db, test_worker_id)
+          test_worker_suffix = "_#{test_worker_id}"
+
+          if db.start_with?("file:") && db.include?("?")
+            db.sub(/(\?.*)$/, "#{test_worker_suffix}\\1")
+          else
+            db + test_worker_suffix
           end
         end
 
-        private
-          attr_reader :db_config
-
-          def database_path
-            db_config.database_path
+        # A sqlite database path can be a file path or a URI (either relative or absolute).  We
+        # can't parse it as a standard URI in all circumstances, though, see
+        # https://sqlite.org/uri.html
+        def path_for(database)
+          if database.start_with?("file:/")
+            URI.parse(database).path
+          elsif database.start_with?("file:")
+            URI.parse(database.sub(/\?.*$/, "")).opaque
+          else
+            database
           end
+        end
       end
     end
   end

lib/active_record/tenanted/database_configurations/base_config.rb

@@ -11,6 +11,11 @@ class BaseConfig < ActiveRecord::DatabaseConfigurations::HashConfig
         def initialize(...)
           super
           @test_worker_id = nil
+          @config_adapter = nil
+        end
+
+        def config_adapter
+          @config_adapter ||= ActiveRecord::Tenanted::DatabaseAdapter.new(self)
         end
 
         def database_tasks?
@@ -20,31 +25,26 @@ def database_tasks?
         def database_for(tenant_name)
           tenant_name = tenant_name.to_s
 
-          validate_tenant_name(tenant_name)
+          config_adapter.validate_tenant_name(tenant_name)
 
-          path = sprintf(database, tenant: tenant_name)
+          db = sprintf(database, tenant: tenant_name)
 
           if test_worker_id
-            test_worker_path(path)
-          else
-            path
+            db = config_adapter.test_workerize(db, test_worker_id)
           end
-        end
 
-        def database_path_for(tenant_name)
-          coerce_path(database_for(tenant_name))
+          db
         end
 
         def tenants
-          ActiveRecord::Tenanted::DatabaseAdapter.tenant_databases(self)
+          config_adapter.tenant_databases
         end
 
         def new_tenant_config(tenant_name)
           config_name = "#{name}_#{tenant_name}"
           config_hash = configuration_hash.dup.tap do |hash|
             hash[:tenant] = tenant_name
             hash[:database] = database_for(tenant_name)
-            hash[:database_path] = database_path_for(tenant_name)
             hash[:tenanted_config_name] = name
           end
           Tenanted::DatabaseConfigurations::TenantConfig.new(env_name, config_name, config_hash)
@@ -60,33 +60,6 @@ def new_connection
         def max_connection_pools
           (configuration_hash[:max_connection_pools] || DEFAULT_MAX_CONNECTION_POOLS).to_i
         end
-
-        private
-          # A sqlite database path can be a file path or a URI (either relative or absolute).
-          # We can't parse it as a standard URI in all circumstances, though, see https://sqlite.org/uri.html
-          def coerce_path(path)
-            if path.start_with?("file:/")
-              URI.parse(path).path
-            elsif path.start_with?("file:")
-              URI.parse(path.sub(/\?.*$/, "")).opaque
-            else
-              path
-            end
-          end
-
-          def validate_tenant_name(tenant_name)
-            ActiveRecord::Tenanted::DatabaseAdapter.validate_tenant_name(self, tenant_name)
-          end
-
-          def test_worker_path(path)
-            test_worker_suffix = "_#{test_worker_id}"
-
-            if path.start_with?("file:") && path.include?("?")
-              path.sub(/(\?.*)$/, "#{test_worker_suffix}\\1")
-            else
-              path + test_worker_suffix
-            end
-          end
       end
     end
   end

lib/active_record/tenanted/database_configurations/tenant_config.rb

@@ -4,12 +4,24 @@ module ActiveRecord
   module Tenanted
     module DatabaseConfigurations
       class TenantConfig < ActiveRecord::DatabaseConfigurations::HashConfig
+        def initialize(...)
+          super
+          @config_adapter = nil
+        end
+
         def tenant
           configuration_hash.fetch(:tenant)
         end
 
+        def config_adapter
+          @config_adapter ||= ActiveRecord::Tenanted::DatabaseAdapter.new(self)
+        end
+
         def new_connection
-          ensure_database_directory_exists # adapter doesn't handle this if the database is a URI
+          # TODO: The Rails SQLite adapter doesn't handle directory creation for file: URIs. I would
+          # like to fix that upstream, and remove this line.
+          config_adapter.ensure_database_directory_exists
+
           super.tap { |conn| conn.tenant = tenant }
         end
 
@@ -36,20 +48,6 @@ def default_schema_cache_path(db_dir = "db")
             File.join(db_dir, "#{tenanted_config_name}_schema_cache.yml")
           end
         end
-
-        def database_path
-          configuration_hash[:database_path]
-        end
-
-        private
-          def ensure_database_directory_exists
-            return unless database_path
-
-            database_dir = File.dirname(database_path)
-            unless File.directory?(database_dir)
-              FileUtils.mkdir_p(database_dir)
-            end
-          end
       end
     end
   end

lib/active_record/tenanted/database_tasks.rb

@@ -6,34 +6,34 @@ module DatabaseTasks # :nodoc:
       extend self
 
       def migrate_all
-        raise ArgumentError, "Could not find a tenanted database" unless root_config = root_database_config
+        raise ArgumentError, "Could not find a tenanted database" unless config = base_config
 
-        tenants = root_config.tenants.presence || [ get_current_tenant ].compact
+        tenants = config.tenants.presence || [ get_current_tenant ].compact
         tenants.each do |tenant|
-          tenant_config = root_config.new_tenant_config(tenant)
+          tenant_config = config.new_tenant_config(tenant)
           migrate(tenant_config)
         end
       end
 
       def migrate_tenant(tenant_name = set_current_tenant)
-        raise ArgumentError, "Could not find a tenanted database" unless root_config = root_database_config
+        raise ArgumentError, "Could not find a tenanted database" unless config = base_config
 
-        tenant_config = root_config.new_tenant_config(tenant_name)
+        tenant_config = config.new_tenant_config(tenant_name)
 
         migrate(tenant_config)
       end
 
       def drop_all
-        raise ArgumentError, "Could not find a tenanted database" unless root_config = root_database_config
+        raise ArgumentError, "Could not find a tenanted database" unless config = base_config
 
-        root_config.tenants.each do |tenant|
-          db_config = root_config.new_tenant_config(tenant)
-          ActiveRecord::Tenanted::DatabaseAdapter.drop_database(db_config)
-          $stdout.puts "Dropped database '#{db_config.database_path}'" if verbose?
+        config.tenants.each do |tenant|
+          db_config = config.new_tenant_config(tenant)
+          db_config.config_adapter.drop_database
+          $stdout.puts "Dropped database '#{db_config.database}'" if verbose?
         end
       end
 
-      def root_database_config
+      def base_config
         db_configs = ActiveRecord::Base.configurations.configs_for(
           env_name: ActiveRecord::Tasks::DatabaseTasks.env,
           include_hidden: true

lib/active_record/tenanted/tenant.rb

@@ -101,8 +101,7 @@ def current_tenant=(tenant_name)
         end
 
         def tenant_exist?(tenant_name)
-          db_config = tenanted_root_config.new_tenant_config(tenant_name)
-          ActiveRecord::Tenanted::DatabaseAdapter.database_ready?(db_config)
+          tenanted_root_config.new_tenant_config(tenant_name).config_adapter.database_ready?
         end
 
         def with_tenant(tenant_name, prohibit_shard_swapping: true, &block)
@@ -121,12 +120,11 @@ def with_tenant(tenant_name, prohibit_shard_swapping: true, &block)
 
         def create_tenant(tenant_name, if_not_exists: false, &block)
           created_db = false
-          db_config = tenanted_root_config.new_tenant_config(tenant_name)
+          adapter = tenanted_root_config.new_tenant_config(tenant_name).config_adapter
 
-          ActiveRecord::Tenanted::DatabaseAdapter.acquire_ready_lock(db_config) do
-            unless ActiveRecord::Tenanted::DatabaseAdapter.database_exist?(db_config)
-
-              ActiveRecord::Tenanted::DatabaseAdapter.create_database(db_config)
+          adapter.acquire_ready_lock do
+            unless adapter.database_exist?
+              adapter.create_database
 
               with_tenant(tenant_name) do
                 connection_pool(schema_version_check: false)
@@ -136,7 +134,7 @@ def create_tenant(tenant_name, if_not_exists: false, &block)
               created_db = true
             end
           rescue
-            ActiveRecord::Tenanted::DatabaseAdapter.drop_database(db_config)
+            adapter.drop_database
             raise
           end
 
@@ -156,8 +154,7 @@ def destroy_tenant(tenant_name)
             end
           end
 
-          db_config = tenanted_root_config.new_tenant_config(tenant_name)
-          ActiveRecord::Tenanted::DatabaseAdapter.drop_database(db_config)
+          tenanted_root_config.new_tenant_config(tenant_name).config_adapter.drop_database
         end
 
         def tenants
@@ -209,7 +206,7 @@ def _create_tenanted_pool(schema_version_check: true) # :nodoc:
           tenant = current_tenant
           db_config = tenanted_root_config.new_tenant_config(tenant)
 
-          unless ActiveRecord::Tenanted::DatabaseAdapter.database_exist?(db_config)
+          unless db_config.config_adapter.database_exist?
             raise TenantDoesNotExistError, "The database for tenant #{tenant.inspect} does not exist."
           end
           pool = establish_connection(db_config)