SeaQL / sea-orm

🐚 An async & dynamic ORM for Rust
https://www.sea-ql.org/SeaORM/
Apache License 2.0
6.94k stars 483 forks source link

sea_orm::DatabaseConnection should implement `Clone` by default #517

Closed feelingnothing closed 2 years ago

feelingnothing commented 2 years ago

Because MockConnection is under an Arc - DatabaseConnection can implement Clone by default https://github.com/SeaQL/sea-orm/blob/f418c4e58069cb7ae42b79c721a44c8bb8915f1a/src/database/db_connection.rs#L18 https://github.com/SeaQL/sea-orm/blob/f418c4e58069cb7ae42b79c721a44c8bb8915f1a/src/database/db_connection.rs#L31

billy1624 commented 2 years ago

Hey @feelingnothing, good observations! We have discussed it previously, see

feelingnothing commented 2 years ago

Hi, looking into this comment I faced the same issue, having to dive into the source code to find the reason on my program not compiling is not a great experience per se, great testing tooling is great to have, until it messes with how you expect a library to work, considering all of the sqlx pool connections are Cloneable. Having a concurrent access to MockDatabase is not a issue at all, considering that no one will actually do tests that way, either way mock is extension and not something to use in production for, so i think it is justifiable to make a changes in favor of production ready code. Another way is to exclude the MockDatabase from DatabaseConnection and let it have a different struct that will not be Cloneable.

billy1624 commented 2 years ago

How about we exclude mock feature from default feature? This way normal user will have cloneable DatabaseConnection out of the box.

feelingnothing commented 2 years ago

Sound cool, considering that not much people would write tests with sea-orm along the lines, but the main problem is that if person would have to write a test with it, that person would lose Clone, also the fact that it is a breaking change. My point on either making separate MockDatabaseConnection specifically for testing purposes, or making Clone a default for DatabaseConnection.