-
Notifications
You must be signed in to change notification settings - Fork 24
MySQL adapter #246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
MySQL adapter #246
Changes from all commits
98169fd
ea0e14d
8466298
d975bd4
bbd4e53
4445329
1b87d51
1c6eda2
94e6d86
132e9c6
9da4474
ef0819d
5929062
e980a15
6c9121b
1b147f0
ce9f288
7b28b4b
f69a5c1
bda859a
7152457
d47aa8c
8418c24
31fe774
e004cd9
b22fa30
6f46023
798c40d
84fd830
bc76b0e
1027b04
8ce81ff
bb2b311
6431651
554f378
c9c421a
c47579c
18ca09c
cb16d87
2e8b21c
3c055d5
eabf8c8
ed0539c
76c2acf
41e1f23
745df5d
d238e6b
d8d75c9
1f8804b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| #!/usr/bin/env bash | ||
| set -e | ||
|
|
||
| CONTAINER_NAME="${CONTAINER_NAME:-activerecord-tenanted-mysql}" | ||
| MYSQL_IMAGE="mysql:latest" | ||
| MYSQL_ROOT_PASSWORD="devcontainer" | ||
| MYSQL_PORT="${MYSQL_PORT:-3307}" | ||
|
|
||
| echo "==> Checking Docker installation..." | ||
| if ! command -v docker &> /dev/null; then | ||
| echo "ERROR: Docker is not installed. Please install Docker and try again." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! docker info &> /dev/null; then | ||
| echo "ERROR: Docker daemon is not running. Please start Docker and try again." | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "==> Setting up MySQL container..." | ||
| if docker ps -a --format '{{.Names}}' | grep -q "^${CONTAINER_NAME}$"; then | ||
| echo " Stopping and removing existing container..." | ||
| docker stop "$CONTAINER_NAME" > /dev/null 2>&1 || true | ||
| docker rm "$CONTAINER_NAME" > /dev/null 2>&1 || true | ||
| fi | ||
|
|
||
| echo "==> Creating and starting MySQL container..." | ||
| docker run -d \ | ||
| --name="$CONTAINER_NAME" \ | ||
| --restart unless-stopped \ | ||
| -p "127.0.0.1:${MYSQL_PORT}:3306" \ | ||
| -e MYSQL_ROOT_PASSWORD="$MYSQL_ROOT_PASSWORD" \ | ||
| "$MYSQL_IMAGE" | ||
|
|
||
| echo "==> Waiting for MySQL to be ready..." | ||
| MAX_TRIES=30 | ||
| TRIES=0 | ||
| while [ $TRIES -lt $MAX_TRIES ]; do | ||
| if docker exec "$CONTAINER_NAME" mysqladmin ping -h localhost --silent &> /dev/null; then | ||
| echo " MySQL is ready!" | ||
| break | ||
| fi | ||
| TRIES=$((TRIES + 1)) | ||
| if [ $TRIES -eq $MAX_TRIES ]; then | ||
| echo "ERROR: MySQL failed to start within expected time" | ||
| exit 1 | ||
| fi | ||
| echo -n "." | ||
| sleep 2 | ||
| done | ||
|
|
||
| echo "==> Verifying database connection..." | ||
| docker exec "$CONTAINER_NAME" mysql -uroot -p"$MYSQL_ROOT_PASSWORD" -e "SELECT 1;" > /dev/null 2>&1 | ||
| echo " Database ready!" | ||
|
|
||
| echo "==> Running bundle install..." | ||
| bundle check > /dev/null 2>&1 || bundle install | ||
|
|
||
| echo "" | ||
| echo "==> Setup complete!" | ||
| echo "" | ||
| echo "MySQL is running on 127.0.0.1:${MYSQL_PORT}" | ||
| echo "" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,12 +17,14 @@ require "fileutils" | |
| require "tmpdir" | ||
| require "yaml" | ||
| require "open3" | ||
| require "erb" | ||
| require "debug" | ||
| require "active_support" | ||
| require "active_support/core_ext/object" # deep_dup | ||
|
|
||
| scenarios = Dir.glob("test/scenarios/*db/*record.rb").map do |path| | ||
| database, models = path.scan(%r{test/scenarios/(.+db)/(.+record).rb}).flatten | ||
| { database: database, models: models } | ||
| scenarios = Dir.glob("test/scenarios/*/*db/*record.rb").map do |path| | ||
| adapter, database, models = path.scan(%r{test/scenarios/(.+)/(.+db)/(.+record).rb}).flatten | ||
| { adapter: adapter, database: database, models: models } | ||
| end | ||
|
|
||
| COLOR_FG_BLUE = "\e[0;34m" | ||
|
|
@@ -49,28 +51,34 @@ def run_scenario(scenario) | |
| at_exit { FileUtils.remove_entry app_path } unless OPT_KEEP_DIR | ||
| puts "Creating integration app for #{COLOR_FG_BLUE}#{scenario}#{COLOR_RESET} at #{app_path}" | ||
|
|
||
| # Generate a unique database prefix for this scenario to avoid conflicts | ||
| db_prefix = "test_#{scenario[:database]}_#{scenario[:models]}_#{Process.pid}".gsub(/[^a-zA-Z0-9_]/, "_") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious what conflicts you're avoiding with this that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was having a bunch of contention issues before this when running integration tests in parallel. In SQLite each test run uses a unique temporary directory which means that multiple test runs never touched the same database file. But with MySQL it's different b/c we have the same shared server. What this means is that all parallel test runs originally used the same database name which caused collisions and contention. Different tests were trying to drop, create, or migrate using the same schema. There are different ways we could set this up but I just went with an env variable that gets evaluated at runtime ( I actually didn't realize you could run the unit tests in parallel and I didn't implement something similar for them. But the problem you're seeing running the unit tests is the same problem that this was supposed to address. |
||
|
|
||
| # make a copy of the smarty app | ||
| FileUtils.copy_entry(SMARTY_PATH, app_path) | ||
|
|
||
| # generate database file | ||
| database_file = File.join(GEM_PATH, "test/scenarios/#{scenario[:database]}/database.yml") | ||
| database_file_contents = sprintf(File.read(database_file), storage: "storage", db_path: "db") | ||
| database_file_hash = YAML.load(database_file_contents) | ||
| database_file = File.join(GEM_PATH, "test/scenarios/#{scenario[:adapter]}/#{scenario[:database]}/database.yml") | ||
| erb_content = ERB.new(File.read(database_file)).result | ||
| storage_path = File.join(app_path, "storage") | ||
| db_path = File.join(app_path, "db") | ||
| database_file_contents = sprintf(erb_content, storage: storage_path, db_path: db_path) | ||
| database_file_hash = YAML.unsafe_load(database_file_contents) | ||
| database_file_hash["development"] = database_file_hash["test"].deep_dup | ||
| database_file_hash["development"].each_value do |hash| | ||
| hash["database"] = hash["database"].sub("/test/", "/development/") | ||
| end | ||
| File.write(File.join(app_path, "config/database.yml"), database_file_hash.to_yaml) | ||
|
|
||
| # generate models using ApplicationRecord | ||
| models_file = File.join(GEM_PATH, "test/scenarios/#{scenario[:database]}/#{scenario[:models]}.rb") | ||
| models_file = File.join(GEM_PATH, "test/scenarios/#{scenario[:adapter]}/#{scenario[:database]}/#{scenario[:models]}.rb") | ||
| models_file_contents = File.read(models_file) | ||
| .gsub("TenantedApplicationRecord", "ApplicationRecord") | ||
| File.write(File.join(app_path, "app/models/application_record.rb"), models_file_contents) | ||
|
|
||
| # copy migrations from scenario and smarty app | ||
| FileUtils.mkdir_p(File.join(app_path, "db")) | ||
| FileUtils.cp_r(Dir.glob(File.join(GEM_PATH, "test/scenarios/#{scenario[:database]}/*migrations")), | ||
| FileUtils.cp_r(Dir.glob(File.join(GEM_PATH, "test/scenarios/#{scenario[:adapter]}/#{scenario[:database]}/*migrations")), | ||
| File.join(app_path, "db")) | ||
| FileUtils.cp_r(Dir.glob(File.join(app_path, "db/migrate/*rb")), | ||
| File.join(app_path, "db/tenanted_migrations")) | ||
|
|
@@ -82,20 +90,21 @@ def run_scenario(scenario) | |
| Bundler.with_original_env do | ||
| run_cmd(scenario, "bundle check || bundle install") | ||
|
|
||
| # set up the database and schema files | ||
| run_cmd(scenario, "bin/rails db:prepare") | ||
| adapter_config = {"RAILS_ENV" => "test","ARTENANT_SCHEMA_DUMP" => "1"} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On Can you say a bit more about this change?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this in b/c I kept getting this error when trying to run any database tasks. But I could easily be doing something wrong! |
||
| if scenario[:adapter] == "mysql" | ||
| adapter_config.merge!({ "MYSQL_UNIQUE_PREFIX" => db_prefix }) | ||
| end | ||
|
|
||
| # Drop existing databases to ensure fresh setup | ||
| run_cmd(scenario, "bin/rails db:drop", env: { **adapter_config }) rescue nil | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe only run this command when testing non-sqlite databases? |
||
|
|
||
| run_cmd(scenario, "bin/rails db:prepare", env: { **adapter_config }) | ||
|
|
||
| # validate-ish the setup | ||
| prefix = scenario[:database].match?(/\Asecondary/) ? "tenanted_" : "" | ||
| File.exist?("db/#{prefix}schema.rb") || abort("Schema dump not generated") | ||
| File.exist?("db/#{prefix}schema_cache.yml") || abort("Schema cache dump not generated") | ||
|
|
||
| # create a fake tenant database to validate that it is deleted in the test suite | ||
| dev_db = Dir.glob("storage/development/*/development-tenant/main.sqlite3").first | ||
| test_db = dev_db.gsub("/development/", "/test/").gsub("development-tenant", "delete-me") | ||
| FileUtils.mkdir_p(File.dirname(test_db)) | ||
| FileUtils.touch(test_db) | ||
|
|
||
|
Comment on lines
-93
to
-98
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this being removed? This test database needs to be here to make this test meaningful: And I think the mysql implementation also needs to delete existing tenant databases at the start of the test suite, though you may need to change this setup to something that will work for both databases, maybe: run_cmd(scenario, "bin/rails db:migrate", env: { ARTENANT: "delete-me" })
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may have misunderstood what this does. I just added in a db:drop at the start of the test run which I thought was a bit simpler / handles all database types. But the test you mentioned would be redundant then: https://github.com/basecamp/activerecord-tenanted/blob/main/test/integration/test/testcase_test.rb#L12-L15. We can keep this in if you want! But we still need to db:drop with mysql just to ensure that we're starting from a clean slate every time. |
||
| # run in parallel half the time | ||
| env = if rand(2).zero? | ||
| { "PARALLEL_WORKERS" => "2" } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,9 +19,10 @@ def connect | |
|
|
||
| private | ||
| def set_current_tenant | ||
| return unless tenant = tenant_resolver.call(request) | ||
| tenant = tenant_resolver.call(request) | ||
| return if tenant.nil? | ||
|
|
||
| if connection_class.tenant_exist?(tenant) | ||
| if tenant.present? && connection_class.tenant_exist?(tenant) | ||
|
Comment on lines
-22
to
+25
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting! Can you say a bit more about the distinction between nil and blank tenant names here in your application? Under what conditions does your tenant resolver return each of those? More specifically, why couldn't the method be rewritten like this to handle untenanted connections? def set_current_tenant
tenant = tenant_resolver.call(request)
if tenant.present?
if connection_class.tenant_exist?(tenant)
self.current_tenant = tenant
else
reject_unauthorized_connection
end
end
end |
||
| self.current_tenant = tenant | ||
| else | ||
| reject_unauthorized_connection | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,8 @@ def new(db_config) | |
| end | ||
|
|
||
| register "sqlite3", "ActiveRecord::Tenanted::DatabaseAdapters::SQLite" | ||
| register "trilogy", "ActiveRecord::Tenanted::DatabaseAdapters::MySQL" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eventually I'd like to run the mysql tests against trilogy. Doesn't need to be in this PR, though.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree! Right now we're using mysql2 in our app but we should be switching over to trilogy this year / early next. So I'll also be able to test it out IRL too. |
||
| register "mysql2", "ActiveRecord::Tenanted::DatabaseAdapters::MySQL" | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth setting up a matrix to test against 8 and 9? And maybe even patch releases like 8.4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could! I'm not that worried that we're doing anything too novel that would break b/w versions of mysql though. Probably testing against the latest version is good enough 🤷♂️. But I'm very open to doing that if you think it's a good idea.
We're really only doing
"SHOW DATABASES LIKE '#{database_path}'andDROP DATABASE IF EXISTSwhich will work across all versions. The rest of the stuff we're just calling the built-in Rails methods which are already well tested. Just not too worried about it I guess!