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

Return Results from DB executor functions #109

Merged
merged 5 commits into from
Jun 8, 2023

Conversation

Ameobea
Copy link
Contributor

@Ameobea Ameobea commented Jun 6, 2023

Previously, the code for Executor::fetch_all and similar functions used .unwrap() internally and panicked if any error happened during the call. This could result in schema detection causing panics in cases of things like spurious network issues, connection pool exhaustion, and other transitory issues with the DB.

PR Info

Fixes #106

Breaking Changes

This introduces a breaking change to the crate's public API surface. Callers will need to be updated to handle the fact that schema detection returns Results now.

This is definitely unavoidable, but updating shouldn't be too difficult. Callers can just .unwrap() to retain the existing behavior of panicking in the case of any DB or connection error.

Changes

  • Migrate all of the executor functions to return Result<T, sqlx::Error> to bubble up errors
  • Update all schema detection functions to bubble up errors as well and migrate code to handle addition of Result return types

 * Previously, the code for `Executor::fetch_all` and similar functions used `.unwrap()` internally and panicked if any error happened during the call.  This could result in schema detection causing panics in cases of things like spurious network issues, connection pool exhaustion, and other transitory issues with the DB.
 * Migrate all of the executor functions to return `Result<T, sqlx::Error>` to bubble up errors
 * Update all schema detection functions to bubble up errors as well and migrate code to handle addition of `Result` return types
 * Note that this is a breaking change to the crate's public API surface.  Callers will need to be updated to handle the fact that schema detection returns `Result`s now.
 * Fixes SeaQL#106
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Hey @Ameobea, thanks for the contribution!! Nice update to the API!

@billy1624 billy1624 requested a review from tyt2y3 June 6, 2023 09:27
Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Very cool. Thank you

@tyt2y3 tyt2y3 changed the base branch from master to api-fix June 8, 2023 10:25
@tyt2y3 tyt2y3 merged commit bb6bee7 into SeaQL:api-fix Jun 8, 2023
tyt2y3 added a commit that referenced this pull request Jun 8, 2023
@github-actions
Copy link

🎉 Released In 0.12.0 🎉

Thank you everyone for the contribution!
This feature is now available in the latest release. Now is a good time to upgrade!
Your participation is what makes us unique; your adoption is what drives us forward.
You can support SeaQL 🌊 by starring our repos, sharing our libraries and becoming a sponsor ⭐.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Unhandled panics when detecting Postgres table schemas
3 participants