From d35093918102e729f087c598660c21b8df3f2cab Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 8 Jan 2022 11:53:08 +0100 Subject: [PATCH] Introduce `ScheduledTransactionTraceCheck` --- error-prone-contrib/pom.xml | 10 ++ .../ScheduledTransactionTraceCheck.java | 88 +++++++++++++++++ .../ScheduledTransactionTraceCheckTest.java | 99 +++++++++++++++++++ pom.xml | 5 + 4 files changed, 202 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceCheck.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceCheckTest.java diff --git a/error-prone-contrib/pom.xml b/error-prone-contrib/pom.xml index 56021df1d5..007c159397 100644 --- a/error-prone-contrib/pom.xml +++ b/error-prone-contrib/pom.xml @@ -56,6 +56,11 @@ jackson-annotations test + + com.google.auto + auto-common + provided + com.google.auto.service auto-service-annotations @@ -76,6 +81,11 @@ guava provided + + com.newrelic.agent.java + newrelic-api + test + io.projectreactor reactor-core diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceCheck.java new file mode 100644 index 0000000000..7d03e7abe3 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceCheck.java @@ -0,0 +1,88 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE; +import static com.google.errorprone.matchers.Matchers.annotations; +import static com.google.errorprone.matchers.Matchers.hasAnnotation; +import static com.google.errorprone.matchers.Matchers.isType; + +import com.google.auto.common.AnnotationMirrors; +import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.LinkType; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.StandardTags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.MultiMatcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; + +/** + * A {@link BugChecker} which flags methods with Spring's {@code @Scheduled} annotation that lack + * New Relic Agent's {@code @Trace(dispatcher = true)}. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "ScheduledTransactionTrace", + summary = "Scheduled operation must start a new New Relic transaction", + linkType = LinkType.NONE, + severity = SeverityLevel.ERROR, + tags = StandardTags.LIKELY_ERROR) +public final class ScheduledTransactionTraceCheck extends BugChecker implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + private static final String TRACE_ANNOTATION_FQCN = "com.newrelic.api.agent.Trace"; + private static final Matcher IS_SCHEDULED = + hasAnnotation("org.springframework.scheduling.annotation.Scheduled"); + private static final MultiMatcher TRACE_ANNOTATION = + annotations(AT_LEAST_ONE, isType(TRACE_ANNOTATION_FQCN)); + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + if (!IS_SCHEDULED.matches(tree, state)) { + return Description.NO_MATCH; + } + + ImmutableList traceAnnotations = + TRACE_ANNOTATION.multiMatchResult(tree, state).matchingNodes(); + if (traceAnnotations.isEmpty()) { + /* This method completely lacks the `@Trace` annotation; add it. */ + return describeMatch( + tree, + SuggestedFix.builder() + .addImport(TRACE_ANNOTATION_FQCN) + .prefixWith(tree, "@Trace(dispatcher = true)") + .build()); + } + + AnnotationTree traceAnnotation = Iterables.getOnlyElement(traceAnnotations); + if (isCorrectAnnotation(traceAnnotation)) { + return Description.NO_MATCH; + } + + /* + * The `@Trace` annotation is present but does not specify `dispatcher = true`. Add or update + * the `dispatcher` annotation element. + */ + return describeMatch( + traceAnnotation, + SuggestedFixes.updateAnnotationArgumentValues( + traceAnnotation, "dispatcher", ImmutableList.of("true")) + .build()); + } + + private static boolean isCorrectAnnotation(AnnotationTree traceAnnotation) { + return Boolean.TRUE.equals( + AnnotationMirrors.getAnnotationValue( + ASTHelpers.getAnnotationMirror(traceAnnotation), "dispatcher") + .getValue()); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceCheckTest.java new file mode 100644 index 0000000000..e56f831d80 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceCheckTest.java @@ -0,0 +1,99 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledForJreRange; +import org.junit.jupiter.api.condition.JRE; + +public final class ScheduledTransactionTraceCheckTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(ScheduledTransactionTraceCheck.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(ScheduledTransactionTraceCheck.class, getClass()); + + @Test + void identification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import com.newrelic.api.agent.Trace;", + "import org.springframework.scheduling.annotation.Scheduled;", + "", + "class A {", + " void notScheduled() {}", + "", + " @Scheduled(fixedDelay = 1)", + " // BUG: Diagnostic contains:", + " void scheduledButNotTraced() {}", + "", + " @Scheduled(fixedDelay = 1)", + " // BUG: Diagnostic contains:", + " @Trace", + " void scheduledButImproperlyTraced1() {}", + "", + " @Scheduled(fixedDelay = 1)", + " // BUG: Diagnostic contains:", + " @Trace(dispatcher = false)", + " void scheduledButImproperlyTraced2() {}", + "", + " @Scheduled(fixedDelay = 1)", + " @Trace(dispatcher = true)", + " void scheduledAndProperlyTraced() {}", + "}") + .doTest(); + } + + // XXX: Enable this test for all JREs once https://github.com/google/error-prone/pull/2820 is + // merged and released. + @Test + @DisabledForJreRange(min = JRE.JAVA_12) + void replacement() { + refactoringTestHelper + .addInputLines( + "in/A.java", + "import com.newrelic.api.agent.Trace;", + "import org.springframework.scheduling.annotation.Scheduled;", + "", + "class A {", + " @Scheduled(fixedDelay = 1)", + " void scheduledButNotTraced() {}", + "", + " @Scheduled(fixedDelay = 1)", + " @Trace", + " void scheduledButImproperlyTraced1() {}", + "", + " @Scheduled(fixedDelay = 1)", + " @Trace(dispatcher = false)", + " void scheduledButImproperlyTraced2() {}", + "", + " @Scheduled(fixedDelay = 1)", + " @Trace(leaf = true)", + " void scheduledButImproperlyTraced3() {}", + "}") + .addOutputLines( + "out/A.java", + "import com.newrelic.api.agent.Trace;", + "import org.springframework.scheduling.annotation.Scheduled;", + "", + "class A {", + " @Trace(dispatcher = true)", + " @Scheduled(fixedDelay = 1)", + " void scheduledButNotTraced() {}", + "", + " @Scheduled(fixedDelay = 1)", + " @Trace(dispatcher = true)", + " void scheduledButImproperlyTraced1() {}", + "", + " @Scheduled(fixedDelay = 1)", + " @Trace(dispatcher = true)", + " void scheduledButImproperlyTraced2() {}", + "", + " @Scheduled(fixedDelay = 1)", + " @Trace(dispatcher = true, leaf = true)", + " void scheduledButImproperlyTraced3() {}", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/pom.xml b/pom.xml index 425bb807dd..3dea95c005 100644 --- a/pom.xml +++ b/pom.xml @@ -257,6 +257,11 @@ nopen-checker ${version.nopen-checker} + + com.newrelic.agent.java + newrelic-api + 7.4.3 +