vertx-sql-client
vertx-sql-client copied to clipboard
eclipse-vertx
Propagate transactions in context
Read me
Read this first before creating an issue:
- do not use this issue tracker to ask questions, instead use one of these channels. Questions will likely be closed without notice.
- you shall create a feature request only when it is general purpose enough.
- make sure that the feature is not already
Describe the feature
While the simplified transaction API works great with singular operations or statements that can be grouped together, there doesn't seem to be any practical way of encapsulating multiple separately implemented DB statements within a transaction, such as in the case of controller layers.
See short discussion in https://github.com/quarkusio/quarkus/issues/26705 where Gavin suggested that propagating the transaction via the Vert.x context is the way to address this.
Use cases
- Transaction handling that is easier to manage
Contribution
@vietj @tsegismont
I'd be happy to try looking further into this myself if you think this could be worth exploring, but I am fairly unexperienced with the project so it'd probably require some time and assistance.
can you describe more precisely what you would expect ? i.e give a code example of how you think it would / should work. It would help a lot.
I am currently using Mutiny and not Vert.x directly so I might be a little out of touch here. But could something like the following work?
- Create a new method on interface Pool that:
- Looks for an active connection via the local context and returns it if found
- Otherwise create another, place that in the local context and return it
- Add the transaction accessor to Connection as requested in #646
Then third party code could easily retrieve and keep using both the active connection and the associated transaction without having to hold onto either of them.
Alternatively, instead of creating a new method, the existing getConnection-methods functionality could just place the connection they acquire in local context while also returning it, but I'm not sure if that could have unfortunate side effects?
@vietj you could look at how HR does it for inspiration: essentially, we store the connection in the vert.x local context from the outermost call to withTransaction(), and pull it from there in nested calls.
@technical-debt-collector so something like getOrCreateContext() but with a connection
@technical-debt-collector so something like
getOrCreateContext()but with a connection
But I would make it accept a function reference.
@gavinking, do you mean like with Pool.withTransaction() where the transaction is committed/rolled back and the connection is released after the function execution? I think I'd personally prefer having a variant without a function reference as well, where I can manage this myself.
do you mean like with Pool.withTransaction() where the transaction is committed/rolled back and the connection is released after the function execution?
Right. This was we can guarantee that things really do get cleaned up and rolled back.
I think I'd personally prefer having a variant without a function reference as well, where I can manage this myself.
If you really want to write custom code and manage things yourself, then that's already possible. I see this more as a feature for people who don't want to think about exception handling, etc.
I guess either way (functional reference or manual management) is already possible. What I'd expect to get out from this would be a way to handle both connections and transactions without having to pass them around and incorporate them across various layers of code just to achieve the necessary transaction management, ideally without having to touch the Vert.x context which isn't really conveniently available in Mutiny anyways.
With that being said I've thought more about it and fully agree with you, @gavinking. It's probably better to implement it in a safer way on behalf of the users as I don't think the manual way provides any significant advantages over the functional reference way.
But in that case, the SQL client is left with two almost identical ways of encapsulating logic within a transaction, one of which requires passing the connection object around. Should the old one be deprecated in this case? The only case I can imagine where the old one is still useful is if you for some reason would like to leave specific operations outside of the current transaction, but I guess this can be done in better ways if you'd actually want something done in separate transactions. And, does this SQL client even support multiple active transactions at a time?
@vietj, what are your thoughts on this? If you agree then I'd be happy to provide a PR for it.
I think that withTransaction already exist and we could overload the method with a boolean bind argument that would locally bind the transaction to the current vertx context (and fail if there is already a transaction) for the duration of the transaction and we would only provide a getter for this statically bound transaction ? i.e something like
pool.withTransaction(true, tx -> {
assertSame(tx, pool.currentTransaction());
return someFuture(tx);
});
I mean, that works, but I sorta hate Boolean parameters. In this case my preference would be to have a new method.
Not sure if I understood the solution described above.
In any case I experimented with the earlier solutions we discussed but it doesn't look like vertx-codegen will accept any other functional interface type than Function as a parameter to such a method. Using Void e.g. pool.withTransaction((Function<Void, Future<@Nullable T>> function) would work though.
I think that the case description is not precise enough @technical-debt-collector
Today, users who wish to do operations transactionally can choose between:
- pool.withTransaction() which provides a Connection object from which statements can be executed within a transaction. And at the end, that transaction automatically commits/rolls back along with the connection being closed
- pool.getConnection which gets the connection, and via that, start a transaction. In this case users must manually hold onto both of these objects if they are to handle everything that 1. does automatically.
The idea is to introduce an alternative to pool.withTransaction() where the connection is stored in the context instead of having to be passed around as a parameter through the Function argument. That way anything running in the same Vert.x process can access the same connection object via the pool instance and there would be no need to pass the object around as a parameter.
E.g. (in Mutiny)
public Uni<Void> transactionalMethod() {
return pool.withTransaction(() -> {
return Uni.combine().all().unis(
insertSomething(),
updateSomethingElse(),
evenMoreWork()
);
});
}
Somewhere else in my code I could have the other methods implemented, and I wouldn't need to supply the connection parameter to them at all, they could just look it up via the pool instance themselves and they would then be a part of the transaction started in transactionalMethod().
public Uni<Void> insertSomething() {
return pool
.getCurrentConnection()
.map(connection -> connection
.preparedQuery("insert into example (value) values ($1)")
.execute(Tuple.of("something")))
.flatMap(r -> Uni.createFrom().voidItem());
}
I see now, I think it requires specific code to duplicate a context for the duration of the transaction because the current context might not be a duplicate context and this it would not work as expected.
And perhaps that there could also be concurrent transaction within the same HTTP request and the duplicate context would not be able to store two transactions.
I think an interface like:
public interface TransactionLocal {
static TransactionLocal create(Pool pool) {
...
}
SqlConnection currentConnection();
<T> Future<@Nullable T> withTransaction(Function<SqlConnection, Future<@Nullable T>> fn);
}
would work, since you can create a static TransactionLocal object that is shared, and that does not prevent one to have multiple instances if that's a requirement.
Sounds pretty solid! Want me to give this PR an attempt? Edit: Actually, the interface should be for the SqlConnection and not the Transaction object, right? Because that's the object being passed around (which we'd like to stop doing) and used to actually encompass things within a transaction - not the actual transaction object itself. But in that case, would this even work? If the context is not an appropriate place to store it, I don't really see where we could hold onto a SqlConnectionLocal-instance.
One connection seems to only support one transaction at a time from the looks of it, but that seems fine to me at least for this issue.
Sure thing, contributions are welcome.
On Thu, Jul 28, 2022 at 11:55 AM Ola Sæter Heitmann < @.***> wrote:
Sounds pretty solid! Want me to give this PR an attempt?
— Reply to this email directly, view it on GitHub https://github.com/eclipse-vertx/vertx-sql-client/issues/1214#issuecomment-1197925093, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCRT3Y7FBRVFCAZOUXLVWJKIDANCNFSM53RIKKFQ . You are receiving this because you were mentioned.Message ID: @.***>
The API described above is not really what I was thinking of, and it doesn't seem to me to be as easy to use as what we have in Hibernate Reactive. In HR, propagation of the connection and transaction happens completely transparently between nested calls to withTransaction().
So instead of calling getCurrentConnection() you just call withTransaction() again, and get passed everything you need (here it would be the connection and transaction objects created by the surrounding call to withTransaction()).
locally bind the transaction to the current vertx context (and fail if there is already a transaction)
To be clear, I think this is where our ideas diverge. You're saying fail; I'm saying just return it.
So instead of calling
getCurrentConnection()you just callwithTransaction()again, and get passed everything you need (here it would be the connection and transaction objects created by the surrounding call towithTransaction()).
Having to wrap every request in its own withTransaction wherever it's impractical to just include it in another seems kind of annoying, though. Wouldn't it be better having to only deal with it once if possible?
Well, I mean, the previous example would look something approximately like:
public Uni<Void> transactionalMethod() {
return pool.withTransaction((tx, c) -> {
return Uni.combine().all().unis(
insertSomething(),
updateSomethingElse(),
evenMoreWork()
);
});
}
public Uni<Void> insertSomething() {
return pool.withTransaction((tx, c) ->
c.preparedQuery("insert into example (value) values ($1)")
.execute(Tuple.of("something"))))
.flatMap(r -> Uni.createFrom().voidItem());
}
Note that I didn't have to write the insertSomething() method to be aware of the fact that it was being called from within the scope of an existing transaction. The outer transaction just automatically propagated.
It might make sense that withTransaction works with this way as nested transaction don't exist. Perhaps we should make this configurable to have nesting reuse the current transaction or fail (it might be an error if that's used unwillingly).