Merge pull request #36104 from Ladicek/fix-duplicate-circuit-breaker-…

…name-detection Fix duplicate circuit breaker name detection
quarkusio · Sep 25, 2023 · 72e23e5 · 72e23e5
2 parents c8d4533 + 5db67ac
commit 72e23e5
Show file tree

Hide file tree

Showing 11 changed files with 73 additions and 18 deletions.
diff --git a/...nt/src/main/java/io/quarkus/smallrye/faulttolerance/deployment/FaultToleranceScanner.java b/...nt/src/main/java/io/quarkus/smallrye/faulttolerance/deployment/FaultToleranceScanner.java
@@ -80,7 +80,7 @@ boolean hasFTAnnotations(ClassInfo clazz) {
     void forEachMethod(ClassInfo clazz, Consumer<MethodInfo> action) {
         for (MethodInfo method : clazz.methods()) {
             if (method.name().startsWith("<")) {
-                // constructors (or static init blocks) can't be intercepted
+                // constructors and static inititalizers can't be intercepted
                 continue;
             }
             if (method.isSynthetic()) {

diff --git a/...n/java/io/quarkus/smallrye/faulttolerance/deployment/SmallRyeFaultToleranceProcessor.java b/...n/java/io/quarkus/smallrye/faulttolerance/deployment/SmallRyeFaultToleranceProcessor.java
@@ -24,7 +24,6 @@
 import org.jboss.jandex.ClassInfo;
 import org.jboss.jandex.DotName;
 import org.jboss.jandex.IndexView;
-import org.jboss.jandex.MethodInfo;
 
 import io.quarkus.arc.deployment.AdditionalBeanBuildItem;
 import io.quarkus.arc.deployment.AnnotationsTransformerBuildItem;
@@ -279,6 +278,7 @@ void processFaultToleranceAnnotations(SmallRyeFaultToleranceRecorder recorder,
 
         List<FaultToleranceMethod> ftMethods = new ArrayList<>();
         List<Throwable> exceptions = new ArrayList<>();
+        Map<String, Set<String>> existingCircuitBreakerNames = new HashMap<>();
 
         for (BeanInfo info : validationPhase.getContext().beans()) {
             ClassInfo beanClass = info.getImplClazz();
@@ -319,6 +319,12 @@ void processFaultToleranceAnnotations(SmallRyeFaultToleranceRecorder recorder,
                                 reflectiveClass.produce(ReflectiveClassBuildItem.builder(exceptionNames.get()).build());
                             }
                         }
+
+                        if (annotationStore.hasAnnotation(method, DotNames.CIRCUIT_BREAKER_NAME)) {
+                            AnnotationInstance ann = annotationStore.getAnnotation(method, DotNames.CIRCUIT_BREAKER_NAME);
+                            existingCircuitBreakerNames.computeIfAbsent(ann.value().asString(), ignored -> new HashSet<>())
+                                    .add(method + " @ " + method.declaringClass());
+                        }
                     }
                 });
 
@@ -337,16 +343,6 @@ void processFaultToleranceAnnotations(SmallRyeFaultToleranceRecorder recorder,
 
         recorder.createFaultToleranceOperation(ftMethods);
 
-        // since annotation transformations are applied lazily, we can't know
-        // all transformed `@CircuitBreakerName`s and have to rely on Jandex here
-        Map<String, Set<String>> existingCircuitBreakerNames = new HashMap<>();
-        for (AnnotationInstance it : index.getAnnotations(DotNames.CIRCUIT_BREAKER_NAME)) {
-            if (it.target().kind() == Kind.METHOD) {
-                MethodInfo method = it.target().asMethod();
-                existingCircuitBreakerNames.computeIfAbsent(it.value().asString(), ignored -> new HashSet<>())
-                        .add(method + " @ " + method.declaringClass());
-            }
-        }
         for (Map.Entry<String, Set<String>> entry : existingCircuitBreakerNames.entrySet()) {
             if (entry.getValue().size() > 1) {
                 exceptions.add(new DefinitionException("Multiple circuit breakers have the same name '"

diff --git a/...e/inheritance/CircuitBreakerService1.java → ...nce/duplicate/CircuitBreakerService1.java b/...e/inheritance/CircuitBreakerService1.java → ...nce/duplicate/CircuitBreakerService1.java
@@ -1,4 +1,4 @@
-package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.inheritance;
+package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.duplicate;
 
 import jakarta.inject.Singleton;
 

diff --git a/...e/inheritance/CircuitBreakerService2.java → ...nce/duplicate/CircuitBreakerService2.java b/...e/inheritance/CircuitBreakerService2.java → ...nce/duplicate/CircuitBreakerService2.java
@@ -1,4 +1,4 @@
-package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.inheritance;
+package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.duplicate;
 
 import jakarta.inject.Singleton;
 

diff --git a/...ance/DuplicateCircuitBreakerNameTest.java → ...cate/DuplicateCircuitBreakerNameTest.java b/...ance/DuplicateCircuitBreakerNameTest.java → ...cate/DuplicateCircuitBreakerNameTest.java
@@ -1,4 +1,4 @@
-package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.inheritance;
+package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.duplicate;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;

diff --git a/...te/CircuitBreakerNameInheritanceTest.java → ...ce/CircuitBreakerNameInheritanceTest.java b/...te/CircuitBreakerNameInheritanceTest.java → ...ce/CircuitBreakerNameInheritanceTest.java
@@ -1,4 +1,4 @@
-package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.duplicate;
+package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.inheritance;
 
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 

diff --git a/...e/duplicate/SubCircuitBreakerService.java → ...inheritance/SubCircuitBreakerService.java b/...e/duplicate/SubCircuitBreakerService.java → ...inheritance/SubCircuitBreakerService.java
@@ -1,4 +1,4 @@
-package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.duplicate;
+package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.inheritance;
 
 import jakarta.inject.Singleton;
 

diff --git a/...duplicate/SuperCircuitBreakerService.java → ...heritance/SuperCircuitBreakerService.java b/...duplicate/SuperCircuitBreakerService.java → ...heritance/SuperCircuitBreakerService.java
@@ -1,4 +1,4 @@
-package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.duplicate;
+package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.inheritance;
 
 import jakarta.inject.Singleton;
 

diff --git a/...ye/faulttolerance/test/circuitbreaker/maintenance/noduplicate/CircuitBreakerService1.java b/...ye/faulttolerance/test/circuitbreaker/maintenance/noduplicate/CircuitBreakerService1.java
@@ -0,0 +1,16 @@
+package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.noduplicate;
+
+import jakarta.inject.Singleton;
+
+import org.eclipse.microprofile.faulttolerance.CircuitBreaker;
+
+import io.smallrye.faulttolerance.api.CircuitBreakerName;
+
+@Singleton
+public class CircuitBreakerService1 {
+    @CircuitBreaker
+    @CircuitBreakerName("hello")
+    public String hello() {
+        return "1";
+    }
+}
diff --git a/...ye/faulttolerance/test/circuitbreaker/maintenance/noduplicate/CircuitBreakerService2.java b/...ye/faulttolerance/test/circuitbreaker/maintenance/noduplicate/CircuitBreakerService2.java
@@ -0,0 +1,14 @@
+package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.noduplicate;
+
+import org.eclipse.microprofile.faulttolerance.CircuitBreaker;
+
+import io.smallrye.faulttolerance.api.CircuitBreakerName;
+
+public class CircuitBreakerService2 {
+    // this class is not a bean, so there's no circuit breaker and hence no duplicate circuit breaker name
+    @CircuitBreaker
+    @CircuitBreakerName("hello")
+    public String hello() {
+        return "2";
+    }
+}
diff --git a/...erance/test/circuitbreaker/maintenance/noduplicate/NoDuplicateCircuitBreakerNameTest.java b/...erance/test/circuitbreaker/maintenance/noduplicate/NoDuplicateCircuitBreakerNameTest.java
@@ -0,0 +1,29 @@
+package io.quarkus.smallrye.faulttolerance.test.circuitbreaker.maintenance.noduplicate;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+import jakarta.inject.Inject;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.quarkus.test.QuarkusUnitTest;
+import io.smallrye.faulttolerance.api.CircuitBreakerMaintenance;
+import io.smallrye.faulttolerance.api.CircuitBreakerState;
+
+public class NoDuplicateCircuitBreakerNameTest {
+    @RegisterExtension
+    static final QuarkusUnitTest config = new QuarkusUnitTest()
+            .withApplicationRoot((jar) -> jar
+                    .addClasses(CircuitBreakerService1.class, CircuitBreakerService2.class));
+
+    @Inject
+    CircuitBreakerMaintenance cb;
+
+    @Test
+    public void deploysWithoutError() {
+        assertNotNull(cb);
+        assertEquals(CircuitBreakerState.CLOSED, cb.currentState("hello"));
+    }
+}