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.

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

Add some additional checks to #[derive(Selectable)] to ensure field #3359

Merged
merged 5 commits into from
Mar 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions diesel/src/deserialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@ pub type Result<T> = result::Result<T, Box<dyn Error + Send + Sync>>;
///
/// This trait can be [derived](derive@Queryable)
///
/// ## How to resolve compiler errors while loading data from the database
///
/// In case you getting uncomprehensable compiler errors while loading data
/// from the database you might want to consider using
/// [`#[derive(Selectable)]`](derive@crate::prelude::Selectable) +
/// `#[diesel(check_for_backend(YourBackendType))]` to check for mismatching fields at compile
/// time. This drastically improves the quality of the generated error messages by pointing
/// to concrete mismatches at field level. You need to specify the concrete database backend
/// this specific struct is indented to be used with, as otherwise rustc cannot correctly
/// identify the required deserialization implementation.
///
/// ## Interaction with `NULL`/`Option`
/// [`Nullable`][crate::sql_types::Nullable] types can be queried into `Option`.
/// This is valid for single fields, tuples, and structures with `Queryable`.
Expand Down Expand Up @@ -409,6 +420,13 @@ pub use diesel_derives::QueryableByName;
/// }
/// }
/// ```
#[cfg_attr(
feature = "nightly-error-messages",
rustc_on_unimplemented(
message = "Cannot deserialize a value of the type `{A}` as `{Self}`",
note = "Double check your type mappings via the documentation of `{A}`"
)
)]
pub trait FromSql<A, DB: Backend>: Sized {
/// See the trait documentation.
fn from_sql(bytes: DB::RawValue<'_>) -> Result<Self>;
Expand Down
13 changes: 13 additions & 0 deletions diesel/src/query_dsl/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1450,6 +1450,19 @@ pub trait RunQueryDsl<Conn>: Sized {
/// [`execute`]: crate::query_dsl::RunQueryDsl::execute()
/// [`sql_query`]: crate::sql_query()
///
/// ## How to resolve compiler errors while loading data from the database
///
/// In case you getting uncomprehensable compiler errors while loading data
/// from the database into a type using [`#[derive(Queryable)]`](derive@crate::prelude::Queryable)
/// you might want to consider
/// using [`#[derive(Selectable)]`](derive@crate::prelude::Selectable) +
/// `#[diesel(check_for_backend(YourBackendType))]`
/// to check for mismatching fields at compile time. This drastically improves
/// the quality of the generated error messages by pointing to concrete type mismatches at
/// field level.You need to specify the concrete database backend
/// this specific struct is indented to be used with, as otherwise rustc cannot correctly
/// identify the required deserialization implementation.
///
/// # Examples
///
/// ## Returning a single field
Expand Down
2 changes: 2 additions & 0 deletions diesel_compile_tests/tests/fail/derive/bad_primary_key.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ error: unexpected end of input, expected parentheses
|
26 | #[diesel(primary_key)]
| ^
|
= help: The correct format looks like `#[diesel(key1, key2)]`

error: expected parentheses
--> tests/fail/derive/bad_primary_key.rs:33:22
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
error: unknown attribute, expected one of `aggregate`, `not_sized`, `foreign_derive`, `table_name`, `sql_type`, `treat_none_as_default_value`, `treat_none_as_null`, `belongs_to`, `mysql_type`, `sqlite_type`, `postgres_type`, `primary_key`
--> $DIR/unknown_attribute.rs:5:10
error: unknown attribute, expected one of `aggregate`, `not_sized`, `foreign_derive`, `table_name`, `sql_type`, `treat_none_as_default_value`, `treat_none_as_null`, `belongs_to`, `mysql_type`, `sqlite_type`, `postgres_type`, `primary_key`, `check_for_backend`
--> tests/fail/derive/unknown_attribute.rs:5:10
|
5 | #[diesel(what = true)]
| ^^^^

error: unknown attribute, expected one of `embed`, `column_name`, `sql_type`, `serialize_as`, `deserialize_as`, `select_expression`, `select_expression_type`
--> $DIR/unknown_attribute.rs:12:14
--> tests/fail/derive/unknown_attribute.rs:12:14
|
12 | #[diesel(what = true)]
| ^^^^
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,13 @@ error[E0412]: cannot find type `foo` in this scope
27 | #[sql_type = "foo"]
| ^^^^^ not found in this scope

error[E0277]: the trait bound `i32: FromSql<_, __DB>` is not satisfied
error[E0277]: Cannot deserialize a value of the type `_` as `i32`
--> tests/fail/derive_deprecated/deprecated_sql_type.rs:25:10
|
25 | #[derive(QueryableByName)]
| ^^^^^^^^^^^^^^^ the trait `FromSql<_, __DB>` is not implemented for `i32`
|
= note: Double check your type mappings via the documentation of `_`
= help: the following other types implement trait `FromSql<A, DB>`:
<i32 as FromSql<diesel::sql_types::Integer, Mysql>>
<i32 as FromSql<diesel::sql_types::Integer, Pg>>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
error[E0277]: the trait bound `i32: FromSql<diesel::sql_types::Text, _>` is not satisfied
error[E0277]: Cannot deserialize a value of the type `diesel::sql_types::Text` as `i32`
--> tests/fail/select_carries_correct_result_type_info.rs:19:39
|
19 | let ids = select_name.load::<i32>(&mut connection);
| ---- ^^^^^^^^^^^^^^^ the trait `FromSql<diesel::sql_types::Text, _>` is not implemented for `i32`
| |
| required by a bound introduced by this call
|
= note: Double check your type mappings via the documentation of `diesel::sql_types::Text`
= help: the following other types implement trait `FromSql<A, DB>`:
<i32 as FromSql<diesel::sql_types::Integer, Mysql>>
<i32 as FromSql<diesel::sql_types::Integer, Pg>>
Expand All @@ -20,14 +21,15 @@ note: required by a bound in `diesel::RunQueryDsl::load`
| Self: LoadQuery<'query, Conn, U>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `RunQueryDsl::load`

error[E0277]: the trait bound `*const str: FromSql<diesel::sql_types::Integer, _>` is not satisfied
error[E0277]: Cannot deserialize a value of the type `diesel::sql_types::Integer` as `*const str`
--> tests/fail/select_carries_correct_result_type_info.rs:20:42
|
20 | let names = select_id.load::<String>(&mut connection);
| ---- ^^^^^^^^^^^^^^^ the trait `FromSql<diesel::sql_types::Integer, _>` is not implemented for `*const str`
| |
| required by a bound introduced by this call
|
= note: Double check your type mappings via the documentation of `diesel::sql_types::Integer`
= help: the following other types implement trait `FromSql<A, DB>`:
<*const str as FromSql<diesel::sql_types::Text, Mysql>>
<*const str as FromSql<diesel::sql_types::Text, Pg>>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
error[E0277]: the trait bound `*const str: FromSql<BigInt, _>` is not satisfied
error[E0277]: Cannot deserialize a value of the type `BigInt` as `*const str`
--> tests/fail/select_sql_still_ensures_result_type.rs:16:51
|
16 | let count = select_count.get_result::<String>(&mut connection).unwrap();
| ---------- ^^^^^^^^^^^^^^^ the trait `FromSql<BigInt, _>` is not implemented for `*const str`
| |
| required by a bound introduced by this call
|
= note: Double check your type mappings via the documentation of `BigInt`
= help: the following other types implement trait `FromSql<A, DB>`:
<*const str as FromSql<diesel::sql_types::Text, Mysql>>
<*const str as FromSql<diesel::sql_types::Text, Pg>>
Expand Down
39 changes: 39 additions & 0 deletions diesel_compile_tests/tests/fail/selectable_with_typemisamatch.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
extern crate diesel;

use diesel::prelude::*;

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

#[derive(Selectable, Queryable)]
#[diesel(table_name = users)]
#[diesel(check_for_backend(diesel::pg::Pg))]
struct User {
id: String,
name: i32,
}

#[derive(Selectable, Queryable)]
#[diesel(table_name = users)]
#[diesel(check_for_backend(diesel::pg::Pg))]
struct UserCorrect {
id: i32,
name: String,
}

fn main() {
let mut conn = PgConnection::establish("...").unwrap();

users::table
.select(User::as_select())
.load(&mut conn)
.unwrap();
users::table
.select(UserCorrect::as_select())
.load(&mut conn)
.unwrap();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
error[E0277]: Cannot deserialize a value of the type `diesel::sql_types::Text` as `i32`
--> tests/fail/selectable_with_typemisamatch.rs:17:11
|
17 | name: i32,
| ^^^ the trait `FromSql<diesel::sql_types::Text, Pg>` is not implemented for `i32`
|
= note: Double check your type mappings via the documentation of `diesel::sql_types::Text`
= help: the following other types implement trait `FromSql<A, DB>`:
<i32 as FromSql<diesel::sql_types::Integer, Mysql>>
<i32 as FromSql<diesel::sql_types::Integer, Pg>>
<i32 as FromSql<diesel::sql_types::Integer, Sqlite>>
= note: required for `i32` to implement `diesel::Queryable<diesel::sql_types::Text, Pg>`
= note: required for `i32` to implement `FromSqlRow<diesel::sql_types::Text, Pg>`
= help: see issue #48214
= help: add `#![feature(trivial_bounds)]` to the crate attributes to enable

error[E0277]: Cannot deserialize a value of the type `diesel::sql_types::Integer` as `*const str`
--> tests/fail/selectable_with_typemisamatch.rs:16:9
|
16 | id: String,
| ^^^^^^ the trait `FromSql<diesel::sql_types::Integer, Pg>` is not implemented for `*const str`
|
= note: Double check your type mappings via the documentation of `diesel::sql_types::Integer`
= help: the following other types implement trait `FromSql<A, DB>`:
<*const str as FromSql<diesel::sql_types::Text, Mysql>>
<*const str as FromSql<diesel::sql_types::Text, Pg>>
<*const str as FromSql<diesel::sql_types::Text, Sqlite>>
= note: required for `std::string::String` to implement `FromSql<diesel::sql_types::Integer, Pg>`
= note: required for `std::string::String` to implement `diesel::Queryable<diesel::sql_types::Integer, Pg>`
= note: required for `std::string::String` to implement `FromSqlRow<diesel::sql_types::Integer, Pg>`
= help: see issue #48214
= help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
20 changes: 13 additions & 7 deletions diesel_derives/src/attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use syn::parse::discouraged::Speculative;
use syn::parse::{Parse, ParseStream, Parser, Result};
use syn::punctuated::Punctuated;
use syn::token::Comma;
use syn::{parenthesized, Attribute, Ident, LitBool, LitStr, Path, Type, TypePath};
use syn::{Attribute, Ident, LitBool, LitStr, Path, Type, TypePath};

use deprecated::ParseDeprecated;
use parsers::{BelongsTo, MysqlType, PostgresType, SqliteType};
Expand All @@ -20,6 +20,7 @@ use util::{
};

use crate::field::SelectExpr;
use crate::util::{parse_paren_list, CHECK_FOR_BACKEND_NOTE};

pub struct AttributeSpanWrapper<T> {
pub item: T,
Expand Down Expand Up @@ -123,7 +124,6 @@ impl Parse for FieldAttr {
name,
parse_eq(input, SELECT_EXPRESSION_TYPE_NOTE)?,
)),

_ => unknown_attribute(
&name,
&[
Expand Down Expand Up @@ -170,6 +170,7 @@ pub enum StructAttr {
SqliteType(Ident, SqliteType),
PostgresType(Ident, PostgresType),
PrimaryKey(Ident, Punctuated<Ident, Comma>),
CheckForBackend(Ident, syn::punctuated::Punctuated<TypePath, syn::Token![,]>),
}

impl Parse for StructAttr {
Expand Down Expand Up @@ -212,11 +213,14 @@ impl Parse for StructAttr {
name,
parse_paren(input, POSTGRES_TYPE_NOTE)?,
)),
"primary_key" => Ok(StructAttr::PrimaryKey(name, {
let content;
parenthesized!(content in input);
content.parse_terminated(Ident::parse)?
})),
"primary_key" => Ok(StructAttr::PrimaryKey(
name,
parse_paren_list(input, "key1, key2")?,
)),
"check_for_backend" => Ok(StructAttr::CheckForBackend(
name,
parse_paren_list(input, CHECK_FOR_BACKEND_NOTE)?,
)),

_ => unknown_attribute(
&name,
Expand All @@ -233,6 +237,7 @@ impl Parse for StructAttr {
"sqlite_type",
"postgres_type",
"primary_key",
"check_for_backend",
],
),
}
Expand All @@ -253,6 +258,7 @@ impl Spanned for StructAttr {
| StructAttr::MysqlType(ident, _)
| StructAttr::SqliteType(ident, _)
| StructAttr::PostgresType(ident, _)
| StructAttr::CheckForBackend(ident, _)
| StructAttr::PrimaryKey(ident, _) => ident.span(),
}
}
Expand Down
16 changes: 14 additions & 2 deletions diesel_derives/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#![recursion_limit = "1024"]
// Clippy lints
#![allow(
clippy::needless_doctest_main,
Expand Down Expand Up @@ -455,7 +454,10 @@ pub fn derive_query_id(input: TokenStream) -> TokenStream {
/// **Note**: When this trait is derived, it will assume that __all fields on
/// your struct__ matches __all fields in the query__, including the order and
/// count. This means that field order is significant if you are using
/// `#[derive(Queryable)]`. __Field name has no effect__.
/// `#[derive(Queryable)]`. __Field name has no effect__. If you see errors while
/// loading data into a struct that derives `Queryable`: Consider using [`#[derive(Selectable)]`]
/// + `#[diesel(check_for_backend(YourBackendType))]` to check for mismatching fields at compile
/// time.
///
/// To provide custom deserialization behavior for a field, you can use
/// `#[diesel(deserialize_as = SomeType)]`. If this attribute is present, Diesel
Expand Down Expand Up @@ -844,6 +846,16 @@ pub fn derive_queryable_by_name(input: TokenStream) -> TokenStream {
/// If this attribute is not used, the type name converted to
/// `snake_case` with an added `s` is used as table name.
///
/// ## Optional Type attributes
///
/// * `#[diesel(check_for_backend(diesel::pg::Pg, diesel::mysql::Mysql))]`, instructs
/// the derive to generate additional code to identify potential type mismatches.
/// It accepts a list of backend types to check the types against. Using this option
/// will result in much better error messages in cases where some types in your `Queryable`
/// struct do not match. You need to specify the concrete database backend
/// this specific struct is indented to be used with, as otherwise rustc cannot correctly
/// identify the required deserialization implementation.
///
/// ## Field attributes
///
/// * `#[diesel(column_name = some_column)]`, overrides the column name for
Expand Down
6 changes: 6 additions & 0 deletions diesel_derives/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ pub struct Model {
pub mysql_type: Option<MysqlType>,
pub sqlite_type: Option<SqliteType>,
pub postgres_type: Option<PostgresType>,
pub check_for_backend: Option<syn::punctuated::Punctuated<syn::TypePath, syn::Token![,]>>,
fields: Vec<Field>,
}

Expand Down Expand Up @@ -61,6 +62,7 @@ impl Model {
let mut mysql_type = None;
let mut sqlite_type = None;
let mut postgres_type = None;
let mut check_for_backend = None;

for attr in parse_attributes(attrs) {
match attr.item {
Expand All @@ -80,6 +82,9 @@ impl Model {
StructAttr::MysqlType(_, val) => mysql_type = Some(val),
StructAttr::SqliteType(_, val) => sqlite_type = Some(val),
StructAttr::PostgresType(_, val) => postgres_type = Some(val),
StructAttr::CheckForBackend(_, b) => {
check_for_backend = Some(b);
}
}
}

Expand All @@ -100,6 +105,7 @@ impl Model {
sqlite_type,
postgres_type,
fields: fields_from_item_data(fields),
check_for_backend,
}
}

Expand Down
Loading