Introduce ScheduledTransactionTraceCheck (#31)

error-prone-contrib/pom.xml

@@ -56,6 +56,11 @@
             <artifactId>jackson-annotations</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>com.google.auto</groupId>
+            <artifactId>auto-common</artifactId>
+            <scope>provided</scope>
+        </dependency>
         <dependency>
             <groupId>com.google.auto.service</groupId>
             <artifactId>auto-service-annotations</artifactId>
@@ -76,6 +81,11 @@
             <artifactId>guava</artifactId>
             <scope>provided</scope>
         </dependency>
+        <dependency>
+            <groupId>com.newrelic.agent.java</groupId>
+            <artifactId>newrelic-api</artifactId>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>io.projectreactor</groupId>
             <artifactId>reactor-core</artifactId>

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<Tree> IS_SCHEDULED =
+      hasAnnotation("org.springframework.scheduling.annotation.Scheduled");
+  private static final MultiMatcher<Tree, AnnotationTree> 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<AnnotationTree> 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());
+  }
+}

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);
+  }
+}

pom.xml

@@ -257,6 +257,11 @@
                 <artifactId>nopen-checker</artifactId>
                 <version>${version.nopen-checker}</version>
             </dependency>
+            <dependency>
+                <groupId>com.newrelic.agent.java</groupId>
+                <artifactId>newrelic-api</artifactId>
+                <version>7.4.3</version>
+            </dependency>
             <!-- Specified as a workaround for
             https://github.com/mojohaus/versions-maven-plugin/issues/244. -->
             <dependency>