Skip to content

Change to fail on load errors#19

Closed
mbj wants to merge 1 commit intoboustrophedon:masterfrom
mbj:fix/load-errors
Closed

Change to fail on load errors#19
mbj wants to merge 1 commit intoboustrophedon:masterfrom
mbj:fix/load-errors

Conversation

@mbj
Copy link
Copy Markdown
Contributor

@mbj mbj commented Mar 27, 2025

  • Before any errors would silently be ignored.
  • And the DB would be partially initialized.
  • This created very hard to debug states.

* Before any errors would silently be ignored.
* And the DB would be partially initialized.
* This created very hard to debug states.
@boustrophedon
Copy link
Copy Markdown
Owner

Hi! Thanks for the contribution, and for using pgtemp!

Could you add a test for this? You can copy and paste the setup from tests/dump.rs and then truncate or otherwise modify the file that's output so that it fails to load, or if you have a small sample broken load file maybe use that directly. Then I guess since we just panic on errors the easiest thing to do is to just mark the test as #[should_panic(<whatever the error message is when psql fails to load>)]

I also wonder if:

  • we could just have a generic "psql load options" that you could pass here rather than these specific options
  • could we add these options to the output of our dumps

but I doubt adding this will break any clients so that can happen later unless you feel like doing it now.

@2xic
Copy link
Copy Markdown
Contributor

2xic commented Apr 9, 2025

@mbj do you want help adding a test for this?

@mbj
Copy link
Copy Markdown
Contributor Author

mbj commented Apr 9, 2025

@mbj do you want help adding a test for this?

I'm to busy ATM to get back to that sorry. I do not need help, I need time ;) If you wanted to spend the time feel free to just close tis PR and take my single line change as a base.

@2xic
Copy link
Copy Markdown
Contributor

2xic commented Apr 18, 2025

I'm to busy ATM to get back to that sorry. I do not need help, I need time ;) If you wanted to spend the time feel free to just close tis PR and take my single line change as a base.

I just wrote a test for it, feel free to add it to your PR. If you don't have time for that, I can also fork your code.

(this test fails on main, but succeeds on your branch)

#[tokio::test]
/// make sure that we correctly error on bad database dumps.
#[should_panic(expected = "syntax error at or near \\\"INVALID\\")]
async fn panic_on_load_error() {
    let temp = tempfile::tempdir().unwrap();
    let db_dump_path = temp.path().join("dump.sql");

    // Create some bad database dumps
    let mut f = std::fs::File::create(&db_dump_path).unwrap();
    f.write_all(b"INVALID SQL").unwrap();
    f.flush().expect("Failed to flush file");

    // Try to load it (it should fail)
    PgTempDB::builder()
        .load_database(&db_dump_path)
        .start_async()
        .await;
}

@boustrophedon
Copy link
Copy Markdown
Owner

That test looks good, can @mbj confirm that's the expected error message they see with their corrupted files? Thanks both of you for the contributions!

@mbj
Copy link
Copy Markdown
Contributor Author

mbj commented Apr 21, 2025

That test looks good, can @mbj confirm that's the expected error message they see with their corrupted files? Thanks both of you for the contributions!

I think its good enough.

@boustrophedon boustrophedon mentioned this pull request Nov 10, 2025
@boustrophedon
Copy link
Copy Markdown
Owner

Fixed in #25 , thanks everyone!

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.

3 participants