diff --git a/sqlx-core/src/error.rs b/sqlx-core/src/error.rs index 688e5445..4ef687a5 100644 --- a/sqlx-core/src/error.rs +++ b/sqlx-core/src/error.rs @@ -193,11 +193,22 @@ pub trait DatabaseError: 'static + Send + Sync + StdError { None } - /// The position in the query where the error occurred, if applicable. + /// The line and column in the executed SQL where the error occurred, + /// if applicable and supported by the database. /// /// ### Note - /// This assumes that Rust and the database server agree on the definition of "character", - /// i.e. a Unicode scalar value. + /// This may return an incorrect result if the database server disagrees with Rust + /// on the definition of a "character", i.e. a Unicode scalar value. This position should not + /// be considered authoritative. + /// + /// This also may not be returned or made readily available by every database flavor. + /// + /// For example, MySQL and MariaDB do not include the error position as a specific field + /// in the `ERR_PACKET` structure; the line number that appears in the error message is part + /// of the message string generated by the database server. + /// + /// SQLx does not attempt to parse the line number from the message string, + /// as we cannot assume that the exact message format is a stable part of the API contract. fn position(&self) -> Option { None } @@ -330,40 +341,53 @@ macro_rules! err_protocol { }; } -/// The line and column (1-based) in the query where the server says an error occurred. +/// Details the position in an SQL string where the server says an error occurred. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct ErrorPosition { - /// The line number (1-based) in the query where the server says the error occurred. + /// The byte offset where the error occurred. + pub byte_offset: usize, + /// The character (Unicode scalar value) offset where the error occurred. + pub char_offset: usize, + /// The line number (1-based) in the string. pub line: usize, - /// The column (1-based) in the query where the server says the error occurred. + /// The column position (1-based) in the string. pub column: usize, } /// The character basis for an error position. Used with [`ErrorPosition`]. -pub enum CharBasis { +#[derive(Debug)] +pub enum PositionBasis { + /// A zero-based byte offset. + ByteOffset(usize), /// A zero-based character index. - Zero(usize), + CharIndex(usize), /// A 1-based character position. - One(usize) + CharPos(usize), } impl ErrorPosition { - /// Given a query string and a character position, return the line and column in the query. + /// Given a query string and a character basis (byte offset, 0-based index or 1-based position), + /// return the line and column. + /// + /// Returns `None` if the character basis is out-of-bounds, + /// does not lie on a character boundary (byte offsets only), + /// or overflows `usize`. /// /// ### Note /// This assumes that Rust and the database server agree on the definition of "character", /// i.e. a Unicode scalar value. - pub fn from_char_pos(query: &str, char_basis: CharBasis) -> Option { - // UTF-8 encoding forces us to count characters from the beginning. - let char_idx = char_basis.to_index()?; + pub fn find(query: &str, basis: PositionBasis) -> Option { + let mut pos = ErrorPosition { byte_offset: 0, char_offset: 0, line: 1, column: 1 }; - let mut pos = ErrorPosition { - line: 1, - column: 1, - }; + for (char_idx, (byte_idx, ch)) in query.char_indices().enumerate() { + pos.byte_offset = byte_idx; + pos.char_offset = char_idx; - for (i, ch) in query.chars().enumerate() { - if i == char_idx { return Some(pos); } + // Note: since line and column are 1-based, + // we technically don't want to advance until the top of the next loop. + if pos.basis_reached(&basis) { + return Some(pos); + } if ch == '\n' { pos.line = pos.line.checked_add(1)?; @@ -373,43 +397,74 @@ impl ErrorPosition { } } - None - } -} + // Check if the end of the string matches our basis. + pos.byte_offset = query.len(); + pos.char_offset = pos.char_offset.checked_add(1)?; -impl CharBasis { - fn to_index(&self) -> Option { - match *self { - CharBasis::Zero(idx) => Some(idx), - CharBasis::One(pos) => pos.checked_sub(1), + pos.basis_reached(&basis).then_some(pos) + } + + fn basis_reached(&self, basis: &PositionBasis) -> bool { + match *basis { + PositionBasis::ByteOffset(offset) => { + self.byte_offset == offset + } + PositionBasis::CharIndex(char_idx) => { + self.char_offset == char_idx + } + PositionBasis::CharPos(char_pos) => { + self.char_offset.checked_add(1) == Some(char_pos) + } } } } #[test] fn test_error_position() { - macro_rules! test_error_position { - // Note: only tests one-based positions since zero-based is most of the same steps. - ($query:expr, pos: $pos:expr, line: $line:expr, col: $column:expr; $($tt:tt)*) => { - let expected = ErrorPosition { line: $line, column: $column }; - let actual = ErrorPosition::from_char_pos($query, CharBasis::One($pos)); - assert_eq!(actual, Some(expected), "for position {} in query {:?}", $pos, $query); + assert_eq!( + ErrorPosition::find( + "SELECT foo", + PositionBasis::CharPos(8), + ), + Some(ErrorPosition { + byte_offset: 7, + char_offset: 7, + line: 1, + column: 8 + }) + ); - test_error_position!($($tt)*); - }; - ($query:expr, pos: $pos:expr, None; $($tt:tt)*) => { - let actual = ErrorPosition::from_char_pos($query, CharBasis::One($pos)); - assert_eq!(actual, None, "for position {} in query {:?}", $pos, $query); + assert_eq!( + ErrorPosition::find( + "SELECT foo\nbar FROM baz", + PositionBasis::CharPos(16), + ), + Some(ErrorPosition { + byte_offset: 16, + char_offset: 16, + line: 2, + column: 5 + }) + ); - test_error_position!($($tt)*); - }; - () => {} - } + assert_eq!( + ErrorPosition::find( + "SELECT foo\r\nbar FROM baz", + PositionBasis::CharPos(17) + ), + Some(ErrorPosition { + byte_offset: 16, + char_offset: 16, + line: 2, + column: 5 + }) + ); - test_error_position! { - "SELECT foo", pos: 8, line: 1, col: 8; - "SELECT foo\nbar FROM baz", pos: 16, line: 2, col: 5; - "SELECT foo\r\nbar FROM baz", pos: 17, line: 2, col: 5; - "SELECT foo\r\nbar FROM baz", pos: 27, None; - } -} \ No newline at end of file + assert_eq!( + ErrorPosition::find( + "SELECT foo\r\nbar FROM baz", + PositionBasis::CharPos(27) + ), + None + ); +} diff --git a/sqlx-postgres/src/connection/executor.rs b/sqlx-postgres/src/connection/executor.rs index bb73db1e..04102c0d 100644 --- a/sqlx-postgres/src/connection/executor.rs +++ b/sqlx-postgres/src/connection/executor.rs @@ -1,5 +1,5 @@ use crate::describe::Describe; -use crate::error::Error; +use crate::error::{Error, PgResultExt}; use crate::executor::{Execute, Executor}; use crate::logger::QueryLogger; use crate::message::{ @@ -168,7 +168,9 @@ impl PgConnection { return Ok((*statement).clone()); } - let statement = prepare(self, sql, parameters, metadata).await?; + let statement = prepare(self, sql, parameters, metadata) + .await + .pg_find_error_pos(sql)?; if store_to_cache && self.cache_statement.is_enabled() { if let Some((id, _)) = self.cache_statement.insert(sql, statement.clone()) { @@ -267,7 +269,9 @@ impl PgConnection { Ok(try_stream! { loop { - let message = self.stream.recv().await?; + let message = self.stream.recv() + .await + .pg_find_error_pos(query)?; match message.format { MessageFormat::BindComplete diff --git a/sqlx-postgres/src/connection/stream.rs b/sqlx-postgres/src/connection/stream.rs index 0cbf405d..786fac6e 100644 --- a/sqlx-postgres/src/connection/stream.rs +++ b/sqlx-postgres/src/connection/stream.rs @@ -104,7 +104,7 @@ impl PgStream { match message.format { MessageFormat::ErrorResponse => { // An error returned from the database server. - return Err(PgDatabaseError(message.decode()?).into()); + return Err(PgDatabaseError::new(message.decode()?).into()); } MessageFormat::NotificationResponse => { diff --git a/sqlx-postgres/src/error.rs b/sqlx-postgres/src/error.rs index b9df8657..b58cf32e 100644 --- a/sqlx-postgres/src/error.rs +++ b/sqlx-postgres/src/error.rs @@ -1,44 +1,63 @@ +use std::borrow::Cow; use std::error::Error as StdError; use std::fmt::{self, Debug, Display, Formatter}; use atoi::atoi; -use smallvec::alloc::borrow::Cow; pub(crate) use sqlx_core::error::*; use crate::message::{Notice, PgSeverity}; /// An error returned from the PostgreSQL database. -pub struct PgDatabaseError(pub(crate) Notice); +pub struct PgDatabaseError { + pub(crate) notice: Notice, + pub(crate) error_pos: Option, +} // Error message fields are documented: // https://www.postgresql.org/docs/current/protocol-error-fields.html impl PgDatabaseError { + pub(crate) fn new(notice: Notice) -> Self { + PgDatabaseError { + notice, + error_pos: None, + } + } + + pub(crate) fn find_error_pos(&mut self, query: &str) { + let error_pos = self + .pg_error_position() + .and_then(|pos_raw| pos_raw.original()) + .and_then(|pos| ErrorPosition::find(query, PositionBasis::CharPos(pos))); + + self.error_pos = error_pos; + } + #[inline] pub fn severity(&self) -> PgSeverity { - self.0.severity() + self.notice.severity() } /// The [SQLSTATE](https://www.postgresql.org/docs/current/errcodes-appendix.html) code for /// this error. #[inline] pub fn code(&self) -> &str { - self.0.code() + self.notice.code() } /// The primary human-readable error message. This should be accurate but /// terse (typically one line). #[inline] pub fn message(&self) -> &str { - self.0.message() + self.notice.message() } /// An optional secondary error message carrying more detail about the problem. /// Might run to multiple lines. #[inline] pub fn detail(&self) -> Option<&str> { - self.0.get(b'D') + self.notice.get(b'D') } /// An optional suggestion what to do about the problem. This is intended to differ from @@ -46,20 +65,20 @@ impl PgDatabaseError { /// Might run to multiple lines. #[inline] pub fn hint(&self) -> Option<&str> { - self.0.get(b'H') + self.notice.get(b'H') } - /// Indicates an error cursor position as an index into the original query string; or, + /// Indicates an error cursor position as a 1-based index into the original query string; or, /// a position into an internally generated query. #[inline] - pub fn position(&self) -> Option> { - self.0 + pub fn pg_error_position(&self) -> Option> { + self.notice .get_raw(b'P') .and_then(atoi) .map(PgErrorPosition::Original) .or_else(|| { - let position = self.0.get_raw(b'p').and_then(atoi)?; - let query = self.0.get(b'q')?; + let position = self.notice.get_raw(b'p').and_then(atoi)?; + let query = self.notice.get(b'q')?; Some(PgErrorPosition::Internal { position, query }) }) @@ -69,61 +88,61 @@ impl PgDatabaseError { /// stack traceback of active procedural language functions and internally-generated queries. /// The trace is one entry per line, most recent first. pub fn r#where(&self) -> Option<&str> { - self.0.get(b'W') + self.notice.get(b'W') } /// If this error is with a specific database object, the /// name of the schema containing that object, if any. pub fn schema(&self) -> Option<&str> { - self.0.get(b's') + self.notice.get(b's') } /// If this error is with a specific table, the name of the table. pub fn table(&self) -> Option<&str> { - self.0.get(b't') + self.notice.get(b't') } /// If the error is with a specific table column, the name of the column. pub fn column(&self) -> Option<&str> { - self.0.get(b'c') + self.notice.get(b'c') } /// If the error is with a specific data type, the name of the data type. pub fn data_type(&self) -> Option<&str> { - self.0.get(b'd') + self.notice.get(b'd') } /// If the error is with a specific constraint, the name of the constraint. /// For this purpose, indexes are constraints, even if they weren't created /// with constraint syntax. pub fn constraint(&self) -> Option<&str> { - self.0.get(b'n') + self.notice.get(b'n') } - /// The file name of the source-code location where this error was reported. + /// The file name of the server source-code location where this error was reported. pub fn file(&self) -> Option<&str> { - self.0.get(b'F') + self.notice.get(b'F') } - /// The line number of the source-code location where this error was reported. + /// The line number of the server source-code location where this error was reported. pub fn line(&self) -> Option { - self.0.get_raw(b'L').and_then(atoi) + self.notice.get_raw(b'L').and_then(atoi) } - /// The name of the source-code routine reporting this error. + /// The name of the server source-code routine reporting this error. pub fn routine(&self) -> Option<&str> { - self.0.get(b'R') + self.notice.get(b'R') } } #[derive(Debug, Eq, PartialEq)] pub enum PgErrorPosition<'a> { - /// A position (in characters) into the original query. + /// A 1-based position (in characters) into the original query. Original(usize), /// A position into the internally-generated query. Internal { - /// The position in characters. + /// The 1-based position, in characters. position: usize, /// The text of a failed internally-generated command. This could be, for example, @@ -135,12 +154,13 @@ pub enum PgErrorPosition<'a> { impl Debug for PgDatabaseError { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { f.debug_struct("PgDatabaseError") + .field("position", &self.error_pos) .field("severity", &self.severity()) .field("code", &self.code()) .field("message", &self.message()) .field("detail", &self.detail()) .field("hint", &self.hint()) - .field("position", &self.position()) + .field("char_position", &self.pg_error_position()) .field("where", &self.r#where()) .field("schema", &self.schema()) .field("table", &self.table()) @@ -156,7 +176,12 @@ impl Debug for PgDatabaseError { impl Display for PgDatabaseError { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - f.write_str(self.message()) + write!(f, "(code {}", self.code())?; + if let Some(error_pos) = self.error_pos { + write!(f, ", line {}, column {}", error_pos.line, error_pos.column)?; + } + + write!(f, ") {}", self.message()) } } @@ -171,6 +196,10 @@ impl DatabaseError for PgDatabaseError { Some(Cow::Borrowed(self.code())) } + fn position(&self) -> Option { + self.error_pos + } + #[doc(hidden)] fn as_error(&self) -> &(dyn StdError + Send + Sync + 'static) { self @@ -219,6 +248,43 @@ impl DatabaseError for PgDatabaseError { } } +impl PgErrorPosition<'_> { + fn original(&self) -> Option { + match *self { + Self::Original(original) => Some(original), + _ => None, + } + } +} + +pub(crate) trait PgResultExt { + fn pg_find_error_pos(self, query: &str) -> Self; +} + +impl PgResultExt for Result { + fn pg_find_error_pos(self, query: &str) -> Self { + self.map_err(|e| { + match e { + Error::Database(e) => { + Error::Database( + // Don't panic in case this gets called in the wrong context; + // it'd be a bug, for sure, but far from a fatal one. + // The trait method has a distinct name to call this out if it happens. + e.try_downcast::().map_or_else( + |e| e, + |mut e| { + e.find_error_pos(query); + e + }, + ), + ) + } + other => other, + } + }) + } +} + /// For reference: pub(crate) mod error_codes { /// Caused when a unique or primary key is violated. diff --git a/tests/postgres/error.rs b/tests/postgres/error.rs index d6f78140..830ee721 100644 --- a/tests/postgres/error.rs +++ b/tests/postgres/error.rs @@ -1,4 +1,5 @@ use sqlx::{error::ErrorKind, postgres::Postgres, Connection}; +use sqlx_core::executor::Executor; use sqlx_test::new; #[sqlx_macros::test] @@ -74,3 +75,70 @@ async fn it_fails_with_check_violation() -> anyhow::Result<()> { Ok(()) } + + +#[sqlx::test] +async fn test_error_includes_position() -> anyhow::Result<()> { + let mut conn = new::().await?; + + let err: sqlx::Error = conn + .prepare("SELECT bar.foo as foo\nFORM bar") + .await + .unwrap_err(); + + let sqlx::Error::Database(dbe) = err else { + panic!("unexpected error kind {err:?}") + }; + + let pos = dbe.position().unwrap(); + + assert_eq!(pos.line, 2); + assert_eq!(pos.column, 1); + assert!( + dbe.to_string().contains("line 2, column 1"), + "{:?}", + dbe.to_string() + ); + + let err: sqlx::Error = sqlx::query("SELECT bar.foo as foo\r\nFORM bar") + .execute(&mut conn) + .await + .unwrap_err(); + + let sqlx::Error::Database(dbe) = err else { + panic!("unexpected error kind {err:?}") + }; + + let pos = dbe.position().unwrap(); + + assert_eq!(pos.line, 2); + assert_eq!(pos.column, 1); + assert!( + dbe.to_string().contains("line 2, column 1"), + "{:?}", + dbe.to_string() + ); + + let err: sqlx::Error = sqlx::query( + "SELECT foo\r\nFROM bar\r\nINNER JOIN baz USING (foo)\r\nWHERE foo=1 ADN baz.foo = 2", + ) + .execute(&mut conn) + .await + .unwrap_err(); + + let sqlx::Error::Database(dbe) = err else { + panic!("unexpected error kind {err:?}") + }; + + let pos = dbe.position().unwrap(); + + assert_eq!(pos.line, 4); + assert_eq!(pos.column, 13); + assert!( + dbe.to_string().contains("line 4, column 13"), + "{:?}", + dbe.to_string() + ); + + Ok(()) +} \ No newline at end of file diff --git a/tests/postgres/postgres.rs b/tests/postgres/postgres.rs index 7edb5a7a..77e495c4 100644 --- a/tests/postgres/postgres.rs +++ b/tests/postgres/postgres.rs @@ -116,7 +116,7 @@ async fn it_can_inspect_errors() -> anyhow::Result<()> { assert_eq!(err.severity(), PgSeverity::Error); assert_eq!(err.message(), "column \"f\" does not exist"); assert_eq!(err.code(), "42703"); - assert_eq!(err.position(), Some(PgErrorPosition::Original(8))); + assert_eq!(err.pg_error_position(), Some(PgErrorPosition::Original(8))); assert_eq!(err.routine(), Some("errorMissingColumn")); assert_eq!(err.constraint(), None); @@ -151,7 +151,7 @@ async fn it_can_inspect_constraint_errors() -> anyhow::Result<()> { "new row for relation \"products\" violates check constraint \"products_price_check\"" ); assert_eq!(err.code(), "23514"); - assert_eq!(err.position(), None); + assert_eq!(err.pg_error_position(), None); assert_eq!(err.routine(), Some("ExecConstraints")); assert_eq!(err.constraint(), Some("products_price_check"));