Extract SQLite into an adapter #204

andrewmarkle · 2025-09-25T14:21:28Z

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 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.

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.

andrewmarkle · 2025-09-25T14:22:56Z

lib/active_record/tenanted/database_adapters/sqlite.rb

+          # 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


This is the only potential breaking change but I think it's a bug fix? This adds also removing the -wal and -shm when we drop the database.

I agree the gem should do this (the Rails SQLiteDatabaseTasks#drop method does this as well).

flavorjones · 2025-09-25T20:06:52Z

Thanks! I'll take a look in the next day or so.

andrewmarkle · 2025-09-29T15:27:01Z

I noticed a few issues since I first pushed this up but I got everything fixed up now. No rush or anything on the review but I'm done making changes now! 😂

flavorjones

This is great work, thank you so much for extracting all of this so nicely!

I made some comments, but they are almost all small things like naming, unnecessary variables, or breaking a method into multiple methods.

One thing I do think still remains to be extracted into the adapters: the BaseConfig#database_path_for method, which only exists because the SQLite :database value might be a URI.

The idea I have in mind is to remove the "pathiness" from the database configs:

delete the BaseConfig#database_path_for method
remove the :database_path value from the TenantConfig hash
move BaseConfig#coerce_path into the SQLite adapter
and change SQLite#get_database_path into an instance variable @database_path that's computed in the initializer by calling coerce_path

but we can take care of that in a second PR if you prefer to keep the complexity down!

flavorjones · 2025-10-04T16:27:10Z

lib/active_record/tenanted/database_configurations/base_config.rb

+        rescue
+          []


I'm curious why we're rescuing here instead of just letting any exceptions bubble up. Is there a specific exception type we can rescue (and a test we can add!) if there's a specific case you want to handle?

flavorjones · 2025-10-04T16:30:53Z

test/unit/database_adapter_test.rb

+    test "raises error for unsupported adapter" do
+      unsupported_config = create_config("mongodb")
+
+      error = assert_raises ActiveRecord::Tenanted::Error do


Let's please create a new exception type for this! Maybe ActiveRecord::Tenanted::UnsupportedDatabaseError?

flavorjones · 2025-10-04T16:38:22Z

lib/active_record/tenanted/database_adapter.rb

+          adapter_for(db_config).drop_database
+        end
+
+        def database_exists?(db_config, arguments = {})


Small nit: can we follow the convention set by the Ruby File class and use exist? instead of exists??

flavorjones · 2025-10-04T16:48:44Z

lib/active_record/tenanted/database_adapters/sqlite.rb

+      class SQLite
+        def initialize(db_config)
+          @db_config = db_config
+          @configuration_hash = db_config.configuration_hash


I don't think @configuration_hash is being used.

flavorjones · 2025-10-04T16:49:02Z

lib/active_record/tenanted/database_adapters/sqlite.rb

+        end
+
+        private
+          attr_reader :db_config, :db_configuration_hash


I also don't see db_configuration_hash being used.

flavorjones · 2025-10-04T17:39:31Z

lib/active_record/tenanted/database_adapters/sqlite.rb

+          database_dir = File.dirname(database_path)
+          FileUtils.mkdir_p(database_dir) unless File.directory?(database_dir)


I think explicitly creating the directory is unnecessary in practice because the ready lock creates it already. I'm OK keeping it for clarity/completeness but I wanted to mention that it's probably duplicative.

flavorjones · 2025-10-04T17:41:33Z

lib/active_record/tenanted/database_adapters/sqlite.rb

+          # 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


I agree the gem should do this (the Rails SQLiteDatabaseTasks#drop method does this as well).

flavorjones · 2025-10-04T17:43:56Z

lib/active_record/tenanted/database_adapters/sqlite.rb

+          glob = db_config.database_path_for("*")
+          scanner = Regexp.new(db_config.database_path_for("(.+)"))
+
+          Dir.glob(glob).filter_map do |path|


map -> filter_map is also probably a bug fix. 👏

flavorjones · 2025-10-04T17:51:11Z

lib/active_record/tenanted/database_adapters/sqlite.rb

+            if db_config.respond_to?(:database_path) && db_config.database_path
+              db_config.database_path
+            else
+              db_config.database
+            end


I think we only ever call this on a DatabaseConfigurations::TenantConfig? If that's true I don't think we need the fallback logic here and this can just be:

Suggested change

if db_config.respond_to?(:database_path) && db_config.database_path

db_config.database_path

else

db_config.database

end

db_config.database_path

and maybe the whole method goes away? WDYT?

flavorjones · 2025-10-04T17:56:02Z

lib/active_record/tenanted/database_tasks.rb

          root_config.database_path_for(tenant).tap do |path|
-            FileUtils.rm(path)
+            db_config = root_config.new_tenant_config(tenant)
+            ActiveRecord::Tenanted::DatabaseAdapter.drop_database(db_config)
            $stdout.puts "Dropped database '#{path}'" if verbose?
          end


I don't think this code should call root_config.database_path_for directly (especially since we don't use the yielded path other than for printing to stdout).

Extract SQLite into an adapter

31ffb94

andrewmarkle commented Sep 25, 2025

View reviewed changes

Andrew Markle added 2 commits September 26, 2025 11:54

Greatly simplify acquire_lock

2876422

Refine locking even more and fix mistake

ae00036

flavorjones requested changes Oct 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Extract SQLite into an adapter #204

Extract SQLite into an adapter #204

Uh oh!

andrewmarkle commented Sep 25, 2025

Uh oh!

andrewmarkle Sep 25, 2025

Uh oh!

flavorjones Oct 4, 2025

Uh oh!

flavorjones commented Sep 25, 2025

Uh oh!

andrewmarkle commented Sep 29, 2025

Uh oh!

flavorjones left a comment

Uh oh!

flavorjones Oct 4, 2025

Uh oh!

flavorjones Oct 4, 2025

Uh oh!

flavorjones Oct 4, 2025

Uh oh!

flavorjones Oct 4, 2025

Uh oh!

flavorjones Oct 4, 2025

Uh oh!

flavorjones Oct 4, 2025

Uh oh!

flavorjones Oct 4, 2025

Uh oh!

flavorjones Oct 4, 2025

Uh oh!

flavorjones Oct 4, 2025

Uh oh!

flavorjones Oct 4, 2025

Uh oh!

Uh oh!

		database_dir = File.dirname(database_path)
		FileUtils.mkdir_p(database_dir) unless File.directory?(database_dir)

Extract SQLite into an adapter #204

Are you sure you want to change the base?

Extract SQLite into an adapter #204

Uh oh!

Conversation

andrewmarkle commented Sep 25, 2025

New pattern that makes it possible to support multiple database adapters

No breaking changes

How I tested

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flavorjones commented Sep 25, 2025

Uh oh!

andrewmarkle commented Sep 29, 2025

Uh oh!

flavorjones left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!