Skip to content

Fix missing error information when running embedded migrations.#1993

Closed
sorccu wants to merge 1 commit intodiesel-rs:masterfrom
sorccu:fix-formatting-error-with-embedded-migrations
Closed

Fix missing error information when running embedded migrations.#1993
sorccu wants to merge 1 commit intodiesel-rs:masterfrom
sorccu:fix-formatting-error-with-embedded-migrations

Conversation

@sorccu
Copy link

@sorccu sorccu commented Feb 21, 2019

When using embedded migrations, all error information is currently lost due to a mysterious formatting error replacing the actual error. The error itself is not important, as long as it happens within the migration run. For example, the following migration:

up.sql

SELECT 1 FROM does_not_exist;

Executed with:

embedded_migrations::run_with_output(&conn, &mut std::io::stdout()).expect("unable to migrate");

Will result in the following output:

Running migration 20190221123919
thread 'main' panicked at 'unable to migrate: MigrationError(IoError(Custom { kind: Other, error: StringError("formatter error") }))', src/libcore/result.rs:997:5
note: Run with RUST_BACKTRACE=1 environment variable to display a backtrace.
Executing migration script

What SHOULD be shown is something along the lines of:

thread 'main' panicked at 'unable to migrate: QueryError(DatabaseError(__Unknown, "relation "does_not_exist" does not exist"))', src/libcore/result.rs:997:5

It turns out that the reason is very simple. Upon encountering an error, diesel attempts to display the filename of the failed migration:

if let Err(e) = migration.run(conn) {
writeln!(
output,
"Executing migration script {}",
file_name(&migration, "up.sql")
)?;
return Err(e);
}

Since the embedded migration's impl Migration is left without a file_path() implementation:

impl Migration for EmbeddedMigration {
fn version(&self) -> &str {
self.version
}
fn run(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
conn.batch_execute(self.up_sql).map_err(Into::into)
}
fn revert(&self, _conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
unreachable!()
}
}

... it returns the default None, which then triggers the formatting error here:

let fpath = match self.migration.file_path() {
None => return Err(fmt::Error),
Some(v) => v.join(self.sql_file),
};

And since the writeln!() for the error message uses ?, the formatting error from above ends up replacing the original error entirely.

This PR introduces a fake path based on the version identifier that lets things proceed as usual while providing useful information.

Fixes #1977

@weiznich weiznich requested a review from a team February 21, 2019 14:56
@sorccu
Copy link
Author

sorccu commented Feb 22, 2019

I believe that Travis simply timed out instead of actually reporting an error. Closing and opening the PR once hoping to trigger the build again.

@sorccu sorccu closed this Feb 22, 2019
@sorccu sorccu reopened this Feb 22, 2019
@sorccu
Copy link
Author

sorccu commented Feb 22, 2019

Same thing again. Trying once more.

@sorccu sorccu closed this Feb 22, 2019
@sorccu sorccu reopened this Feb 22, 2019
}

fn file_path(&self) -> Option<&Path> {
Some(Path::new(self.version))
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of returning a fake path here, we should fix the formatting to just print the version if the path is not set.

@JohnTitor
Copy link
Member

ping @sorccu, any updates?

geomaniac added a commit to geomaniac/diesel that referenced this pull request Sep 2, 2019
In Display impl for`MigrationName`, use the migration version if `Migration::file_path` returns None.

This is based on the work from @sorccu in diesel-rs#1993.

Fixes diesel-rs#1977.
geomaniac added a commit to geomaniac/diesel that referenced this pull request Sep 2, 2019
In Display impl for`MigrationName`, use the migration version if `Migration::file_path` returns None.

This is based on the work from @sorccu in diesel-rs#1993.

Fixes diesel-rs#1977.
geomaniac added a commit to geomaniac/diesel that referenced this pull request Sep 2, 2019
In Display impl for`MigrationName`, use the migration version if `Migration::file_path` returns None.

This is based on the work from @sorccu in diesel-rs#1993.

Fixes diesel-rs#1977.
@weiznich
Copy link
Member

weiznich commented Sep 3, 2019

Closed by #2159

@weiznich weiznich closed this Sep 3, 2019
disconsented pushed a commit to NarrativeApp/diesel that referenced this pull request Nov 26, 2020
In Display impl for`MigrationName`, use the migration version if `Migration::file_path` returns None.

This is based on the work from @sorccu in diesel-rs#1993.

Fixes diesel-rs#1977.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

formatter error

3 participants