Skip to content
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.

Sign up for GitHub

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

ArC: detect unsupported Inject annotations #31012

Merged
merged 1 commit into from
Feb 9, 2023
Merged

ArC: detect unsupported Inject annotations #31012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions ...arc/deployment/src/main/java/io/quarkus/arc/deployment/WrongAnnotationUsageProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.function.Function;

import org.jboss.jandex.AnnotationInstance;
import org.jboss.jandex.AnnotationTarget;
Expand Down Expand Up @@ -38,24 +37,25 @@ void detect(ArcConfig config, ApplicationIndexBuildItem applicationIndex, Custom

// Detect unsupported annotations
List<UnsupportedAnnotation> unsupported = new ArrayList<>();
Function<AnnotationInstance, String> singletonFun = new Function<AnnotationInstance, String>() {

@Override
public String apply(AnnotationInstance annotationInstance) {
return String.format("%s declared on %s, use @jakarta.inject.Singleton instead",
annotationInstance.toString(false), getTargetInfo(annotationInstance));
}
};
unsupported.add(new UnsupportedAnnotation(DotName.createSimple("com.google.inject.Singleton"), singletonFun));
unsupported.add(new UnsupportedAnnotation(DotName.createSimple("jakarta.ejb.Singleton"), singletonFun));
unsupported.add(new UnsupportedAnnotation(DotName.createSimple("groovy.lang.Singleton"), singletonFun));
unsupported.add(new UnsupportedAnnotation(DotName.createSimple("jakarta.ejb.Singleton"), singletonFun));
String correctSingleton = "@jakarta.inject.Singleton";
unsupported.add(new UnsupportedAnnotation("com.google.inject.Singleton", correctSingleton));
unsupported.add(new UnsupportedAnnotation("jakarta.ejb.Singleton", correctSingleton));
unsupported.add(new UnsupportedAnnotation("groovy.lang.Singleton", correctSingleton));

String correctInject = "@jakarta.inject.Inject";
unsupported.add(new UnsupportedAnnotation("javax.inject.Inject", correctInject));
unsupported.add(new UnsupportedAnnotation("com.google.inject.Inject", correctInject));
unsupported.add(new UnsupportedAnnotation("com.oracle.svm.core.annotate.Inject", correctInject));
unsupported.add(new UnsupportedAnnotation("org.gradle.internal.impldep.javax.inject.Inject",
correctInject));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you didn't do it for let's say @ApplicationScoped?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Inject and @Singleton are the most common ones and the problematic cases are mainly when you migrate to Quarkus 3 - the compiler won't tell you about these in tests (as seen from changed files).
I am not aware of similar issue with @ApplicationScoped, at least not in basic dependency setup?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ApplicationScoped is CDI specific, so it's unlikely to be forked that much. I agree @Inject and @Singleton are the most important. We could add @Named as well. I'm not sure about @Qualifier and @Scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, no reason really... except that if you use a wrong scope the bean class is just ignored and usually results in a build failure (because of an unsatisfied dependency) while if you use a wrong @Inject you can get NPEs at runtime.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah OK. You will have javax.inject around in our test scope so I suppose it makes sense to limit ourselves to these.

I was more thinking of someone who just upgraded the Quarkus version without changing the source code but they will have a ton of other issues anyway so I suppose we can ignore @ApplicationScoped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can try to address scopes in a separate pull request...


Map<AnnotationInstance, String> wrongUsages = new HashMap<>();

for (UnsupportedAnnotation annotation : unsupported) {
for (AnnotationInstance annotationInstance : index.getAnnotations(annotation.name)) {
wrongUsages.put(annotationInstance, annotation.messageFun.apply(annotationInstance));
wrongUsages.put(annotationInstance, String.format("%s declared on %s, use %s instead",
annotationInstance.toString(false), getTargetInfo(annotationInstance), annotation.correctAnnotation));
}
}

Expand Down Expand Up @@ -108,11 +108,11 @@ public String apply(AnnotationInstance annotationInstance) {
private static class UnsupportedAnnotation {

final DotName name;
final Function<AnnotationInstance, String> messageFun;
final String correctAnnotation;

UnsupportedAnnotation(DotName name, Function<AnnotationInstance, String> messageFun) {
this.name = name;
this.messageFun = messageFun;
UnsupportedAnnotation(String name, String correctAnnotation) {
this.name = DotName.createSimple(name);
this.correctAnnotation = correctAnnotation;
}

}
Expand Down
3 changes: 1 addition & 2 deletions ...ns/arc/deployment/src/test/java/io/quarkus/arc/test/observer/ObserverTransformerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import jakarta.enterprise.event.Event;
import jakarta.enterprise.event.Observes;
import jakarta.inject.Inject;
import jakarta.inject.Qualifier;
import jakarta.inject.Singleton;

Expand All @@ -26,8 +27,6 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import com.google.inject.Inject;

import io.quarkus.arc.deployment.ObserverTransformerBuildItem;
import io.quarkus.arc.processor.ObserverTransformer;
import io.quarkus.builder.BuildChainBuilder;
Expand Down
21 changes: 21 additions & 0 deletions ...eployment/src/test/java/io/quarkus/arc/test/wrongannotations/BeanWithIncorrectInject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package io.quarkus.arc.test.wrongannotations;

import jakarta.enterprise.inject.spi.BeanManager;

import com.google.inject.Inject;

public class BeanWithIncorrectInject {

@Inject
BeanManager bm1;

@javax.inject.Inject
BeanManager bm2;

@com.oracle.svm.core.annotate.Inject
BeanManager bm3;

@org.gradle.internal.impldep.javax.inject.Inject
BeanManager bm4;

}
50 changes: 50 additions & 0 deletions ...ns/arc/deployment/src/test/java/io/quarkus/arc/test/wrongannotations/WrongInjectTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package io.quarkus.arc.test.wrongannotations;

import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

import javax.inject.Inject;

import jakarta.enterprise.inject.spi.BeanManager;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.runtime.util.ExceptionUtil;
import io.quarkus.test.QuarkusUnitTest;

public class WrongInjectTest {

@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
.addClasses(BeanWithIncorrectInject.class))
.assertException(t -> {
Throwable rootCause = ExceptionUtil.getRootCause(t);
assertTrue(rootCause.getMessage().contains(
"@com.google.inject.Inject declared on io.quarkus.arc.test.wrongannotations.BeanWithIncorrectInject.bm1, use @jakarta.inject.Inject instead"),
t.toString());
assertTrue(rootCause.getMessage().contains(
"@javax.inject.Inject declared on io.quarkus.arc.test.wrongannotations.BeanWithIncorrectInject.bm2, use @jakarta.inject.Inject instead"),
t.toString());
assertTrue(rootCause.getMessage().contains(
"@com.oracle.svm.core.annotate.Inject declared on io.quarkus.arc.test.wrongannotations.BeanWithIncorrectInject.bm3, use @jakarta.inject.Inject instead"),
t.toString());
assertTrue(rootCause.getMessage().contains(
"@org.gradle.internal.impldep.javax.inject.Inject declared on io.quarkus.arc.test.wrongannotations.BeanWithIncorrectInject.bm4, use @jakarta.inject.Inject instead"),
t.toString());
assertTrue(rootCause.getMessage().contains(
"@javax.inject.Inject declared on io.quarkus.arc.test.wrongannotations.WrongInjectTest.beanManager, use @jakarta.inject.Inject instead"),
t.toString());
});

@Inject
BeanManager beanManager;

@Test
public void testValidationFailed() {
// This method should not be invoked
fail();
}

}