Skip to content

Commit

Permalink
Add a flag to help users better debug bad Queryable impls
Browse files Browse the repository at this point in the history
Since the impl we generate is so generic, when a bad `Queryable` impl is
given, the error doesn't occur until they actually try to load a query.
This ends up being really non-local, and gives little to no feedback
about which field is actually the problem.

There is a new option called `#[check_types]` which completely changes
how the impl is generated, in such a way that when the `unstable`
feature is enabled the error will point at the specific field that is
the problem. The actual message is the same, but where it points makes
debugging much easier.

This flag also generates code which validates that your fields are in
the right order to match your columns.

Unfortunately, we cannot have this generate an actual usable impl.
Trait bounds not matching in types or where clauses will always result
in an error pointing at the derive itself, which is no more helpful than
where we started. For that reason, we can't actually have any usable
`Row` type. So this flag is for debugging purposes only.

This does not help with cases where the number of fields is incorrect.
  • Loading branch information
sgrif committed Apr 6, 2018
1 parent 8785523 commit 2d554dd
Show file tree
Hide file tree
Showing 9 changed files with 299 additions and 30 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
rounding issues. This is primarily to support things like `avg(int_col)`,
which we define as returning `Numeric`

* `#[check_types]` has been added to help debug missing `Queryable` impls. See
[the docs][check_types-1.2.0] for details.

[check_types-1.2.0]: http://docs.diesel.rs/diesel/deserialize/trait.Queryable.html#debugging

### Changed

* The bounds on `impl ToSql for Cow<'a, T>` have been loosened to no longer
Expand Down
87 changes: 87 additions & 0 deletions diesel/src/deserialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,93 @@ pub type Result<T> = result::Result<T, Box<Error + Send + Sync>>;
/// assert_eq!(expected, first_user);
/// # Ok(())
/// # }
/// ```
///
/// # Debugging
///
/// When your struct has the wrong types for your query, the error messages
/// can often be hard to interpret. For example, take this struct where
/// `hair_color` and `updated_at` should both be `Option`s. Additionally,
/// `created_at` and `updated_at` are in the wrong order.
///
/// ```ignore
/// table! {
/// users {
/// id -> Integer,
/// name -> Text,
/// hair_color -> Nullable<Text>,
/// created_at -> Timestamp,
/// updated_at -> Nullable<Timestamp>,
/// }
/// }
///
/// #[derive(Queryable)]
/// struct User {
/// id: i32,
/// name: String,
/// hair_color: String,
/// updated_at: SystemTime,
/// created_at: SystemTime,
/// }
/// ```
///
/// We get this error:
///
/// ```text
/// |
/// 28 | users::table.load::<User>(&conn).unwrap();
/// | ^^^^ the trait `diesel::deserialize::FromSql<diesel::sql_types::Nullable<diesel::sql_types::Timestamp>, diesel::pg::Pg>` is not implemented for `std::time::SystemTime`
///
/// |
/// 28 | users::table.load::<User>(&conn).unwrap();
/// | ^^^^ the trait `diesel::deserialize::FromSql<diesel::sql_types::Nullable<diesel::sql_types::Text>, diesel::pg::Pg>` is not implemented for `*const str`
/// ```
///
/// While we could probably figure out what's wrong from this if we really
/// needed to, it'd be nice if this told us which fields are the problem. Diesel
/// provides `#[check_type]` to help with this. You'll need to provide the
/// backend you plan to use, and either the table name you're querying or the
/// SQL types you're expecting. By changing our struct definition to this:
///
/// ```ignore
/// #[derive(Queryable)]
/// #[check_types(backend = "diesel::pg::Pg")]
/// #[table_name = "users"]
/// struct User {
/// id: i32,
/// name: String,
/// hair_color: String,
/// updated_at: SystemTime,
/// created_at: SystemTime,
/// }
/// ```
///
/// We get much more specific error messages:
///
/// ```text
/// |
/// 23 | hair_color: String,
/// | ^^^^^^^^^^ the trait `diesel::deserialize::FromSql<diesel::sql_types::Nullable<diesel::sql_types::Text>, diesel::pg::Pg>` is not implemented for `*const str`
///
/// |
/// 24 | updated_at: SystemTime,
/// | ^^^^^^^^^^ the trait `diesel::deserialize::FromSql<diesel::sql_types::Nullable<diesel::sql_types::Timestamp>, diesel::pg::Pg>` is not implemented for `std::time::SystemTime`
///
/// |
/// 24| / updated_at: SystemTime,
/// |
/// = note: expected type `users::columns::created_at`
/// found type `users::columns::updated_at`
///
/// |
/// 25| / created_at: SystemTime,
/// |
/// = note: expected type `users::columns::updated_at`
/// found type `users::columns::created_at`
/// ```
///
/// Keep in mind that `#[check_types]` is only for debugging purposes. The impl
/// it generates will cause an error at runtime.
pub trait Queryable<ST, DB>
where
DB: Backend,
Expand Down
37 changes: 37 additions & 0 deletions diesel_compile_tests/tests/ui/derive_queryable_checked_sql_type.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#[macro_use]
extern crate diesel;

use diesel::sql_types::{Nullable, Text};
use diesel::pg::Pg;

table! {
users {
id -> Integer,
}
}

#[derive(Queryable)]
#[table_name = "users"]
#[check_types(backend = "Pg")]
struct User {
id: String,
#[sql_type = "Nullable<Text>"]
name: String,
}

table! {
posts {
id -> Integer,
user_id -> Integer,
}
}

#[derive(Queryable)]
#[table_name = "posts"]
#[check_types(backend = "Pg")]
struct Post {
user_id: i32,
id: i32,
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
error[E0277]: the trait bound `*const str: diesel::deserialize::FromSql<diesel::sql_types::Integer, diesel::pg::Pg>` is not satisfied
--> $DIR/derive_queryable_checked_sql_type.rs:17:5
|
17 | id: String,
| ^^ the trait `diesel::deserialize::FromSql<diesel::sql_types::Integer, diesel::pg::Pg>` is not implemented for `*const str`
|
= help: the following implementations were found:
<*const str as diesel::deserialize::FromSql<diesel::sql_types::Text, diesel::sqlite::Sqlite>>
<*const str as diesel::deserialize::FromSql<diesel::sql_types::Text, DB>>
<*const str as diesel::deserialize::FromSql<diesel::sql_types::Time, diesel::sqlite::Sqlite>>
<*const [u8] as diesel::deserialize::FromSql<diesel::sql_types::Binary, diesel::sqlite::Sqlite>>
and 3 others
= note: required because of the requirements on the impl of `diesel::deserialize::FromSql<diesel::sql_types::Integer, diesel::pg::Pg>` for `std::string::String`
= note: required because of the requirements on the impl of `diesel::Queryable<diesel::sql_types::Integer, diesel::pg::Pg>` for `std::string::String`

error[E0277]: the trait bound `*const str: diesel::deserialize::FromSql<diesel::sql_types::Nullable<diesel::sql_types::Text>, diesel::pg::Pg>` is not satisfied
--> $DIR/derive_queryable_checked_sql_type.rs:18:5
|
18 | #[sql_type = "Nullable<Text>"]
| ^ the trait `diesel::deserialize::FromSql<diesel::sql_types::Nullable<diesel::sql_types::Text>, diesel::pg::Pg>` is not implemented for `*const str`
|
= help: the following implementations were found:
<*const str as diesel::deserialize::FromSql<diesel::sql_types::Text, diesel::sqlite::Sqlite>>
<*const str as diesel::deserialize::FromSql<diesel::sql_types::Text, DB>>
<*const str as diesel::deserialize::FromSql<diesel::sql_types::Time, diesel::sqlite::Sqlite>>
<*const [u8] as diesel::deserialize::FromSql<diesel::sql_types::Binary, diesel::sqlite::Sqlite>>
and 3 others
= note: required because of the requirements on the impl of `diesel::deserialize::FromSql<diesel::sql_types::Nullable<diesel::sql_types::Text>, diesel::pg::Pg>` for `std::string::String`
= note: required because of the requirements on the impl of `diesel::Queryable<diesel::sql_types::Nullable<diesel::sql_types::Text>, diesel::pg::Pg>` for `std::string::String`

error[E0308]: mismatched types
--> $DIR/derive_queryable_checked_sql_type.rs:33:5
|
1 | | #[macro_use]
| |_^ expected struct `posts::columns::id`, found struct `posts::columns::user_id`
...
33| / user_id: i32,
|
= note: expected type `posts::columns::id`
found type `posts::columns::user_id`

error[E0308]: mismatched types
--> $DIR/derive_queryable_checked_sql_type.rs:34:5
|
1 | | #[macro_use]
| |_^ expected struct `posts::columns::user_id`, found struct `posts::columns::id`
...
34| / id: i32,
|
= note: expected type `posts::columns::user_id`
found type `posts::columns::id`

error: aborting due to 4 previous errors

3 changes: 2 additions & 1 deletion diesel_derives/src/from_sql_row.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,12 @@ pub fn derive(mut item: syn::DeriveInput) -> Result<Tokens, Diagnostic> {
dummy_mod,
quote! {
use self::diesel::deserialize::{self, FromSql, FromSqlRow, Queryable};
use self::diesel::row::Row;

impl #impl_generics FromSqlRow<__ST, __DB> for #struct_ty
#where_clause
{
fn build_from_row<R: diesel::row::Row<__DB>>(row: &mut R)
fn build_from_row<R: Row<__DB>>(row: &mut R)
-> deserialize::Result<Self>
{
FromSql::<__ST, __DB>::from_sql(row.take())
Expand Down
2 changes: 1 addition & 1 deletion diesel_derives/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ pub fn derive_query_id(input: TokenStream) -> TokenStream {
expand_derive(input, query_id::derive)
}

#[proc_macro_derive(Queryable, attributes(column_name))]
#[proc_macro_derive(Queryable, attributes(table_name, column_name, sql_type, diesel, check_types))]
pub fn derive_queryable(input: TokenStream) -> TokenStream {
expand_derive(input, queryable::derive)
}
Expand Down
26 changes: 26 additions & 0 deletions diesel_derives/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,32 @@ impl Model {
pub fn has_table_name_attribute(&self) -> bool {
self.table_name_from_attribute.is_some()
}

pub fn sql_type_of(&self, field: &Field) -> syn::Type {
let table_name = self.table_name();
let column_name = field.column_name();

match field.sql_type {
Some(ref st) => st.clone(),
None => if self.has_table_name_attribute() {
parse_quote!(diesel::dsl::SqlTypeOf<#table_name::#column_name>)
} else {
let field_name = match field.name {
FieldName::Named(ref x) => x.as_ref(),
_ => "field",
};
field
.span
.error(format!("Cannot determine the SQL type of {}", field_name))
.help(
"Your struct must either be annotated with `#[table_name = \"foo\"]` \
or have all of its fields annotated with `#[sql_type = \"Integer\"]`",
)
.emit();
parse_quote!(())
},
}
}
}

pub fn camel_to_snake(name: &str) -> String {
Expand Down
85 changes: 85 additions & 0 deletions diesel_derives/src/queryable.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,24 @@
use proc_macro2::Span;
use quote;
use syn;

use field::*;
use meta::*;
use model::*;
use util::*;

pub fn derive(item: syn::DeriveInput) -> Result<quote::Tokens, Diagnostic> {
if let Some(meta) = MetaItem::with_name(&item.attrs, "check_types") {
if !cfg!(feature = "nightly") {
meta.span()
.error(
"`#[check_types]` requires the `unstable` feature on Diesel and a nightly compiler",
)
.emit();
}

return derive_checked(item, meta);
}
let model = Model::from_item(&item)?;

let struct_name = item.ident;
Expand Down Expand Up @@ -49,3 +63,74 @@ pub fn derive(item: syn::DeriveInput) -> Result<quote::Tokens, Diagnostic> {
},
))
}

fn derive_checked(
item: syn::DeriveInput,
check_types: MetaItem,
) -> Result<quote::Tokens, Diagnostic> {
let model = Model::from_item(&item)?;

let struct_name = item.ident;
let backend = check_types.nested_item("backend")?.ty_value()?;
let sql_tys = model.fields().iter().map(|f| model.sql_type_of(f));
let field_exprs = model
.fields()
.iter()
.enumerate()
.map(|(i, f)| field_expr(i, f, &model, &backend));

let (impl_generics, ty_generics, where_clause) = item.generics.split_for_impl();

Ok(wrap_in_dummy_mod(
model.dummy_mod_name("queryable"),
quote! {
use self::diesel::deserialize::{self, Queryable, FromSqlRow};

pub enum DummyRow {}

impl<ST> FromSqlRow<ST, #backend> for DummyRow {
fn build_from_row<R: self::diesel::row::Row<#backend>>(_: &mut R)
-> deserialize::Result<Self>
{
use self::std::result::Result::Err;
use self::std::convert::Into;

Err("`#[check_types]` is only for debugging purposes".into())
}
}

impl #impl_generics Queryable<(#(#sql_tys,)*), #backend>
for #struct_name #ty_generics
#where_clause
{
type Row = DummyRow;

#[allow(unreachable_code)]
fn build(row: Self::Row) -> Self {
Self {
#(#field_exprs,)*
}
}
}
},
))
}

fn field_expr(idx: usize, field: &Field, model: &Model, backend: &syn::Type) -> syn::FieldValue {
let st = model.sql_type_of(field);
let mut tokens = quote_spanned! {field.span.resolved_at(Span::def_site())=>
Queryable::<#st, #backend>::build(unimplemented!())
};
if field.sql_type.is_none() {
let table_name = model.table_name();
let column_name = field.column_name();
let idx = syn::Index::from(idx);
tokens = quote_spanned! {column_name.span=>
{
let #table_name::#column_name = #table_name::all_columns.#idx;
#tokens
}
}
}
field.name.assign(parse_quote!(#tokens))
}
Loading

0 comments on commit 2d554dd

Please sign in to comment.