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

fix(error handling): ORM-439 map query error to user facing error so it propagates correctly #5135

Closed
wants to merge 1 commit into from

Conversation

FGoessler
Copy link
Contributor

This PR fixes an issue where a generic QueryError would not propagate through our error handling all the way to the user. This lead to interactive transactions not failing (although they rolled back). Such QueryErrors happen in particular in case of failing DB triggers.

Fixes prisma/prisma#17303.

@FGoessler FGoessler requested a review from a team as a code owner January 24, 2025 09:46
@FGoessler FGoessler requested review from jacek-prisma and removed request for a team January 24, 2025 09:46
@FGoessler
Copy link
Contributor Author

FGoessler commented Jan 24, 2025

@jkomyno I had trouble finding a good spot to test this in the prisma-engines repo. Am adding an E2E test into the prisma/prisma repo for this instead (see prisma/prisma#26166). Or do you have a better idea / pointer for me? 🤔

Copy link
Contributor

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.120MiB 2.120MiB 363.000B
Postgres (gzip) 848.694KiB 848.485KiB 214.000B
Mysql 2.083MiB 2.082MiB 363.000B
Mysql (gzip) 834.538KiB 834.396KiB 145.000B
Sqlite 1.990MiB 1.990MiB 353.000B
Sqlite (gzip) 798.228KiB 797.975KiB 259.000B

Copy link

codspeed-hq bot commented Jan 24, 2025

CodSpeed Performance Report

Merging #5135 will not alter performance

Comparing fix/ORM-439-interactive-transaction-failure (275a7e7) with main (2ad07dd)

Summary

✅ 11 untouched benchmarks

ErrorKind::QueryError(e) => Some(KnownError::new(user_facing_errors::query_engine::QueryError {
message: format!("{}", e),
})),

_ => None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw I am a bit worried about this line here. Because there is a looong list of errors that we do NOT map here and hence likely tend to swallow somewhere upstreams?

List of not mapped errors:

ErrorKind::ConnectionError(_) => {}
ErrorKind::InvalidConnectionArguments => {}
ErrorKind::ColumnReadFailure(_) => {}
ErrorKind::FieldCannotBeNull { .. } => {}
ErrorKind::DomainError(_) => {}
ErrorKind::RecordNotFoundForWhere(_) => {}
ErrorKind::RelationViolation { .. } => {}
ErrorKind::RecordsNotConnected { .. } => {}
ErrorKind::InternalConversionError(_) => {}
ErrorKind::DatabaseCreationError(_) => {}
ErrorKind::DatabaseDoesNotExist { .. } => {}
ErrorKind::DatabaseAccessDenied { .. } => {}
ErrorKind::AuthenticationFailed { .. } => {}
ErrorKind::RawApiError(_) => {}
ErrorKind::RollbackWithoutBegin => {}
ErrorKind::UnsupportedConnector(_) => {}
ErrorKind::InvalidDriverAdapter(_) => {}
ErrorKind::UnexpectedDatabaseVersion { .. } => {}

@FGoessler
Copy link
Contributor Author

Actually seeing all the CI failures, I think I found a better an easier way to solve this issue. => Closing this PR.

@FGoessler FGoessler closed this Jan 24, 2025
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.

Prisma interactive transaction ignores DB exception on commit (with triggers)
1 participant