Skip to content

feat: Allow nesting of structs deriving FromQueryResult (and DerivePartialModel) #2179

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.

Sign up for GitHub

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

Closed
wants to merge 22 commits into from
Closed

feat: Allow nesting of structs deriving FromQueryResult (and DerivePartialModel) #2179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
e6bcd1b
feat: implement optional from query result in derive
jreppnow Mar 29, 2024
f8cbf35
feat: allow for recursive usage of FromQueryResult derive
jreppnow Mar 29, 2024
1bb14b1
fix: fix tests to maintain API (even though probably should not..)
jreppnow Mar 29, 2024
aba5bec
fix: do not compile informative code snippet
jreppnow Mar 29, 2024
24306ca
feat: allow nested attribute in DerivePartialModel
jreppnow Mar 29, 2024
765cab1
feat: test cases and fixes for DerivePartialModel
jreppnow Mar 29, 2024
06bdfca
feat: enable DerivePartialModel for Option<T>
jreppnow Mar 29, 2024
002bd05
fix: fix typos
jreppnow Mar 29, 2024
009d21b
feat: update documentation
jreppnow Mar 29, 2024
f5e9360
fix: fix comment
jreppnow Mar 30, 2024
9248ab6
fix: remove unused import
jreppnow Mar 30, 2024
585c92f
feat: add test and some fixes regarding option
jreppnow Mar 30, 2024
fef8999
feat: properly differentiate same-name columns via explicit AS
jreppnow Mar 30, 2024
1b8c231
fix: cargo fmt
jreppnow Mar 30, 2024
27a5cf4
fix: more formatting
jreppnow Mar 30, 2024
e210c24
feat: more tests
jreppnow Mar 30, 2024
804b44e
fix: reorganize tests
jreppnow Mar 30, 2024
8064a74
fix: use - instead of _ to separate identifiers to avoid mixups
jreppnow Mar 30, 2024
e8dbaac
fix: add comment explaining error handling in FromQueryResult derive
jreppnow Mar 30, 2024
1b816b4
feat: add aspirational test
jreppnow Apr 7, 2024
8db6ba8
fix: remove eprintln
jreppnow Apr 11, 2024
34b76a6
chore: clippy
jreppnow Jan 29, 2025
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
119 changes: 96 additions & 23 deletions sea-orm-macros/src/derives/from_query_result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,81 @@ use syn::{
ext::IdentExt, punctuated::Punctuated, token::Comma, Data, DataStruct, Fields, Generics, Meta,
};

pub struct FromQueryResultItem {
pub skip: bool,
enum ItemType {
Normal,
Skipped,
Nested,
}

struct FromQueryResultItem {
pub typ: ItemType,
pub ident: Ident,
}
impl ToTokens for FromQueryResultItem {

/// Initially, we try to obtain the value for each field and check if it is an ordinary DB error
/// (which we return immediatly), or a null error.
///
/// ### Background
///
/// Null errors do not necessarily mean that the deserialization as a whole fails,
/// since structs embedding the current one might have wrapped the current one in an `Option`.
/// In this case, we do not want to swallow other errors, which are very likely to actually be
/// programming errors that should be noticed (and fixed).
struct TryFromQueryResultCheck<'a>(&'a FromQueryResultItem);

impl ToTokens for TryFromQueryResultCheck<'_> {
fn to_tokens(&self, tokens: &mut TokenStream) {
let FromQueryResultItem { ident, typ } = self.0;

match typ {
ItemType::Normal => {
let name = ident.unraw().to_string();
tokens.extend(quote! {
let #ident = match row.try_get_nullable(pre, #name) {
Err(v @ sea_orm::TryGetError::DbErr(_)) => {
return Err(v);
}
v => v,
};
});
}
ItemType::Skipped => {
tokens.extend(quote! {
let #ident = std::default::Default::default();
});
}
ItemType::Nested => {
let name = ident.unraw().to_string();
tokens.extend(quote! {
let #ident = match sea_orm::FromQueryResult::from_query_result_nullable(row, &format!("{pre}{}-", #name)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the package name to the prefix makes the whole thing unusable unless you are using it with a DerivePartialModel because the base model don't add the entity name in front of the column. An integration test would have caught that easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This is not the package name but a prefix computed based on the location (property name) of the nested struct in the parent struct.
  2. There are integration tests.
  3. nested only makes sense when used with DerivePartialModel.
  4. (I have specified this in the comments explicitly as well) This is required to avoid name collisions.

Copy link
Contributor Author

@jreppnow jreppnow Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is about using a struct with nested in it in into_model::<..>(), I guess? Yup, that is most likely going to break, but due to the name collision thing above there is not really anything I can do about it. The relationship between FromQueryResult and DerivePartialModel is weird and they should arguably be mutually exclusive, but that would involve copying the entire code from FromQueryResult into DerivePartialModel. That would also mean doing things like the skip I proposed elsewhere twice: #2167

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont remember what this was really but it didnt work when I wanted to do:

#[derive(FromQueryResult)]
pub struct PackageWithDownloads {
   #[sea_orm(nested)]
   pub package: plugin_packages::Model,
   pub downloads: i64,
}

I fixed it in my fork by removing the format,see caido@30cdea1#diff-50509553d7ac1acce8ee99595b98afc8307096fe6fa1a85056e00a29eba829e1R53

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this change does not work with the plan model structs (and I don't know an easy way to make it work). You need the prefixing because otherwise you get name collisions between common names like "id" and such, so the format cannot be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tyt2y3 thoughts? It would be important to make it work since not all nested need to derive the full model.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I might get it now. so there are two scenarios:

  1. only FromQueryResult. and we don't need prefix
  2. both FromQueryResult and DerivePartialModel. we need to put prefix

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the solution is for DerivePartialModel to also assumes the function of FromQueryResult.
So we will no longer recommend using both macros together.

Err(v @ sea_orm::TryGetError::DbErr(_)) => {
return Err(v);
}
v => v,
};
});
}
}
}
}

struct TryFromQueryResultAssignment<'a>(&'a FromQueryResultItem);

impl ToTokens for TryFromQueryResultAssignment<'_> {
fn to_tokens(&self, tokens: &mut TokenStream) {
let Self { ident, skip } = self;
if *skip {
tokens.extend(quote! {
#ident: std::default::Default::default(),
});
} else {
let name = ident.unraw().to_string();
tokens.extend(quote! {
#ident: row.try_get(pre, #name)?,
});
let FromQueryResultItem { ident, typ, .. } = self.0;

match typ {
ItemType::Normal | ItemType::Nested => {
tokens.extend(quote! {
#ident: #ident?,
});
}
ItemType::Skipped => {
tokens.extend(quote! {
#ident,
});
}
}
}
}
Expand All @@ -31,7 +90,7 @@ pub fn expand_derive_from_query_result(
data: Data,
generics: Generics,
) -> syn::Result<TokenStream> {
let fields = match data {
let parsed_fields = match data {
Data::Struct(DataStruct {
fields: Fields::Named(named),
..
Expand All @@ -42,40 +101,54 @@ pub fn expand_derive_from_query_result(
})
}
};
let mut field = Vec::with_capacity(fields.len());

for parsed_field in fields.into_iter() {
let mut skip = false;
let mut fields = Vec::with_capacity(parsed_fields.len());
for parsed_field in parsed_fields.into_iter() {
let mut typ = ItemType::Normal;
for attr in parsed_field.attrs.iter() {
if !attr.path().is_ident("sea_orm") {
continue;
}
if let Ok(list) = attr.parse_args_with(Punctuated::<Meta, Comma>::parse_terminated) {
for meta in list.iter() {
skip = meta.exists("skip");
if meta.exists("skip") {
typ = ItemType::Skipped;
} else if meta.exists("nested") {
typ = ItemType::Nested;
}
}
}
}
let ident = format_ident!("{}", parsed_field.ident.unwrap().to_string());
field.push(FromQueryResultItem { skip, ident });
fields.push(FromQueryResultItem { typ, ident });
}
let (impl_generics, ty_generics, where_clause) = generics.split_for_impl();

let ident_try_init: Vec<_> = fields.iter().map(TryFromQueryResultCheck).collect();
let ident_try_assign: Vec<_> = fields.iter().map(TryFromQueryResultAssignment).collect();

Ok(quote!(
#[automatically_derived]
impl #impl_generics sea_orm::FromQueryResult for #ident #ty_generics #where_clause {
fn from_query_result(row: &sea_orm::QueryResult, pre: &str) -> std::result::Result<Self, sea_orm::DbErr> {
fn from_query_result(row: &sea_orm::QueryResult, pre: &str) -> Result<Self, sea_orm::DbErr> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use std::result::Result

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid criticism. The correct way is to use ::std::.. or ::sea_orm::.. everywhere, so I will adjust it to that.

Ok(Self::from_query_result_nullable(row, pre)?)
}

fn from_query_result_nullable(row: &sea_orm::QueryResult, pre: &str) -> Result<Self, sea_orm::TryGetError> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, use std::result::Result

#(#ident_try_init)*

Ok(Self {
#(#field)*
#(#ident_try_assign)*
})
}
}
))
}
mod util {

pub(super) mod util {
use syn::Meta;

pub(super) trait GetMeta {
pub trait GetMeta {
fn exists(&self, k: &str) -> bool;
}

Expand Down
100 changes: 75 additions & 25 deletions sea-orm-macros/src/derives/partial_model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,25 @@ use proc_macro2::TokenStream;
use quote::format_ident;
use quote::quote;
use quote::quote_spanned;
use syn::ext::IdentExt;
use syn::punctuated::Punctuated;
use syn::spanned::Spanned;
use syn::token::Comma;
use syn::Expr;

use syn::Meta;
use syn::Type;

use super::from_query_result::util::GetMeta;

use self::util::GetAsKVMeta;

#[derive(Debug)]
enum Error {
InputNotStruct,
EntityNotSpecific,
EntityNotSpecified,
NotSupportGeneric(Span),
BothFromColAndFromExpr(Span),
OverlappingAttributes(Span),
Syn(syn::Error),
}
#[derive(Debug, PartialEq, Eq)]
Expand All @@ -29,6 +33,8 @@ enum ColumnAs {
ColAlias { col: syn::Ident, field: String },
/// from an expr
Expr { expr: syn::Expr, field_name: String },
/// nesting another struct
Nested { typ: Type, field_name: String },
}

struct DerivePartialModel {
Expand Down Expand Up @@ -78,6 +84,7 @@ impl DerivePartialModel {

let mut from_col = None;
let mut from_expr = None;
let mut nested = false;

for attr in field.attrs.iter() {
if !attr.path().is_ident("sea_orm") {
Expand All @@ -94,35 +101,37 @@ impl DerivePartialModel {
.get_as_kv("from_expr")
.map(|s| syn::parse_str::<Expr>(&s).map_err(Error::Syn))
.transpose()?;
nested = meta.exists("nested");
}
}
}

let field_name = field.ident.unwrap();

let col_as = match (from_col, from_expr) {
(None, None) => {
let col_as = match (from_col, from_expr, nested) {
(Some(col), None, false) => {
if entity.is_none() {
return Err(Error::EntityNotSpecific);
return Err(Error::EntityNotSpecified);
}
ColumnAs::Col(format_ident!(
"{}",
field_name.to_string().to_upper_camel_case()
))

let field = field_name.to_string();
ColumnAs::ColAlias { col, field }
}
(None, Some(expr)) => ColumnAs::Expr {
(None, Some(expr), false) => ColumnAs::Expr {
expr,
field_name: field_name.to_string(),
},
(Some(col), None) => {
(None, None, true) => ColumnAs::Nested {
typ: field.ty,
field_name: field_name.unraw().to_string(),
},
(None, None, false) => {
if entity.is_none() {
return Err(Error::EntityNotSpecific);
return Err(Error::EntityNotSpecified);
}

let field = field_name.to_string();
ColumnAs::ColAlias { col, field }
ColumnAs::Col(field_name)
}
(Some(_), Some(_)) => return Err(Error::BothFromColAndFromExpr(field_span)),
(_, _, _) => return Err(Error::OverlappingAttributes(field_span)),
};
column_as_list.push(col_as);
}
Expand All @@ -148,23 +157,64 @@ impl DerivePartialModel {
let select_col_code_gen = fields.iter().map(|col_as| match col_as {
ColumnAs::Col(ident) => {
let entity = entity.as_ref().unwrap();
let col_value = quote!( <#entity as sea_orm::EntityTrait>::Column:: #ident);
quote!(let #select_ident = sea_orm::SelectColumns::select_column(#select_ident, #col_value);)
let uppercase_ident = format_ident!(
"{}",
ident.to_string().to_upper_camel_case()
);
let col_value = quote!( <#entity as sea_orm::EntityTrait>::Column:: #uppercase_ident);
let ident_stringified = ident.unraw().to_string();
quote!(let #select_ident =
if let Some(prefix) = pre {
let ident = format!("{prefix}{}", #ident_stringified);
sea_orm::SelectColumns::select_column_as(#select_ident, #col_value, ident)
} else {
sea_orm::SelectColumns::select_column_as(#select_ident, #col_value, #ident_stringified)
};
)
},
ColumnAs::ColAlias { col, field } => {
let entity = entity.as_ref().unwrap();
let col_value = quote!( <#entity as sea_orm::EntityTrait>::Column:: #col);
quote!(let #select_ident = sea_orm::SelectColumns::select_column_as(#select_ident, #col_value, #field);)
quote!(let #select_ident =
if let Some(prefix) = pre {
let ident = format!("{prefix}{}", #field);
sea_orm::SelectColumns::select_column_as(#select_ident, #col_value, ident)
} else {
sea_orm::SelectColumns::select_column_as(#select_ident, #col_value, #field)
};
)
},
ColumnAs::Expr { expr, field_name } => {
quote!(let #select_ident = sea_orm::SelectColumns::select_column_as(#select_ident, #expr, #field_name);)
quote!(let #select_ident =
if let Some(prefix) = pre {
let ident = format!("{prefix}{}", #field_name);
sea_orm::SelectColumns::select_column_as(#select_ident, #expr, ident)
} else {
sea_orm::SelectColumns::select_column_as(#select_ident, #expr, #field_name)
};
)
},
ColumnAs::Nested { typ, field_name } => {
quote!(let #select_ident =
<#typ as sea_orm::PartialModelTrait>::select_cols_nested(#select_ident,
Some(&if let Some(prefix) = pre {
format!("{prefix}{}-", #field_name) }
else {
format!("{}-", #field_name)
}
));
)
},
});

quote! {
#[automatically_derived]
impl sea_orm::PartialModelTrait for #ident{
fn select_cols<S: sea_orm::SelectColumns>(#select_ident: S) -> S{
fn select_cols<S: sea_orm::SelectColumns>(#select_ident: S) -> S {
Self::select_cols_nested(#select_ident, None)
}

fn select_cols_nested<S: sea_orm::SelectColumns>(#select_ident: S, pre: Option<&str>) -> S {
#(#select_col_code_gen)*
#select_ident
}
Expand All @@ -181,10 +231,10 @@ pub fn expand_derive_partial_model(input: syn::DeriveInput) -> syn::Result<Token
Err(Error::NotSupportGeneric(span)) => Ok(quote_spanned! {
span => compile_error!("you can only derive `DerivePartialModel` on named struct");
}),
Err(Error::BothFromColAndFromExpr(span)) => Ok(quote_spanned! {
span => compile_error!("you can only use one of `from_col` or `from_expr`");
Err(Error::OverlappingAttributes(span)) => Ok(quote_spanned! {
span => compile_error!("you can only use one of `from_col`, `from_expr`, `nested`");
}),
Err(Error::EntityNotSpecific) => Ok(quote_spanned! {
Err(Error::EntityNotSpecified) => Ok(quote_spanned! {
ident_span => compile_error!("you need specific which entity you are using")
}),
Err(Error::InputNotStruct) => Ok(quote_spanned! {
Expand Down Expand Up @@ -258,7 +308,7 @@ struct PartialModel{
assert_eq!(middle.fields.len(), 3);
assert_eq!(
middle.fields[0],
ColumnAs::Col(format_ident!("DefaultField"))
ColumnAs::Col(format_ident!("default_field"))
);
assert_eq!(
middle.fields[1],
Expand Down
25 changes: 23 additions & 2 deletions sea-orm-macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ pub fn derive_from_json_query_result(input: TokenStream) -> TokenStream {
/// }
/// ```
///
/// If all fields in the partial model is `from_expr`, the `entity` can be ignore.
/// If all fields in the partial model is `from_expr`, the specifying the `entity` can be skipped.
/// ```
/// use sea_orm::{entity::prelude::*, sea_query::Expr, DerivePartialModel, FromQueryResult};
///
Expand All @@ -769,7 +769,28 @@ pub fn derive_from_json_query_result(input: TokenStream) -> TokenStream {
/// }
/// ```
///
/// A field cannot have attributes `from_col` and `from_expr` at the same time.
/// It is possible to nest structs deriving `FromQueryResult` and `DerivePartialModel`, including
/// optionally, which is useful for specifying columns from tables added via left joins, as well as
/// when building up complicated queries programmatically.
/// ```
/// use sea_orm::{entity::prelude::*, sea_query::Expr, DerivePartialModel, FromQueryResult};
///
/// #[derive(Debug, FromQueryResult, DerivePartialModel)]
/// struct Inner {
/// #[sea_orm(from_expr = "Expr::val(1).add(1)")]
/// sum: i32,
/// }
///
/// #[derive(Debug, FromQueryResult, DerivePartialModel)]
/// struct Outer {
/// #[sea_orm(nested)]
/// inner: Inner,
/// #[sea_orm(nested)]
/// inner_opt: Option<Inner>,
/// }
/// ```
///
/// A field cannot have attributes `from_col`, `from_expr` or `nested` at the same time.
/// Or, it will result in a compile error.
///
/// ```compile_fail
Expand Down
Loading
Loading