Transactions support

[nappa85]

Coming from #118, I start a new issue con go on with the analisys:

What @tyt2y3 proposed:

I think transaction is highly driver specific. I don't think we can cleanly wrap and generalizes over different database+driver.
Perhaps we can start by exposing some of the underlying types and methods from SQLx.

https://docs.rs/sqlx/0.5.7/sqlx/trait.Connection.html#method.transaction

It can be a starting point, if we're able to maintain sea-orm typing.
What I mean is, sqlx Connection::transaction takes an fn(&mut Transaction), we would need to wrap it up so inside the function we can still use our DatabaseConnection type.

And it's kind of an overkill IMHO, Transaction is only a sugar wrapper over a normal connection, where a START TRANSACTION has been executed.

I would prefere an approach like this: https://docs.rs/mysql_async/0.28.0/mysql_async/struct.Conn.html#method.start_transaction
where Transaction is simply a tiny wrapper over Connection and can be used in any method that takes &Conn because it impl Deref<Conn>

About isolation levels, mysql and postgres are quite similars:
https://dev.mysql.com/doc/refman/8.0/en/set-transaction.html
https://www.postgresql.org/docs/9.5/sql-set-transaction.html

Sqlite has transactions too, it has a little different dialect, like BEGIN [TRANSACTION] instead of START TRANSACTION, and isolation levels have to be set using the PRAGMA keyword, like https://www.sqlite.org/pragma.html#pragma_read_uncommitted

[tyt2y3]

There is a reason that SQLx takes in a FnOnce for transaction. It is using a connection pool, this is to prevent that the user forget to COMMIT or ROLLBACK thereafter.

I think, probably, the SeaORM API can take a Vec<Statement> and execute them in a single transaction. This is rather simplistic, and does not cover all use cases.

All in all, I think transaction in an async context is a complex matters that requires more investigation.

[nappa85]

Transactions are often used to lock to a certain state (isolation) to be sure that there are not changes between select queries. How do you cover that case with a Vec<Statement>?

I can try to see if I can fit inside sqlx apis with a wrapper, if it covers all scenarios

[tyt2y3]

You are right.
I think it is still useful when there are multiple inserts and want to maintain 'domain consistency'.
But definitely, FnOnce is the most flexible way.

[nappa85]

I've analyzed further the situation.

I think we want to give users consistent APIs, therefore a function that accepts &DatabaseConnection should work both inside and outside transactions.
At least, this is what I would want, since I write reusable code and some methods may o may not be called inside an isolation transaction.
Example:
I'm developing a bank account application, I made two methods: one to query the base deposit balance, the other to query my investments balance. On single call I don't need a transaction, since the query itself has a form of isolation. If I want to know both balance at the same time, I need a transaction because I can't permit data modification between the two queries.

To do so I see 2 ways:

• put the transaction as a variant of DatabaseConnection enum
• create a Connection trait, impl it for DatabaseConnection and Transaction and let user rely in the trait with a generic

Both ways permits both transaction closure style (like sqlx::Connection::transaction) and free transaction style (like sqlx::Connection::begin).

Let's analyze the 2 ways:
DatabaseConnection::Transaction
This way sounds simplier, user's code remains the same.
Unfortunately Transaction has a lifetime, because it contains a reference to the parent connection, so we should break APIs and add a lifetime to DatabaseConnection.
I've tried it, I've end up in a lifetime hell, but maybe it was my 2 AM mistake...

Connection trait
I think this is the better solution at the end, because users only need to import an additional trait (putting it in prelude would skip importing for a lot of users) and who wants to use both standard connection and transaction in their methods has to rely on a generic instead of a concrete type.
Doesn't sound like a big deal to me.

Other problems I've found with DatabaseConnection::Transaction way:

• sqlx::Connection::transaction requires the FnMut to be Send, but sqlite doesn't seems to accomplish this trait, so as soon we enable sqlite the crate won't build,
• sqlx::Executor trait requires a mutable reference to sqlx::Connection, therefore we'll need a Mutex to wrap the Transaction. Async Mutex comes from tokio or async-std (I don't even know if actix has it, as far as I know it's based on tokio itself), and those crate aren't actually between the deps, so we'll need to add optional deps to tokio and async-std. This shouldn't be a bid deal, since they are alrady sqlx's deps.

[nappa85]

Just to inform you, I've found a lifetime problem with my transaction implementation.
When capturing references from outside you'll end up in an impossible lifetime.
I've added a test that replicates the situation and I'm trying to solve it.
I'll post a patch as soon as I find a solution

[nappa85]

My implementation allows you to return anything that impl Error, wrapper in an error Type that permits you to know if the error happened opening the transaction

[Stumblinbear]

My implementation allows you to return anything that impl Error, wrapper in an error Type that permits you to know if the error happened opening the transaction

Yeah, just noticed that, haha. I wasn't expecting it to be a generic argument (in the test it's set to DbErr, which I missed).

[Stumblinbear]

I managed to work around the lifetime issue by cloning the Arc just before starting the transaction, so thankfully that's a non-issue for me. I do have an additional question, though. Is it possible to accept both a normal connection and a transaction connection in a single function? I assume it would use ConnectionTrait somehow, but it doesn't seem to like that.

[nappa85]

Yes, use a generic over trait ConnectionTrait

[Stumblinbear]

Hm. Well, I guess my inexperience with Rust is the issue, here. I'll see if I can't figure out how to actually do that. Anyway, thanks for the tip. Seems like you thought of everything, already, haha

[Stumblinbear]

I've got a suggestion, if it's in scope for this change. Could there be support for running tests inside of a transaction that always rolls back once the function has completed? Perhaps a procedural macro? The current way to do this is to start a transaction in each test, and always return Rollback in an Err, but this is a bit awkward