From 3044d74eb3e81eee9821211498d1f332d065d6ca Mon Sep 17 00:00:00 2001 From: Gary Coady Date: Thu, 25 Jul 2024 23:04:39 +0200 Subject: [PATCH 1/4] Specify type of diesel::row::NamedRow::get in QueryableByName macro. This has the signature ``` fn get(&self, column_name: &str) -> Result where T: FromSql ``` This can be inferred from the resulting value, if there is only one `ST` type in scope for `FromSql`. But if multiple types are in scope, e.g. ``` impl<__DB: diesel::backend::Backend> QueryableByName<__DB> for StructWithTextAndCitext where String: diesel::deserialize::FromSql, String: diesel::deserialize::FromSql, { ``` then `ST` cannot be inferred. --- diesel_derives/src/queryable_by_name.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/diesel_derives/src/queryable_by_name.rs b/diesel_derives/src/queryable_by_name.rs index 16763059a70b..7848e20ed58c 100644 --- a/diesel_derives/src/queryable_by_name.rs +++ b/diesel_derives/src/queryable_by_name.rs @@ -23,12 +23,13 @@ pub fn derive(item: DeriveInput) -> Result { if f.embed() { Ok(quote!(<#field_ty as QueryableByName<__DB>>::build(row)?)) } else { + let st = sql_type(f, &model)?; let deserialize_ty = f.ty_for_deserialize(); let name = f.column_name()?; let name = LitStr::new(&name.to_string(), name.span()); Ok(quote!( { - let field = diesel::row::NamedRow::get(row, #name)?; + let field = diesel::row::NamedRow::get::<#st, #deserialize_ty>(row, #name)?; <#deserialize_ty as Into<#field_ty>>::into(field) } )) From ecf87ef7aabd1c54817c191ffd43876386731aa9 Mon Sep 17 00:00:00 2001 From: Gary Coady Date: Fri, 26 Jul 2024 11:56:20 +0200 Subject: [PATCH 2/4] Update compilation error output for failed test cases. These tests are still failing for the correctly reported reason. The shorter error messages seem to be because this change reduces the inference that's happening; this reduces some noise when the compilation fails for other reasons. --- ...s_table_name_or_sql_type_annotation.stderr | 43 ------------------- .../deprecated_sql_type.stderr | 21 --------- 2 files changed, 64 deletions(-) diff --git a/diesel_compile_tests/tests/fail/derive/queryable_by_name_requires_table_name_or_sql_type_annotation.stderr b/diesel_compile_tests/tests/fail/derive/queryable_by_name_requires_table_name_or_sql_type_annotation.stderr index 2dd72fafbb74..f46370568073 100644 --- a/diesel_compile_tests/tests/fail/derive/queryable_by_name_requires_table_name_or_sql_type_annotation.stderr +++ b/diesel_compile_tests/tests/fail/derive/queryable_by_name_requires_table_name_or_sql_type_annotation.stderr @@ -6,49 +6,6 @@ error: All fields of tuple structs must be annotated with `#[diesel(column_name) | = note: this error originates in the derive macro `QueryableByName` (in Nightly builds, run with -Z macro-backtrace for more info) -error[E0277]: cannot deserialize a value of the database type `_` as `i32` - --> tests/fail/derive/queryable_by_name_requires_table_name_or_sql_type_annotation.rs:4:10 - | -4 | #[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`: - > - > - > -note: required by a bound in `diesel::row::NamedRow::get` - --> $DIESEL/src/row.rs - | - | fn get(&self, column_name: &str) -> deserialize::Result - | --- required by a bound in this associated function - | where - | T: FromSql; - | ^^^^^^^^^^^^^^^ required by this bound in `NamedRow::get` - = note: this error originates in the derive macro `QueryableByName` (in Nightly builds, run with -Z macro-backtrace for more info) - -error[E0277]: cannot deserialize a value of the database type `_` as `*const str` - --> tests/fail/derive/queryable_by_name_requires_table_name_or_sql_type_annotation.rs:4:10 - | -4 | #[derive(QueryableByName)] - | ^^^^^^^^^^^^^^^ the trait `FromSql<_, __DB>` is not implemented for `*const str`, which is required by `std::string::String: FromSql<_, __DB>` - | - = note: double check your type mappings via the documentation of `_` - = help: the following other types implement trait `FromSql`: - <*const str as FromSql> - <*const str as FromSql> - <*const str as FromSql> - = note: required for `std::string::String` to implement `FromSql<_, __DB>` -note: required by a bound in `diesel::row::NamedRow::get` - --> $DIESEL/src/row.rs - | - | fn get(&self, column_name: &str) -> deserialize::Result - | --- required by a bound in this associated function - | where - | T: FromSql; - | ^^^^^^^^^^^^^^^ required by this bound in `NamedRow::get` - = note: this error originates in the derive macro `QueryableByName` (in Nightly builds, run with -Z macro-backtrace for more info) - error[E0433]: failed to resolve: use of undeclared crate or module `foos` --> tests/fail/derive/queryable_by_name_requires_table_name_or_sql_type_annotation.rs:5:8 | diff --git a/diesel_compile_tests/tests/fail/derive_deprecated/deprecated_sql_type.stderr b/diesel_compile_tests/tests/fail/derive_deprecated/deprecated_sql_type.stderr index 5b5592b5bf6e..94c66d44a278 100644 --- a/diesel_compile_tests/tests/fail/derive_deprecated/deprecated_sql_type.stderr +++ b/diesel_compile_tests/tests/fail/derive_deprecated/deprecated_sql_type.stderr @@ -59,24 +59,3 @@ error[E0412]: cannot find type `foo` in this scope | 27 | #[sql_type = "foo"] | ^^^^^ not found in this scope - -error[E0277]: cannot deserialize a value of the database 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`: - > - > - > -note: required by a bound in `diesel::row::NamedRow::get` - --> $DIESEL/src/row.rs - | - | fn get(&self, column_name: &str) -> deserialize::Result - | --- required by a bound in this associated function - | where - | T: FromSql; - | ^^^^^^^^^^^^^^^ required by this bound in `NamedRow::get` - = note: this error originates in the derive macro `QueryableByName` (in Nightly builds, run with -Z macro-backtrace for more info) From 99546fda79f6a74871a64a825c4ee57bd807bab4 Mon Sep 17 00:00:00 2001 From: Gary Coady Date: Fri, 26 Jul 2024 13:34:24 +0200 Subject: [PATCH 3/4] Add some tests that QueryableByName compiles with multiple SQL types for one output type. This tests that `QueryableByName` compiles, and outputs some data, for one example where the types are pulled from a `table!` macro; and another example where the values are specified as annotations in the struct. It's difficult to find a type that has the required properties across all three databases; these tests are compiled only for Postgres. To do something similar for the other databases would probably require compiling with one of the time libraries. --- diesel_derives/tests/queryable_by_name.rs | 58 +++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/diesel_derives/tests/queryable_by_name.rs b/diesel_derives/tests/queryable_by_name.rs index d865ebba8804..9f8faed201f0 100644 --- a/diesel_derives/tests/queryable_by_name.rs +++ b/diesel_derives/tests/queryable_by_name.rs @@ -12,6 +12,15 @@ table! { } } +#[cfg(feature = "postgres")] +table! { + multiple_sql_types_for_text { + id -> Integer, + name -> Text, + email -> Citext + } +} + #[test] fn named_struct_definition() { #[derive(Debug, Clone, Copy, PartialEq, Eq, QueryableByName)] @@ -86,6 +95,55 @@ fn struct_with_path_in_name() { ); } +#[cfg(feature = "postgres")] +#[test] +fn struct_with_multiple_sql_types_for_text() { + #[derive(Debug, PartialEq, QueryableByName)] + struct MultipleSqlTypesForText { + #[diesel(sql_type = diesel::sql_types::Text)] + name: String, + #[diesel(sql_type = diesel::sql_types::Citext)] + email: String, + } + + let conn = &mut connection(); + sql_query("CREATE EXTENSION IF NOT EXISTS citext") + .execute(conn) + .unwrap(); + let data = sql_query("SELECT 'name'::text AS name, 'email'::citext AS email").get_result(conn); + assert_eq!( + Ok(MultipleSqlTypesForText { + name: "name".into(), + email: "email".into() + }), + data + ); +} + +#[cfg(feature = "postgres")] +#[test] +fn struct_with_multiple_sql_types_for_text_from_table() { + #[derive(Debug, PartialEq, QueryableByName)] + #[diesel(table_name = multiple_sql_types_for_text)] + struct MultipleSqlTypesForText { + name: String, + email: String, + } + + let conn = &mut connection(); + sql_query("CREATE EXTENSION IF NOT EXISTS citext") + .execute(conn) + .unwrap(); + let data = sql_query("SELECT 'name'::text AS name, 'email'::citext AS email").get_result(conn); + assert_eq!( + Ok(MultipleSqlTypesForText { + name: "name".into(), + email: "email".into() + }), + data + ); +} + // FIXME: Test usage with renamed columns #[test] From 302fcd217def991f59665a8a8d3b59466a758401 Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Wed, 31 Jul 2024 21:16:33 +0200 Subject: [PATCH 4/4] Switch to an test case that does not require the citext extension --- diesel_derives/tests/queryable_by_name.rs | 39 ++++++++++------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/diesel_derives/tests/queryable_by_name.rs b/diesel_derives/tests/queryable_by_name.rs index 9f8faed201f0..4135935b4b2f 100644 --- a/diesel_derives/tests/queryable_by_name.rs +++ b/diesel_derives/tests/queryable_by_name.rs @@ -12,12 +12,12 @@ table! { } } -#[cfg(feature = "postgres")] +#[cfg(feature = "sqlite")] table! { multiple_sql_types_for_text { id -> Integer, - name -> Text, - email -> Citext + string -> Text, + time -> Timestamp, } } @@ -95,57 +95,50 @@ fn struct_with_path_in_name() { ); } -#[cfg(feature = "postgres")] +#[cfg(feature = "sqlite")] #[test] fn struct_with_multiple_sql_types_for_text() { #[derive(Debug, PartialEq, QueryableByName)] struct MultipleSqlTypesForText { #[diesel(sql_type = diesel::sql_types::Text)] - name: String, - #[diesel(sql_type = diesel::sql_types::Citext)] - email: String, + string: String, + #[diesel(sql_type = diesel::sql_types::Timestamp)] + time: String, } let conn = &mut connection(); - sql_query("CREATE EXTENSION IF NOT EXISTS citext") - .execute(conn) - .unwrap(); - let data = sql_query("SELECT 'name'::text AS name, 'email'::citext AS email").get_result(conn); + let data = sql_query("SELECT 'name' AS string, '2024-07-31T21:09:00' AS time").get_result(conn); assert_eq!( Ok(MultipleSqlTypesForText { - name: "name".into(), - email: "email".into() + string: "name".into(), + time: "2024-07-31T21:09:00".into() }), data ); } -#[cfg(feature = "postgres")] +#[cfg(feature = "sqlite")] #[test] fn struct_with_multiple_sql_types_for_text_from_table() { #[derive(Debug, PartialEq, QueryableByName)] #[diesel(table_name = multiple_sql_types_for_text)] struct MultipleSqlTypesForText { - name: String, - email: String, + string: String, + time: String, } let conn = &mut connection(); - sql_query("CREATE EXTENSION IF NOT EXISTS citext") - .execute(conn) - .unwrap(); - let data = sql_query("SELECT 'name'::text AS name, 'email'::citext AS email").get_result(conn); + let data = sql_query("SELECT 'name' AS string, '2024-07-31T21:09:00' AS time").get_result(conn); assert_eq!( Ok(MultipleSqlTypesForText { - name: "name".into(), - email: "email".into() + string: "name".into(), + time: "2024-07-31T21:09:00".into() }), data ); } // FIXME: Test usage with renamed columns - #[test] fn struct_with_no_table() { #[derive(Debug, Clone, Copy, PartialEq, Eq, QueryableByName)]