SeaQL / sea-orm

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

Simpler and ergonomics commit and rollback. #2184

Closed sirlordt closed 2 months ago

sirlordt commented 2 months ago

When you use commit or rollback the methods are self consuming. This is very confusing for new rust users who do not understand that the transaction variable can no longer be used after the commit or rollback method is called. Especially if you come from environments like Java or C#, where the commit or the rollback don't have this behavior.

Additionally, these self-consuming methods are more difficult to use with Arc<Mutex> because it is necessary to extract the value of the Arc and the Mutex. To be able to call the Commit or the Rollback. That is much more confusing and unergonomic.

That is why alternative methods commit_by_ref and rollback_by_ref are proposed that allow commit and rollback to be used in a simpler way.

Thanks.

PR Info

New Features

Bug Fixes

Breaking Changes

Changes

jreppnow commented 2 months ago

(Opinion as a fellow library user) Using the transaction after either the rollback or the commit is always an error and explicitly allowing that seems like a really bad idea. Speaking as someone working with majority Rust beginners, enabling error-prone code is certainly not the way to teach Rust (and make them appreciate it). For my beginner colleagues' sake as well I would much rather this stays not an option.

Also, cannot speak for your code of course, but having the transaction behind a mutex smells like an anti-pattern. Even then if you must, turning it into an Mutex<Option<Transaction>> is really easy and illegal state stays unrepresentable as it should.

sirlordt commented 2 months ago

(Opinion as a fellow library user) Using the transaction after either the rollback or the commit is always an error and explicitly allowing that seems like a really bad idea. Speaking as someone working with majority Rust beginners, enabling error-prone code is certainly not the way to teach Rust (and make them appreciate it). For my beginner colleagues' sake as well I would much rather this stays not an option.

Also, cannot speak for your code of course, but having the transaction behind a mutex smells like an anti-pattern. Even then if you must, turning it into an Mutex<Option<Transaction>> is really easy and illegal state stays unrepresentable as it should.

Ok. But we need to recognize that rust is a very particular language with very particular features and very particular behaviors. I have been programming in C, C++, Java, C#, Golang and TypeScript/JavaScript professionally for over 25 years and I have never seen a language with so many "special features".

For a programmer with experience in all these languages the self consuming method is a great "whaaaaat?". It does not exist in another languages. And I really thank to god for github and open source and custom branches because many developer teams in my company see rust as super confusing and not because of rust's design. because of certain patterns in library api desing that are super strange. Superfluous lifetimes and others.

In my opinion the self consuming method is a very strange feature that does not allow rust lang intelligent pointers to do what they were designed to do. Instead they force a behavior that is imposed by the library designer. But they no add a real benefit or more security. Just a strange behavior, if you compare it with the rest of the mainstream languages.

In my company we use to create a rest api with end points /begin /commit /rollback /command, the transaction start with /begin and return transaction uuid, and later applied to call /commit or /rollback with that uuid. We already implemented in go lang. But we a some performance problems for very huge customer almost 3.7 millon transaction ever 24 hours. We decide implement that in rust using global Arc<Mutex < Transaction > > and work like a charm. I not pretty sure is anti pattern, but i pretty sure it work.

tyt2y3 commented 2 months ago

Not trying to start a debate, but I agree with @jreppnow . Reusing the object afterwards is always an error. And the drop function is magically defined as a 'self consuming' function.

tyt2y3 commented 2 months ago

I am really glad that you have found success with Rust / SeaORM. Just my two cents, I'd probably do message passing and have one single ServiceHandler in the app. There are many good mpsc channel options in Rust.