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

Operations could take connection with unknown size #523

Closed
wants to merge 3 commits into from

Conversation

billy1624
Copy link
Member

PR Info

Fixes

  • Methods that take in ConnectionTrait is implied to be with known sized, aka ConnectionTrait + Sized. But this assumption can be removed.

Breaking Changes

  • Methods that take in ConnectionTrait, now take in ConnectionTrait + ?Sized

@billy1624 billy1624 force-pushed the conn-relax-sized-bound branch from 2395108 to 2fada2d Compare February 13, 2022 11:28
@billy1624 billy1624 requested a review from tyt2y3 February 13, 2022 11:36
@billy1624 billy1624 marked this pull request as ready for review February 13, 2022 11:36
@tyt2y3
Copy link
Member

tyt2y3 commented Feb 20, 2022

I don't think it is an appropriate fix.
Should really try to fix it on SeaSchema side.
The root of the problem is the use of dyn vs use of enum for polymorphism, and SeaORM chose not to use dyn

@tyt2y3 tyt2y3 closed this Feb 20, 2022
@billy1624
Copy link
Member Author

billy1624 commented Mar 1, 2022

I think we should keep the SeaSchema migrator flexible and allow it to take anything that implemented ConnectionTrait. This way user can migrate with an ordinary connection or a transaction.

The SeaSchema Migration Manager: https://github.com/SeaQL/sea-schema/blob/db674ed28daff9bbfa3be51519f98ebaebf591b4/src/migration/manager.rs#L13

Also, in this PR, I relaxed the trait bound inside SeaORM, and I think the impact is minimum.

CC @tyt2y3

@Sytten
Copy link
Contributor

Sytten commented Mar 1, 2022

It would be nice to decouple the schema connection from sea-orm. Someone using sea-query might want to use the schema migration (we were considering it yesterday to replace diesel migrations) without pulling the orm.
Maybe extracting the base trait to a core crate would do it. Should be fairly easy to then write bridges to any executor.

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 2, 2022

I only think that SeaSchema should use generics and enums instead of dyn, unless there are clear reasons we can't

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 2, 2022

It definitely is something we will look into - on breaking down SeaORM into types, driver & frontend crates.

@tyt2y3 tyt2y3 deleted the conn-relax-sized-bound branch January 6, 2023 04:00
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.

How do you seed data in migrations using ActiveModels?
3 participants