From ea3fb4a7b5da35d16dc331daa039b2322d9b4157 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Sat, 8 May 2021 14:59:51 +0800 Subject: [PATCH 1/4] QueryableByName supoort non ident column name delete #[allow(dead_code)] --- diesel_derives/src/field.rs | 25 ++++++++++++++++++---- diesel_derives/src/queryable_by_name.rs | 26 ++++++++++++++++------- diesel_derives/tests/queryable_by_name.rs | 10 +++++++++ 3 files changed, 49 insertions(+), 12 deletions(-) diff --git a/diesel_derives/src/field.rs b/diesel_derives/src/field.rs index 0e1efe25e7bb..8efb2981817b 100644 --- a/diesel_derives/src/field.rs +++ b/diesel_derives/src/field.rs @@ -13,13 +13,12 @@ pub struct Field { pub span: Span, pub sql_type: Option, pub flags: MetaItem, - column_name_from_attribute: Option, + column_name_from_attribute: Option, } impl Field { pub fn from_struct_field(field: &syn::Field, index: usize) -> Self { - let column_name_from_attribute = - MetaItem::with_name(&field.attrs, "column_name").map(|m| m.expect_ident_value()); + let column_name_from_attribute = MetaItem::with_name(&field.attrs, "column_name"); let name = match field.ident.clone() { Some(mut x) => { // https://github.com/rust-lang/rust/issues/47983#issuecomment-362817105 @@ -51,7 +50,8 @@ impl Field { pub fn column_name(&self) -> syn::Ident { self.column_name_from_attribute - .clone() + .as_ref() + .map(|m| m.expect_ident_value()) .unwrap_or_else(|| match self.name { FieldName::Named(ref x) => x.clone(), _ => { @@ -65,6 +65,23 @@ impl Field { }) } + pub fn column_str_name(&self) -> String { + self.column_name_from_attribute + .as_ref() + .map(|m| { + m.str_value().unwrap_or_else(|e| { + e.emit(); + "unknown_column".to_string() + }) + }) + .unwrap_or_else(|| { + self.span + .error("All fields of tuple structs must be annotated with `#[column_name]`") + .emit(); + "unknown_column".to_string() + }) + } + pub fn has_flag(&self, flag: &str) -> bool { self.flags.has_flag(flag) } diff --git a/diesel_derives/src/queryable_by_name.rs b/diesel_derives/src/queryable_by_name.rs index bc954d5b7358..402e371177e5 100644 --- a/diesel_derives/src/queryable_by_name.rs +++ b/diesel_derives/src/queryable_by_name.rs @@ -22,15 +22,25 @@ pub fn derive(item: syn::DeriveInput) -> Result>::into(field) - } - )) + if model.has_table_name_attribute() { + let name = f.column_name(); + Ok(quote!( + { + let field = diesel::row::NamedRow::get(row, stringify!(#name))?; + <#deserialize_ty as Into<#field_ty>>::into(field) + } + )) + } else { + let name = f.column_str_name(); + Ok(quote!( + { + let field = diesel::row::NamedRow::get(row, stringify!(#name))?; + <#deserialize_ty as Into<#field_ty>>::into(field) + } + )) + } } }) .collect::, Diagnostic>>()?; @@ -93,12 +103,12 @@ fn get_ident(field: &Field) -> Ident { fn sql_type(field: &Field, model: &Model) -> syn::Type { let table_name = model.table_name(); - let column_name = field.column_name(); match field.sql_type { Some(ref st) => st.clone(), None => { if model.has_table_name_attribute() { + let column_name = field.column_name(); parse_quote!(diesel::dsl::SqlTypeOf<#table_name::#column_name>) } else { let field_name = match field.name { diff --git a/diesel_derives/tests/queryable_by_name.rs b/diesel_derives/tests/queryable_by_name.rs index a70595652f73..51515e212e7e 100644 --- a/diesel_derives/tests/queryable_by_name.rs +++ b/diesel_derives/tests/queryable_by_name.rs @@ -24,6 +24,16 @@ fn named_struct_definition() { assert_eq!(Ok(MyStruct { foo: 1, bar: 2 }), data); } +#[test] +fn non_ident_column_name() { + #[derive(QueryableByName)] + struct Out { + #[sql_type = "diesel::sql_types::Text"] + #[column_name = "QUERY PLAN "] + qp: String, + } +} + #[test] fn tuple_struct() { #[derive(Debug, Clone, Copy, PartialEq, Eq, QueryableByName)] From f3c43b2d152e08245fa39efe632a49992937f47b Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Sat, 8 May 2021 19:27:20 +0800 Subject: [PATCH 2/4] address comments Fix tests Fix tests --- diesel_cli/src/database.rs | 2 +- diesel_derives/src/as_changeset.rs | 6 ++--- diesel_derives/src/field.rs | 21 +++++++++------- diesel_derives/src/insertable.rs | 8 +++---- diesel_derives/src/model.rs | 2 +- diesel_derives/src/queryable_by_name.rs | 16 ++++++------- diesel_derives/src/selectable.rs | 4 ++-- diesel_derives/tests/queryable_by_name.rs | 29 +++++++++++++++-------- 8 files changed, 51 insertions(+), 37 deletions(-) diff --git a/diesel_cli/src/database.rs b/diesel_cli/src/database.rs index 356022090cc9..a8b71495bd98 100644 --- a/diesel_cli/src/database.rs +++ b/diesel_cli/src/database.rs @@ -365,7 +365,7 @@ fn change_database_of_url(database_url: &str, default_database: &str) -> (String let database = base.path_segments().unwrap().last().unwrap().to_owned(); let mut new_url = base.join(default_database).unwrap(); new_url.set_query(base.query()); - (database, new_url.into_string()) + (database, new_url.into()) } #[allow(clippy::needless_pass_by_value)] diff --git a/diesel_derives/src/as_changeset.rs b/diesel_derives/src/as_changeset.rs index ca5ee03f5b3e..4ab695e8db3f 100644 --- a/diesel_derives/src/as_changeset.rs +++ b/diesel_derives/src/as_changeset.rs @@ -28,7 +28,7 @@ pub fn derive(item: syn::DeriveInput) -> Result>(); let ref_changeset_ty = fields_for_update.iter().map(|field| { field_changeset_ty( @@ -92,7 +92,7 @@ fn field_changeset_ty( treat_none_as_null: bool, lifetime: Option, ) -> syn::Type { - let column_name = field.column_name(); + let column_name = field.column_name_ident(); if !treat_none_as_null && is_option_ty(&field.ty) { let field_ty = inner_of_option_ty(&field.ty); parse_quote!(std::option::Option>) @@ -109,7 +109,7 @@ fn field_changeset_expr( lifetime: Option, ) -> syn::Expr { let field_access = field.name.access(); - let column_name = field.column_name(); + let column_name = field.column_name_ident(); if !treat_none_as_null && is_option_ty(&field.ty) { if lifetime.is_some() { parse_quote!(self#field_access.as_ref().map(|x| #table_name::#column_name.eq(x))) diff --git a/diesel_derives/src/field.rs b/diesel_derives/src/field.rs index 8efb2981817b..9bfe979f1b6c 100644 --- a/diesel_derives/src/field.rs +++ b/diesel_derives/src/field.rs @@ -48,7 +48,7 @@ impl Field { } } - pub fn column_name(&self) -> syn::Ident { + pub fn column_name_ident(&self) -> syn::Ident { self.column_name_from_attribute .as_ref() .map(|m| m.expect_ident_value()) @@ -65,20 +65,25 @@ impl Field { }) } - pub fn column_str_name(&self) -> String { + pub fn column_name_str(&self) -> String { self.column_name_from_attribute .as_ref() .map(|m| { m.str_value().unwrap_or_else(|e| { e.emit(); - "unknown_column".to_string() + m.name().segments.first().unwrap().ident.to_string() }) }) - .unwrap_or_else(|| { - self.span - .error("All fields of tuple structs must be annotated with `#[column_name]`") - .emit(); - "unknown_column".to_string() + .unwrap_or_else(|| match self.name { + FieldName::Named(ref x) => x.to_string(), + _ => { + self.span + .error( + "All fields of tuple structs must be annotated with `#[column_name]`", + ) + .emit(); + "unknown_column".to_string() + } }) } diff --git a/diesel_derives/src/insertable.rs b/diesel_derives/src/insertable.rs index b20dc7f9df5f..22879de39f83 100644 --- a/diesel_derives/src/insertable.rs +++ b/diesel_derives/src/insertable.rs @@ -126,7 +126,7 @@ fn field_expr_embed(field: &Field, lifetime: Option) - fn field_ty_serialize_as(field: &Field, table_name: &syn::Path, ty: &syn::Type) -> syn::Type { let inner_ty = inner_of_option_ty(&ty); - let column_name = field.column_name(); + let column_name = field.column_name_ident(); parse_quote!( std::option::Option syn::Expr { let field_access = field.name.access(); - let column_name = field.column_name(); + let column_name = field.column_name_ident(); let column: syn::Expr = parse_quote!(#table_name::#column_name); if is_option_ty(&ty) { @@ -154,7 +154,7 @@ fn field_ty( lifetime: Option, ) -> syn::Type { let inner_ty = inner_of_option_ty(&field.ty); - let column_name = field.column_name(); + let column_name = field.column_name_ident(); parse_quote!( std::option::Option, ) -> syn::Expr { let field_access = field.name.access(); - let column_name = field.column_name(); + let column_name = field.column_name_ident(); let column: syn::Expr = parse_quote!(#table_name::#column_name); if is_option_ty(&field.ty) { if lifetime.is_some() { diff --git a/diesel_derives/src/model.rs b/diesel_derives/src/model.rs index 4f1763d2ef9d..2d083544ea0e 100644 --- a/diesel_derives/src/model.rs +++ b/diesel_derives/src/model.rs @@ -51,7 +51,7 @@ impl Model { pub fn find_column(&self, column_name: &syn::Ident) -> Result<&Field, Diagnostic> { self.fields() .iter() - .find(|f| &f.column_name() == column_name) + .find(|f| &f.column_name_ident() == column_name) .ok_or_else(|| { column_name .span() diff --git a/diesel_derives/src/queryable_by_name.rs b/diesel_derives/src/queryable_by_name.rs index 402e371177e5..ac0ac463c147 100644 --- a/diesel_derives/src/queryable_by_name.rs +++ b/diesel_derives/src/queryable_by_name.rs @@ -25,18 +25,18 @@ pub fn derive(item: syn::DeriveInput) -> Result>::into(field) - } + { + let field = diesel::row::NamedRow::get(row, stringify!(#name))?; + <#deserialize_ty as Into<#field_ty>>::into(field) + } )) } else { - let name = f.column_str_name(); + let name = f.column_name_str(); Ok(quote!( { - let field = diesel::row::NamedRow::get(row, stringify!(#name))?; + let field = diesel::row::NamedRow::get(row, #name)?; <#deserialize_ty as Into<#field_ty>>::into(field) } )) @@ -108,7 +108,7 @@ fn sql_type(field: &Field, model: &Model) -> syn::Type { Some(ref st) => st.clone(), None => { if model.has_table_name_attribute() { - let column_name = field.column_name(); + let column_name = field.column_name_ident(); parse_quote!(diesel::dsl::SqlTypeOf<#table_name::#column_name>) } else { let field_name = match field.name { diff --git a/diesel_derives/src/selectable.rs b/diesel_derives/src/selectable.rs index 7329b595e7ed..2a54be8f1763 100644 --- a/diesel_derives/src/selectable.rs +++ b/diesel_derives/src/selectable.rs @@ -50,7 +50,7 @@ fn field_column_ty(field: &Field, model: &Model) -> Result>::SelectExpression)) } else { let table_name = model.table_name(); - let column_name = field.column_name(); + let column_name = field.column_name_ident(); Ok(parse_quote!(#table_name::#column_name)) } } @@ -61,7 +61,7 @@ fn field_column_inst(field: &Field, model: &Model) -> Result>::construct_selection())) } else { let table_name = model.table_name(); - let column_name = field.column_name(); + let column_name = field.column_name_ident(); Ok(parse_quote!(#table_name::#column_name)) } } diff --git a/diesel_derives/tests/queryable_by_name.rs b/diesel_derives/tests/queryable_by_name.rs index 51515e212e7e..8be1da5ea13a 100644 --- a/diesel_derives/tests/queryable_by_name.rs +++ b/diesel_derives/tests/queryable_by_name.rs @@ -24,16 +24,6 @@ fn named_struct_definition() { assert_eq!(Ok(MyStruct { foo: 1, bar: 2 }), data); } -#[test] -fn non_ident_column_name() { - #[derive(QueryableByName)] - struct Out { - #[sql_type = "diesel::sql_types::Text"] - #[column_name = "QUERY PLAN "] - qp: String, - } -} - #[test] fn tuple_struct() { #[derive(Debug, Clone, Copy, PartialEq, Eq, QueryableByName)] @@ -76,6 +66,25 @@ fn struct_with_no_table() { assert_eq!(Ok(MyStructNamedSoYouCantInferIt { foo: 1, bar: 2 }), data); } +#[test] +fn struct_with_non_ident_column_name() { + #[derive(Debug, Clone, PartialEq, Eq, QueryableByName)] + struct QueryPlan { + #[sql_type = "diesel::sql_types::Text"] + #[column_name = "QUERY PLAN"] + qp: String, + } + + let conn = connection(); + let data = sql_query("SELECT 'some plan' AS \"QUERY PLAN\"").get_result(&conn); + assert_eq!( + Ok(QueryPlan { + qp: "some plan".to_string() + }), + data + ); +} + #[test] fn embedded_struct() { #[derive(Debug, Clone, Copy, PartialEq, Eq, QueryableByName)] From d210f45fca4016f711d10bba3940fe59773ee114 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Sat, 8 May 2021 23:02:14 +0800 Subject: [PATCH 3/4] Bump the url version --- diesel_cli/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/diesel_cli/Cargo.toml b/diesel_cli/Cargo.toml index 1544d43c76bc..2d4ee46b5659 100644 --- a/diesel_cli/Cargo.toml +++ b/diesel_cli/Cargo.toml @@ -44,7 +44,7 @@ path = "../diesel_migrations/" difference = "2.0" tempfile = "3.1.0" regex = "1.3.9" -url = { version = "2.1.0" } +url = { version = "2.2.2" } [features] default = ["postgres", "sqlite", "mysql"] From ea02c366298cd9c54dc30509b57f10f92e1ca8f3 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Mon, 10 May 2021 15:10:16 +0800 Subject: [PATCH 4/4] Address comment --- diesel_derives/src/queryable_by_name.rs | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/diesel_derives/src/queryable_by_name.rs b/diesel_derives/src/queryable_by_name.rs index ac0ac463c147..912857b8f7fe 100644 --- a/diesel_derives/src/queryable_by_name.rs +++ b/diesel_derives/src/queryable_by_name.rs @@ -24,23 +24,13 @@ pub fn derive(item: syn::DeriveInput) -> Result>::into(field) - } - )) - } else { - let name = f.column_name_str(); - Ok(quote!( - { - let field = diesel::row::NamedRow::get(row, #name)?; - <#deserialize_ty as Into<#field_ty>>::into(field) - } - )) - } + let name = f.column_name_str(); + Ok(quote!( + { + let field = diesel::row::NamedRow::get(row, #name)?; + <#deserialize_ty as Into<#field_ty>>::into(field) + } + )) } }) .collect::, Diagnostic>>()?;