Merge pull request quarkusio#3133 from mkouba/issue-3126-bean-constru…

…ctor

Arc - fix constructor injection

independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java

@@ -236,7 +236,7 @@ public List<InjectionPointInfo> getAllInjectionPoints() {
     }
 
     Optional<Injection> getConstructorInjection() {
-        return injections.isEmpty() ? Optional.empty() : injections.stream().filter(i -> i.isConstructor()).findAny();
+        return injections.isEmpty() ? Optional.empty() : injections.stream().filter(Injection::isConstructor).findAny();
     }
 
     Map<MethodInfo, InterceptionInfo> getInterceptedMethods() {

independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Injection.java

@@ -4,6 +4,9 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import javax.enterprise.inject.spi.DefinitionException;
 import org.jboss.jandex.AnnotationInstance;
 import org.jboss.jandex.AnnotationTarget;
 import org.jboss.jandex.AnnotationTarget.Kind;
@@ -40,7 +43,14 @@ static List<Injection> forBean(AnnotationTarget beanTarget, BeanInfo declaringBe
             InjectionPointModifier transformer) {
         if (Kind.CLASS.equals(beanTarget.kind())) {
             List<Injection> injections = new ArrayList<>();
-            forClassBean(beanTarget.asClass(), beanTarget.asClass(), beanDeployment, injections, transformer);
+            forClassBean(beanTarget.asClass(), beanTarget.asClass(), beanDeployment, injections, transformer, false);
+            Set<AnnotationTarget> injectConstructors = injections.stream().filter(Injection::isConstructor)
+                    .map(Injection::getTarget).collect(Collectors.toSet());
+            if (injectConstructors.size() > 1) {
+                throw new DefinitionException(
+                        "Multiple @Inject constructors found on " + beanTarget.asClass().name() + ":\n"
+                                + injectConstructors.stream().map(Object::toString).collect(Collectors.joining("\n")));
+            }
             return injections;
         } else if (Kind.METHOD.equals(beanTarget.kind())) {
             if (beanTarget.asMethod().parameters().isEmpty()) {
@@ -56,9 +66,10 @@ static List<Injection> forBean(AnnotationTarget beanTarget, BeanInfo declaringBe
     }
 
     private static void forClassBean(ClassInfo beanClass, ClassInfo classInfo, BeanDeployment beanDeployment,
-            List<Injection> injections, InjectionPointModifier transformer) {
+            List<Injection> injections, InjectionPointModifier transformer, boolean skipConstructors) {
 
-        List<AnnotationInstance> injectAnnotations = getAllInjectionPoints(beanDeployment, classInfo, DotNames.INJECT);
+        List<AnnotationInstance> injectAnnotations = getAllInjectionPoints(beanDeployment, classInfo, DotNames.INJECT,
+                skipConstructors);
 
         for (AnnotationInstance injectAnnotation : injectAnnotations) {
             AnnotationTarget injectTarget = injectAnnotation.target();
@@ -79,21 +90,12 @@ private static void forClassBean(ClassInfo beanClass, ClassInfo classInfo, BeanD
                     continue;
             }
         }
-        // if the class has a single non no-arg constructor that is not annotated with @Inject,
-        // the class is not a non-static inner or and it not a superclass of of a bean
-        // we consider that constructor as an injection
+        // if the class has no no-arg constructor and has a single non no-arg constructor that is not annotated with @Inject,
+        // the class is not a non-static inner or a superclass of a bean we consider that constructor as an injection
         if (beanClass.equals(classInfo)) {
-            boolean constrInjectionExists = false;
-            for (Injection injection : injections) {
-                if (injection.isConstructor()) {
-                    constrInjectionExists = true;
-                    break;
-                }
-            }
-
             final boolean isNonStaticInnerClass = classInfo.name().isInner()
                     && !Modifier.isStatic(classInfo.flags());
-            if (!isNonStaticInnerClass && !constrInjectionExists) {
+            if (!isNonStaticInnerClass && !hasConstructorInjection(injections) && !beanClass.hasNoArgsConstructor()) {
                 List<MethodInfo> nonNoargConstrs = new ArrayList<>();
                 for (MethodInfo constr : classInfo.methods()) {
                     if (Methods.INIT.equals(constr.name()) && constr.parameters().size() > 0) {
@@ -110,32 +112,39 @@ private static void forClassBean(ClassInfo beanClass, ClassInfo classInfo, BeanD
 
         for (DotName resourceAnnotation : beanDeployment.getResourceAnnotations()) {
             List<AnnotationInstance> resourceAnnotations = getAllInjectionPoints(beanDeployment, classInfo,
-                    resourceAnnotation);
-            if (resourceAnnotations != null) {
-                for (AnnotationInstance resourceAnnotationInstance : resourceAnnotations) {
-                    if (Kind.FIELD == resourceAnnotationInstance.target().kind()
-                            && resourceAnnotationInstance.target().asField().annotations().stream()
-                                    .noneMatch(a -> DotNames.INJECT.equals(a.name()))) {
-                        // Add special injection for a resource field
-                        injections.add(new Injection(resourceAnnotationInstance.target(), Collections
-                                .singletonList(InjectionPointInfo
-                                        .fromResourceField(resourceAnnotationInstance.target().asField(), beanClass,
-                                                beanDeployment, transformer))));
-                    }
-                    // TODO setter injection
+                    resourceAnnotation, true);
+            for (AnnotationInstance resourceAnnotationInstance : resourceAnnotations) {
+                if (Kind.FIELD == resourceAnnotationInstance.target().kind()
+                        && resourceAnnotationInstance.target().asField().annotations().stream()
+                                .noneMatch(a -> DotNames.INJECT.equals(a.name()))) {
+                    // Add special injection for a resource field
+                    injections.add(new Injection(resourceAnnotationInstance.target(), Collections
+                            .singletonList(InjectionPointInfo
+                                    .fromResourceField(resourceAnnotationInstance.target().asField(), beanClass,
+                                            beanDeployment, transformer))));
                 }
+                // TODO setter injection
             }
         }
 
         if (!classInfo.superName().equals(DotNames.OBJECT)) {
             ClassInfo info = beanDeployment.getIndex().getClassByName(classInfo.superName());
             if (info != null) {
-                forClassBean(beanClass, info, beanDeployment, injections, transformer);
+                forClassBean(beanClass, info, beanDeployment, injections, transformer, true);
             }
         }
 
     }
 
+    private static boolean hasConstructorInjection(List<Injection> injections) {
+        for (Injection injection : injections) {
+            if (injection.isConstructor()) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     static Injection forDisposer(MethodInfo disposerMethod, ClassInfo beanClass, BeanDeployment beanDeployment,
             InjectionPointModifier transformer) {
         return new Injection(disposerMethod, InjectionPointInfo.fromMethod(disposerMethod, beanClass, beanDeployment,
@@ -171,8 +180,12 @@ boolean isField() {
         return Kind.FIELD == target.kind();
     }
 
+    public AnnotationTarget getTarget() {
+        return target;
+    }
+
     private static List<AnnotationInstance> getAllInjectionPoints(BeanDeployment beanDeployment, ClassInfo beanClass,
-            DotName name) {
+            DotName name, boolean skipConstructors) {
         List<AnnotationInstance> injectAnnotations = new ArrayList<>();
         for (FieldInfo field : beanClass.fields()) {
             AnnotationInstance inject = beanDeployment.getAnnotation(field, name);
@@ -181,6 +194,9 @@ private static List<AnnotationInstance> getAllInjectionPoints(BeanDeployment bea
             }
         }
         for (MethodInfo method : beanClass.methods()) {
+            if (skipConstructors && method.name().equals(Methods.INIT)) {
+                continue;
+            }
             AnnotationInstance inject = beanDeployment.getAnnotation(method, name);
             if (inject != null) {
                 injectAnnotations.add(inject);

independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/injection/constructornoinject/MultiInjectConstructorFailureTest.java

@@ -0,0 +1,50 @@
+package io.quarkus.arc.test.injection.constructornoinject;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import io.quarkus.arc.test.ArcTestContainer;
+import javax.enterprise.context.Dependent;
+import javax.enterprise.inject.spi.DefinitionException;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import org.junit.Rule;
+import org.junit.Test;
+
+public class MultiInjectConstructorFailureTest {
+
+    @Rule
+    public ArcTestContainer container = ArcTestContainer.builder()
+            .beanClasses(CombineHarvester.class, Head.class)
+            .shouldFail()
+            .build();
+
+    @Test
+    public void testInjection() {
+        Throwable error = container.getFailure();
+        assertNotNull(error);
+        assertTrue(error instanceof DefinitionException);
+    }
+
+    @Dependent
+    static class Head {
+
+    }
+
+    @Singleton
+    static class CombineHarvester {
+
+        Head head;
+
+        @Inject
+        public CombineHarvester() {
+            this.head = null;
+        }
+
+        @Inject
+        public CombineHarvester(Head head) {
+            this.head = head;
+        }
+
+    }
+}

independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/injection/constructornoinject/NoArgConstructorTakesPrecedenceTest.java

@@ -0,0 +1,39 @@
+package io.quarkus.arc.test.injection.constructornoinject;
+
+import static org.junit.Assert.assertEquals;
+
+import io.quarkus.arc.Arc;
+import io.quarkus.arc.test.ArcTestContainer;
+import javax.inject.Singleton;
+import org.junit.Rule;
+import org.junit.Test;
+
+public class NoArgConstructorTakesPrecedenceTest {
+
+    @Rule
+    public ArcTestContainer container = new ArcTestContainer(CombineHarvester.class);
+
+    @Test
+    public void testInjection() {
+        assertEquals("OK", Arc.container().instance(CombineHarvester.class).get().getHead());
+    }
+
+    @Singleton
+    static class CombineHarvester {
+
+        private String head;
+
+        public CombineHarvester() {
+            this.head = "OK";
+        }
+
+        public CombineHarvester(String head) {
+            this.head = head;
+        }
+
+        public String getHead() {
+            return head;
+        }
+
+    }
+}

independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/injection/constructornoinject/SingleNonNoArgConstructorInjectionTest.java

@@ -28,10 +28,10 @@ static class Head {
     @Singleton
     static class CombineHarvester {
 
-        private final Head head;
+        final Head head;
 
         @Inject
-        private Head head2;
+        Head head2;
 
         public CombineHarvester(Head head) {
             this.head = head;