From 4fc5b30d6592dc6ff790a2aebc2cef91e002d00a Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Fri, 19 Jul 2024 23:03:47 -0700 Subject: [PATCH] breaking: fix name collision in `FromRow`, return `Error::ColumnDecode` for `TryFrom` errors (#3356) * chore: create regression test for #3344 * fix(derives): use a parameter name that's less likely to collide * breaking(derives): emit `Error::ColumnDecode` when a `TryFrom` conversion fails in `FromRow` Breaking because `#[sqlx(default)]` on an individual field or the struct itself would have previously suppressed the error. This doesn't seem like good behavior as it could result in some potentially very difficult bugs. Instead of using `TryFrom` for these fields, just implement `From` and apply the default explicitly. * fix: run `cargo fmt` * fix: use correct field in `ColumnDecode` --- sqlx-macros-core/src/derives/row.rs | 82 ++++++++++++++++++------- src/lib.rs | 3 + src/spec_error.rs | 95 +++++++++++++++++++++++++++++ tests/postgres/derives.rs | 23 +++++++ 4 files changed, 181 insertions(+), 22 deletions(-) create mode 100644 src/spec_error.rs diff --git a/sqlx-macros-core/src/derives/row.rs b/sqlx-macros-core/src/derives/row.rs index 1391bb2c..5f7b6dca 100644 --- a/sqlx-macros-core/src/derives/row.rs +++ b/sqlx-macros-core/src/derives/row.rs @@ -104,17 +104,30 @@ fn expand_derive_from_row_struct( .push(parse_quote!(#ty: ::sqlx::decode::Decode<#lifetime, R::Database>)); predicates.push(parse_quote!(#ty: ::sqlx::types::Type)); - parse_quote!(row.try_get(#id_s)) + parse_quote!(__row.try_get(#id_s)) } // Flatten (true, None, false) => { predicates.push(parse_quote!(#ty: ::sqlx::FromRow<#lifetime, R>)); - parse_quote!(<#ty as ::sqlx::FromRow<#lifetime, R>>::from_row(row)) + parse_quote!(<#ty as ::sqlx::FromRow<#lifetime, R>>::from_row(__row)) } // Flatten + Try from (true, Some(try_from), false) => { predicates.push(parse_quote!(#try_from: ::sqlx::FromRow<#lifetime, R>)); - parse_quote!(<#try_from as ::sqlx::FromRow<#lifetime, R>>::from_row(row).and_then(|v| <#ty as ::std::convert::TryFrom::<#try_from>>::try_from(v).map_err(|e| ::sqlx::Error::ColumnNotFound("FromRow: try_from failed".to_string())))) + parse_quote!( + <#try_from as ::sqlx::FromRow<#lifetime, R>>::from_row(__row) + .and_then(|v| { + <#ty as ::std::convert::TryFrom::<#try_from>>::try_from(v) + .map_err(|e| { + // Triggers a lint warning if `TryFrom::Err = Infallible` + #[allow(unreachable_code)] + ::sqlx::Error::ColumnDecode { + index: #id_s.to_string(), + source: sqlx::__spec_error!(e), + } + }) + }) + ) } // Flatten + Json (true, _, true) => { @@ -126,7 +139,20 @@ fn expand_derive_from_row_struct( .push(parse_quote!(#try_from: ::sqlx::decode::Decode<#lifetime, R::Database>)); predicates.push(parse_quote!(#try_from: ::sqlx::types::Type)); - parse_quote!(row.try_get(#id_s).and_then(|v| <#ty as ::std::convert::TryFrom::<#try_from>>::try_from(v).map_err(|e| ::sqlx::Error::ColumnNotFound("FromRow: try_from failed".to_string())))) + parse_quote!( + __row.try_get(#id_s) + .and_then(|v| { + <#ty as ::std::convert::TryFrom::<#try_from>>::try_from(v) + .map_err(|e| { + // Triggers a lint warning if `TryFrom::Err = Infallible` + #[allow(unreachable_code)] + ::sqlx::Error::ColumnDecode { + index: #id_s.to_string(), + source: sqlx::__spec_error!(e), + } + }) + }) + ) } // Try from + Json (false, Some(try_from), true) => { @@ -135,10 +161,18 @@ fn expand_derive_from_row_struct( predicates.push(parse_quote!(::sqlx::types::Json<#try_from>: ::sqlx::types::Type)); parse_quote!( - row.try_get::<::sqlx::types::Json<_>, _>(#id_s).and_then(|v| - <#ty as ::std::convert::TryFrom::<#try_from>>::try_from(v.0) - .map_err(|e| ::sqlx::Error::ColumnNotFound("FromRow: try_from failed".to_string())) - ) + __row.try_get::<::sqlx::types::Json<_>, _>(#id_s) + .and_then(|v| { + <#ty as ::std::convert::TryFrom::<#try_from>>::try_from(v.0) + .map_err(|e| { + // Triggers a lint warning if `TryFrom::Err = Infallible` + #[allow(unreachable_code)] + ::sqlx::Error::ColumnDecode { + index: #id_s.to_string(), + source: sqlx::__spec_error!(e), + } + }) + }) ) }, // Json @@ -147,24 +181,28 @@ fn expand_derive_from_row_struct( .push(parse_quote!(::sqlx::types::Json<#ty>: ::sqlx::decode::Decode<#lifetime, R::Database>)); predicates.push(parse_quote!(::sqlx::types::Json<#ty>: ::sqlx::types::Type)); - parse_quote!(row.try_get::<::sqlx::types::Json<_>, _>(#id_s).map(|x| x.0)) + parse_quote!(__row.try_get::<::sqlx::types::Json<_>, _>(#id_s).map(|x| x.0)) }, }; if attributes.default { - Some(parse_quote!(let #id: #ty = #expr.or_else(|e| match e { - ::sqlx::Error::ColumnNotFound(_) => { - ::std::result::Result::Ok(Default::default()) - }, - e => ::std::result::Result::Err(e) - })?;)) + Some(parse_quote!( + let #id: #ty = #expr.or_else(|e| match e { + ::sqlx::Error::ColumnNotFound(_) => { + ::std::result::Result::Ok(Default::default()) + }, + e => ::std::result::Result::Err(e) + })?; + )) } else if container_attributes.default { - Some(parse_quote!(let #id: #ty = #expr.or_else(|e| match e { - ::sqlx::Error::ColumnNotFound(_) => { - ::std::result::Result::Ok(__default.#id) - }, - e => ::std::result::Result::Err(e) - })?;)) + Some(parse_quote!( + let #id: #ty = #expr.or_else(|e| match e { + ::sqlx::Error::ColumnNotFound(_) => { + ::std::result::Result::Ok(__default.#id) + }, + e => ::std::result::Result::Err(e) + })?; + )) } else { Some(parse_quote!( let #id: #ty = #expr?; @@ -180,7 +218,7 @@ fn expand_derive_from_row_struct( Ok(quote!( #[automatically_derived] impl #impl_generics ::sqlx::FromRow<#lifetime, R> for #ident #ty_generics #where_clause { - fn from_row(row: &#lifetime R) -> ::sqlx::Result { + fn from_row(__row: &#lifetime R) -> ::sqlx::Result { #default_instance #(#reads)* diff --git a/src/lib.rs b/src/lib.rs index 16cce9cb..fa87cc22 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -86,6 +86,9 @@ mod macros; #[doc(hidden)] pub mod ty_match; +#[cfg(feature = "macros")] +pub mod spec_error; + #[doc(hidden)] pub use sqlx_core::rt as __rt; diff --git a/src/spec_error.rs b/src/spec_error.rs new file mode 100644 index 00000000..a37fcb85 --- /dev/null +++ b/src/spec_error.rs @@ -0,0 +1,95 @@ +use std::any::Any; +use std::error::Error; +use std::fmt::{Debug, Display}; + +// Autoderef specialization similar to `clap::value_parser!()`. +pub struct SpecErrorWrapper(pub E); + +pub trait SpecError: Sized { + fn __sqlx_spec_error( + &self, + ) -> fn(SpecErrorWrapper) -> Box; +} + +impl SpecError for &&&&SpecErrorWrapper +where + E: Error + Send + Sync + 'static, +{ + fn __sqlx_spec_error( + &self, + ) -> fn(SpecErrorWrapper) -> Box { + |e| Box::new(e.0) + } +} + +impl SpecError for &&&SpecErrorWrapper +where + E: Display, +{ + fn __sqlx_spec_error( + &self, + ) -> fn(SpecErrorWrapper) -> Box { + |e| e.0.to_string().into() + } +} + +impl SpecError for &&SpecErrorWrapper +where + E: Debug, +{ + fn __sqlx_spec_error( + &self, + ) -> fn(SpecErrorWrapper) -> Box { + |e| format!("{:?}", e.0).into() + } +} + +impl SpecError for &SpecErrorWrapper +where + E: Any, +{ + fn __sqlx_spec_error( + &self, + ) -> fn(SpecErrorWrapper) -> Box { + |_e| format!("unprintable error: {}", std::any::type_name::()).into() + } +} + +impl SpecError for SpecErrorWrapper { + fn __sqlx_spec_error( + &self, + ) -> fn(SpecErrorWrapper) -> Box { + |_e| "unprintable error: (unprintable type)".into() + } +} + +#[doc(hidden)] +#[macro_export] +macro_rules! __spec_error { + ($e:expr) => {{ + use $crate::spec_error::{SpecError, SpecErrorWrapper}; + + let wrapper = SpecErrorWrapper($e); + let wrap_err = wrapper.__sqlx_spec_error(); + wrap_err(wrapper) + }}; +} + +#[test] +fn test_spec_error() { + #[derive(Debug)] + struct DebugError; + + struct AnyError; + + let _e: Box = + __spec_error!(std::io::Error::from(std::io::ErrorKind::Unsupported)); + + let _e: Box = __spec_error!("displayable error"); + + let _e: Box = __spec_error!(DebugError); + + let _e: Box = __spec_error!(AnyError); + + let _e: Box = __spec_error!(&1i32); +} diff --git a/tests/postgres/derives.rs b/tests/postgres/derives.rs index b4c29b48..023cd69f 100644 --- a/tests/postgres/derives.rs +++ b/tests/postgres/derives.rs @@ -769,3 +769,26 @@ async fn test_enum_with_schema() -> anyhow::Result<()> { Ok(()) } + +#[cfg(feature = "macros")] +#[sqlx_macros::test] +async fn test_from_row_hygiene() -> anyhow::Result<()> { + // A field named `row` previously would shadow the `row` parameter of `FromRow::from_row()`: + // https://github.com/launchbadge/sqlx/issues/3344 + #[derive(Debug, sqlx::FromRow)] + pub struct Foo { + pub row: i32, + pub bar: i32, + } + + let mut conn = new::().await?; + + let foo: Foo = sqlx::query_as("SELECT 1234 as row, 5678 as bar") + .fetch_one(&mut conn) + .await?; + + assert_eq!(foo.row, 1234); + assert_eq!(foo.bar, 5678); + + Ok(()) +}