Skip to content
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

Removes the need for pg client tools #942

Merged
merged 3 commits into from
Jun 24, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions config/database.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
33 changes: 33 additions & 0 deletions spec/avram/migrator/runner_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fully closes the DB connection when the block ends allowing me to drop the 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
6 changes: 2 additions & 4 deletions spec/avram/tasks/db_create_spec.cr
Original file line number Diff line number Diff line change
@@ -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
Expand Down
10 changes: 7 additions & 3 deletions spec/avram/tasks/db_schema_restore_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
jwoertink marked this conversation as resolved.
Show resolved Hide resolved

# 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
jwoertink marked this conversation as resolved.
Show resolved Hide resolved
# make sure this is dropped before another spec runs
Avram::Migrator::Runner.drop_db(quiet?: true)
end
end
end
12 changes: 0 additions & 12 deletions spec/spec_helper.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Comment on lines -37 to -40
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this to config/database.cr where all the others are. It should have been there in the first place anyway 😂

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
Expand Down
9 changes: 8 additions & 1 deletion src/avram/connection.cr
Original file line number Diff line number Diff line change
@@ -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
Comment on lines 8 to 15
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand this. We've never had a db.close before, but it feels like there should have been... I think we just open a connection, then let it sit open in the connection pool forever. If you redeploy your app, that kills all of the connections which may explain why we always get IO errors randomly... But what's a better way to handle this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plausible explanation. That would solve the bulk of the use cases and the stuff that did generate errors would just get dismissed by restarting the container.

I think this is 👍


def connect_listen(*channels : String, &block : PQ::Notification ->) : Nil
Expand Down
16 changes: 12 additions & 4 deletions src/avram/credentials.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Comment on lines +85 to +86
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to run SQL before a database exists, we need the connection string postgres://user:pass@host:5432

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was just saying that I moved this logic out so I can differentiate between

postgres://user:pass@host:5432

and

postgres://user:pass@host:5432/some_db

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I get it. I thought you were saying that you were filling in some sample connection string details for type safety .... o_O I was way off...

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
Expand All @@ -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
Expand Down
6 changes: 6 additions & 0 deletions src/avram/database.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
jwoertink marked this conversation as resolved.
Show resolved Hide resolved

# :nodoc:
def listen(*channels : String, &block : PQ::Notification ->) : Nil
connection.connect_listen(*channels, &block)
Expand Down
32 changes: 17 additions & 15 deletions src/avram/migrator/runner.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
jwoertink marked this conversation as resolved.
Show resolved Hide resolved
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what level the issue is at, but when you can't connect, a PQ::PQError is raised with a nice helpful error... then that gets swallowed somewhere and a DB::ConnectionRefused is raised in it's place with no error message... so e.message is nil. They just expect you to read through the stack trace.

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)
Comment on lines +79 to 80
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think this won't ever get here now... I'll have to double check, but I believe this is the same as above. It raises a DB::ConnectionRefused error with no message, and then in the stack trace there's a "Caused by..."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole exception logic could use a refactor. For example, matching on exception message text is probably not necessary at all if quiet? is set. And I feel like matching on exception message text isn't a good idea anyway because I wouldn't ever consider that a stable API -- if I were to raise an exception I expect the message to be read by a human.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally. This definitely needs a refactor. The main idea here is that the error coming form postgres is generally not too helpful. I find myself usually having to google why I'm getting "could not connect to template1" and other weird errors. So in this case, we catch those, and re-raise a new helpful error that gives you a list of things to look for as to why it failed.

I'll think about this a bit more and circle back.

message << " ▸ Check connection settings in 'config/database.cr'\n"
message << " ▸ Be sure the database exists (lucky db.create)\n"
message << " ▸ Check that you have access to connect to #{connection_details.host} on port #{connection_details.port || DEFAULT_PG_PORT}\n"
if connection_details.password.blank?
message << " ▸ You didn't supply a password, did you mean to?\n"
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there's no reason it needs to be addressed in your pull here, it's out of scope.

else
raise e
Expand Down