-
Notifications
You must be signed in to change notification settings - Fork 3k
Support @Transactional for Hibernate Reactive #51063
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
base: main
Are you sure you want to change the base?
Conversation
Fixes: quarkusio#47698 Introduces a new extension enabling @transactional annotation support for Hibernate Reactive. This allows developers to use familiar transaction semantics with reactive database operations. Key implementation details: * New hibernate-reactive-transactions extension with dedicated CDI interceptors * TransactionalContextPool wraps the SQL client pool to lazily open transactions based on Vert.x context flags set by the @transactional interceptor * TransactionalInterceptorBase manages the complete transaction lifecycle (begin, commit, rollback) within the reactive Uni pipeline * TransactionalContextConnection wrapper prevents premature connection closure before transaction commit/rollback * OpenedSessionState refactored from Panache to centrally manage session lifecycle and caching (possibly to be reused in Panache as well) * Validation prevents mixing @transactional with @WithTransaction and @WithSessionOnDemand annotations to avoid conflicting transaction semantics * Disable JTA interceptor execution for methods returning Uni to prevent mixups Limitations: * Currently supports only Transactional.TxType.REQUIRED, other TxType values throw UnsupportedOperationException Dependencies updated: * hibernate-orm.version → 7.2.0.CR1 * hibernate-reactive.version → 3.2.0-SNAPSHOT
Renamed module name from hibernate-reactive-transactions => quarkus-reactive-transactions Removed panache dependency from transaction (was test) Adds quarkus-reactive-transactions dependency to hibernate-reactive-panache-common/runtime and quarkus-reactive-transactions-deployment dependency to hibernate-reactive-panache/deployment to support the relocated tests. Move @transactional mixing tests to hibernate-reactive-panache
import static io.quarkus.hibernate.reactive.transactions.runtime.TransactionalInterceptorBase.isUniReturnType; import static io.quarkus.hibernate.reactive.transactions.runtime.TransactionalInterceptorBase.proceedUni; Moved the WITH_TRANSACTION_METHOD_KEY constant inside Transaction module Moved the SESSION_ON_DEMAND_KEY constant inside Transaction module
|
/cc @gsmet (hibernate-orm) |
|
@yrodiere @FroMage this is the first Draft of the @transactional support on Reactive Here's what it's currently missing on my todo list
|
|
Also @DavideD take a look at this please |
yrodiere
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't go through the tests but I feel they're WIP anyway, so I felt it would be better to just send this.
| <artifactId>quarkus-reactive-transactions-parent</artifactId> | ||
| <version>999-SNAPSHOT</version> | ||
| </parent> | ||
| <artifactId>quarkus-reactive-transactions-deployment</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The directory name is hibernate-reactive-transactions and that's wrong, it should be reactive-transactions to match the artifact ID.
Though I'm wondering if it makes sense to have this artifact ID if you're putting Hibernate-specific things and tests in here...
I wonder how much of this module can be moved directly to the Hibernate Reactive extensions?
Couldn't this module be just about the interceptor + setting some "transaction context", with the rest of the implementation, and tests, living directly in the Hibernate Reactive module?
Perhaps this is something that will be easier to discuss live... Feel free to hit me with a meeting if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. This could be in the quarkus-hibernate-reactive module, no?
|
|
||
| @Override | ||
| protected Mutiny.Session newSessionMethod(Mutiny.SessionFactory sessionFactory) { | ||
| return sessionFactory.createSession(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires Davide to backport his patch to open connections lazily, correct?
| return delegate.getConnection() | ||
| .compose(connection -> { | ||
| LOG.tracef("New connection, about to start transaction: %s", connection); | ||
| return connection.begin().map(t -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, if we open transactions "under the hood", we will need Hibernate Reactive to correctly detect that and expose Mutiny.Session#currentTransaction accordingly. Do you have an issue for that in Reactive already?
| @Override | ||
| public SharedStatelessSessionBuilder statelessWithOptions() { | ||
| return delegate.get().statelessWithOptions(); | ||
| } | ||
|
|
||
| @Override | ||
| public SharedSessionBuilder sessionWithOptions() { | ||
| return delegate.get().sessionWithOptions(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the change 100% makes sense, I'm curious: how did you detect it was needed... ?
| private static final String ALLOW_ENHANCEMENT_AS_PROXY = "hibernate.bytecode.allow_enhancement_as_proxy"; | ||
|
|
||
| private static final CoreMessageLogger LOG = messageLogger(FastBootMetadataBuilder.class); | ||
| private static final CoreMessageLogger LOG = CoreMessageLogger.CORE_LOGGER; // TODO Luca review this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review this indeed :)
Was this needed simply for the upgrade to Hibernate Reactive 3.2 or 4.2? If so we need to move it to a separate commit, at least, and eventually to a separate PR (like #50519)
| Transaction transaction = connection.transaction(); | ||
| LOG.tracef("Transaction started: %s", transaction); | ||
| Vertx.currentContext().putLocal(CURRENT_TRANSACTION_KEY, transaction); | ||
| return new TransactionalContextConnection(connection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this wrapper, a call to close is a no-op.
Did you make sure the underlying connection gets closed after commit/rollback, somehow?
| } | ||
| } | ||
|
|
||
| private static final Logger LOG = Logger.getLogger(TransactionalContextPool.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a general warning, take it or ignore it: some people here feel strongly about code style, and I've never seen code in Quarkus where attributes/constants are in the middle of methods, instead of at the top of the file. You might want to fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, move it up :) I had the same observation.
| // Disable Interceptor on Reactive (uni) methods | ||
| // in The Reactive transaction module only REQUIRED is supported so far | ||
| if (ic.getMethod().getReturnType().equals(Uni.class)) { | ||
| log.debugf("method is annoted @Transactional but returns a Uni<?>, JTA transactions will be disabled"); | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling we may come to regret this method of detection...
For example:
- What if a method is meant to be actually "blocking transactional", but returns a Uni that when subscribed to, will execute code on the event loop?
- What if someone (wrongly) adds
@Transactionalto a method returning aMulti?
Etc.
Could it possibly make sense that, instead of looking at the return type, we check whether we're on an event loop (Context.isOnEventLoopThread()), and if so, we use reactive transactions, otherwise we use blocking transactions?
Then the Reactive interceptor can check the return type and fail if it's not Uni.
I think this would solve the two scenarios above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, but I think most of the logic in Quarkus equates Uni with reactive. So this seems like a fine first rule.
What if someone (wrongly) adds @transactional to a method returning a Multi
We could detect that and throw at build time.
| <dependency> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-reactive-transactions</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a transitive dependency of quarkus-hibernate-reactive, which this module already depends on?
| <artifactId>quarkus-integration-tests-parent</artifactId> | ||
| <version>999-SNAPSHOT</version> | ||
| </parent> | ||
| <artifactId>quarkus-reactive-transactions-integration-tests</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make quarkus-reactive-transactions a dependency of quarkus-hibernate-reactive, you can probably avoid this additional module and just test everything in the Hibernate Reactive integration tests.
Bonus points if this allows you to test transactions on multiple databases :)
| private static final String ALLOW_ENHANCEMENT_AS_PROXY = "hibernate.bytecode.allow_enhancement_as_proxy"; | ||
|
|
||
| private static final CoreMessageLogger LOG = messageLogger(FastBootMetadataBuilder.class); | ||
| private static final CoreMessageLogger LOG = CoreMessageLogger.CORE_LOGGER; // TODO Luca review this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These seem like an oversight
| public final class QuarkusProxyFactory implements ProxyFactory { | ||
|
|
||
| private static final CoreMessageLogger LOG = messageLogger(QuarkusProxyFactory.class); | ||
| private static final CoreMessageLogger LOG = CoreMessageLogger.CORE_LOGGER; // TODO Luca review this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
|
|
||
| @Override | ||
| public SharedStatelessSessionBuilder statelessWithOptions() { | ||
| try (SessionResult emr = acquireSession()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely sure why the method below called checkBlocking() and this one not.
|
|
||
| @BuildStep | ||
| void register( | ||
| BuildProducer<ReflectiveClassBuildItem> reflectiveClass // TOOD Luca hack to make this build step run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every build step needs an output otherwise it won't be invoked.
| @RunOnVertxContext | ||
| @Disabled("WIP") | ||
| public void avoidMixingTransactionalAndWithTransactionTest(UniAsserter asserter) { | ||
| avoidMixingBlockingEMWithReactiveSessionFactory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is not how you run HR tests with a vertx context :-/
| } else { | ||
| delegate.getConnection(result -> { | ||
| if (result.failed()) { | ||
| handler.handle(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should return here, no? Avoid going on further.
| return connection.begin().map(t -> { | ||
| Transaction transaction = connection.transaction(); | ||
| LOG.tracef("Transaction started: %s", transaction); | ||
| Vertx.currentContext().putLocal(CURRENT_TRANSACTION_KEY, transaction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the same code as the above other version of getConnection: the handler-based one does not store the tx in the context and does not wrap in a TransactionalContextConnection. Is this normal?
| } else if (context.getLocal(TRANSACTIONAL_METHOD_KEY) == null) { | ||
| throw new IllegalStateException("No current Mutiny.Session found" | ||
| + "\n\t- no reactive session was found in the Vert.x context and the context was not marked to open a new session lazily" | ||
| + "\n\t- a session is opened automatically for JAX-RS resource methods annotated with an HTTP method (@GET, @POST, etc.); inherited annotations are not taken into account" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true, though? This looks like it is only true for @Transactional methods. Unlike ORM (blocking) sessions which are automatically opened (without TX) regardless of whether there's any @Transactional annotation.
|
|
||
| @Override | ||
| protected boolean isSessionOpen(Mutiny.Session session) { | ||
| return session.isOpen(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like this method should be in shared session contract @DavideD .
| <bytebuddy.version>1.17.6</bytebuddy.version> <!-- version controlled by Hibernate ORM's needs --> | ||
| <hibernate-models.version>1.0.1</hibernate-models.version> <!-- version controlled by Hibernate ORM's needs --> | ||
| <hibernate-reactive.version>3.1.8.Final</hibernate-reactive.version> <!-- highly sensitive to Hibernate ORM upgrades --> | ||
| <hibernate-reactive.version>3.2.0-SNAPSHOT</hibernate-reactive.version> <!-- highly sensitive to Hibernate ORM upgrades --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's required from these two upgraded deps?
Introduces a new extension enabling @transactional annotation support for
Hibernate Reactive. This allows developers to use familiar transaction semantics
with reactive database operations.
Key implementation details:
quarkus-reactive-transactionsextension with dedicated CDI interceptorsbased on Vert.x context flags set by the @transactional interceptor
(begin, commit, rollback) within the reactive Uni pipeline
before transaction commit/rollback
lifecycle and caching (possibly to be reused in Panache as well)
@WithSessionOnDemand annotations to avoid conflicting transaction semantics
mixups
Limitations:
throw UnsupportedOperationException
Dependencies updated:
Fixes #47462
Fixes #47698
Support @transactional on Reactive methods as well
Injection of Mutiny.Session as injectable bean