From 268e0361afa8856cdbb8a6c9570ab8c0c91756bc Mon Sep 17 00:00:00 2001 From: Jeremy Woertink Date: Sun, 14 May 2023 18:42:27 -0700 Subject: [PATCH 1/3] Removes the need for pg client tools when running create and drop db commands. Ref #926 --- config/database.cr | 12 ++++++++ spec/avram/migrator/runner_spec.cr | 33 ++++++++++++++++++++++ spec/avram/tasks/db_create_spec.cr | 6 ++-- spec/avram/tasks/db_schema_restore_spec.cr | 10 +++++-- spec/spec_helper.cr | 12 -------- src/avram/connection.cr | 9 +++++- src/avram/credentials.cr | 16 ++++++++--- src/avram/database.cr | 6 ++++ src/avram/migrator/runner.cr | 32 +++++++++++---------- 9 files changed, 97 insertions(+), 39 deletions(-) diff --git a/config/database.cr b/config/database.cr index 393c066a8..710249982 100644 --- a/config/database.cr +++ b/config/database.cr @@ -15,6 +15,18 @@ TestDatabase.configure do |settings| ) end +class SampleBackupDatabase < Avram::Database +end + +SampleBackupDatabase.configure do |settings| + settings.credentials = Avram::Credentials.parse?(ENV["BACKUP_DATABASE_URL"]?) || Avram::Credentials.new( + hostname: "db", + database: "sample_backup", + username: "lucky", + password: "developer" + ) +end + DatabaseWithIncorrectSettings.configure do |settings| settings.credentials = Avram::Credentials.new( hostname: "db", diff --git a/spec/avram/migrator/runner_spec.cr b/spec/avram/migrator/runner_spec.cr index 51b660a7a..0627f7106 100644 --- a/spec/avram/migrator/runner_spec.cr +++ b/spec/avram/migrator/runner_spec.cr @@ -11,4 +11,37 @@ describe Avram::Migrator::Runner do end end end + + describe ".create_db" do + context "when the DB doesn't exist yet" do + it "creates the new DB" do + Avram.temp_config(database_to_migrate: SampleBackupDatabase) do + Avram::Migrator::Runner.create_db(quiet?: true) + + DB.open(SampleBackupDatabase.credentials.url) do |db| + results = db.query_all("SELECT datname FROM pg_database WHERE datistemplate = false", as: String) + results.should contain("sample_backup") + end + # ensure it's deleted before moving on to another spec + Avram::Migrator::Runner.drop_db(quiet?: true) + end + end + end + end + + describe ".drop_db" do + context "when it already exists" do + it "drops the DB" do + Avram.temp_config(database_to_migrate: SampleBackupDatabase) do + Avram::Migrator::Runner.create_db(quiet?: true) + Avram::Migrator::Runner.drop_db(quiet?: true) + + expect_raises(DB::ConnectionRefused) do + DB.open(SampleBackupDatabase.credentials.url) do |_| + end + end + end + end + end + end end diff --git a/spec/avram/tasks/db_create_spec.cr b/spec/avram/tasks/db_create_spec.cr index 3995cbc9d..77d6fe1b1 100644 --- a/spec/avram/tasks/db_create_spec.cr +++ b/spec/avram/tasks/db_create_spec.cr @@ -1,11 +1,9 @@ require "../../spec_helper" describe Db::Create do - # This is currently failing with the wrong message. - # it may be related to https://github.com/actions/virtual-environments/issues/4269 - pending "raises a connection error when unable to connect" do + it "raises a connection error when unable to connect" do Avram.temp_config(database_to_migrate: DatabaseWithIncorrectSettings) do - expect_raises(Exception, /It looks like Postgres is not running/) do + expect_raises(Avram::ConnectionError, /Failed to connect to database/) do Db::Create.new(quiet: true).run_task end end diff --git a/spec/avram/tasks/db_schema_restore_spec.cr b/spec/avram/tasks/db_schema_restore_spec.cr index cfd7c9217..2f80fb99c 100644 --- a/spec/avram/tasks/db_schema_restore_spec.cr +++ b/spec/avram/tasks/db_schema_restore_spec.cr @@ -17,15 +17,19 @@ describe Db::Schema::Restore do it "restores from the sample_backup file" do Avram.temp_config(database_to_migrate: SampleBackupDatabase) do - Db::Drop.new(quiet: true).run_task - Db::Create.new(quiet: true).run_task - + Avram::Migrator::Runner.create_db(quiet?: true) Db::Schema::Restore.new(SQL_DUMP_FILE).run_task SampleBackupDatabase.run do |db| value = db.scalar("SELECT COUNT(*) FROM sample_records").as(Int64) value.should eq 0 end + + # HACK: This is needed because the `run` command above + # opens the DB, and since it's never closed, we can't drop it + SampleBackupDatabase.close_connections + # make sure this is dropped before another spec runs + Avram::Migrator::Runner.drop_db(quiet?: true) end end end diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index 8d2dd6d33..5b9f844c0 100644 --- a/spec/spec_helper.cr +++ b/spec/spec_helper.cr @@ -34,18 +34,6 @@ Spec.before_each do Fiber.current.query_cache = LuckyCache::NullStore.new end -class SampleBackupDatabase < Avram::Database -end - -SampleBackupDatabase.configure do |settings| - settings.credentials = Avram::Credentials.parse?(ENV["BACKUP_DATABASE_URL"]?) || Avram::Credentials.new( - hostname: "db", - database: "sample_backup", - username: "lucky", - password: "developer" - ) -end - Lucky::Session.configure do |settings| settings.key = "_app_session" end diff --git a/src/avram/connection.cr b/src/avram/connection.cr index cb6c8bf9f..418e73e0b 100644 --- a/src/avram/connection.cr +++ b/src/avram/connection.cr @@ -1,10 +1,17 @@ # Handles the connection to the DB. class Avram::Connection + private getter db : DB::Database? = nil + def initialize(@connection_string : String, @database_class : Avram::Database.class) end def open : DB::Database - try_connection! + @db = try_connection! + end + + def close + @db.try(&.close) + @db = nil end def connect_listen(*channels : String, &block : PQ::Notification ->) : Nil diff --git a/src/avram/credentials.cr b/src/avram/credentials.cr index 35ba1ce8a..ec42a5722 100644 --- a/src/avram/credentials.cr +++ b/src/avram/credentials.cr @@ -80,6 +80,17 @@ class Avram::Credentials @query.try(&.strip).presence end + # This is the full connection string used + # to connect to the PostgreSQL server. + def connection_string : String + String.build do |io| + set_url_protocol(io) + set_url_creds(io) + set_url_host(io) + set_url_port(io) + end + end + # Returns the postgres connection string without # any query params. def url_without_query_params : String @@ -88,10 +99,7 @@ class Avram::Credentials private def build_url String.build do |io| - set_url_protocol(io) - set_url_creds(io) - set_url_host(io) - set_url_port(io) + io << connection_string set_url_db(io) set_url_query(io) end diff --git a/src/avram/database.cr b/src/avram/database.cr index 8612af05d..93ce793ea 100644 --- a/src/avram/database.cr +++ b/src/avram/database.cr @@ -158,6 +158,12 @@ abstract class Avram::Database end end + # Close all available connections as well as the DB + def self.close_connections + connections.values.map(&.close) + @@db.try(&.close) + end + # :nodoc: def listen(*channels : String, &block : PQ::Notification ->) : Nil connection.connect_listen(*channels, &block) diff --git a/src/avram/migrator/runner.cr b/src/avram/migrator/runner.cr index 768c4156a..7bfad35bd 100644 --- a/src/avram/migrator/runner.cr +++ b/src/avram/migrator/runner.cr @@ -48,33 +48,35 @@ class Avram::Migrator::Runner end def self.drop_db(quiet? : Bool = false) - run "dropdb #{cmd_args}" + DB.connect(credentials.connection_string) do |db| + db.exec "DROP DATABASE IF EXISTS #{db_name}" + end unless quiet? puts "Done dropping #{Avram::Migrator::Runner.db_name.colorize(:green)}" end - rescue e : Exception - if (message = e.message) && message.includes?(%("#{self.db_name}" does not exist)) - unless quiet? - puts "Already dropped #{self.db_name.colorize(:green)}" - end - else - raise e - end end def self.create_db(quiet? : Bool = false) - run "createdb #{cmd_args}" + DB.connect(credentials.connection_string) do |db| + db.exec "CREATE DATABASE #{db_name} WITH OWNER DEFAULT" + end unless quiet? - puts "Done creating #{Avram::Migrator::Runner.db_name.colorize(:green)}" + puts "Done creating #{db_name.colorize(:green)}" + end + rescue e : DB::ConnectionRefused + message = e.message.to_s + if message.blank? + raise ConnectionError.new(URI.parse(credentials.url_without_query_params), Avram.settings.database_to_migrate) + else + raise e end rescue e : Exception - if (message = e.message) && message.includes?(%("#{self.db_name}" already exists)) + message = e.message.to_s + if message.includes?(%("#{self.db_name}" already exists)) unless quiet? puts "Already created #{self.db_name.colorize(:green)}" end - elsif (message = e.message) && (message.includes?("createdb: not found") || message.includes?("No command 'createdb' found")) - raise PGClientNotInstalledError.new(message) - elsif (message = e.message) && message.includes?("could not connect to database template") + elsif message.includes?("Cannot establish connection") raise PGNotRunningError.new(message) else raise e From a7f9258b4fb281cdff7f7f1608791e54650e45af Mon Sep 17 00:00:00 2001 From: Jeremy Woertink Date: Sat, 20 May 2023 08:24:55 -0700 Subject: [PATCH 2/3] applyin some suggestions --- spec/avram/tasks/db_schema_restore_spec.cr | 23 ++++++++++++++-------- src/avram/database.cr | 2 +- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/spec/avram/tasks/db_schema_restore_spec.cr b/spec/avram/tasks/db_schema_restore_spec.cr index 2f80fb99c..8501c591b 100644 --- a/spec/avram/tasks/db_schema_restore_spec.cr +++ b/spec/avram/tasks/db_schema_restore_spec.cr @@ -16,20 +16,27 @@ describe Db::Schema::Restore do end it "restores from the sample_backup file" do - Avram.temp_config(database_to_migrate: SampleBackupDatabase) do - Avram::Migrator::Runner.create_db(quiet?: true) + swap_database_with_cleanup(SampleBackupDatabase) do Db::Schema::Restore.new(SQL_DUMP_FILE).run_task SampleBackupDatabase.run do |db| value = db.scalar("SELECT COUNT(*) FROM sample_records").as(Int64) value.should eq 0 end - - # HACK: This is needed because the `run` command above - # opens the DB, and since it's never closed, we can't drop it - SampleBackupDatabase.close_connections - # make sure this is dropped before another spec runs - Avram::Migrator::Runner.drop_db(quiet?: true) end end end + +private def swap_database_with_cleanup(database_to_migrate, &) + Avram.temp_config(database_to_migrate: database_to_migrate) do + # Create this new DB + Avram::Migrator::Runner.create_db(quiet?: true) + + yield + + # Ensure all connections to this DB are closed + SampleBackupDatabase.close_connections! + # make sure this is dropped before another spec runs + Avram::Migrator::Runner.drop_db(quiet?: true) + end +end diff --git a/src/avram/database.cr b/src/avram/database.cr index 93ce793ea..ebeef465a 100644 --- a/src/avram/database.cr +++ b/src/avram/database.cr @@ -159,7 +159,7 @@ abstract class Avram::Database end # Close all available connections as well as the DB - def self.close_connections + def self.close_connections! connections.values.map(&.close) @@db.try(&.close) end From 515e2eee4ac9d8db9760ac84abb146598d12bffc Mon Sep 17 00:00:00 2001 From: Jeremy Woertink Date: Sat, 20 May 2023 10:21:03 -0700 Subject: [PATCH 3/3] Adding the maintanence database defaulted to the username to ensure some DB is connected when there's no DB by the running username. --- src/avram/migrator/runner.cr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/avram/migrator/runner.cr b/src/avram/migrator/runner.cr index 7bfad35bd..bdd34d976 100644 --- a/src/avram/migrator/runner.cr +++ b/src/avram/migrator/runner.cr @@ -48,7 +48,7 @@ class Avram::Migrator::Runner end def self.drop_db(quiet? : Bool = false) - DB.connect(credentials.connection_string) do |db| + DB.connect("#{credentials.connection_string}/#{db_user}") do |db| db.exec "DROP DATABASE IF EXISTS #{db_name}" end unless quiet? @@ -57,7 +57,7 @@ class Avram::Migrator::Runner end def self.create_db(quiet? : Bool = false) - DB.connect(credentials.connection_string) do |db| + DB.connect("#{credentials.connection_string}/#{db_user}") do |db| db.exec "CREATE DATABASE #{db_name} WITH OWNER DEFAULT" end unless quiet?