Move database path handling from BaseConfig into the SQLite adapter

Closes #214

Author: flavorjones

lib/active_record/tenanted/database_adapters/sqlite.rb

@@ -2,12 +2,44 @@
 
 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 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
+
         def create_database
           ensure_database_directory_exists
           FileUtils.touch(database_path)
@@ -28,29 +60,10 @@ 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("(.+)"))
-
-          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 acquire_ready_lock(&block)
           ActiveRecord::Tenanted::Mutex::Ready.lock(database_path, &block)
         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
-
         def ensure_database_directory_exists
           return unless database_path
 
@@ -60,25 +73,32 @@ def ensure_database_directory_exists
           end
         end
 
-        private
-          attr_reader :db_config
+        def database_path
+          path_for(db_config.database)
+        end
+
+        def test_workerize(db, test_worker_id)
+          test_worker_suffix = "_#{test_worker_id}"
 
-          def database_path
-            coerce_path(db_config.database)
+          if db.start_with?("file:") && db.include?("?")
+            db.sub(/(\?.*)$/, "#{test_worker_suffix}\\1")
+          else
+            db + test_worker_suffix
           end
+        end
 
-          # 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
+        # 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

@@ -25,19 +25,15 @@ 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
@@ -64,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)
-            config_adapter.validate_tenant_name(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

@@ -48,10 +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
       end
     end
   end

test/unit/database_adapters_sqlite_test.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+describe ActiveRecord::Tenanted::DatabaseAdapters::SQLite do
+  let(:adapter) { ActiveRecord::Tenanted::DatabaseAdapters::SQLite.new(Object.new) }
+  let(:dir) { Dir.mktmpdir }
+
+  describe "path_for" do
+    test "file path" do
+      database = "storage/db/tenanted/foo/main.sqlite3"
+      expected = "storage/db/tenanted/foo/main.sqlite3"
+      assert_equal(expected, adapter.path_for(database))
+    end
+
+    test "absolute URI" do
+      database = "file:#{dir}/storage/db/tenanted/foo/main.sqlite3"
+      expected = "#{dir}/storage/db/tenanted/foo/main.sqlite3"
+      assert_equal(expected, adapter.path_for(database))
+    end
+
+    test "absolute URI with query params" do
+      database = "file:#{dir}/storage/db/tenanted/foo/main.sqlite3?vfs=unix-dotfile"
+      expected = "#{dir}/storage/db/tenanted/foo/main.sqlite3"
+      assert_equal(expected, adapter.path_for(database))
+    end
+
+    test "relative URI" do
+      database = "file:storage/db/tenanted/foo/main.sqlite3"
+      expected = "storage/db/tenanted/foo/main.sqlite3"
+      assert_equal(expected, adapter.path_for(database))
+    end
+
+    test "relative URI with query params" do
+      database = "file:storage/db/tenanted/foo/main.sqlite3?vfs=unix-dotfile"
+      expected = "storage/db/tenanted/foo/main.sqlite3"
+      assert_equal(expected, adapter.path_for(database))
+    end
+  end
+end