From e12617a07bdc4f5e0e2e29f7c6449e9a86017a78 Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Wed, 10 Jun 2020 20:02:21 -0700 Subject: [PATCH 1/9] feat(macros): type override annotations for columns --- sqlx-macros/src/query/mod.rs | 9 ++ sqlx-macros/src/query/output.rs | 200 ++++++++++++++++++++++---------- tests/postgres/macros.rs | 47 ++++++++ 3 files changed, 196 insertions(+), 60 deletions(-) diff --git a/sqlx-macros/src/query/mod.rs b/sqlx-macros/src/query/mod.rs index 4e366c0f7a..d2876eaa95 100644 --- a/sqlx-macros/src/query/mod.rs +++ b/sqlx-macros/src/query/mod.rs @@ -208,6 +208,15 @@ where RecordType::Generated => { let record_name: Type = syn::parse_str("Record").unwrap(); + for rust_col in &columns { + if rust_col.type_.is_none() { + return Err( + "columns may not have wildcard overrides in `query!()` or `query_as!()" + .into(), + ); + } + } + let record_fields = columns.iter().map( |&output::RustColumn { ref ident, diff --git a/sqlx-macros/src/query/output.rs b/sqlx-macros/src/query/output.rs index cdc6c7384f..e93b386743 100644 --- a/sqlx-macros/src/query/output.rs +++ b/sqlx-macros/src/query/output.rs @@ -1,17 +1,19 @@ use proc_macro2::{Ident, Span, TokenStream}; -use quote::quote; +use quote::{quote, ToTokens}; use syn::Type; -use sqlx_core::describe::Describe; +use sqlx_core::describe::{Column, Describe}; use crate::database::DatabaseExt; use crate::query::QueryMacroInput; use std::fmt::{self, Display, Formatter}; +use syn::parse::{Parse, ParseStream}; +use syn::Token; pub struct RustColumn { pub(super) ident: Ident, - pub(super) type_: TokenStream, + pub(super) type_: Option, } struct DisplayColumn<'a> { @@ -20,6 +22,18 @@ struct DisplayColumn<'a> { name: &'a str, } +struct ColumnDecl { + ident: Ident, + // TIL Rust still has OOP keywords like `abstract`, `final`, `override` and `virtual` reserved + r#override: Option, +} + +enum ColumnOverride { + NonNull, + Wildcard, + Exact(Type), +} + impl Display for DisplayColumn<'_> { fn fmt(&self, f: &mut Formatter) -> fmt::Result { write!(f, "column #{} ({:?})", self.idx + 1, self.name) @@ -32,58 +46,29 @@ pub fn columns_to_rust(describe: &Describe) -> crate::Resul .iter() .enumerate() .map(|(i, column)| -> crate::Result<_> { - let name = &*column.name; - let ident = parse_ident(name)?; - - let mut type_ = if let Some(type_info) = &column.type_info { - ::return_type_for_id(&type_info).map_or_else( - || { - let message = if let Some(feature_gate) = - ::get_feature_gate(&type_info) - { - format!( - "optional feature `{feat}` required for type {ty} of {col}", - ty = &type_info, - feat = feature_gate, - col = DisplayColumn { - idx: i, - name: &*column.name - } - ) - } else { - format!( - "unsupported type {ty} of {col}", - ty = type_info, - col = DisplayColumn { - idx: i, - name: &*column.name - } - ) - }; - syn::Error::new(Span::call_site(), message).to_compile_error() - }, - |t| t.parse().unwrap(), - ) - } else { - syn::Error::new( - Span::call_site(), - format!( - "database couldn't tell us the type of {col}; \ - this can happen for columns that are the result of an expression", - col = DisplayColumn { - idx: i, - name: &*column.name - } - ), - ) - .to_compile_error() + // add raw prefix to all identifiers + let decl = ColumnDecl::parse(&column.name) + .map_err(|e| format!("column name {:?} is invalid: {}", column.name, e))?; + + let type_ = match decl.r#override { + Some(ColumnOverride::Exact(ty)) => Some(ty.to_token_stream()), + Some(ColumnOverride::Wildcard) => None, + Some(ColumnOverride::NonNull) => Some(get_column_type(i, column)), + None => { + let type_ = get_column_type(i, column); + + if !column.not_null.unwrap_or(false) { + Some(quote! { Option<#type_> }) + } else { + Some(type_) + } + } }; - if !column.not_null.unwrap_or(false) { - type_ = quote! { Option<#type_> }; - } - - Ok(RustColumn { ident, type_ }) + Ok(RustColumn { + ident: decl.ident, + type_, + }) }) .collect::>>() } @@ -103,13 +88,15 @@ pub fn quote_query_as( .. }, )| { - // For "checked" queries, the macro checks these at compile time and using "try_get" - // would also perform pointless runtime checks - - if input.checked { - quote!( #ident: row.try_get_unchecked::<#type_, _>(#i).try_unwrap_optional()? ) - } else { - quote!( #ident: row.try_get_unchecked(#i)? ) + match (input.checked, type_) { + // we guarantee the type is valid so we can skip the runtime check + (true, Some(type_)) => quote! { + #ident: row.try_get_unchecked::<#type_, _>(#i).try_unwrap_optional()? + }, + // type was overridden to be a wildcard so we fallback to the runtime check + (true, None) => quote! ( #ident: row.try_get(#i)? ), + // macro is the `_unchecked!()` variant so this will die in decoding if it's wrong + (false, _) => quote!( #ident: row.try_get_unchecked(#i)? ), } }, ); @@ -128,6 +115,99 @@ pub fn quote_query_as( } } +fn get_column_type(i: usize, column: &Column) -> TokenStream { + if let Some(type_info) = &column.type_info { + ::return_type_for_id(&type_info).map_or_else( + || { + let message = + if let Some(feature_gate) = ::get_feature_gate(&type_info) { + format!( + "optional feature `{feat}` required for type {ty} of {col}", + ty = &type_info, + feat = feature_gate, + col = DisplayColumn { + idx: i, + name: &*column.name + } + ) + } else { + format!( + "unsupported type {ty} of {col}", + ty = type_info, + col = DisplayColumn { + idx: i, + name: &*column.name + } + ) + }; + syn::Error::new(Span::call_site(), message).to_compile_error() + }, + |t| t.parse().unwrap(), + ) + } else { + syn::Error::new( + Span::call_site(), + format!( + "database couldn't tell us the type of {col}; \ + this can happen for columns that are the result of an expression", + col = DisplayColumn { + idx: i, + name: &*column.name + } + ), + ) + .to_compile_error() + } +} + +impl ColumnDecl { + fn parse(col_name: &str) -> crate::Result { + // find the end of the identifier because we want to use our own logic to parse it + // if we tried to feed this into `syn::parse_str()` we might get an un-great error + // for some kinds of invalid identifiers + let (ident, remainder) = if let Some(i) = col_name.find(&[':', '!'][..]) { + let (ident, remainder) = col_name.split_at(i); + + (parse_ident(ident)?, remainder) + } else { + (parse_ident(col_name)?, "") + }; + + Ok(ColumnDecl { + ident, + r#override: if !remainder.is_empty() { + Some(syn::parse_str(remainder)?) + } else { + None + }, + }) + } +} + +impl Parse for ColumnOverride { + fn parse(input: ParseStream) -> syn::Result { + let lookahead = input.lookahead1(); + + if lookahead.peek(Token![:]) { + input.parse::()?; + + let ty = Type::parse(input)?; + + if let Type::Infer(_) = ty { + Ok(ColumnOverride::Wildcard) + } else { + Ok(ColumnOverride::Exact(ty)) + } + } else if lookahead.peek(Token![!]) { + input.parse::()?; + + Ok(ColumnOverride::NonNull) + } else { + Err(lookahead.error()) + } + } +} + fn parse_ident(name: &str) -> crate::Result { // workaround for the following issue (it's semi-fixed but still spits out extra diagnostics) // https://github.com/dtolnay/syn/issues/749#issuecomment-575451318 diff --git a/tests/postgres/macros.rs b/tests/postgres/macros.rs index 5026fe9fd8..c927bec367 100644 --- a/tests/postgres/macros.rs +++ b/tests/postgres/macros.rs @@ -254,3 +254,50 @@ async fn fetch_is_usable_issue_224() -> anyhow::Result<()> { Ok(()) } + +#[sqlx_macros::test] +async fn test_column_override_not_null() -> anyhow::Result<()> { + let mut conn = new::().await?; + + let record = sqlx::query!(r#"select 1 as "id!""#) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, 1); + + Ok(()) +} + +#[derive(PartialEq, Eq, Debug, sqlx::Type)] +#[sqlx(transparent)] +struct MyInt4(i32); + +#[sqlx_macros::test] +async fn test_column_override_wildcard() -> anyhow::Result<()> { + struct Record { + id: MyInt4, + } + + let mut conn = new::().await?; + + let record = sqlx::query_as!(Record, r#"select 1 as "id: _""#) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, MyInt4(1)); + + Ok(()) +} + +#[sqlx_macros::test] +async fn test_column_override_exact() -> anyhow::Result<()> { + let mut conn = new::().await?; + + let record = sqlx::query!(r#"select 1 as "id: MyInt4""#) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, MyInt4(1)); + + Ok(()) +} From e852780dfe70f4849a1d7f26c97bc39a2d28fc2b Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Wed, 10 Jun 2020 21:03:36 -0700 Subject: [PATCH 2/9] feat(macros): support wildcard in bind param type overrides --- sqlx-macros/src/query/args.rs | 48 +++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/sqlx-macros/src/query/args.rs b/sqlx-macros/src/query/args.rs index 5129259f95..228ab1c89c 100644 --- a/sqlx-macros/src/query/args.rs +++ b/sqlx-macros/src/query/args.rs @@ -1,6 +1,6 @@ use proc_macro2::TokenStream; use syn::spanned::Spanned; -use syn::Expr; +use syn::{Expr, Type}; use quote::{quote, quote_spanned, ToTokens}; use sqlx_core::describe::Describe; @@ -34,26 +34,29 @@ pub fn quote_args( // TODO: We could remove the ParamChecking flag and just filter to only test params that are non-null let param_ty = param_ty.as_ref().unwrap(); - let param_ty = get_type_override(expr) - .or_else(|| { - Some( - DB::param_type_for_id(¶m_ty)? + let param_ty = match get_type_override(expr) { + // don't emit typechecking code for this binding + Some(Type::Infer(_)) => return Ok(quote!()), + Some(ty) => ty.to_token_stream(), + None => { + DB::param_type_for_id(¶m_ty) + .ok_or_else(|| { + if let Some(feature_gate) = ::get_feature_gate(¶m_ty) { + format!( + "optional feature `{}` required for type {} of param #{}", + feature_gate, + param_ty, + i + 1, + ) + } else { + format!("unsupported type {} for param #{}", param_ty, i + 1) + } + })? .parse::() - .unwrap(), - ) - }) - .ok_or_else(|| { - if let Some(feature_gate) = ::get_feature_gate(¶m_ty) { - format!( - "optional feature `{}` required for type {} of param #{}", - feature_gate, - param_ty, - i + 1, - ) - } else { - format!("unsupported type {} for param #{}", param_ty, i + 1) + .map_err(|_| format!("Rust type mapping for {} not parsable", param_ty))? + } - })?; + }; Ok(quote_spanned!(expr.span() => // this shouldn't actually run @@ -97,10 +100,11 @@ pub fn quote_args( }) } -fn get_type_override(expr: &Expr) -> Option { +fn get_type_override(expr: &Expr) -> Option<&Type> { match expr { - Expr::Cast(cast) => Some(cast.ty.to_token_stream()), - Expr::Type(ascription) => Some(ascription.ty.to_token_stream()), + Expr::Group(group) => get_type_override(&group.expr), + Expr::Cast(cast) => Some(&cast.ty), + Expr::Type(ascription) => Some(&ascription.ty), _ => None, } } From 64cb8977a6792b01686ed91f8e392080afbab65f Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Thu, 11 Jun 2020 19:14:12 -0700 Subject: [PATCH 3/9] chore(macros): add test for column overrides for MySQL --- tests/mysql/macros.rs | 47 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/mysql/macros.rs b/tests/mysql/macros.rs index f949fb4846..1b2abfedba 100644 --- a/tests/mysql/macros.rs +++ b/tests/mysql/macros.rs @@ -92,3 +92,50 @@ async fn test_query_bytes() -> anyhow::Result<()> { Ok(()) } + +#[sqlx_macros::test] +async fn test_column_override_not_null() -> anyhow::Result<()> { + let mut conn = new::().await?; + + let record = sqlx::query!("select * from (select 1 as `id!`) records") + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, 1); + + Ok(()) +} + +#[derive(PartialEq, Eq, Debug, sqlx::Type)] +#[sqlx(transparent)] +struct MyInt4(i32); + +#[sqlx_macros::test] +async fn test_column_override_wildcard() -> anyhow::Result<()> { + struct Record { + id: MyInt4, + } + + let mut conn = new::().await?; + + let record = sqlx::query_as!(Record, "select * from (select 1 as `id: _`) records") + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, MyInt4(1)); + + Ok(()) +} + +#[sqlx_macros::test] +async fn test_column_override_exact() -> anyhow::Result<()> { + let mut conn = new::().await?; + + let record = sqlx::query!("select * from (select 1 as `id: MyInt4`) records") + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, MyInt4(1)); + + Ok(()) +} From c882e7c6a033887807aba6f559278aa8a6f9086b Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Thu, 11 Jun 2020 19:21:22 -0700 Subject: [PATCH 4/9] chore(macros): add test for column overrides for SQLite --- tests/sqlite/macros.rs | 69 ++++++++++++++++++++++++++++++++++++++--- tests/sqlite/sqlite.db | Bin 36864 -> 36864 bytes 2 files changed, 65 insertions(+), 4 deletions(-) diff --git a/tests/sqlite/macros.rs b/tests/sqlite/macros.rs index 2e0825c979..60ab4bf94e 100644 --- a/tests/sqlite/macros.rs +++ b/tests/sqlite/macros.rs @@ -11,7 +11,7 @@ async fn macro_select() -> anyhow::Result<()> { assert_eq!(1, account.id); assert_eq!("Herp Derpinson", account.name); - assert_eq!(account.is_active, None); + assert_eq!(account.is_active, Some(true)); Ok(()) } @@ -29,7 +29,7 @@ async fn macro_select_bind() -> anyhow::Result<()> { assert_eq!(1, account.id); assert_eq!("Herp Derpinson", account.name); - assert_eq!(account.is_active, None); + assert_eq!(account.is_active, Some(true)); Ok(()) } @@ -51,7 +51,7 @@ async fn test_query_as_raw() -> anyhow::Result<()> { assert_eq!(account.id, 1); assert_eq!(account.name, "Herp Derpinson"); - assert_eq!(account.is_active, None); + assert_eq!(account.is_active, Some(true)); Ok(()) } @@ -67,7 +67,68 @@ async fn macro_select_from_view() -> anyhow::Result<()> { // SQLite tells us the true origin of these columns even through the view assert_eq!(account.id, 1); assert_eq!(account.name, "Herp Derpinson"); - assert_eq!(account.is_active, None); + assert_eq!(account.is_active, Some(true)); + + Ok(()) +} + +#[sqlx_macros::test] +async fn test_column_override_not_null() -> anyhow::Result<()> { + let mut conn = new::().await?; + + let record = sqlx::query!(r#"select is_active as "is_active!" from accounts"#) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.is_active, true); + + Ok(()) +} + +#[derive(PartialEq, Eq, Debug, sqlx::Type)] +#[sqlx(transparent)] +struct MyInt(i32); + +#[sqlx_macros::test] +async fn test_column_override_wildcard() -> anyhow::Result<()> { + struct Record { + id: MyInt, + } + + let mut conn = new::().await?; + + let record = sqlx::query_as!(Record, r#"select id as "id: _" from accounts"#) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, MyInt(1)); + + // this syntax is also useful for expressions + let record = sqlx::query_as!(Record, r#"select 1 as "id: _""#) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, MyInt(1)); + + Ok(()) +} + +#[sqlx_macros::test] +async fn test_column_override_exact() -> anyhow::Result<()> { + let mut conn = new::().await?; + + let record = sqlx::query!(r#"select id as "id: MyInt" from accounts"#) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, MyInt(1)); + + // we can also support this syntax for expressions + let record = sqlx::query!(r#"select 1 as "id: MyInt""#) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, MyInt(1)); Ok(()) } diff --git a/tests/sqlite/sqlite.db b/tests/sqlite/sqlite.db index 9faa9c8e7ed7dfa6e4d7a95e8ee4beb03fac3660..1daa80eee8b42c6e1bc57faf67d7423d292d5d60 100644 GIT binary patch delta 26 icmZozz|^pSX@WGP@ Date: Thu, 11 Jun 2020 19:39:20 -0700 Subject: [PATCH 5/9] chore(macros): add tests for bind parameter overrides --- tests/mysql/macros.rs | 2 ++ tests/postgres/macros.rs | 46 ++++++++++++++++++++++++++++++++++++++++ tests/sqlite/macros.rs | 2 ++ 3 files changed, 50 insertions(+) diff --git a/tests/mysql/macros.rs b/tests/mysql/macros.rs index 1b2abfedba..135b7056d8 100644 --- a/tests/mysql/macros.rs +++ b/tests/mysql/macros.rs @@ -139,3 +139,5 @@ async fn test_column_override_exact() -> anyhow::Result<()> { Ok(()) } + +// we don't emit bind parameter typechecks for MySQL so testing the overrides is redundant diff --git a/tests/postgres/macros.rs b/tests/postgres/macros.rs index c927bec367..4d9c90e257 100644 --- a/tests/postgres/macros.rs +++ b/tests/postgres/macros.rs @@ -301,3 +301,49 @@ async fn test_column_override_exact() -> anyhow::Result<()> { Ok(()) } + +#[sqlx_macros::test] +async fn test_bind_arg_override_exact() -> anyhow::Result<()> { + let mut conn = new::().await?; + + let my_int = MyInt4(1); + + // this query should require a bind parameter override as we would otherwise expect the bind + // to be the same type + let record = sqlx::query!( + "select * from (select 1::int4) records(id) where id = $1", + my_int as MyInt4 + ) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, Some(1i32)); + + // test that we're actually emitting the typecast by requiring the bound type to be the same + let record = sqlx::query!("select $1::int8 as id", 1i32 as i64) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, Some(1i64)); + + Ok(()) +} + +// we can't test this yet but will want to when 1.45 drops and we can strip casts to `_` +// #[sqlx_macros::test] +// async fn test_bind_arg_override_wildcard() -> anyhow::Result<()> { +// let mut conn = new::().await?; +// +// let my_int = MyInt4(1); +// +// let record = sqlx::query!( +// "select * from (select 1::int4) records(id) where id = $1", +// my_int as _ +// ) +// .fetch_one(&mut conn) +// .await?; +// +// assert_eq!(record.id, 1i32); +// +// Ok(()) +// } diff --git a/tests/sqlite/macros.rs b/tests/sqlite/macros.rs index 60ab4bf94e..338b1609ae 100644 --- a/tests/sqlite/macros.rs +++ b/tests/sqlite/macros.rs @@ -132,3 +132,5 @@ async fn test_column_override_exact() -> anyhow::Result<()> { Ok(()) } + +// we don't emit bind parameter typechecks for SQLite so testing the overrides is redundant From 6cf4242d6e84060c2b5c6b2f06ee7a9b7c297efb Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Thu, 11 Jun 2020 20:23:56 -0700 Subject: [PATCH 6/9] fix(macros): forbid casts to `_` in bind parameters --- sqlx-macros/src/query/args.rs | 50 ++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/sqlx-macros/src/query/args.rs b/sqlx-macros/src/query/args.rs index 228ab1c89c..4b63dee892 100644 --- a/sqlx-macros/src/query/args.rs +++ b/sqlx-macros/src/query/args.rs @@ -35,28 +35,34 @@ pub fn quote_args( let param_ty = param_ty.as_ref().unwrap(); let param_ty = match get_type_override(expr) { - // don't emit typechecking code for this binding - Some(Type::Infer(_)) => return Ok(quote!()), - Some(ty) => ty.to_token_stream(), - None => { - DB::param_type_for_id(¶m_ty) - .ok_or_else(|| { - if let Some(feature_gate) = ::get_feature_gate(¶m_ty) { - format!( - "optional feature `{}` required for type {} of param #{}", - feature_gate, - param_ty, - i + 1, - ) - } else { - format!("unsupported type {} for param #{}", param_ty, i + 1) - } - })? - .parse::() - .map_err(|_| format!("Rust type mapping for {} not parsable", param_ty))? - - } - }; + // TODO: enable this in 1.45 when we can strip `as _` + // without stripping these we get some pretty nasty type errors + Some(Type::Infer(_)) => return Err( + syn::Error::new_spanned( + expr, + "casts to `_` are not allowed in bind parameters yet" + ).into() + ), + Some(ty) => ty.to_token_stream(), + None => { + DB::param_type_for_id(¶m_ty) + .ok_or_else(|| { + if let Some(feature_gate) = ::get_feature_gate(¶m_ty) { + format!( + "optional feature `{}` required for type {} of param #{}", + feature_gate, + param_ty, + i + 1, + ) + } else { + format!("unsupported type {} for param #{}", param_ty, i + 1) + } + })? + .parse::() + .map_err(|_| format!("Rust type mapping for {} not parsable", param_ty))? + + } + }; Ok(quote_spanned!(expr.span() => // this shouldn't actually run From 233d8986814a6bd6a0d1935809c2b7393c44e8b4 Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Thu, 11 Jun 2020 20:56:37 -0700 Subject: [PATCH 7/9] doc(macros): document type override feature for macros --- src/macros.rs | 105 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 102 insertions(+), 3 deletions(-) diff --git a/src/macros.rs b/src/macros.rs index 0004a2219a..34c8e7a024 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -1,6 +1,6 @@ /// Statically checked SQL query with `println!()` style syntax. /// -/// This expands to an instance of [QueryAs] that outputs an ad-hoc anonymous struct type, +/// This expands to an instance of [QueryAs][crate::QueryAs] that outputs an ad-hoc anonymous struct type, /// if the query has output columns, or `()` (unit) otherwise: /// /// ```rust,ignore @@ -111,9 +111,78 @@ /// `NULL` which then depends on the semantics of what functions are used. Consult the MySQL /// manual for the functions you are using to find the cases in which they return `NULL`. /// -/// To override the nullability of an output column, use [query_as!]. +/// To override the nullability of an output column, use [query_as!], or see below. /// -/// ### Offline Mode (requires the `offline` feature) +/// ## Type Overrides: Bind Parameters (Postgres only) +/// For typechecking of bind parameters, casts using `as` are treated as overrides for the inferred +/// types of bind parameters and no typechecking is emitted: +/// +/// ```rust,ignore +/// #[derive(sqlx::Type)] +/// #[sqlx(transparent)] +/// struct MyInt4(i32); +/// +/// let my_int = MyInt4(1); +/// +/// sqlx::query!("select $1::int4 as id", my_int as MyInt4) +/// ``` +/// +/// In Rust 1.45 we can eliminate this redundancy by allowing casts using `as _` or type ascription +/// syntax, i.e. `my_int: _` (which is unstable but can be stripped), but this requires modifying +/// the expression which is not possible as the macros are currently implemented. Casts to `_` are +/// forbidden for now as they produce rather nasty type errors. +/// +/// ## Type Overrides: Output Columns +/// Type overrides are also available for output columns, utilizing the SQL standard's support +/// for arbitrary text in column names: +/// +/// * selecting a column `foo as "foo!"` (Postgres / SQLite) or `` foo as `foo!` `` overrides +/// inferred nullability and forces the column to be treated as `NOT NULL`; this is useful e.g. for +/// selecting expressions in Postgres where we cannot infer nullability: +/// +/// ```rust,ignore +/// # async fn main() { +/// # let mut conn = panic!(); +/// // Postgres: using a raw query string lets us use unescaped double-quotes +/// // Note that this query wouldn't work in SQLite as we still don't know the exact type of `id` +/// let record = sqlx::query!(r#"select 1 as "id!""#) // MySQL: use "select 1 as `id!`" instead +/// .fetch_one(&mut conn) +/// .await?; +/// +/// // For Postgres this would have been inferred to be Option instead +/// assert_eq!(record.id, 1i32); +/// # } +/// ``` +/// +/// * selecting a column `foo as "foo: T"` (Postgres / SQLite) or `` foo as `foo: T` `` (MySQL) +/// overrides the inferred type which is useful when selecting user-defined custom types +/// (dynamic type checking is still done so if the types are incompatible this will be an error +/// at runtime instead of compile-time): +/// +/// ```rust,ignore +/// # async fn main() { +/// # let mut conn = panic!(); +/// #[derive(sqlx::Type)] +/// #[sqlx(transparent) +/// struct MyInt4(i32); +/// +/// let my_int = MyInt4(1); +/// +/// // Postgres/SQLite +/// sqlx::query!(r#"select 1 as "id: MyInt4""#) // MySQL: use "select 1 as `id: MyInt4`" instead +/// .fetch_one(&mut conn) +/// .await?; +/// +/// // For Postgres this would have been inferred to be `Option`, MySQL `i32` +/// // and SQLite it wouldn't have worked at all because we couldn't know the type. +/// assert_eq!(record.id, MyInt4(1)); +/// # } +/// ``` +/// +/// As mentioned, this allows specifying the type of a pure expression column which is normally +/// forbidden for SQLite as there's no way we can ask SQLite what type the column is expected to be. +/// +/// ## Offline Mode (requires the `offline` feature) /// The macros can be configured to not require a live database connection for compilation, /// but it requires a couple extra steps: /// @@ -313,6 +382,36 @@ macro_rules! query_file_unchecked ( /// ## Nullability /// Use `Option` for columns which may be `NULL` in order to avoid a runtime error being returned /// from `.fetch_*()`. +/// +/// ### Additional Column Type Override Option +/// In addition to the column type overrides supported by [query!], `query_as!()` supports an +/// additional override option: +/// +/// If you select a column `foo as "foo: _"` (Postgres/SQLite) or `` foo as `foo: _` `` (MySQL) +/// it causes that column to be inferred based on the type of the corresponding field in the given +/// record struct. Runtime type-checking is still done so an error will be emitted if the types +/// are not compatible. +/// +/// This allows you to override the inferred type of a column to instead use a custom-defined type: +/// +/// ```rust,ignore +/// #[derive(sqlx::Type)] +/// #[sqlx(transparent) +/// struct MyInt4(i32); +/// +/// struct Record { +/// id: MyInt4, +/// } +/// +/// let my_int = MyInt4(1); +/// +/// // Postgres/SQLite +/// sqlx::query!(r#"select 1 as "id: _""#) // MySQL: use "select 1 as `id: _`" instead +/// .fetch_one(&mut conn) +/// .await?; +/// +/// assert_eq!(record.id, MyInt4(1)); +/// ``` #[macro_export] #[cfg_attr(docsrs, doc(cfg(feature = "macros")))] macro_rules! query_as ( From 3e8d354f455ad4d90d416d3967907eeade49bc42 Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Fri, 12 Jun 2020 19:47:16 -0700 Subject: [PATCH 8/9] feat(macros): support nullable column override --- sqlx-macros/src/query/output.rs | 18 ++++++++++--- src/macros.rs | 46 ++++++++++++++++++++++++++++++++ tests/mysql/macros.rs | 14 ++++++++++ tests/postgres/macros.rs | 17 ++++++++++++ tests/sqlite/macros.rs | 21 ++++++++++++--- tests/sqlite/setup.sql | 2 ++ tests/sqlite/sqlite.db | Bin 36864 -> 36864 bytes 7 files changed, 110 insertions(+), 8 deletions(-) diff --git a/sqlx-macros/src/query/output.rs b/sqlx-macros/src/query/output.rs index e93b386743..659e747bd9 100644 --- a/sqlx-macros/src/query/output.rs +++ b/sqlx-macros/src/query/output.rs @@ -30,6 +30,7 @@ struct ColumnDecl { enum ColumnOverride { NonNull, + Nullable, Wildcard, Exact(Type), } @@ -53,14 +54,19 @@ pub fn columns_to_rust(describe: &Describe) -> crate::Resul let type_ = match decl.r#override { Some(ColumnOverride::Exact(ty)) => Some(ty.to_token_stream()), Some(ColumnOverride::Wildcard) => None, + // these three could be combined but I prefer the clarity here Some(ColumnOverride::NonNull) => Some(get_column_type(i, column)), + Some(ColumnOverride::Nullable) => { + let type_ = get_column_type(i, column); + Some(quote! { Option<#type_> }) + } None => { let type_ = get_column_type(i, column); - if !column.not_null.unwrap_or(false) { - Some(quote! { Option<#type_> }) - } else { + if column.not_null.unwrap_or(false) { Some(type_) + } else { + Some(quote! { Option<#type_> }) } } }; @@ -165,7 +171,7 @@ impl ColumnDecl { // find the end of the identifier because we want to use our own logic to parse it // if we tried to feed this into `syn::parse_str()` we might get an un-great error // for some kinds of invalid identifiers - let (ident, remainder) = if let Some(i) = col_name.find(&[':', '!'][..]) { + let (ident, remainder) = if let Some(i) = col_name.find(&[':', '!', '?'][..]) { let (ident, remainder) = col_name.split_at(i); (parse_ident(ident)?, remainder) @@ -202,6 +208,10 @@ impl Parse for ColumnOverride { input.parse::()?; Ok(ColumnOverride::NonNull) + } else if lookahead.peek(Token![?]) { + input.parse::()?; + + Ok(ColumnOverride::Nullable) } else { Err(lookahead.error()) } diff --git a/src/macros.rs b/src/macros.rs index 34c8e7a024..09320646e4 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -152,8 +152,54 @@ /// // For Postgres this would have been inferred to be Option instead /// assert_eq!(record.id, 1i32); /// # } +/// +/// ``` +/// * selecting a column `foo as "foo?"` (Postgres / SQLite) or `` foo as `foo?` `` overrides +/// inferred nullability and forces the column to be treated as nullable; this is provided mainly +/// for symmetry with `!`, but also because nullability inference currently has some holes and false +/// negatives that may not be completely fixable without doing our own complex analysis on the given +/// query. +/// +/// ```rust,ignore +/// # async fn main() { +/// # let mut conn = panic!(); +/// // Postgres: +/// // Note that this query wouldn't work in SQLite as we still don't know the exact type of `id` +/// let record = sqlx::query!(r#"select 1 as "id?""#) // MySQL: use "select 1 as `id?`" instead +/// .fetch_one(&mut conn) +/// .await?; +/// +/// // For Postgres this would have been inferred to be Option anyway +/// // but this is just a basic example +/// assert_eq!(record.id, Some(1i32)); +/// # } +/// ``` +/// +/// One current such hole is exposed by left-joins involving `NOT NULL` columns in Postgres and +/// SQLite; as we only know nullability for a given column based on the `NOT NULL` constraint +/// of its original column in a table, if that column is then brought in via a `LEFT JOIN` +/// we have no good way to know and so continue assuming it may not be null which may result +/// in some `UnexpectedNull` errors at runtime. +/// +/// Using `?` as an override we can fix this for columns we know to be nullable in practice: +/// +/// ```rust,ignore +/// # async fn main() { +/// # let mut conn = panic!(); +/// // Ironically this is the exact column we look at to determine nullability in Postgres +/// let record = sqlx::query!( +/// r#"select attnotnull as "attnotnull?" from (values (1)) ids left join pg_attribute on false"# +/// ) +/// .fetch_one(&mut conn) +/// .await?; +/// +/// // For Postgres this would have been inferred to be `bool` and we would have gotten an error +/// assert_eq!(record.attnotnull, None); +/// # } /// ``` /// +/// See [launchbadge/sqlx#367](https://github.com/launchbadge/sqlx/issues/367) for more details on this issue. +/// /// * selecting a column `foo as "foo: T"` (Postgres / SQLite) or `` foo as `foo: T` `` (MySQL) /// overrides the inferred type which is useful when selecting user-defined custom types /// (dynamic type checking is still done so if the types are incompatible this will be an error diff --git a/tests/mysql/macros.rs b/tests/mysql/macros.rs index 135b7056d8..bd25979334 100644 --- a/tests/mysql/macros.rs +++ b/tests/mysql/macros.rs @@ -106,6 +106,20 @@ async fn test_column_override_not_null() -> anyhow::Result<()> { Ok(()) } +#[sqlx_macros::test] +async fn test_column_override_nullable() -> anyhow::Result<()> { + let mut conn = new::().await?; + + // MySQL by default tells us `id` is not-null + let record = sqlx::query!("select * from (select 1 as `id?`) records") + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, Some(1)); + + Ok(()) +} + #[derive(PartialEq, Eq, Debug, sqlx::Type)] #[sqlx(transparent)] struct MyInt4(i32); diff --git a/tests/postgres/macros.rs b/tests/postgres/macros.rs index 4d9c90e257..c19d76a8b7 100644 --- a/tests/postgres/macros.rs +++ b/tests/postgres/macros.rs @@ -268,6 +268,23 @@ async fn test_column_override_not_null() -> anyhow::Result<()> { Ok(()) } +#[sqlx_macros::test] +async fn test_column_override_nullable() -> anyhow::Result<()> { + let mut conn = new::().await?; + + // workaround for https://github.com/launchbadge/sqlx/issues/367 + // declare a `NOT NULL` column from a left-joined table to be nullable + let record = sqlx::query!( + r#"select text as "text?" from (values (1)) foo(id) left join tweet on false"# + ) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.text, None); + + Ok(()) +} + #[derive(PartialEq, Eq, Debug, sqlx::Type)] #[sqlx(transparent)] struct MyInt4(i32); diff --git a/tests/sqlite/macros.rs b/tests/sqlite/macros.rs index 338b1609ae..f0b424852b 100644 --- a/tests/sqlite/macros.rs +++ b/tests/sqlite/macros.rs @@ -76,11 +76,24 @@ async fn macro_select_from_view() -> anyhow::Result<()> { async fn test_column_override_not_null() -> anyhow::Result<()> { let mut conn = new::().await?; - let record = sqlx::query!(r#"select is_active as "is_active!" from accounts"#) + let record = sqlx::query!(r#"select owner_id as `owner_id!` from tweet"#) .fetch_one(&mut conn) .await?; - assert_eq!(record.is_active, true); + assert_eq!(record.owner_id, 1); + + Ok(()) +} + +#[sqlx_macros::test] +async fn test_column_override_nullable() -> anyhow::Result<()> { + let mut conn = new::().await?; + + let record = sqlx::query!(r#"select text as `text?` from tweet"#) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.text.as_deref(), Some("#sqlx is pretty cool!")); Ok(()) } @@ -97,7 +110,7 @@ async fn test_column_override_wildcard() -> anyhow::Result<()> { let mut conn = new::().await?; - let record = sqlx::query_as!(Record, r#"select id as "id: _" from accounts"#) + let record = sqlx::query_as!(Record, r#"select id as "id: _" from tweet"#) .fetch_one(&mut conn) .await?; @@ -117,7 +130,7 @@ async fn test_column_override_wildcard() -> anyhow::Result<()> { async fn test_column_override_exact() -> anyhow::Result<()> { let mut conn = new::().await?; - let record = sqlx::query!(r#"select id as "id: MyInt" from accounts"#) + let record = sqlx::query!(r#"select id as "id: MyInt" from tweet"#) .fetch_one(&mut conn) .await?; diff --git a/tests/sqlite/setup.sql b/tests/sqlite/setup.sql index 18cc842664..f0306ab98b 100644 --- a/tests/sqlite/setup.sql +++ b/tests/sqlite/setup.sql @@ -6,3 +6,5 @@ CREATE TABLE tweet is_sent BOOLEAN NOT NULL DEFAULT TRUE, owner_id BIGINT ); + +insert into tweet(id, text, owner_id) values (1, '#sqlx is pretty cool!', 1); diff --git a/tests/sqlite/sqlite.db b/tests/sqlite/sqlite.db index 1daa80eee8b42c6e1bc57faf67d7423d292d5d60..9b925366fb4188048686712968aec3c400bdccc6 100644 GIT binary patch delta 86 zcmZozz|^pSX@WGP>O>i5M%9f8OZeFs`JXWGKiMoOaEo6}ijkGmoRd?zxG<+eA+uPa opeVJZq*5U{KR-v2i-CcGk^c_^{~w^*_x$WYE;BPH=OTjy04L5D6951J delta 53 zcmZozz|^pSX@WGP@1U3sMyyu@7z|F?R00eOEqym=BECzq#0hm<} Af&c&j From 5ecf81bca237386849023be73fca454bdf665b04 Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Mon, 15 Jun 2020 20:42:09 -0700 Subject: [PATCH 9/9] fix(macros): don't emit typechecking code for explicit type overrides on bind params --- sqlx-macros/src/query/args.rs | 5 +++-- tests/postgres/macros.rs | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/sqlx-macros/src/query/args.rs b/sqlx-macros/src/query/args.rs index 4b63dee892..756e3984a4 100644 --- a/sqlx-macros/src/query/args.rs +++ b/sqlx-macros/src/query/args.rs @@ -2,7 +2,7 @@ use proc_macro2::TokenStream; use syn::spanned::Spanned; use syn::{Expr, Type}; -use quote::{quote, quote_spanned, ToTokens}; +use quote::{quote, quote_spanned}; use sqlx_core::describe::Describe; use crate::database::{DatabaseExt, ParamChecking}; @@ -43,7 +43,8 @@ pub fn quote_args( "casts to `_` are not allowed in bind parameters yet" ).into() ), - Some(ty) => ty.to_token_stream(), + // cast or type ascription will fail to compile if the type does not match + Some(_) => return Ok(quote!()), None => { DB::param_type_for_id(¶m_ty) .ok_or_else(|| { diff --git a/tests/postgres/macros.rs b/tests/postgres/macros.rs index c19d76a8b7..ec263f49cc 100644 --- a/tests/postgres/macros.rs +++ b/tests/postgres/macros.rs @@ -343,6 +343,18 @@ async fn test_bind_arg_override_exact() -> anyhow::Result<()> { assert_eq!(record.id, Some(1i64)); + // test the override with `Option` + let my_opt_int = Some(MyInt4(1)); + + let record = sqlx::query!( + "select * from (select 1::int4) records(id) where id = $1", + my_opt_int as Option + ) + .fetch_one(&mut conn) + .await?; + + assert_eq!(record.id, Some(1i32)); + Ok(()) }