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.

Sign up for GitHub

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

breaking: fix name collision in FromRow, return Error::ColumnDecode for TryFrom errors #3356

Merged
merged 5 commits into from
Jul 20, 2024
Merged

breaking: fix name collision in FromRow, return Error::ColumnDecode for TryFrom errors #3356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
901040e
chore: create regression test for #3344
abonander Jul 17, 2024
6ede502
fix(derives): use a parameter name that's less likely to collide
abonander Jul 17, 2024
ed281f5
breaking(derives): emit `Error::ColumnDecode` when a `TryFrom` conver…
abonander Jul 17, 2024
2c8938f
fix: run `cargo fmt`
abonander Jul 19, 2024
76cf629
fix: use correct field in `ColumnDecode`
abonander Jul 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 60 additions & 22 deletions sqlx-macros-core/src/derives/row.rs
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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(())
}
Loading