Skip to content

Commit

Permalink
breaking: fix name collision in FromRow, return `Error::ColumnDecod…
Browse files Browse the repository at this point in the history
…e` 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`
  • Loading branch information
abonander authored Jul 20, 2024
1 parent b37b34b commit 4fc5b30
Show file tree
Hide file tree
Showing 4 changed files with 181 additions and 22 deletions.
82 changes: 60 additions & 22 deletions sqlx-macros-core/src/derives/row.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<R::Database>));

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) => {
Expand All @@ -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<R::Database>));

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) => {
Expand All @@ -135,10 +161,18 @@ fn expand_derive_from_row_struct(
predicates.push(parse_quote!(::sqlx::types::Json<#try_from>: ::sqlx::types::Type<R::Database>));

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
Expand All @@ -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<R::Database>));

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?;
Expand All @@ -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<Self> {
fn from_row(__row: &#lifetime R) -> ::sqlx::Result<Self> {
#default_instance

#(#reads)*
Expand Down
3 changes: 3 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
95 changes: 95 additions & 0 deletions src/spec_error.rs
Original file line number Diff line number Diff line change
@@ -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<E>(pub E);

pub trait SpecError<E>: Sized {
fn __sqlx_spec_error(
&self,
) -> fn(SpecErrorWrapper<E>) -> Box<dyn Error + Send + Sync + 'static>;
}

impl<E> SpecError<E> for &&&&SpecErrorWrapper<E>
where
E: Error + Send + Sync + 'static,
{
fn __sqlx_spec_error(
&self,
) -> fn(SpecErrorWrapper<E>) -> Box<dyn Error + Send + Sync + 'static> {
|e| Box::new(e.0)
}
}

impl<E> SpecError<E> for &&&SpecErrorWrapper<E>
where
E: Display,
{
fn __sqlx_spec_error(
&self,
) -> fn(SpecErrorWrapper<E>) -> Box<dyn Error + Send + Sync + 'static> {
|e| e.0.to_string().into()
}
}

impl<E> SpecError<E> for &&SpecErrorWrapper<E>
where
E: Debug,
{
fn __sqlx_spec_error(
&self,
) -> fn(SpecErrorWrapper<E>) -> Box<dyn Error + Send + Sync + 'static> {
|e| format!("{:?}", e.0).into()
}
}

impl<E> SpecError<E> for &SpecErrorWrapper<E>
where
E: Any,
{
fn __sqlx_spec_error(
&self,
) -> fn(SpecErrorWrapper<E>) -> Box<dyn Error + Send + Sync + 'static> {
|_e| format!("unprintable error: {}", std::any::type_name::<E>()).into()
}
}

impl<E> SpecError<E> for SpecErrorWrapper<E> {
fn __sqlx_spec_error(
&self,
) -> fn(SpecErrorWrapper<E>) -> Box<dyn Error + Send + Sync + 'static> {
|_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<dyn Error + Send + Sync + 'static> =
__spec_error!(std::io::Error::from(std::io::ErrorKind::Unsupported));

let _e: Box<dyn Error + Send + Sync + 'static> = __spec_error!("displayable error");

let _e: Box<dyn Error + Send + Sync + 'static> = __spec_error!(DebugError);

let _e: Box<dyn Error + Send + Sync + 'static> = __spec_error!(AnyError);

let _e: Box<dyn Error + Send + Sync + 'static> = __spec_error!(&1i32);
}
23 changes: 23 additions & 0 deletions tests/postgres/derives.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Postgres>().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(())
}

0 comments on commit 4fc5b30

Please sign in to comment.