Introduce ScheduledTransactionTraceCheck
#31
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
|
Comment on lines
+48
to
+51
There was a problem hiding this comment. For easy clicking: google/error-prone#2820. |
||
| 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); | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning is applied to the method if the annotation is absent, and to the annotation if present but incorrect.