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

Represent several relations between same types #104

Merged
merged 9 commits into from
Sep 3, 2021
Merged

Represent several relations between same types #104

merged 9 commits into from
Sep 3, 2021

Conversation

billy1624
Copy link
Member

Resolve #89

@billy1624 billy1624 self-assigned this Aug 23, 2021
@billy1624
Copy link
Member Author

Just a simple POC @tyt2y3

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 23, 2021

okay looks reasonable. But the intention of find_related is to select the ToEntity, and thus, it walks backwards from ToEntity back to FromEntity

@billy1624
Copy link
Member Author

okay looks reasonable. But the intention of find_related is to select the ToEntity, and thus, it walks backwards from ToEntity back to FromEntity

I walk (join) in the wrong direction here?

fn find_linked() -> Select<Self::ToEntity> {
let mut select = Select::new();
for rel in Self::link() {
select = select.join(JoinType::InnerJoin, rel);
}
select
}

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 24, 2021

I think we were using join_rev

fn find_related() -> Select<R> {
Select::<R>::new().join_join_rev(JoinType::InnerJoin, Self::to(), Self::via())
}

sea-orm/src/query/helper.rs

Lines 121 to 127 in 8cfa142

fn join_join_rev(mut self, join: JoinType, rel: RelationDef, via: Option<RelationDef>) -> Self {
self = self.join_rev(join, rel);
if let Some(via) = via {
self = self.join_rev(join, via)
}
self
}

@billy1624
Copy link
Member Author

I don't understand why we need join_rev here.

fn find_linked() -> Select<Self::ToEntity> {
let mut select = Select::new();
for rel in Self::link() {
select = select.join(JoinType::InnerJoin, rel);
}
select
}

pub fn find_also_linked<L, T>(self, _: L) -> SelectTwo<E, T>
where
L: Linked<FromEntity = E, ToEntity = T>,
T: EntityTrait,
{
let mut slf = self;
for rel in L::link() {
slf = slf.join(JoinType::LeftJoin, rel);
}
slf.select_also(T::default())
}

Check these two test cases and see if the resulting SQL statements are expected

sea-orm/src/query/join.rs

Lines 237 to 275 in 31941d3

#[test]
fn join_10() {
let cake_model = cake::Model {
id: 12,
name: "".to_owned(),
};
assert_eq!(
cake_model
.find_linked(cake::CakeToFilling)
.build(DbBackend::MySql)
.to_string(),
[
r#"SELECT `filling`.`id`, `filling`.`name`"#,
r#"FROM `filling`"#,
r#"INNER JOIN `cake_filling` ON `cake`.`id` = `cake_filling`.`cake_id`"#,
r#"INNER JOIN `filling` ON `cake_filling`.`filling_id` = `filling`.`id`"#,
]
.join(" ")
);
}
#[test]
fn join_11() {
assert_eq!(
cake::Entity::find()
.find_also_linked(cake::CakeToFilling)
.build(DbBackend::MySql)
.to_string(),
[
r#"SELECT `cake`.`id` AS `A_id`, `cake`.`name` AS `A_name`,"#,
r#"`filling`.`id` AS `B_id`, `filling`.`name` AS `B_name`"#,
r#"FROM `cake`"#,
r#"LEFT JOIN `cake_filling` ON `cake`.`id` = `cake_filling`.`cake_id`"#,
r#"LEFT JOIN `filling` ON `cake_filling`.`filling_id` = `filling`.`id`"#,
]
.join(" ")
);
}

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 26, 2021

sea-orm/src/query/join.rs

Lines 250 to 253 in 31941d3

r#"SELECT `filling`.`id`, `filling`.`name`"#,
r#"FROM `filling`"#,
r#"INNER JOIN `cake_filling` ON `cake`.`id` = `cake_filling`.`cake_id`"#,
r#"INNER JOIN `filling` ON `cake_filling`.`filling_id` = `filling`.`id`"#,

Well filling inner join filling, this is not right right?

I think ideally this simple case should matches the original Related implementation.

sea-orm/src/query/join.rs

Lines 171 to 184 in 8cfa142

fn join_7() {
use crate::{Related, Select};
let find_filling: Select<filling::Entity> = cake::Entity::find_related();
assert_eq!(
find_filling.build(DbBackend::MySql).to_string(),
[
"SELECT `filling`.`id`, `filling`.`name` FROM `filling`",
"INNER JOIN `cake_filling` ON `cake_filling`.`filling_id` = `filling`.`id`",
"INNER JOIN `cake` ON `cake`.`id` = `cake_filling`.`cake_id`",
]
.join(" ")
);
}

@billy1624
Copy link
Member Author

I see what you mean now. See 447947e for the fix.

@billy1624
Copy link
Member Author

billy1624 commented Aug 26, 2021

I also added test cases. Anything is still missing?

@nicoulaj
Copy link

This looks nice 👍

What about sea-orm-cli codegen though ? Maybe it could have a flag to generate Linked instead of Related for all associations as a first step ?

@billy1624
Copy link
Member Author

This looks nice 👍

What about sea-orm-cli codegen though ? Maybe it could have a flag to generate Linked instead of Related for all associations as a first step ?

But generating Linked for all associations will be annoying loll

i.e. each table will be Linked to all other tables in some way unless it has no association with any table

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 28, 2021

For the scope of this PR we are not changing the codegen.
But as a rough idea, I think these 'links' does not belong to the Entity file.
I am thinking to put them in a dedicated module because it works across many Entities.

@billy1624 billy1624 marked this pull request as ready for review August 28, 2021 12:20
@billy1624 billy1624 requested a review from tyt2y3 August 28, 2021 12:25
@tyt2y3 tyt2y3 force-pushed the master branch 5 times, most recently from 2d2fff7 to 6847dab Compare August 31, 2021 08:02
tests/relational_tests.rs Outdated Show resolved Hide resolved
tests/relational_tests.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Represent several relations between same types
3 participants