From 2239362f518c992cfbbe6ff566d63de923875594 Mon Sep 17 00:00:00 2001 From: Martin Kouba Date: Tue, 14 Mar 2023 21:08:12 +0100 Subject: [PATCH] Hibernate reactive panache - validation of interceptor bindings - the build only fails if any of [WithSession, WithTransaction, WithSessionOnDemand] is declared on a method that does not return Uni - if declared on a class then the methods that do not return Uni are effectively ignored --- .../PanacheJpaCommonResourceProcessor.java | 36 +++++++++++----- .../reactive/panache/common/WithSession.java | 4 +- .../panache/common/WithSessionOnDemand.java | 11 ++--- .../panache/common/WithTransaction.java | 11 ++--- .../runtime/AbstractUniInterceptor.java | 4 ++ .../runtime/WithSessionInterceptor.java | 9 ++-- .../WithSessionOnDemandInterceptor.java | 9 ++-- .../runtime/WithTransactionInterceptor.java | 9 ++-- ...thTransactionClassLevelValidationTest.java | 41 +++++++++++++++++++ ...hTransactionMethodLevelValidationTest.java | 36 ++++++++++++++++ 10 files changed, 139 insertions(+), 31 deletions(-) create mode 100644 extensions/panache/hibernate-reactive-panache/deployment/src/test/java/io/quarkus/hibernate/reactive/panache/test/WithTransactionClassLevelValidationTest.java create mode 100644 extensions/panache/hibernate-reactive-panache/deployment/src/test/java/io/quarkus/hibernate/reactive/panache/test/WithTransactionMethodLevelValidationTest.java diff --git a/extensions/panache/hibernate-reactive-panache-common/deployment/src/main/java/io/quarkus/hibernate/reactive/panache/common/deployment/PanacheJpaCommonResourceProcessor.java b/extensions/panache/hibernate-reactive-panache-common/deployment/src/main/java/io/quarkus/hibernate/reactive/panache/common/deployment/PanacheJpaCommonResourceProcessor.java index fecb66017602c..f2ab103f76e41 100644 --- a/extensions/panache/hibernate-reactive-panache-common/deployment/src/main/java/io/quarkus/hibernate/reactive/panache/common/deployment/PanacheJpaCommonResourceProcessor.java +++ b/extensions/panache/hibernate-reactive-panache-common/deployment/src/main/java/io/quarkus/hibernate/reactive/panache/common/deployment/PanacheJpaCommonResourceProcessor.java @@ -8,7 +8,6 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Set; -import java.util.stream.Collectors; import jakarta.annotation.Priority; import jakarta.interceptor.Interceptor; @@ -22,6 +21,7 @@ import org.jboss.jandex.DotName; import org.jboss.jandex.MethodInfo; import org.jboss.jandex.Type; +import org.jboss.logging.Logger; import io.quarkus.arc.deployment.AdditionalBeanBuildItem; import io.quarkus.arc.deployment.AnnotationsTransformerBuildItem; @@ -59,6 +59,8 @@ @BuildSteps(onlyIf = HibernateOrmEnabled.class) public final class PanacheJpaCommonResourceProcessor { + private static final Logger LOG = Logger.getLogger(PanacheJpaCommonResourceProcessor.class); + private static final DotName DOTNAME_NAMED_QUERY = DotName.createSimple(NamedQuery.class.getName()); private static final DotName DOTNAME_NAMED_QUERIES = DotName.createSimple(NamedQueries.class.getName()); private static final String TEST_REACTIVE_TRANSACTION = "io.quarkus.test.TestReactiveTransaction"; @@ -114,15 +116,7 @@ void validateInterceptedMethods(ValidationPhaseBuildItem validationPhase, // Method returns Uni - no need to iterate over the bindings continue; } - if (Annotations.containsAny(e.getValue(), bindings)) { - errors.produce(new ValidationErrorBuildItem( - new IllegalStateException( - "A method annotated with " - + bindings.stream().map(b -> "@" + b.withoutPackagePrefix()) - .collect(Collectors.toList()) - + " must return Uni: " - + e.getKey() + " declared on " + e.getKey().declaringClass()))); - } + validateBindings(bindings, e, errors); } } } @@ -247,4 +241,26 @@ private void lookupNamedQueries(CombinedIndexBuildItem index, DotName name, Set< } } } + + private void validateBindings(List bindings, Entry> entry, + BuildProducer errors) { + for (DotName binding : bindings) { + for (AnnotationInstance annotation : entry.getValue()) { + if (annotation.name().equals(binding)) { + if (annotation.target().kind() == Kind.METHOD) { + errors.produce(new ValidationErrorBuildItem( + new IllegalStateException( + "A method annotated with @" + + binding.withoutPackagePrefix() + + " must return io.smallrye.mutiny.Uni: " + + entry.getKey() + " declared on " + entry.getKey().declaringClass()))); + } else { + LOG.debugf("Class-level binding %s will be ignored for method %s() declared on %s", binding, + entry.getKey().name(), entry.getKey().declaringClass()); + } + return; + } + } + } + } } diff --git a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithSession.java b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithSession.java index d9099daa3a3c5..75ab119f27e8b 100644 --- a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithSession.java +++ b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithSession.java @@ -16,8 +16,8 @@ * triggered, then this session is reused. Otherwise, a new session is opened and eventually closed when the * {@link io.smallrye.mutiny.Uni} completes. *

- * A method annotated with this annotation must return either {@link io.smallrye.mutiny.Uni}. If declared on a class then all - * methods that are intercepted must return {@link io.smallrye.mutiny.Uni}. + * A method annotated with this annotation must return {@link io.smallrye.mutiny.Uni}. If declared on a class then all methods + * that return {@link io.smallrye.mutiny.Uni} are considered; all other methods are ignored. */ @Inherited @InterceptorBinding diff --git a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithSessionOnDemand.java b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithSessionOnDemand.java index 3ebc4f4ac668f..98e5fea830b7f 100644 --- a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithSessionOnDemand.java +++ b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithSessionOnDemand.java @@ -10,13 +10,14 @@ /** * Instructs Panache to trigger the {@link io.smallrye.mutiny.Uni} returned from the intercepted method within a scope of a - * reactive {@link org.hibernate.reactive.mutiny.Mutiny.Session} (if needed). If a reactive session exists when the - * {@link io.smallrye.mutiny.Uni} returned from the annotated method is triggered, then this session is reused. Otherwise, a new - * session is opened when needed and eventually closed when the + * reactive {@link org.hibernate.reactive.mutiny.Mutiny.Session} (if needed). + *

+ * If a reactive session exists when the {@link io.smallrye.mutiny.Uni} returned from the annotated method is triggered, then + * this session is reused. Otherwise, a new session is opened when needed and eventually closed when the * {@link io.smallrye.mutiny.Uni} completes. *

- * A method annotated with this annotation must return {@link io.smallrye.mutiny.Uni}. If declared on a class then all methods - * that are intercepted must return {@link io.smallrye.mutiny.Uni}. + * * A method annotated with this annotation must return {@link io.smallrye.mutiny.Uni}. If declared on a class then all methods + * that return {@link io.smallrye.mutiny.Uni} are considered; all other methods are ignored. */ @Inherited @InterceptorBinding diff --git a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithTransaction.java b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithTransaction.java index 71f063d778e0f..ea0c0df6a27b6 100644 --- a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithTransaction.java +++ b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithTransaction.java @@ -10,13 +10,14 @@ /** * Instructs Panache to trigger the {@link io.smallrye.mutiny.Uni} returned from the intercepted method within a scope of a - * reactive {@link org.hibernate.reactive.mutiny.Mutiny.Transaction}. If a reactive session exists when the - * {@link io.smallrye.mutiny.Uni} returned from the annotated method is triggered, then this session is reused. Otherwise, a new - * session is opened and eventually closed when the - * {@link io.smallrye.mutiny.Uni} completes. + * reactive {@link org.hibernate.reactive.mutiny.Mutiny.Transaction}. + *

+ * If a reactive session exists when the {@link io.smallrye.mutiny.Uni} returned from the annotated method is triggered, then + * this session is reused. Otherwise, a new + * session is opened and eventually closed when the {@link io.smallrye.mutiny.Uni} completes. *

* A method annotated with this annotation must return {@link io.smallrye.mutiny.Uni}. If declared on a class then all methods - * that are intercepted must return {@link io.smallrye.mutiny.Uni}. + * that return {@link io.smallrye.mutiny.Uni} are considered; all other methods are ignored. * * @see org.hibernate.reactive.mutiny.Mutiny.SessionFactory#withTransaction(java.util.function.Function) */ diff --git a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/AbstractUniInterceptor.java b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/AbstractUniInterceptor.java index 2b9b1ea23923c..5bfdadd421390 100644 --- a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/AbstractUniInterceptor.java +++ b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/AbstractUniInterceptor.java @@ -15,4 +15,8 @@ protected Uni proceedUni(InvocationContext context) { } } + protected boolean isUniReturnType(InvocationContext context) { + return context.getMethod().getReturnType().equals(Uni.class); + } + } diff --git a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithSessionInterceptor.java b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithSessionInterceptor.java index a3457b40b18f9..2976b9bb5cb5b 100644 --- a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithSessionInterceptor.java +++ b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithSessionInterceptor.java @@ -14,9 +14,12 @@ public class WithSessionInterceptor extends AbstractUniInterceptor { @AroundInvoke public Object intercept(InvocationContext context) throws Exception { - // Note that intercepted methods annotated with @WithSession are validated at build time - // The build fails if a method does not return Uni - return SessionOperations.withSession(s -> proceedUni(context)); + // Bindings are validated at build time - method-level binding declared on a method that does not return Uni results in a build failure + // However, a class-level binding implies that methods that do not return Uni are just a no-op + if (isUniReturnType(context)) { + return SessionOperations.withSession(s -> proceedUni(context)); + } + return context.proceed(); } } diff --git a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithSessionOnDemandInterceptor.java b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithSessionOnDemandInterceptor.java index 8969e0a38653f..6275bd3e2a42b 100644 --- a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithSessionOnDemandInterceptor.java +++ b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithSessionOnDemandInterceptor.java @@ -14,9 +14,12 @@ public class WithSessionOnDemandInterceptor extends AbstractUniInterceptor { @AroundInvoke public Object intercept(InvocationContext context) throws Exception { - // Note that intercepted methods annotated with @WithSessionOnDemand are validated at build time - // The build fails if a method does not return Uni - return SessionOperations.withSessionOnDemand(() -> proceedUni(context)); + // Bindings are validated at build time - method-level binding declared on a method that does not return Uni results in a build failure + // However, a class-level binding implies that methods that do not return Uni are just a no-op + if (isUniReturnType(context)) { + return SessionOperations.withSessionOnDemand(() -> proceedUni(context)); + } + return context.proceed(); } } diff --git a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithTransactionInterceptor.java b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithTransactionInterceptor.java index 1063f47b99a09..fa7a7e2c74375 100644 --- a/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithTransactionInterceptor.java +++ b/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/WithTransactionInterceptor.java @@ -14,9 +14,12 @@ public class WithTransactionInterceptor extends AbstractUniInterceptor { @AroundInvoke public Object intercept(InvocationContext context) throws Exception { - // Note that intercepted methods annotated with @WithTransaction are validated at build time - // The build fails if the method does not return Uni - return SessionOperations.withTransaction(() -> proceedUni(context)); + // Bindings are validated at build time - method-level binding declared on a method that does not return Uni results in a build failure + // However, a class-level binding implies that methods that do not return Uni are just a no-op + if (isUniReturnType(context)) { + return SessionOperations.withTransaction(() -> proceedUni(context)); + } + return context.proceed(); } } diff --git a/extensions/panache/hibernate-reactive-panache/deployment/src/test/java/io/quarkus/hibernate/reactive/panache/test/WithTransactionClassLevelValidationTest.java b/extensions/panache/hibernate-reactive-panache/deployment/src/test/java/io/quarkus/hibernate/reactive/panache/test/WithTransactionClassLevelValidationTest.java new file mode 100644 index 0000000000000..7f5ed1fd985f3 --- /dev/null +++ b/extensions/panache/hibernate-reactive-panache/deployment/src/test/java/io/quarkus/hibernate/reactive/panache/test/WithTransactionClassLevelValidationTest.java @@ -0,0 +1,41 @@ +package io.quarkus.hibernate.reactive.panache.test; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.quarkus.arc.Unremovable; +import io.quarkus.hibernate.reactive.panache.common.WithTransaction; +import io.quarkus.test.QuarkusUnitTest; + +public class WithTransactionClassLevelValidationTest { + + @RegisterExtension + static QuarkusUnitTest runner = new QuarkusUnitTest() + .withApplicationRoot(root -> root + .addClasses(Bean.class)); + + @Inject + Bean bean; + + @Test + public void testBindingIgnored() { + assertEquals("ok", bean.ping()); + } + + @WithTransaction + @Unremovable + @ApplicationScoped + static class Bean { + + // this method is just ignored by the interceptor + String ping() { + return "ok"; + } + + } +} diff --git a/extensions/panache/hibernate-reactive-panache/deployment/src/test/java/io/quarkus/hibernate/reactive/panache/test/WithTransactionMethodLevelValidationTest.java b/extensions/panache/hibernate-reactive-panache/deployment/src/test/java/io/quarkus/hibernate/reactive/panache/test/WithTransactionMethodLevelValidationTest.java new file mode 100644 index 0000000000000..4e6bebd272b88 --- /dev/null +++ b/extensions/panache/hibernate-reactive-panache/deployment/src/test/java/io/quarkus/hibernate/reactive/panache/test/WithTransactionMethodLevelValidationTest.java @@ -0,0 +1,36 @@ +package io.quarkus.hibernate.reactive.panache.test; + +import static org.junit.jupiter.api.Assertions.fail; + +import jakarta.enterprise.context.ApplicationScoped; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.quarkus.arc.Unremovable; +import io.quarkus.hibernate.reactive.panache.common.WithTransaction; +import io.quarkus.test.QuarkusUnitTest; + +public class WithTransactionMethodLevelValidationTest { + + @RegisterExtension + static QuarkusUnitTest runner = new QuarkusUnitTest() + .setExpectedException(IllegalStateException.class) + .withApplicationRoot(root -> root + .addClasses(Bean.class)); + + @Test + public void testValidationFailed() { + fail(); + } + + @Unremovable + @ApplicationScoped + static class Bean { + + @WithTransaction + void ping() { + } + + } +}