From d96b0d44b6e7486a913a2869d52353581d2c581a Mon Sep 17 00:00:00 2001 From: Serhii Tatarintsev Date: Fri, 22 Sep 2023 17:06:28 +0200 Subject: [PATCH] driver-adapters: Map neon errors to Prisma errors (#4213) * driver-adapters: Map neon errors to Prisma errors Approach taken: instead of duplicating error handling logic in TS, we extract all information we need from the error and forward it to engine. Engine is than reuses error parsing code from native driver. `PostgresError` is intermediate form for storing this information. It can be created from eithe `tokio_postgres` error or from JS. It then gets parsed and converted to a `quaint:Error`. This allows to handle most of the DB level errors (read: everything that actually reached running DB). It does not handle TLS and networking errors since these look completely different in JS. We'll need to handle them later. * Add hint & severity fields on TS side --- quaint/src/connector/postgres.rs | 2 +- quaint/src/connector/postgres/error.rs | 414 +++++++++--------- quaint/src/error.rs | 2 + .../js/adapter-neon/src/neon.ts | 56 ++- .../js/driver-adapter-utils/src/types.ts | 8 + .../smoke-test-js/prisma/mysql/schema.prisma | 4 + .../prisma/postgres/schema.prisma | 4 + .../js/smoke-test-js/src/libquery/libquery.ts | 32 +- query-engine/driver-adapters/src/result.rs | 22 +- 9 files changed, 287 insertions(+), 257 deletions(-) diff --git a/quaint/src/connector/postgres.rs b/quaint/src/connector/postgres.rs index d4dc008bd5f..c35208f8419 100644 --- a/quaint/src/connector/postgres.rs +++ b/quaint/src/connector/postgres.rs @@ -1,5 +1,5 @@ mod conversion; -mod error; +pub mod error; use crate::{ ast::{Query, Value}, diff --git a/quaint/src/connector/postgres/error.rs b/quaint/src/connector/postgres/error.rs index 40634e2aa33..4f7bb23a5c8 100644 --- a/quaint/src/connector/postgres/error.rs +++ b/quaint/src/connector/postgres/error.rs @@ -1,37 +1,64 @@ +use std::fmt::{Display, Formatter}; + +use tokio_postgres::error::DbError; + use crate::error::{DatabaseConstraint, Error, ErrorKind, Name}; -impl From for Error { - fn from(e: tokio_postgres::error::Error) -> Error { - use tokio_postgres::error::DbError; +#[derive(Debug)] +pub struct PostgresError { + pub code: String, + pub message: String, + pub severity: String, + pub detail: Option, + pub column: Option, + pub hint: Option, +} - if e.is_closed() { - return Error::builder(ErrorKind::ConnectionClosed).build(); +impl std::error::Error for PostgresError {} + +impl Display for PostgresError { + // copy of DbError::fmt + fn fmt(&self, fmt: &mut Formatter<'_>) -> std::fmt::Result { + write!(fmt, "{}: {}", self.severity, self.message)?; + if let Some(detail) = &self.detail { + write!(fmt, "\nDETAIL: {}", detail)?; + } + if let Some(hint) = &self.hint { + write!(fmt, "\nHINT: {}", hint)?; } + Ok(()) + } +} - match e.code().map(|c| c.code()) { - Some(code) if code == "22001" => { - let code = code.to_string(); +impl From<&DbError> for PostgresError { + fn from(value: &DbError) -> Self { + PostgresError { + code: value.code().code().to_string(), + severity: value.severity().to_string(), + message: value.message().to_string(), + detail: value.detail().map(ToString::to_string), + column: value.column().map(ToString::to_string), + hint: value.hint().map(ToString::to_string), + } + } +} +impl From for Error { + fn from(value: PostgresError) -> Self { + match value.code.as_str() { + "22001" => { let mut builder = Error::builder(ErrorKind::LengthMismatch { column: Name::Unavailable, }); - builder.set_original_code(code); - - let db_error = e.into_source().and_then(|e| e.downcast::().ok()); - if let Some(db_error) = db_error { - builder.set_original_message(db_error.to_string()); - } + builder.set_original_code(&value.code); + builder.set_original_message(value.to_string()); builder.build() } - Some(code) if code == "23505" => { - let code = code.to_string(); - - let db_error = e.into_source().and_then(|e| e.downcast::().ok()); - let detail = db_error.as_ref().and_then(|e| e.detail()).map(ToString::to_string); - - let constraint = detail + "23505" => { + let constraint = value + .detail .as_ref() .and_then(|d| d.split(")=(").next()) .and_then(|d| d.split(" (").nth(1).map(|s| s.replace('\"', ""))) @@ -41,189 +68,138 @@ impl From for Error { let kind = ErrorKind::UniqueConstraintViolation { constraint }; let mut builder = Error::builder(kind); - builder.set_original_code(code); + builder.set_original_code(value.code); - if let Some(detail) = detail { + if let Some(detail) = value.detail { builder.set_original_message(detail); } builder.build() } - // Even lipstick will not save this... - Some(code) if code == "23502" => { - let code = code.to_string(); - let db_error = e.into_source().and_then(|e| e.downcast::().ok()); - let detail = db_error.as_ref().and_then(|e| e.detail()).map(ToString::to_string); - - let constraint = db_error - .as_ref() - .map(|e| e.column()) - .map(DatabaseConstraint::fields) - .unwrap_or(DatabaseConstraint::CannotParse); + // Even lipstick will not save this... + "23502" => { + let constraint = DatabaseConstraint::fields(value.column); let kind = ErrorKind::NullConstraintViolation { constraint }; let mut builder = Error::builder(kind); - builder.set_original_code(code); + builder.set_original_code(value.code); - if let Some(detail) = detail { + if let Some(detail) = value.detail { builder.set_original_message(detail); } builder.build() } - Some(code) if code == "23503" => { - let code = code.to_string(); - let db_error = e.into_source().and_then(|e| e.downcast::().ok()); - - match db_error.as_ref().and_then(|e| e.column()) { - Some(column) => { - let mut builder = Error::builder(ErrorKind::ForeignKeyConstraintViolation { - constraint: DatabaseConstraint::fields(Some(column)), - }); - - builder.set_original_code(code); - - if let Some(message) = db_error.as_ref().map(|e| e.message()) { - builder.set_original_message(message); - } - - builder.build() - } - None => { - let constraint = db_error - .as_ref() - .map(|e| e.message()) - .and_then(|e| e.split_whitespace().nth(10)) - .and_then(|s| s.split('"').nth(1)) - .map(ToString::to_string) - .map(DatabaseConstraint::Index) - .unwrap_or(DatabaseConstraint::CannotParse); - - let kind = ErrorKind::ForeignKeyConstraintViolation { constraint }; - let mut builder = Error::builder(kind); - - builder.set_original_code(code); - - if let Some(message) = db_error.as_ref().map(|e| e.message()) { - builder.set_original_message(message); - } - - builder.build() - } - } - } - Some(code) if code == "3D000" => { - let code = code.to_string(); - let db_error = e.into_source().and_then(|e| e.downcast::().ok()); - let message = db_error.as_ref().map(|e| e.message()); + "23503" => match value.column { + Some(column) => { + let mut builder = Error::builder(ErrorKind::ForeignKeyConstraintViolation { + constraint: DatabaseConstraint::fields(Some(column)), + }); - let db_name = message - .as_ref() - .and_then(|s| s.split_whitespace().nth(1)) + builder.set_original_code(value.code); + builder.set_original_message(value.message); + + builder.build() + } + None => { + let constraint = value + .message + .split_whitespace() + .nth(10) + .and_then(|s| s.split('"').nth(1)) + .map(ToString::to_string) + .map(DatabaseConstraint::Index) + .unwrap_or(DatabaseConstraint::CannotParse); + + let kind = ErrorKind::ForeignKeyConstraintViolation { constraint }; + let mut builder = Error::builder(kind); + + builder.set_original_code(value.code); + builder.set_original_message(value.message); + + builder.build() + } + }, + "3D000" => { + let db_name = value + .message + .split_whitespace() + .nth(1) .and_then(|s| s.split('"').nth(1)) .into(); let kind = ErrorKind::DatabaseDoesNotExist { db_name }; let mut builder = Error::builder(kind); - builder.set_original_code(code); - - if let Some(message) = message { - builder.set_original_message(message); - } + builder.set_original_code(value.code); + builder.set_original_message(value.message); builder.build() } - Some(code) if code == "28000" => { - let code = code.to_string(); - let db_error = e.into_source().and_then(|e| e.downcast::().ok()); - let message = db_error.as_ref().map(|e| e.message()); - - let db_name = message - .as_ref() - .and_then(|m| m.split_whitespace().nth(5)) + "28000" => { + let db_name = value + .message + .split_whitespace() + .nth(5) .and_then(|s| s.split('"').nth(1)) .into(); let kind = ErrorKind::DatabaseAccessDenied { db_name }; let mut builder = Error::builder(kind); - builder.set_original_code(code); - - if let Some(message) = message { - builder.set_original_message(message); - } + builder.set_original_code(value.code); + builder.set_original_message(value.message); builder.build() } - Some(code) if code == "28P01" => { - let code = code.to_string(); - let db_error = e.into_source().and_then(|e| e.downcast::().ok()); - let message = db_error.as_ref().map(|e| e.message()); + "28P01" => { + let message = value.message; let user = message - .as_ref() - .and_then(|m| m.split_whitespace().last()) + .split_whitespace() + .last() .and_then(|s| s.split('"').nth(1)) .into(); let kind = ErrorKind::AuthenticationFailed { user }; let mut builder = Error::builder(kind); - builder.set_original_code(code); - - if let Some(message) = message { - builder.set_original_message(message); - } + builder.set_original_code(value.code); + builder.set_original_message(message); builder.build() } - Some(code) if code == "40001" => { - let code = code.to_string(); - let db_error = e.into_source().and_then(|e| e.downcast::().ok()); - let message = db_error.as_ref().map(|e| e.message()); - let mut builder = Error::builder(ErrorKind::TransactionWriteConflict); + "40001" => { + let mut builder: crate::error::ErrorBuilder = Error::builder(ErrorKind::TransactionWriteConflict); - builder.set_original_code(code); - - if let Some(message) = message { - builder.set_original_message(message); - } + builder.set_original_code(value.code); + builder.set_original_message(value.message); builder.build() } - Some(code) if code == "42P01" => { - let code = code.to_string(); - let db_error = e.into_source().and_then(|e| e.downcast::().ok()); - let message = db_error.as_ref().map(|e| e.message()); - - let table = message - .as_ref() - .and_then(|m| m.split_whitespace().nth(1)) + "42P01" => { + let table = value + .message + .split_whitespace() + .nth(1) .and_then(|s| s.split('"').nth(1)) .into(); let kind = ErrorKind::TableDoesNotExist { table }; let mut builder = Error::builder(kind); - builder.set_original_code(code); - - if let Some(message) = message { - builder.set_original_message(message); - } + builder.set_original_code(value.code); + builder.set_original_message(value.message); builder.build() } - Some(code) if code == "42703" => { - let code = code.to_string(); - let db_error = e.into_source().and_then(|e| e.downcast::().ok()); - let message = db_error.as_ref().map(|e| e.message()); - - let column = message - .as_ref() - .and_then(|m| m.split_whitespace().nth(1)) + "42703" => { + let column = value + .message + .split_whitespace() + .nth(1) .map(|s| s.split('\"')) .and_then(|mut s| match (s.next(), s.next()) { (Some(column), _) if !column.is_empty() => Some(column), @@ -235,92 +211,102 @@ impl From for Error { let kind = ErrorKind::ColumnNotFound { column }; let mut builder = Error::builder(kind); - builder.set_original_code(code); - - if let Some(message) = message { - builder.set_original_message(message); - } - + builder.set_original_code(value.code); + builder.set_original_message(value.message); builder.build() } - Some(code) if code == "42P04" => { - let code = code.to_string(); - let db_error = e.into_source().and_then(|e| e.downcast::().ok()); - let message = db_error.as_ref().map(|e| e.message()); - - let db_name = message - .as_ref() - .and_then(|m| m.split_whitespace().nth(1)) + "42P04" => { + let db_name = value + .message + .split_whitespace() + .nth(1) .and_then(|s| s.split('"').nth(1)) .into(); let kind = ErrorKind::DatabaseAlreadyExists { db_name }; let mut builder = Error::builder(kind); - builder.set_original_code(code); + builder.set_original_code(value.code); + builder.set_original_message(value.message); - if let Some(message) = message { - builder.set_original_message(message); - } + builder.build() + } + + _ => { + let code = value.code.to_owned(); + let message = value.to_string(); + let mut builder = Error::builder(ErrorKind::QueryError(value.into())); + builder.set_original_code(code); + builder.set_original_message(message); builder.build() } - code => { - // This is necessary, on top of the other conversions, for the cases where a - // native_tls error comes wrapped in a tokio_postgres error. - if let Some(tls_error) = try_extracting_tls_error(&e) { - return tls_error; - } + } + } +} - // Same for IO errors. - if let Some(io_error) = try_extracting_io_error(&e) { - return io_error; - } +impl From for Error { + fn from(e: tokio_postgres::error::Error) -> Error { + if e.is_closed() { + return Error::builder(ErrorKind::ConnectionClosed).build(); + } - #[cfg(feature = "uuid")] - if let Some(uuid_error) = try_extracting_uuid_error(&e) { - return uuid_error; - } + if let Some(db_error) = e.as_db_error() { + return PostgresError::from(db_error).into(); + } - let reason = format!("{e}"); - - match reason.as_str() { - "error connecting to server: timed out" => { - let mut builder = Error::builder(ErrorKind::ConnectTimeout); - - if let Some(code) = code { - builder.set_original_code(code); - }; - - builder.set_original_message(reason); - builder.build() - } // sigh... - // https://github.com/sfackler/rust-postgres/blob/0c84ed9f8201f4e5b4803199a24afa2c9f3723b2/tokio-postgres/src/connect_tls.rs#L37 - "error performing TLS handshake: server does not support TLS" => { - let mut builder = Error::builder(ErrorKind::TlsError { - message: reason.clone(), - }); - - if let Some(code) = code { - builder.set_original_code(code); - }; - - builder.set_original_message(reason); - builder.build() - } // double sigh - _ => { - let code = code.map(|c| c.to_string()); - let mut builder = Error::builder(ErrorKind::QueryError(e.into())); - - if let Some(code) = code { - builder.set_original_code(code); - }; - - builder.set_original_message(reason); - builder.build() - } - } + if let Some(tls_error) = try_extracting_tls_error(&e) { + return tls_error; + } + + // Same for IO errors. + if let Some(io_error) = try_extracting_io_error(&e) { + return io_error; + } + + #[cfg(feature = "uuid")] + if let Some(uuid_error) = try_extracting_uuid_error(&e) { + return uuid_error; + } + + let reason = format!("{e}"); + let code = e.code().map(|c| c.code()); + + match reason.as_str() { + "error connecting to server: timed out" => { + let mut builder = Error::builder(ErrorKind::ConnectTimeout); + + if let Some(code) = code { + builder.set_original_code(code); + }; + + builder.set_original_message(reason); + return builder.build(); + } // sigh... + // https://github.com/sfackler/rust-postgres/blob/0c84ed9f8201f4e5b4803199a24afa2c9f3723b2/tokio-postgres/src/connect_tls.rs#L37 + "error performing TLS handshake: server does not support TLS" => { + let mut builder = Error::builder(ErrorKind::TlsError { + message: reason.clone(), + }); + + if let Some(code) = code { + builder.set_original_code(code); + }; + + builder.set_original_message(reason); + return builder.build(); + } // double sigh + _ => { + let code = code.map(|c| c.to_string()); + let mut builder = Error::builder(ErrorKind::QueryError(e.into())); + + if let Some(code) = code { + builder.set_original_code(code); + }; + + builder.set_original_message(reason); + return builder.build(); } } } diff --git a/quaint/src/error.rs b/quaint/src/error.rs index 5ca712c7be7..e9bdc890f27 100644 --- a/quaint/src/error.rs +++ b/quaint/src/error.rs @@ -6,6 +6,8 @@ use thiserror::Error; #[cfg(feature = "pooled")] use std::time::Duration; +pub use crate::connector::postgres::error::PostgresError; + #[derive(Debug, PartialEq, Eq)] pub enum DatabaseConstraint { Fields(Vec), diff --git a/query-engine/driver-adapters/js/adapter-neon/src/neon.ts b/query-engine/driver-adapters/js/adapter-neon/src/neon.ts index 0c915e954db..e2dac37a911 100644 --- a/query-engine/driver-adapters/js/adapter-neon/src/neon.ts +++ b/query-engine/driver-adapters/js/adapter-neon/src/neon.ts @@ -27,29 +27,25 @@ abstract class NeonQueryable implements Queryable { const tag = '[js::query_raw]' debug(`${tag} %O`, query) - const { fields, rows } = await this.performIO(query) - - const columns = fields.map((field) => field.name) - const resultSet: ResultSet = { - columnNames: columns, - columnTypes: fields.map((field) => fieldToColumnType(field.dataTypeID)), - rows, - } - - return ok(resultSet) + return (await this.performIO(query)).map(({ fields, rows }) => { + const columns = fields.map((field) => field.name) + return { + columnNames: columns, + columnTypes: fields.map((field) => fieldToColumnType(field.dataTypeID)), + rows, + } + }) } async executeRaw(query: Query): Promise> { const tag = '[js::execute_raw]' debug(`${tag} %O`, query) - const { rowCount: rowsAffected } = await this.performIO(query) - // Note: `rowsAffected` can sometimes be null (e.g., when executing `"BEGIN"`) - return ok(rowsAffected ?? 0) + return (await this.performIO(query)).map((r) => r.rowCount ?? 0) } - abstract performIO(query: Query): Promise + abstract performIO(query: Query): Promise> } /** @@ -60,15 +56,25 @@ class NeonWsQueryable extends NeonQ super() } - override async performIO(query: Query): Promise { + override async performIO(query: Query): Promise> { const { sql, args: values } = query try { - return await this.client.query({ text: sql, values, rowMode: 'array' }) + return ok(await this.client.query({ text: sql, values, rowMode: 'array' })) } catch (e) { - const error = e as Error - debug('Error in performIO: %O', error) - throw error + debug('Error in performIO: %O', e) + if (e && e.code) { + return err({ + kind: 'PostgresError', + code: e.code, + severity: e.severity, + message: e.message, + detail: e.detail, + column: e.column, + hint: e.hint, + }) + } + throw e } } } @@ -126,12 +132,14 @@ export class PrismaNeonHTTP extends NeonQueryable implements DriverAdapter { super() } - override async performIO(query: Query): Promise { + override async performIO(query: Query): Promise> { const { sql, args: values } = query - return await this.client(sql, values, { - arrayMode: true, - fullResults: true, - }) + return ok( + await this.client(sql, values, { + arrayMode: true, + fullResults: true, + }), + ) } startTransaction(): Promise> { diff --git a/query-engine/driver-adapters/js/driver-adapter-utils/src/types.ts b/query-engine/driver-adapters/js/driver-adapter-utils/src/types.ts index 763a85b7be6..409f3958bcd 100644 --- a/query-engine/driver-adapters/js/driver-adapter-utils/src/types.ts +++ b/query-engine/driver-adapters/js/driver-adapter-utils/src/types.ts @@ -36,6 +36,14 @@ export type Query = { export type Error = { kind: 'GenericJsError' id: number +} | { + kind: 'PostgresError' + code: string, + severity: string + message: string + detail: string | undefined + column: string | undefined + hint: string | undefined } export interface Queryable { diff --git a/query-engine/driver-adapters/js/smoke-test-js/prisma/mysql/schema.prisma b/query-engine/driver-adapters/js/smoke-test-js/prisma/mysql/schema.prisma index 6681f70e6c6..00418d57cc2 100644 --- a/query-engine/driver-adapters/js/smoke-test-js/prisma/mysql/schema.prisma +++ b/query-engine/driver-adapters/js/smoke-test-js/prisma/mysql/schema.prisma @@ -114,3 +114,7 @@ model Product { properties Json properties_null Json? } + +model Unique { + email String @id +} diff --git a/query-engine/driver-adapters/js/smoke-test-js/prisma/postgres/schema.prisma b/query-engine/driver-adapters/js/smoke-test-js/prisma/postgres/schema.prisma index 4c92945ea85..74ffd428c72 100644 --- a/query-engine/driver-adapters/js/smoke-test-js/prisma/postgres/schema.prisma +++ b/query-engine/driver-adapters/js/smoke-test-js/prisma/postgres/schema.prisma @@ -103,3 +103,7 @@ model User { id String @id @default(uuid()) email String } + +model Unique { + email String @id +} diff --git a/query-engine/driver-adapters/js/smoke-test-js/src/libquery/libquery.ts b/query-engine/driver-adapters/js/smoke-test-js/src/libquery/libquery.ts index 44d07abb9a4..3f659a6cb59 100644 --- a/query-engine/driver-adapters/js/smoke-test-js/src/libquery/libquery.ts +++ b/query-engine/driver-adapters/js/smoke-test-js/src/libquery/libquery.ts @@ -21,21 +21,6 @@ export function smokeTestLibquery(adapter: ErrorCapturingDriverAdapter, prismaSc await adapter.close() }) - it('raw error', async () => { - await assert.rejects(async () => { - await doQuery({ - action: 'queryRaw', - query: { - selection: { $scalars: true }, - arguments: { - query: 'NOT A VALID SQL, THIS WILL FAIL', - parameters: '[]', - }, - }, - }) - }) - }) - it('create JSON values', async () => { const json = JSON.stringify({ foo: 'bar', @@ -294,6 +279,23 @@ export function smokeTestLibquery(adapter: ErrorCapturingDriverAdapter, prismaSc console.log('[nodejs] commited', commitResponse) }) + it('expected error', async () => { + const result = await doQuery({ + modelName: 'Unique', + action: 'createMany', + query: { + arguments: { + data: [{ email: 'duplicate@example.com' }, { email: 'duplicate@example.com' }], + }, + selection: { + $scalars: true, + }, + }, + }) + + console.log('[nodejs] error result', JSON.stringify(result, null, 2)) + }) + describe('read scalar and non scalar types', () => { if (['mysql'].includes(flavour)) { it('mysql', async () => { diff --git a/query-engine/driver-adapters/src/result.rs b/query-engine/driver-adapters/src/result.rs index a5965509ef8..fc6f52bd274 100644 --- a/query-engine/driver-adapters/src/result.rs +++ b/query-engine/driver-adapters/src/result.rs @@ -1,14 +1,29 @@ use napi::{bindgen_prelude::FromNapiValue, Env, JsUnknown, NapiValue}; -use quaint::error::Error as QuaintError; +use quaint::error::{Error as QuaintError, PostgresError}; use serde::Deserialize; -#[derive(Deserialize, Debug)] +#[derive(Deserialize)] +#[serde(remote = "PostgresError")] +pub struct PostgresErrorDef { + code: String, + message: String, + severity: String, + detail: Option, + column: Option, + hint: Option, +} + +#[derive(Deserialize)] #[serde(tag = "kind")] /// Wrapper for JS-side errors /// See driver-adapters/js/adapter-utils/src/types.ts file for example pub(crate) enum DriverAdapterError { /// Unexpected JS exception - GenericJsError { id: i32 }, + GenericJsError { + id: i32, + }, + + PostgresError(#[serde(with = "PostgresErrorDef")] PostgresError), // in the future, expected errors that map to known user errors with PXXX codes will also go here } @@ -24,6 +39,7 @@ impl From for QuaintError { fn from(value: DriverAdapterError) -> Self { match value { DriverAdapterError::GenericJsError { id } => QuaintError::external_error(id), + DriverAdapterError::PostgresError(e) => e.into(), // in future, more error types would be added and we'll need to convert them to proper QuaintErrors here } }