Don't obscure errors during migrations #577
Conversation
| @@ -96,16 +96,6 @@ abstract class Avram::Migrator::Migration::V1 | |||
| yield tx | |||
| end | |||
| end | |||
| rescue e : PQ::PQError | |||
There was a problem hiding this comment.
Any idea if the actual stack trace will include the statement that failed? I'm wondering if instead of removing this one, we just figure out how to add the filename to it. I'm worried letting it error on its own wouldn't really provide that nice of an error.
There was a problem hiding this comment.
So it wouldn't. One thing I was thinking about is handling all errors that are raised from migrations, wrapping them with more information (name of migration, statement that caused it), and printing them ourselves so that we can include the cause of exceptions and then exiting as a failure. What do you think?
There was a problem hiding this comment.
Calling https://crystal-lang.org/api/0.35.1/Exception.html#inspect_with_backtrace:String-instance-method on exceptions includes the cause, I don't know why Crystal doesn't use it in the unhandled exception code flow.
There was a problem hiding this comment.
Yeah, I like that. One thing that sets us apart from others is how we display really nice error messages. Do you want to do that in this PR, or a different one? I'm cool with either way.
There was a problem hiding this comment.
I'll update this PR. I believe this will help #578 (comment) as well
There was a problem hiding this comment.
@jwoertink done 👍 this is much better, I think
Also wrap exceptions so that we can point to the name of the migration and the statements
matthewmcgarvey
force-pushed
the
migration-errors
branch
from
31a8b9f to
bb3ea58
Compare
This allows specs to call tasks without running into the exit code
| @@ -66,7 +66,7 @@ end | |||
|
|
|||
| describe Avram::Migrator::Migration::V1 do | |||
| it "executes statements in a transaction" do | |||
| expect_raises Exception, %(relation "table_does_not_exist" does not exist) do | |||
| expect_raises Avram::FailedMigration do | |||
There was a problem hiding this comment.
Can no longer assert on the message because it is in the cause, not in the expected error
| raise e.message.to_s | ||
| rescue e | ||
| puts e.inspect_with_backtrace | ||
| exit 1 |
There was a problem hiding this comment.
This is to be able to print the error with the cause but it means we are explicitly exit-ing in code
| @@ -0,0 +1,9 @@ | |||
| abstract class BaseTask < LuckyCli::Task | |||
There was a problem hiding this comment.
This abstract class was pulled out so that we could test and use tasks without accidentally running into the exit code.
| @@ -3,19 +3,5 @@ require "./src/avram" | |||
| require "./config/*" | |||
| require "./db/migrations/*" | |||
|
|
|||
| class Db::Reset < LuckyCli::Task | |||
There was a problem hiding this comment.
There is already a separate Db::Reset task that was overwriting this one. This was old and I noticed it while updating tasks to the new pattern
Fixes #422
While the errors might look "uglier", they will now definitely contain the error message and the stacktrace.
Instead of dropping exceptions for new messages with a new stacktrace, this raises a new error with the original error as the cause so that we can add info without forgetting context. The only problem was that Crystal does not print the "cause" of unhandled exceptions (source). To overcome this, I updated
Avram::Migratorto print the error so that we can get the cause and then to exit so that it's still recognized as a failure.