Support @Transactional for Hibernate Reactive #51063
+2,331
−77
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,6 @@ | |
| import static org.hibernate.cfg.AvailableSettings.URL; | ||
| import static org.hibernate.cfg.AvailableSettings.USER; | ||
| import static org.hibernate.cfg.AvailableSettings.XML_MAPPING_ENABLED; | ||
|
|
||
| import java.lang.reflect.InvocationTargetException; | ||
| import java.util.ArrayList; | ||
|
|
@@ -98,7 +97,7 @@ public class FastBootMetadataBuilder { | |
| @Deprecated | ||
| private static final String ALLOW_ENHANCEMENT_AS_PROXY = "hibernate.bytecode.allow_enhancement_as_proxy"; | ||
|
|
||
| private static final CoreMessageLogger LOG = CoreMessageLogger.CORE_LOGGER; // TODO Luca review this | ||
|
Member
There was a problem hiding this comment. 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) |
||
|
|
||
| private final PersistenceUnitDescriptor persistenceUnit; | ||
| private final BuildTimeSettings buildTimeSettings; | ||
|
|
@@ -268,7 +267,8 @@ private MergedSettings mergeSettings(QuarkusPersistenceUnitDefinition puDefiniti | |
|
|
||
| if (readBooleanConfigurationValue(cfg, AvailableSettings.FLUSH_BEFORE_COMPLETION)) { | ||
| cfg.put(AvailableSettings.FLUSH_BEFORE_COMPLETION, "false"); | ||
| // TODO Luca review this | ||
| // LOG.definingFlushBeforeCompletionIgnoredInHem(AvailableSettings.FLUSH_BEFORE_COMPLETION); | ||
|
Member
There was a problem hiding this comment. Probably another "Hibernate Reactive upgrade" change that needs to be isolated. |
||
| } | ||
|
|
||
| // Quarkus specific | ||
|
|
@@ -607,7 +607,8 @@ private static void applyTransactionProperties(PersistenceUnitDescriptor persist | |
| } | ||
| boolean hasTransactionStrategy = configurationValues.containsKey(TRANSACTION_COORDINATOR_STRATEGY); | ||
| if (hasTransactionStrategy) { | ||
| // TODO Luca review this | ||
| // LOG.overridingTransactionStrategyDangerous(TRANSACTION_COORDINATOR_STRATEGY); | ||
|
Comment on lines
-610
to
+611
Member
There was a problem hiding this comment. Probably another "Hibernate Reactive upgrade" change that needs to be isolated. |
||
| } else { | ||
| if (transactionType == PersistenceUnitTransactionType.JTA) { | ||
| configurationValues.put(TRANSACTION_COORDINATOR_STRATEGY, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
| <modelVersion>4.0.0</modelVersion> | ||
|
|
||
| <parent> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-reactive-transactions-parent</artifactId> | ||
| <version>999-SNAPSHOT</version> | ||
| </parent> | ||
| <artifactId>quarkus-reactive-transactions-deployment</artifactId> | ||
|
Member
There was a problem hiding this comment. The directory name is 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.
Member
There was a problem hiding this comment. Good question. This could be in the |
||
| <name>Quarkus - Hibernate Reactive Transactions - Deployment</name> | ||
|
|
||
| <dependencies> | ||
| <dependency> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-arc-deployment</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-reactive-transactions</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-junit5-internal</artifactId> | ||
| <scope>test</scope> | ||
|
|
||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-hibernate-reactive-deployment</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.hibernate.reactive</groupId> | ||
| <artifactId>hibernate-reactive-core</artifactId> | ||
| <exclusions> | ||
| <!-- Make sure Quarkus can manage Mutiny via quarkus-mutiny --> | ||
| <exclusion> | ||
| <groupId>io.smallrye.reactive</groupId> | ||
| <artifactId>mutiny</artifactId> | ||
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
|
|
||
| <!-- TEST --> | ||
| <dependency> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-test-vertx</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.assertj</groupId> | ||
| <artifactId>assertj-core</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-reactive-pg-client-deployment</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-narayana-jta</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> | ||
|
Comment on lines
+60
to
+64
Member
There was a problem hiding this comment. Could this be a forced dependency, added only for the specific test where Narayana is relevant? I think you used some "forced depdendency" thing when testing combinations of ORM + Reactive, and this seems rather similar. I'm worried the rest of the tests still rely on this extension being a dependency... |
||
| </dependencies> | ||
|
|
||
| <build> | ||
| <plugins> | ||
| <plugin> | ||
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <executions> | ||
| <execution> | ||
| <id>default-compile</id> | ||
| <configuration> | ||
| <annotationProcessorPaths> | ||
| <path> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-extension-processor</artifactId> | ||
| <version>${quarkus.version}</version> | ||
| </path> | ||
| </annotationProcessorPaths> | ||
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> | ||
| </plugins> | ||
| </build> | ||
| </project> | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| package io.quarkus.hibernate.reactive.transactions.deployment; | ||
|
|
||
| import jakarta.inject.Inject; | ||
| import jakarta.transaction.Transactional; | ||
|
|
||
| import org.jboss.jandex.AnnotationInstance; | ||
| import org.jboss.jandex.AnnotationTarget; | ||
| import org.jboss.jandex.DotName; | ||
| import org.jboss.jandex.IndexView; | ||
|
|
||
| import io.quarkus.deployment.annotations.BuildProducer; | ||
| import io.quarkus.deployment.annotations.BuildStep; | ||
| import io.quarkus.deployment.builditem.CombinedIndexBuildItem; | ||
| import io.quarkus.deployment.builditem.FeatureBuildItem; | ||
| import io.quarkus.deployment.builditem.nativeimage.ReflectiveClassBuildItem; | ||
| import io.quarkus.runtime.configuration.ConfigurationException; | ||
|
|
||
| class HibernateReactiveTransactionsProcessor { | ||
|
|
||
| private static final String FEATURE = "quarkus-reactive-transactions"; | ||
|
|
||
| private static final DotName TRANSACTIONAL = DotName.createSimple(Transactional.class.getName()); | ||
|
|
||
| private static final String WITH_SESSION_ON_DEMAND = "io.quarkus.hibernate.reactive.panache.common.WithSessionOnDemand"; | ||
| private static final DotName WITH_SESSION = DotName | ||
| .createSimple("io.quarkus.hibernate.reactive.panache.common.WithSession"); | ||
| private static final DotName WITH_TRANSACTION = DotName | ||
| .createSimple("io.quarkus.hibernate.reactive.panache.common.WithTransaction"); | ||
|
|
||
| @BuildStep | ||
| FeatureBuildItem feature() { | ||
| return new FeatureBuildItem(FEATURE); | ||
| } | ||
|
|
||
| @Inject | ||
| CombinedIndexBuildItem combinedIndexBuildItem; | ||
|
|
||
| @BuildStep | ||
| void register( | ||
| BuildProducer<ReflectiveClassBuildItem> reflectiveClass // TOOD Luca hack to make this build step run | ||
|
Member
There was a problem hiding this comment. I think you're after the Though as explained below, perhaps this build step should simply go away.
Member
There was a problem hiding this comment. Every build step needs an output otherwise it won't be invoked. |
||
| ) { | ||
|
|
||
| IndexView index = combinedIndexBuildItem.getIndex(); | ||
|
|
||
| for (AnnotationInstance deserializeInstance : index.getAnnotations(TRANSACTIONAL)) { | ||
| AnnotationTarget annotationTarget = deserializeInstance.target(); | ||
|
|
||
| if (annotationTarget.hasAnnotation(WITH_SESSION_ON_DEMAND)) { | ||
| throw new ConfigurationException("Cannot mix @Transactional and @WithSessionOnDemand"); | ||
| } | ||
|
|
||
| if (annotationTarget.hasAnnotation(WITH_SESSION)) { | ||
| throw new ConfigurationException("Cannot mix @Transactional and @WithSession"); | ||
| } | ||
|
|
||
| if (annotationTarget.hasAnnotation(WITH_TRANSACTION)) { | ||
| throw new ConfigurationException("Cannot mix @Transactional and @WithTransaction"); | ||
| } | ||
|
Comment on lines
+48
to
+58
Member
There was a problem hiding this comment. Unless we provide more explanation (why they can't be combined) and actionable messages (list possible courses of action and their consequences), I don't think this is much better than the runtime checks that would happen anyway if you removed this code. |
||
|
|
||
| } | ||
|
|
||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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... ?