Extract SQLite into an adapter (#204)

### New pattern that makes it possible to support multiple database
adapters
This extracts anything SQLite-specific into a generic database adapter
class which forwards to a SQLite adapter class. This is a new pattern
that will make it pretty easy to support different databases (I've got
MySQL working in a different branch). Since this is a big change I
wanted to validate the approach first and get this merged in for SQLite.
But adding support for MYSQL is fairly trivial after this change.

This pattern is very similar to how Rails does it. In Rails there's the
main
[ActiveRecord::Tasks::DatabaseTasks](https://github.com/rails/rails/blob/main/activerecord/lib/active_record/tasks/database_tasks.rb)
which defines the API and then forwards the implementation over to the
specific adapter to do the work. For example here's the class for
[SQLite](https://github.com/rails/rails/blob/main/activerecord/lib/active_record/tasks/sqlite_database_tasks.rb).

Originally I tried tackling this by using the adapters / methods that
are provided by Rails but...it's just slightly different enough that it
won't work as is. I think there's a world where we could upstream a
bunch of these changes to into Rails and make it more compatible and/or
make this gem more compatible for how Rails does it. Either way I
thought it best to just do our own thing for now and cross that bridge
another day. 😊


### No breaking changes
There should be no actual changes from the current implementation for
SQLite—except for one place when we drop the db (I'll add a comment at
that line of code).

### How I tested
I relied on the test suite for this one and extracted everything without
breaking any tests. I also added tests for the adapter to ensure that it
forwards everything over to the correct adapter methods. I didn't unit
test the actual SQLite adapter since this is well covered by other tests
but I would be happy to add those as well.

Author: flavorjones

lib/active_record/tenanted.rb

@@ -4,6 +4,9 @@
 
 require "zeitwerk"
 loader = Zeitwerk::Loader.for_gem_extension(ActiveRecord)
+loader.inflector.inflect(
+  "sqlite" => "SQLite",
+)
 loader.setup
 
 module ActiveRecord
@@ -35,6 +38,9 @@ class TenantDoesNotExistError < Error; end
     # Raised when the Rails integration is being invoked but has not been configured.
     class IntegrationNotConfiguredError < Error; end
 
+    # Raised when an unsupported database adapter is used.
+    class UnsupportedDatabaseError < Error; end
+
     def self.connection_class
       # TODO: cache this / speed this up
       Rails.application.config.active_record_tenanted.connection_class&.constantize

lib/active_record/tenanted/database_adapter.rb

@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+module ActiveRecord
+  module Tenanted
+    class DatabaseAdapter # :nodoc:
+      ADAPTERS = {
+        "sqlite3" => "ActiveRecord::Tenanted::DatabaseAdapters::SQLite",
+      }.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)
+          adapter_class_name = ADAPTERS[db_config.adapter]
+
+          if adapter_class_name.nil?
+            raise ActiveRecord::Tenanted::UnsupportedDatabaseError,
+                  "Unsupported database adapter for tenanting: #{db_config.adapter}. " \
+                  "Supported adapters: #{ADAPTERS.keys.join(', ')}"
+          end
+
+          adapter_class_name.constantize.new(db_config)
+        end
+      end
+    end
+  end
+end

lib/active_record/tenanted/database_adapters/sqlite.rb

@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+module ActiveRecord
+  module Tenanted
+    module DatabaseAdapters
+      class SQLite # :nodoc:
+        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)
+
+          # Create the SQLite database file
+          FileUtils.touch(database_path)
+        end
+
+        def drop_database
+          # Remove the SQLite database file and associated files
+          FileUtils.rm_f(database_path)
+          FileUtils.rm_f("#{database_path}-wal")  # Write-Ahead Logging file
+          FileUtils.rm_f("#{database_path}-shm")  # Shared Memory file
+        end
+
+        def database_exist?
+          File.exist?(database_path)
+        end
+
+        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(db_config, &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
+
+        private
+          attr_reader :db_config
+
+          def database_path
+            db_config.database_path
+          end
+      end
+    end
+  end
+end

lib/active_record/tenanted/database_configurations/base_config.rb

@@ -36,17 +36,7 @@ def database_path_for(tenant_name)
         end
 
         def tenants
-          glob = database_path_for("*")
-          scanner = Regexp.new(database_path_for("(.+)"))
-
-          Dir.glob(glob).map do |path|
-            result = path.scan(scanner).flatten.first
-            if result.nil?
-              warn "WARN: ActiveRecord::Tenanted: Cannot parse tenant name from filename #{path.inspect}. " \
-                   "This is a bug, please report it to https://github.com/basecamp/activerecord-tenanted/issues"
-            end
-            result
-          end
+          ActiveRecord::Tenanted::DatabaseAdapter.tenant_databases(self)
         end
 
         def new_tenant_config(tenant_name)
@@ -85,9 +75,7 @@ def coerce_path(path)
           end
 
           def validate_tenant_name(tenant_name)
-            if tenant_name.match?(%r{[/'"`]})
-              raise BadTenantNameError, "Tenant name contains an invalid character: #{tenant_name.inspect}"
-            end
+            ActiveRecord::Tenanted::DatabaseAdapter.validate_tenant_name(self, tenant_name)
           end
 
           def test_worker_path(path)

lib/active_record/tenanted/database_tasks.rb

@@ -27,13 +27,9 @@ def drop_all
         raise ArgumentError, "Could not find a tenanted database" unless root_config = root_database_config
 
         root_config.tenants.each do |tenant|
-          # NOTE: This is obviously a sqlite-specific implementation.
-          # TODO: Create a `drop_database` method upstream in the sqlite3 adapter, and call it.
-          #       Then this would delegate to the adapter and become adapter-agnostic.
-          root_config.database_path_for(tenant).tap do |path|
-            FileUtils.rm(path)
-            $stdout.puts "Dropped database '#{path}'" if verbose?
-          end
+          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?
         end
       end
 

lib/active_record/tenanted/tenant.rb

@@ -101,10 +101,8 @@ def current_tenant=(tenant_name)
         end
 
         def tenant_exist?(tenant_name)
-          # this will have to be an adapter-specific implementation if we support other than sqlite
-          database_path = tenanted_root_config.database_path_for(tenant_name)
-
-          File.exist?(database_path) && !ActiveRecord::Tenanted::Mutex::Ready.locked?(database_path)
+          db_config = tenanted_root_config.new_tenant_config(tenant_name)
+          ActiveRecord::Tenanted::DatabaseAdapter.database_ready?(db_config)
         end
 
         def with_tenant(tenant_name, prohibit_shard_swapping: true, &block)
@@ -123,14 +121,12 @@ def with_tenant(tenant_name, prohibit_shard_swapping: true, &block)
 
         def create_tenant(tenant_name, if_not_exists: false, &block)
           created_db = false
-          database_path = tenanted_root_config.database_path_for(tenant_name)
+          db_config = tenanted_root_config.new_tenant_config(tenant_name)
+
+          ActiveRecord::Tenanted::DatabaseAdapter.acquire_ready_lock(db_config) do
+            unless ActiveRecord::Tenanted::DatabaseAdapter.database_exist?(db_config)
 
-          ActiveRecord::Tenanted::Mutex::Ready.lock(database_path) do
-            unless File.exist?(database_path)
-              # NOTE: This is obviously a sqlite-specific implementation.
-              # TODO: Add a `create_database` method upstream in the sqlite3 adapter, and call it.
-              #       Then this would delegate to the adapter and become adapter-agnostic.
-              FileUtils.touch(database_path)
+              ActiveRecord::Tenanted::DatabaseAdapter.create_database(db_config)
 
               with_tenant(tenant_name) do
                 connection_pool(schema_version_check: false)
@@ -140,7 +136,7 @@ def create_tenant(tenant_name, if_not_exists: false, &block)
               created_db = true
             end
           rescue
-            FileUtils.rm_f(database_path)
+            ActiveRecord::Tenanted::DatabaseAdapter.drop_database(db_config)
             raise
           end
 
@@ -160,10 +156,8 @@ def destroy_tenant(tenant_name)
             end
           end
 
-          # NOTE: This is obviously a sqlite-specific implementation.
-          # TODO: Create a `drop_database` method upstream in the sqlite3 adapter, and call it.
-          #       Then this would delegate to the adapter and become adapter-agnostic.
-          FileUtils.rm_f(tenanted_root_config.database_path_for(tenant_name))
+          db_config = tenanted_root_config.new_tenant_config(tenant_name)
+          ActiveRecord::Tenanted::DatabaseAdapter.drop_database(db_config)
         end
 
         def tenants
@@ -213,12 +207,12 @@ def _create_tenanted_pool(schema_version_check: true) # :nodoc:
           return superclass._create_tenanted_pool unless connection_class?
 
           tenant = current_tenant
-          unless File.exist?(tenanted_root_config.database_path_for(tenant))
-            raise TenantDoesNotExistError, "The database file for tenant #{tenant.inspect} does not exist."
-          end
+          db_config = tenanted_root_config.new_tenant_config(tenant)
 
-          config = tenanted_root_config.new_tenant_config(tenant)
-          pool = establish_connection(config)
+          unless ActiveRecord::Tenanted::DatabaseAdapter.database_exist?(db_config)
+            raise TenantDoesNotExistError, "The database for tenant #{tenant.inspect} does not exist."
+          end
+          pool = establish_connection(db_config)
 
           if schema_version_check
             pending_migrations = pool.migration_context.open.pending_migrations

test/unit/database_adapter_test.rb

@@ -0,0 +1,122 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+describe ActiveRecord::Tenanted::DatabaseAdapter do
+  describe ".adapter_for" do
+    test "selects correct adapter for sqlite3" do
+      adapter = ActiveRecord::Tenanted::DatabaseAdapter.adapter_for(create_config("sqlite3"))
+      assert_instance_of ActiveRecord::Tenanted::DatabaseAdapters::SQLite, adapter
+    end
+
+    test "raises error for unsupported adapter" do
+      unsupported_config = create_config("mongodb")
+
+      error = assert_raises ActiveRecord::Tenanted::UnsupportedDatabaseError do
+        ActiveRecord::Tenanted::DatabaseAdapter.adapter_for(unsupported_config)
+      end
+
+      assert_includes error.message, "Unsupported database adapter for tenanting: mongodb."
+    end
+  end
+
+  describe "delegation" do
+    ActiveRecord::Tenanted::DatabaseAdapter::ADAPTERS.each do |adapter, adapter_class_name|
+      test "#{adapter} .create_database calls adapter's #create_database" do
+        adapter_mock = Minitest::Mock.new
+        adapter_mock.expect(:create_database, nil)
+
+        adapter_class_name.constantize.stub(:new, adapter_mock) do
+          ActiveRecord::Tenanted::DatabaseAdapter.create_database(create_config(adapter))
+        end
+
+        assert_mock adapter_mock
+      end
+
+      test "#{adapter} .drop_database calls adapter's #drop_database" do
+        adapter_mock = Minitest::Mock.new
+        adapter_mock.expect(:drop_database, nil)
+
+        adapter_class_name.constantize.stub(:new, adapter_mock) do
+          ActiveRecord::Tenanted::DatabaseAdapter.drop_database(create_config(adapter))
+        end
+
+        assert_mock adapter_mock
+      end
+
+      test "#{adapter} .database_exist? calls adapter's #database_exist?" do
+        adapter_mock = Minitest::Mock.new
+        adapter_mock.expect(:database_exist?, true)
+
+        result = adapter_class_name.constantize.stub(:new, adapter_mock) do
+          ActiveRecord::Tenanted::DatabaseAdapter.database_exist?(create_config(adapter))
+        end
+
+        assert_equal true, result
+        assert_mock adapter_mock
+      end
+
+      test "#{adapter} .database_ready? calls adapter's #database_ready?" do
+        adapter_mock = Minitest::Mock.new
+        adapter_mock.expect(:database_ready?, true)
+
+        result = adapter_class_name.constantize.stub(:new, adapter_mock) do
+          ActiveRecord::Tenanted::DatabaseAdapter.database_ready?(create_config(adapter))
+        end
+
+        assert_equal true, result
+        assert_mock adapter_mock
+      end
+
+      test "#{adapter} .tenant_databases calls adapter's #tenant_databases" do
+        adapter_mock = Minitest::Mock.new
+        adapter_mock.expect(:tenant_databases, [ "foo", "bar" ])
+
+        result = adapter_class_name.constantize.stub(:new, adapter_mock) do
+          ActiveRecord::Tenanted::DatabaseAdapter.tenant_databases(create_config(adapter))
+        end
+
+        assert_equal [ "foo", "bar" ], result
+        assert_mock adapter_mock
+      end
+
+      test "#{adapter} .validate_tenant_name calls adapter's #validate_tenant_name" do
+        adapter_mock = Minitest::Mock.new
+        adapter_mock.expect(:validate_tenant_name, nil, [ "tenant1" ])
+
+        adapter_class_name.constantize.stub(:new, adapter_mock) do
+          ActiveRecord::Tenanted::DatabaseAdapter.validate_tenant_name(create_config(adapter), "tenant1")
+        end
+
+        assert_mock adapter_mock
+      end
+
+      test "#{adapter} .acquire_ready_lock calls adapter's #acquire_ready_lock" do
+        fake_adapter = Object.new
+        fake_adapter.define_singleton_method(:acquire_ready_lock) do |id, &blk|
+          blk&.call
+        end
+
+        yielded = false
+        result = adapter_class_name.constantize.stub(:new, fake_adapter) do
+          ActiveRecord::Tenanted::DatabaseAdapter.acquire_ready_lock(create_config(adapter)) { yielded = true; :ok }
+        end
+
+        assert_equal true, yielded
+        assert_equal :ok, result
+      end
+    end
+  end
+
+  private
+    def create_config(adapter)
+      ActiveRecord::DatabaseConfigurations::HashConfig.new(
+        "test",
+        "test_config",
+        {
+          adapter: adapter,
+          database: "db_name",
+        }
+      )
+    end
+end

test/unit/database_tasks_test.rb

@@ -14,10 +14,7 @@
   describe ".migrate_tenant" do
     for_each_scenario do
       setup do
-        # TODO: This should really be a create_database method on the sqlite3 adapter, see the notes
-        #       in Tenant.create_tenant.
-        FileUtils.mkdir_p(File.dirname(tenanted_config.database_path_for("foo")))
-        FileUtils.touch(tenanted_config.database_path_for("foo"))
+        ActiveRecord::Tenanted::DatabaseAdapter.create_database(tenanted_config.new_tenant_config("foo"))
       end
 
       test "database should be created" do