feat: add method find_both_related()

[ProbablyClem]

PR Info

• Closes find_also_related() with non null relation, or using inner join, should not return Option

New Features

• new selector SelectBoth in which the return type is Vec<(E::Model, F::Model)>
• a new method fn select_both<F>(mut self, _: F) -> SelectBoth<E, F> on Select
• new method fn find_both_related()<R>(self, r: R) -> SelectBoth<E, R>

[Pascualex]

I'm not familiar with the internals of SeaORM, but this looks like a very direct and clean conversion of the implementation for select_also.

Wouldn't it make sense to also add a find_both_related method? Thank you for your contribution.

[ProbablyClem]

I'm not familiar with the internals of SeaORM, but this looks like a very direct and clean conversion of the implementation for select_also.

Wouldn't it make sense to also add a find_both_related method? Thank you for your contribution.

Thanks @Pascualex
Yeah, I do like that name, as it matches the find_xxx_related as talked in the issue.
Do you think I should rename the inner_join_and_select or create a new one @tyt2y3 ?

[ProbablyClem]

I renamed it for now, as I think find_both_related is a better name.
I can still go back to inner_join_and_select if wanted just let me know

[tyt2y3]

Thank you for the contribution, find_both_related is a good name.

[Bolognafingers]

Shame this wasn't merged

[rakshith-ravi]

@tyt2y3 @ProbablyClem - how do I help push this PR forward? The general consensus among some people I spoke to seems to be that this implementation (mandatory joins and selects) is the last blocker to adopting SeaORM in quite a few projects. What can I do to help here?

[ProbablyClem]

Hey,
Thanks for your interest in this PR.
I did not receive any feedback from the maintainers so honestly I don't really know what we can do.
We probably need to rebase it but then it's on the maintainers hands to merge it anyway

[rakshith-ravi]

Alternative solution - Does it make sense to create an extension trait (and a separate crate) to provide this functionality? Is there anything about this PR that uses private items? Once this gets upstreamed, our external crate can be depreciated. Until then perhaps this temporary workaround can be made?

I imagine the maintainers are either busy or ping-bombed, so hopefully this can be a stop-gap solution for the interim

[Huliiiiii]

LGTM. The only thing left is to rebase onto master. If you prefer, you can replace generics with impl Trait to simplify the code.

[rakshith-ravi]

Seeding some thoughts into your head here:

• Does SelectBoth explain what it does?
• Is SelectBoth adequately distinguishable from SelectTwo for a new developer?
• Does it make sense to rename SelectBoth -> SelectTwo and SelectTwo -> SelectTwoOptional / SelectMaybeTwo?

FYI - These are all breaking changes, so if there's any contention about the naming of these types, making these changes before the 2.0 release freezes would be the best time.

[tyt2y3]

I think we can keep the name SelectTwo. the new struct however can be named SelectTwoRequired or something more descriptive

[rakshith-ravi]

Awesome! Thanks for the inputs!

@ProbablyClem would you happen to have the time to rebase this PR with the naming change? Happy to lend a hand if required

[ProbablyClem]

Honnestly I have very little time at the moment and I've not looked into this codebase for years so I would appreciate your help.
Thanks

[rakshith-ravi]

No worries! Thanks for the work you've put in so far. Really appreciate it.

To be honest, I've never "rebased someone else's PR" before. Do I get write access to your repo or do I fork it and put a new PR? My concern with the latter being whether you get attribution for your work

[ProbablyClem]

I think you can either fork my branch and open your own PR,
Or open a PR on my fork of the repo and I'll merge it, it'll update this PR.
Thanks

[rakshith-ravi]

I've opened #2997 that supersedes this PR. This can be closed

[rakshith-ravi]

@Huliiiiii if you can please approve workflow runs on my PR, I can make sure the tests pass

[Expurple]

Thanks. Approved the run

[rakshith-ravi]

Awesome! The build for #2997 passes. Can we have that merged please? This PR can be closed