Merge pull request #137 from basecamp/flavorjones/path-based-tenanting

Improve testing framework support for parallel testing

Author: flavorjones

GUIDE.md

@@ -24,7 +24,7 @@ Documentation outline:
   - support and documentation for Solid Cache, Solid Queue, Solid Cable, and Turbo Rails
 - a Tenant is just a string that is used for:
   - the sqlite database filename (or perhaps the pg/mysql database name in the future)
-  - the subdomain (or path element)
+  - configuring either tenant-by-subdomain or a tenant-by-root-path-element
   - fragment cache disambiguation
   - global id disambiguation
   - invalid characters in a tenant name
@@ -73,9 +73,12 @@ Documentation outline:
     - `tenant_resolver`
     - `tenanted_rails_records`
     - `log_tenant_tag`
-  - demonstrate how to configure an app for subdomain tenants
+  - demonstrate how to configure an app with subdomain tenanting
     - app.config.hosts
-    - example TenantSelector
+    - example TenantSelector config
+  - demonstrate how to configure an app with root path tenanting
+    - app.config.hosts
+    - example TenantSelector config
 
 - migrations
   - create_tenant migrates the new database
@@ -251,7 +254,7 @@ TODO:
   - [x] tenanted_rails_records
 
 - additional configuration
-  - [ ] default_tenant (development only)
+  - [x] default_tenant (local only)
 
 
 ### Tenanting in your application

bin/test-unit

@@ -5,3 +5,5 @@ ENV["DEFAULT_TEST"] ||= "test/unit/**_test.rb"
 
 require "bundler/setup"
 require "rails/plugin/test"
+
+Rails::TestUnitReporter.executable = __FILE__

lib/active_record/tenanted/database_configurations.rb

@@ -6,6 +6,13 @@ module ActiveRecord
   module Tenanted
     module DatabaseConfigurations
       class RootConfig < ActiveRecord::DatabaseConfigurations::HashConfig
+        attr_accessor :test_worker_id
+
+        def initialize(...)
+          super
+          @test_worker_id = nil
+        end
+
         def database_tasks?
           false
         end
@@ -16,16 +23,27 @@ def database_for(tenant_name)
             raise BadTenantNameError, "Tenant name contains an invalid character: #{tenant_name.inspect}"
           end
 
-          sprintf(database, tenant: tenant_name)
+          path = sprintf(database, tenant: tenant_name)
+
+          if test_worker_id
+            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
+
+          path
         end
 
         def database_path_for(tenant_name)
           coerce_path(database_for(tenant_name))
         end
 
         def tenants
-          glob = coerce_path(sprintf(database, tenant: "*"))
-          scanner = Regexp.new(coerce_path(sprintf(database, tenant: "(.+)")))
+          glob = database_path_for("*")
+          scanner = Regexp.new(database_path_for("(.+)"))
 
           Dir.glob(glob).map do |path|
             result = path.scan(scanner).flatten.first

lib/active_record/tenanted/database_tasks.rb

@@ -45,17 +45,13 @@ def root_database_config
         db_configs.detect { |c| c.configuration_hash[:tenanted] }
       end
 
-      def default_tenant
-        "#{Rails.env}-tenant"
-      end
-
       def get_current_tenant
         tenant = ENV["ARTENANT"]
 
         if tenant.present?
           $stdout.puts "Setting current tenant to #{tenant.inspect}" if verbose?
         elsif Rails.env.local?
-          tenant = default_tenant
+          tenant = Rails.application.config.active_record_tenanted.default_tenant
           $stdout.puts "Defaulting current tenant to #{tenant.inspect}" if verbose?
         else
           tenant = nil

lib/active_record/tenanted/railtie.rb

@@ -46,6 +46,17 @@ class Railtie < ::Rails::Railtie
       # Defaults to false in development and test environments, and true in all other environments.
       config.active_record_tenanted.log_tenant_tag = !Rails.env.local?
 
+      # Set this to override the default tenant name used in development and test environments.
+      #
+      # This is the default tenant name used by database tasks and in the Rails console. In both
+      # cases, this can be overridden at runtime by setting the `ARTENANT` environment variable.
+      #
+      # Notably, it's also the tenant name used by the testing frameworks, so you may need to set
+      # this if you have application-specific constraints on tenant names.
+      #
+      # Defaults to "development-tenant" in development and "test-tenant" in test environments.
+      config.active_record_tenanted.default_tenant = Rails.env.local? ? "#{Rails.env}-tenant" : nil
+
       config.before_initialize do
         Rails.application.configure do
           if config.active_record_tenanted.connection_class.present?

lib/active_record/tenanted/tenant.rb

@@ -123,19 +123,18 @@ def create_tenant(tenant_name, if_not_exists: false, &block)
         end
 
         def destroy_tenant(tenant_name)
-          return unless tenant_exist?(tenant_name)
+          ActiveRecord::Base.logger.info "  DESTROY [tenant=#{tenant_name}] Destroying tenant database"
 
-          with_tenant(tenant_name) do
-            connection_pool(schema_version_check: false)
-            lease_connection.send(:log, "/* destroying tenant database */", "DESTROY [tenant=#{tenant_name}]")
-          ensure
-            remove_connection
+          with_tenant(tenant_name, prohibit_shard_swapping: false) do
+            if retrieve_connection_pool(strict: false)
+              remove_connection
+            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(tenanted_root_config.database_path_for(tenant_name))
+          FileUtils.rm_f(tenanted_root_config.database_path_for(tenant_name))
         end
 
         def tenants

lib/active_record/tenanted/testing.rb

@@ -8,22 +8,31 @@ module ActiveSupportTestCase
 
         prepended do
           if klass = ActiveRecord::Tenanted.connection_class
-            klass.current_tenant = "#{Rails.env}-tenant"
-            klass.destroy_tenant(klass.current_tenant)
-            klass.create_tenant(klass.current_tenant)
+            klass.current_tenant = Rails.application.config.active_record_tenanted.default_tenant
+            tenant_genesis(klass)
 
             parallelize_setup do |worker|
-              klass.current_tenant = "#{Rails.env}-tenant-#{worker}"
+              # free up the connection pool for the tenant name
               klass.destroy_tenant(klass.current_tenant)
-              klass.create_tenant(klass.current_tenant)
-            end
 
-            # clean up any non-default tenants left over from the last test run
-            klass.tenants.each do |tenant|
-              klass.destroy_tenant(tenant) unless tenant.start_with?("#{Rails.env}-tenant")
+              # destroy and create tenant databases for this worker's unique id
+              klass.tenanted_root_config.test_worker_id = worker
+              tenant_genesis(klass)
             end
           end
         end
+
+        class_methods do
+          private
+            # Destroy any existing tenants from the last test run, and create a fresh tenant
+            # database for the current tenant.
+            #
+            # Yes, this is a Star Trek reference.
+            def tenant_genesis(klass)
+              klass.tenants.each { |tenant| klass.destroy_tenant(tenant) }
+              klass.create_tenant(klass.current_tenant)
+            end
+        end
       end
 
       module ActionDispatchIntegrationTest
@@ -77,7 +86,7 @@ def transactional_tests_for_pool?(pool)
           # 2. having an open transaction will prevent the test from being able to destroy the tenant.
           is_non_default_tenant = (
             config.instance_of?(Tenanted::DatabaseConfigurations::TenantConfig) &&
-            !config.tenant.start_with?("#{Rails.env}-tenant")
+            config.tenant != Rails.application.config.active_record_tenanted.default_tenant.to_s
           )
 
           return false if is_root_config || is_non_default_tenant

test/unit/database_configurations_test.rb

@@ -37,6 +37,12 @@
           assert_equal("storage/db/tenanted/foo/main.sqlite3", config.database_path_for("foo"))
         end
 
+        test "parallel test workers have unique files" do
+          config.test_worker_id = 99
+
+          assert_equal("storage/db/tenanted/foo/main.sqlite3_99", config.database_path_for("foo"))
+        end
+
         test "raises if the tenant name contains a path separator" do
           assert_raises(ActiveRecord::Tenanted::BadTenantNameError) { config.database_path_for("foo/bar") }
         end
@@ -50,8 +56,23 @@
         test "returns all tenants" do
           Dir.chdir(dir) do
             [ "foo", "bar", "baz" ].each do |tenant|
-              FileUtils.mkdir_p("storage/db/tenanted/#{tenant}")
-              FileUtils.touch("storage/db/tenanted/#{tenant}/main.sqlite3")
+              path = config.database_path_for(tenant)
+              FileUtils.mkdir_p(File.dirname(path))
+              FileUtils.touch(path)
+            end
+
+            assert_equal(Set.new(config.tenants), Set.new([ "foo", "bar", "baz" ]))
+          end
+        end
+
+        test "parallel test worker returns all tenants" do
+          config.test_worker_id = 99
+
+          Dir.chdir(dir) do
+            [ "foo", "bar", "baz" ].each do |tenant|
+              path = config.database_path_for(tenant)
+              FileUtils.mkdir_p(File.dirname(path))
+              FileUtils.touch(path)
             end
 
             assert_equal(Set.new(config.tenants), Set.new([ "foo", "bar", "baz" ]))
@@ -68,16 +89,38 @@
           assert_equal("#{dir}/storage/db/tenanted/foo/main.sqlite3", config.database_path_for("foo"))
         end
 
+        test "parallel test workers have unique files" do
+          config.test_worker_id = 99
+
+          assert_equal("file:#{dir}/storage/db/tenanted/foo/main.sqlite3_99", config.database_for("foo"))
+          assert_equal("#{dir}/storage/db/tenanted/foo/main.sqlite3_99", config.database_path_for("foo"))
+        end
+
         test "returns all tenants" do
           Dir.chdir(dir) do
             [ "foo", "bar", "baz" ].each do |tenant|
-              FileUtils.mkdir_p("storage/db/tenanted/#{tenant}")
-              FileUtils.touch("storage/db/tenanted/#{tenant}/main.sqlite3")
+              path = config.database_path_for(tenant)
+              FileUtils.mkdir_p(File.dirname(path))
+              FileUtils.touch(path)
             end
           end
 
           assert_equal(Set.new(config.tenants), Set.new([ "foo", "bar", "baz" ]))
         end
+
+        test "parallel test worker returns all tenants" do
+          config.test_worker_id = 99
+
+          Dir.chdir(dir) do
+            [ "foo", "bar", "baz" ].each do |tenant|
+              path = config.database_path_for(tenant)
+              FileUtils.mkdir_p(File.dirname(path))
+              FileUtils.touch(path)
+            end
+
+            assert_equal(Set.new(config.tenants), Set.new([ "foo", "bar", "baz" ]))
+          end
+        end
       end
 
       describe "absolute URI with query params" do
@@ -89,16 +132,38 @@
           assert_equal("#{dir}/storage/db/tenanted/foo/main.sqlite3", config.database_path_for("foo"))
         end
 
+        test "parallel test workers have unique files" do
+          config.test_worker_id = 99
+
+          assert_equal("file:#{dir}/storage/db/tenanted/foo/main.sqlite3_99?vfs=unix-dotfile", config.database_for("foo"))
+          assert_equal("#{dir}/storage/db/tenanted/foo/main.sqlite3_99", config.database_path_for("foo"))
+        end
+
         test "returns all tenants" do
           Dir.chdir(dir) do
             [ "foo", "bar", "baz" ].each do |tenant|
-              FileUtils.mkdir_p("storage/db/tenanted/#{tenant}")
-              FileUtils.touch("storage/db/tenanted/#{tenant}/main.sqlite3")
+              path = config.database_path_for(tenant)
+              FileUtils.mkdir_p(File.dirname(path))
+              FileUtils.touch(path)
             end
           end
 
           assert_equal(Set.new(config.tenants), Set.new([ "foo", "bar", "baz" ]))
         end
+
+        test "parallel test worker returns all tenants" do
+          config.test_worker_id = 99
+
+          Dir.chdir(dir) do
+            [ "foo", "bar", "baz" ].each do |tenant|
+              path = config.database_path_for(tenant)
+              FileUtils.mkdir_p(File.dirname(path))
+              FileUtils.touch(path)
+            end
+
+            assert_equal(Set.new(config.tenants), Set.new([ "foo", "bar", "baz" ]))
+          end
+        end
       end
 
       describe "relative URI" do
@@ -110,11 +175,33 @@
           assert_equal("storage/db/tenanted/foo/main.sqlite3", config.database_path_for("foo"))
         end
 
+        test "parallel test workers have unique files" do
+          config.test_worker_id = 99
+
+          assert_equal("file:storage/db/tenanted/foo/main.sqlite3_99", config.database_for("foo"))
+          assert_equal("storage/db/tenanted/foo/main.sqlite3_99", config.database_path_for("foo"))
+        end
+
         test "returns all tenants" do
           Dir.chdir(dir) do
             [ "foo", "bar", "baz" ].each do |tenant|
-              FileUtils.mkdir_p("storage/db/tenanted/#{tenant}")
-              FileUtils.touch("storage/db/tenanted/#{tenant}/main.sqlite3")
+              path = config.database_path_for(tenant)
+              FileUtils.mkdir_p(File.dirname(path))
+              FileUtils.touch(path)
+            end
+
+            assert_equal(Set.new(config.tenants), Set.new([ "foo", "bar", "baz" ]))
+          end
+        end
+
+        test "parallel test worker returns all tenants" do
+          config.test_worker_id = 99
+
+          Dir.chdir(dir) do
+            [ "foo", "bar", "baz" ].each do |tenant|
+              path = config.database_path_for(tenant)
+              FileUtils.mkdir_p(File.dirname(path))
+              FileUtils.touch(path)
             end
 
             assert_equal(Set.new(config.tenants), Set.new([ "foo", "bar", "baz" ]))
@@ -131,6 +218,13 @@
           assert_equal("storage/db/tenanted/foo/main.sqlite3", config.database_path_for("foo"))
         end
 
+        test "parallel test workers have unique files" do
+          config.test_worker_id = 99
+
+          assert_equal("file:storage/db/tenanted/foo/main.sqlite3_99?vfs=unix-dotfile", config.database_for("foo"))
+          assert_equal("storage/db/tenanted/foo/main.sqlite3_99", config.database_path_for("foo"))
+        end
+
         test "returns all tenants" do
           Dir.chdir(dir) do
             [ "foo", "bar", "baz" ].each do |tenant|
@@ -141,6 +235,20 @@
             assert_equal(Set.new(config.tenants), Set.new([ "foo", "bar", "baz" ]))
           end
         end
+
+        test "parallel test worker returns all tenants" do
+          config.test_worker_id = 99
+
+          Dir.chdir(dir) do
+            [ "foo", "bar", "baz" ].each do |tenant|
+              path = config.database_path_for(tenant)
+              FileUtils.mkdir_p(File.dirname(path))
+              FileUtils.touch(path)
+            end
+
+            assert_equal(Set.new(config.tenants), Set.new([ "foo", "bar", "baz" ]))
+          end
+        end
       end
     end
 

test/unit/tenant_test.rb

@@ -620,7 +620,7 @@
           log = capture_log do
             TenantedApplicationRecord.destroy_tenant("foo")
           end
-          assert_includes(log.string, "destroying tenant database")
+          assert_includes(log.string, "Destroying tenant database")
           assert_includes(log.string, "DESTROY [tenant=foo]")
         end