-
-
Notifications
You must be signed in to change notification settings - Fork 517
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
feat: Allow nest
ing of structs deriving FromQueryResult
(and DerivePartialModel
)
#2179
base: master
Are you sure you want to change the base?
Conversation
The nest feature you implements seems great, should I keep the PR I opened opening? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see an example. Can you include some integration tests where we actually put this Nest
into select queries? This can server as both example and testcase. Ideally, we'd have a hand-unrolled implementation of the macro and being able to compare it against the derive macro generated version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really love to accept this PR and make a patch release. I wish we can have more throughout tests and examples.
For example, when I include this in changelog / documentation, how should I describe this feature? Actually, may be it'd be helpful to first show an example of the problem you are trying to solve?
Like, without this macro extension, I'd have to...
And now, with this feature, we can simply...
@tyt2y3 Hi, thanks for the comments! I'll get on to addressing them now. Two things:
|
Ah, also, I would appreciate if we could also get #2167 in the same release (however, that one is a lot more minor and has an easy workaround). |
I added a couple of test cases, but stumbled over a really annoying issue - when you nest a struct into another which both refer to columns of the same name, but from different tables, sqlx just overwrites the value of the one deserialized first with the second one.. The user can do a workaround (renaming the fields), but this is honestly really ugly and above all, very error-prone (as values actually get overwritten without warning in the "good" case). A better solution would be to do what other ORMs do and rename the columns in the query based on some random, unique string per FromQueryResult struct, but that would also involve changing the non-partial Model logic. Possible solutions:
|
Think I figured out a workaround, need a little bit more time though.. I will notify you when I have something. |
Okay, I think I found a solution which will change slightly how some queries are generated, but should not affect the API otherwise (possible via patch release I would think). Basically, in order to combat the issue of the same id showing up twice in the same query, I use I also added some more tests which hopefully show how useful this is. Two usecases come to mind (and are the reason we wanted this in the first place):
For a practical example, using the test cases added: Before, we were forced to write something along the lines of: #[derive(FromQueryResult, DerivePartialModel)]
#[sea_orm(entity = "cake::Entity")]
struct Cake {
id: i32,
name: String,
#[sea_orm(from_expr = "bakery::Column::Id")
bakery_id: Option<i32>,
#[sea_orm(from_expr = "bakery::Column::Name")
bakery_title: Option<String>,
} What's particularly annoying about this is that both #[derive(FromQueryResult, DerivePartialModel)]
#[sea_orm(entity = "cake::Entity")]
struct Cake {
id: i32,
name: String,
#[sea_orm(nested)]
bakery: Option<Bakery>,
}
#[derive(FromQueryResult, DerivePartialModel)]
#[sea_orm(entity = "bakery::Entity")]
struct Bakery {
id: i32,
#[sea_orm(from_col = "Name")]
title: String,
} Notice how the existence of the row in the In our code base, we generally associate the queries to obtain a struct with the struct (as in, as a function), and we could also use this to model varying degrees of details (and join partners) for larger queries. |
One caveat: I decided to also do an Regarding naming: I went with As I mentioned, there are some breaking changes that I would like to make to some traits. I can make a separate PR for them to be included in 1.0. |
da2d5eb
to
14258a0
Compare
nest
ing of structs deriving FromQueryResult (and DerivePartialModel)
nest
ing of structs deriving FromQueryResult (and DerivePartialModel)nest
ing of structs deriving FromQueryResult
(and DerivePartialModel
)
Thanks for the massive update! I am going through them |
2f1e8db
to
984827a
Compare
@tyt2y3 Just an FYI, this PR (specifically a 0.12.x backport) has been in production use with us for about a month now, without any problems and without breaking any existing queries. |
Cool stuff, thanks for working on this @jreppnow!
Example: erDiagram
Product ||--o{ Order : "id"
Customer ||--o{ Order : "customer_id"
Order ||--o{ Product : "product_id"
Order {
id UUID
product_id UUID
customer_id UUID
}
Address {
id UUID
}
Customer {
id UUID
address_id UUID
}
Product {
id UUID
}
Address ||--o{ Customer : "id"
CREATE TABLE "Address" (
id UUID NOT NULL;
);
CREATE TABLE "Customer" (
id UUID NOT NULL;
address_id UUID NOT NULL REFERENCES Address("id");
);
CREATE TABLE "Order" (
id UUID NOT NULL;
product_id UUID NOT NULL REFERENCES Product("id");
customer_id UUID NOT NULL REFERENCES Customer("id");
);
CREATE TABLE "Product" (
id UUID NOT NULL;
); struct Product; // [...]
struct Address; // [...]
#[derive(Debug, FromQueryResult, DerivePartialModel)]
#[sea_orm(entity = "Order")]
struct Order {
id: Uuid,
#[sea_orm(nested)]
product: Product,
#[sea_orm(nested)]
customer: Customer,
}
#[derive(Debug, FromQueryResult, DerivePartialModel)]
#[sea_orm(entity = "Customer")]
struct Customer {
id: Uuid,
#[sea_orm(nested)]
product: Address
} I'd like to get a list of |
@seijikun Thanks! Both should work perfectly well (we have multiple layers of recursion as well as multiple nested structs within various places in our code). Should you try this out and it unexpectedly does not work for some reason, I would consider this a bug and try to fix it if you let me know. As you mentioned, relation handling is rather loose, so you need to write the joins by hand and keep them in sync with the structs. |
@jreppnow I just tested your branch and I have to say ... it's fabulous! Seems to work flawlessly. Also with a lot of column name collisions between all the joined entities. Thanks again for working on this! |
Hi, this seems incredible useful!, Are there any plan to stabilize this into |
I would love to see this stabilized soon too! Coming from other ORM's this is one of the biggest things I miss. |
any progress? |
This PR is feature-complete since beginning of May. If I can get a review and some feedback (positive or negative), I will rebase it and get it ready-to-merge. @tyt2y3 I really don't like to put pressure on open source maintainers and have intentionally refrained from explicitly asking for another review/feedback, but this PR has been waiting for quite a while now, with a few people deeming it useful/desirable. It's still in use in production for us and has not caused any problems so far. Would you mind having another look (or, at least state that this PR does not match your vision and will not be merged)? Sorry! CC @billy1624 |
I have started to use this in my code base, merged the most recent version of For all of us that only use fn column_ref_into_alias_str(col_ref: &ColumnRef) -> Result<String, DbErr> {
const UNSUPPORTED_VARIANT_ERR_MSG: &str =
"Can not build alias unless column names and table name is known";
match col_ref {
ColumnRef::Column(_) => Err(DbErr::Custom(String::from(UNSUPPORTED_VARIANT_ERR_MSG))),
ColumnRef::TableColumn(t, c) => {
let mut temp = t.to_string();
temp.push('-');
temp.push_str(&c.to_string());
Ok(temp)
}
ColumnRef::SchemaTableColumn(s, t, c) => {
let mut temp = s.to_string();
temp.push('-');
temp.push_str(&t.to_string());
temp.push('-');
temp.push_str(&c.to_string());
Ok(temp)
}
ColumnRef::Asterisk => Err(DbErr::Custom(String::from(UNSUPPORTED_VARIANT_ERR_MSG))),
ColumnRef::TableAsterisk(_) => {
Err(DbErr::Custom(String::from(UNSUPPORTED_VARIANT_ERR_MSG)))
}
}
}
pub trait SelectStatementExtensions {
fn nested_alias<C, I>(&mut self, cols: I) -> &mut Self
where
C: IntoColumnRef,
I: IntoIterator<Item = C>;
}
impl SelectStatementExtensions for SelectStatement {
fn nested_alias<C, I>(&mut self, cols: I) -> &mut Self
where
C: IntoColumnRef,
I: IntoIterator<Item = C>,
{
self.exprs(
cols.into_iter()
.map(|x| {
let col_ref = x.into_column_ref();
SelectExpr {
alias: Some(
Alias::new(column_ref_into_alias_str(&col_ref).unwrap()).into_iden(),
),
expr: SimpleExpr::Column(col_ref),
window: None,
}
})
.collect::<Vec<SelectExpr>>(),
)
}
} Example: ...
Query::select()
.columns(ingredient_get_by_id::Column::iter().map(|x| (Entity, x)))
.nested_alias(only_name::Column::iter().map(|x| (serving_size_unit::Entity, x)))
.nested_alias(brand_owner::Column::iter().map(|x| (brand_owner::Entity, x)))
.from(Entity)
... |
PR Info
Hi, this is a separate implementation of #1716 , which seems to have somewhat stalled.
Normally I would not cut in from the side like this, but this feature would allow us to cut down on code duplication massively and avoid some quite annoying bugs from re-occurring in our code bases, so we would appreciate if could be merged in the near future.
New Features
nested
attribute in bothFromQueryResult
andDerivePartialModel
Breaking Changes
FromQueryResult
trait, but that could cause breakage in dependent crates..)Changes