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

Fix related & linked with enum columns #376

Merged
merged 2 commits into from
Dec 10, 2021
Merged

Fix related & linked with enum columns #376

merged 2 commits into from
Dec 10, 2021

Conversation

billy1624
Copy link
Member

@billy1624 billy1624 self-assigned this Dec 10, 2021
@billy1624
Copy link
Member Author

Hey @MorganNesbitt, can you try this PR?

You can update your toml

sea-orm = { git = "https://github.com/SeaQL/sea-orm.git", branch = "issues/374", ... }

@billy1624 billy1624 marked this pull request as ready for review December 10, 2021 09:38
@tyt2y3
Copy link
Member

tyt2y3 commented Dec 10, 2021

Can briefly explain what's the root cause?

@MorganNesbitt
Copy link

hi @billy1624 that fixed the panic!

One thing I did notice now that I have a join working, is that it adds an order by id no matter what. which seemed odd, and when you add an order to the query it doesn't replace it just puts it right after.

A normal single query doesn't add an order, but the join does.

is this expected?

Below is what's being generated when I do a find_with_related but provide no order_by myself.
This is part of the debug that's being printed when I run the query

...
FROM
      "customer_employee_record"
      LEFT JOIN "historical_customer_employee_record" ON "customer_employee_record"."id" = "historical_customer_employee_record"."customerEmployeeRecordId"
    WHERE
      "customer_employee_record"."companyId" = $1
    ORDER BY
      "customer_employee_record"."id" ASC

},
_ => panic!("cannot apply alias for expr other than Column"),
Copy link
Member Author

Choose a reason for hiding this comment

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

It panic at this line. It didn't catch ColumnRef::AsEnum and apply alias for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -30,7 +30,7 @@ pub trait Linked {

select.query().join_as(
JoinType::InnerJoin,
unpack_table_ref(&rel.from_tbl),
rel.from_tbl,
Copy link
Member Author

@billy1624 billy1624 Dec 10, 2021

Choose a reason for hiding this comment

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

And I caught some other bug as well. This should pass the TableRef in directly, so that schema name will also be included if needed.

@@ -79,21 +79,28 @@ where

slf.query().join_as(
JoinType::LeftJoin,
unpack_table_ref(&rel.to_tbl),
rel.to_tbl,
Copy link
Member Author

@billy1624 billy1624 Dec 10, 2021

Choose a reason for hiding this comment

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

And I caught some other bug as well. This should pass the TableRef in directly, so that schema name will also be included if needed.

Same here

@billy1624
Copy link
Member Author

hi @billy1624 that fixed the panic!

One thing I did notice now that I have a join working, is that it adds an order by id no matter what. which seemed odd, and when you add an order to the query it doesn't replace it just puts it right after.

A normal single query doesn't add an order, but the join does.

is this expected?

Below is what's being generated when I do a find_with_related but provide no order_by myself. This is part of the debug that's being printed when I run the query

...
FROM
      "customer_employee_record"
      LEFT JOIN "historical_customer_employee_record" ON "customer_employee_record"."id" = "historical_customer_employee_record"."customerEmployeeRecordId"
    WHERE
      "customer_employee_record"."companyId" = $1
    ORDER BY
      "customer_employee_record"."id" ASC

That additional order by is intended. If you look into the implementation of SelectTwoMany, you will notice that it consolidate single dimension query rows into nested result, Vec<(E::Model, Vec<F::Model>)>. Hence, the relative order of E::Model result and its related F::Model result is important.

fn consolidate_query_result<L, R>(
rows: Vec<(L::Model, Option<R::Model>)>,
) -> Vec<(L::Model, Vec<R::Model>)>
where
L: EntityTrait,
R: EntityTrait,
{
let mut acc: Vec<(L::Model, Vec<R::Model>)> = Vec::new();
for (l, r) in rows {
if let Some((last_l, last_r)) = acc.last_mut() {
let mut same_l = true;
for pk_col in <L::PrimaryKey as Iterable>::iter() {
let col = pk_col.into_column();
let val = l.get(col);
let last_val = last_l.get(col);
if !val.eq(&last_val) {
same_l = false;
break;
}
}
if same_l {
if let Some(r) = r {
last_r.push(r);
continue;
}
}
}
if r.is_some() {
acc.push((l, vec![r.unwrap()]));
} else {
acc.push((l, vec![]));
}
}
acc
}

(My explanation might be confusing loll)

Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Great thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

find_with_related panic
3 participants