jlengrand/error-prone-support
1
0
Fork 0
mirror of https://github.com/jlengrand/error-prone-support.git synced 2026-03-10 08:11:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

Drop ScheduledTransactionTrace check (#788)

Browse Source 
As well as any other references to New Relic.
This commit is contained in:
Stephan Schroevers
2023-10-04 15:30:13 +02:00
committed by GitHub
parent ef75b80e7b
commit 9a77a3f35f
7 changed files with 6 additions and 228 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
error-prone-contrib/README.md
View File

@@ -125,9 +125,6 @@ The following is a list of checks we'd like to see implemented:
statement. Idem for other exception types.
- A Guava-specific check that replaces simple anonymous `CacheLoader` subclass
declarations with `CacheLoader.from(someLambda)`.
- A Spring-specific check that enforces that methods with the `@Scheduled`
annotation are also annotated with New Relic's `@Trace` annotation. Such
methods should ideally not also represent Spring MVC endpoints.
- A Spring-specific check that enforces that `@RequestMapping` annotations,
when applied to a method, explicitly specify one or more target HTTP methods.
- A Spring-specific check that looks for classes in which all `@RequestMapping`

5
error-prone-contrib/pom.xml
View File

@@ -81,11 +81,6 @@
<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>

94
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTrace.java
View File

@@ -1,94 +0,0 @@
package tech.picnic.errorprone.bugpatterns;
import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR;
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 static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
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.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;
import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary;
/**
* A {@link BugChecker} that flags methods with Spring's {@code @Scheduled} annotation that lack New
* Relic Agent's {@code @Trace(dispatcher = true)}.
*/
@AutoService(BugChecker.class)
@BugPattern(
summary = "Scheduled operation must start a new New Relic transaction",
link = BUG_PATTERNS_BASE_URL + "ScheduledTransactionTrace",
linkType = CUSTOM,
severity = ERROR,
tags = LIKELY_ERROR)
public final class ScheduledTransactionTrace 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));
/** Instantiates a new {@link ScheduledTransactionTrace} instance. */
public ScheduledTransactionTrace() {}
@Override
public Description matchMethod(MethodTree tree, VisitorState state) {
if (!ThirdPartyLibrary.NEW_RELIC_AGENT_API.isIntroductionAllowed(state)
|| !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, state, "dispatcher", ImmutableList.of("true"))
.build());
}
private static boolean isCorrectAnnotation(AnnotationTree traceAnnotation) {
return Boolean.TRUE.equals(
AnnotationMirrors.getAnnotationValue(
ASTHelpers.getAnnotationMirror(traceAnnotation), "dispatcher")
.getValue());
}
}

7
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java
View File

@@ -32,13 +32,6 @@ public enum ThirdPartyLibrary {
* @see <a href="https://github.com/google/guava">Guava on GitHub</a>
*/
GUAVA("com.google.common.collect.ImmutableList"),
/**
* New Relic's Java agent API.
*
* @see <a href="https://github.com/newrelic/newrelic-java-agent/tree/main/newrelic-api">New Relic
* Java agent API on GitHub</a>
*/
NEW_RELIC_AGENT_API("com.newrelic.api.agent.Agent"),
/**
* VMWare's Project Reactor.
*

105
error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTraceTest.java
View File

@@ -1,105 +0,0 @@
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.springframework.scheduling.annotation.Scheduled;
final class ScheduledTransactionTraceTest {
@Test
void identification() {
CompilationTestHelper.newInstance(ScheduledTransactionTrace.class, getClass())
.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();
}
@Test
void identificationWithoutNewRelicAgentApiOnClasspath() {
CompilationTestHelper.newInstance(ScheduledTransactionTrace.class, getClass())
.withClasspath(Scheduled.class)
.addSourceLines(
"A.java",
"import org.springframework.scheduling.annotation.Scheduled;",
"",
"class A {",
" @Scheduled(fixedDelay = 1)",
" void scheduledButNotTraced() {}",
"}")
.doTest();
}
@Test
void replacement() {
BugCheckerRefactoringTestHelper.newInstance(ScheduledTransactionTrace.class, getClass())
.addInputLines(
"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(
"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);
}
}

14
error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibraryTest.java
View File

@@ -23,7 +23,7 @@ final class ThirdPartyLibraryTest {
CompilationTestHelper.newInstance(TestChecker.class, getClass())
.addSourceLines(
"A.java",
"// BUG: Diagnostic contains: ASSERTJ: true, GUAVA: true, NEW_RELIC_AGENT_API: true, REACTOR: true",
"// BUG: Diagnostic contains: ASSERTJ: true, GUAVA: true, REACTOR: true",
"class A {}")
.doTest();
}
@@ -34,16 +34,14 @@ final class ThirdPartyLibraryTest {
.addSourceLines(
"A.java",
"import com.google.common.collect.ImmutableList;",
"import com.newrelic.api.agent.Agent;",
"import org.assertj.core.api.Assertions;",
"import reactor.core.publisher.Flux;",
"",
"// BUG: Diagnostic contains: ASSERTJ: true, GUAVA: true, NEW_RELIC_AGENT_API: true, REACTOR: true",
"// BUG: Diagnostic contains: ASSERTJ: true, GUAVA: true, REACTOR: true",
"class A {",
" void m(Class<?> clazz) {",
" m(Assertions.class);",
" m(ImmutableList.class);",
" m(Agent.class);",
" m(Flux.class);",
" }",
"}")
@@ -56,7 +54,7 @@ final class ThirdPartyLibraryTest {
.withClasspath(ImmutableList.class, Flux.class)
.addSourceLines(
"A.java",
"// BUG: Diagnostic contains: ASSERTJ: false, GUAVA: true, NEW_RELIC_AGENT_API: false, REACTOR: true",
"// BUG: Diagnostic contains: ASSERTJ: false, GUAVA: true, REACTOR: true",
"class A {}")
.doTest();
}
@@ -67,7 +65,7 @@ final class ThirdPartyLibraryTest {
.withClasspath()
.addSourceLines(
"A.java",
"// BUG: Diagnostic contains: ASSERTJ: false, GUAVA: false, NEW_RELIC_AGENT_API: false, REACTOR:",
"// BUG: Diagnostic contains: ASSERTJ: false, GUAVA: false, REACTOR:",
"// false",
"class A {}")
.doTest();
@@ -82,8 +80,8 @@ final class ThirdPartyLibraryTest {
.addSourceLines(
"A.java",
String.format(
"// BUG: Diagnostic contains: ASSERTJ: %s, GUAVA: true, NEW_RELIC_AGENT_API: %s, REACTOR: true",
ignoreClassPath, ignoreClassPath),
"// BUG: Diagnostic contains: ASSERTJ: %s, GUAVA: true, REACTOR: true",
ignoreClassPath),
"class A {}")
.doTest();
}

6
pom.xml
View File

@@ -323,11 +323,6 @@
<artifactId>nopen-checker</artifactId>
<version>${version.nopen-checker}</version>
</dependency>
<dependency>
<groupId>com.newrelic.agent.java</groupId>
<artifactId>newrelic-api</artifactId>
<version>8.6.0</version>
</dependency>
<!-- Specified as a workaround for
https://github.com/mojohaus/versions-maven-plugin/issues/244. -->
<dependency>
@@ -728,7 +723,6 @@
<property name="illegalPkgs" value="com.google.common.cache">
<!-- Instead, please use Caffeine. -->
</property>
<property name="illegalPkgs" value="com.newrelic.agent.deps" />
<property name="illegalPkgs" value="com.tngtech.archunit.thirdparty" />
<property name="illegalPkgs" value="jdk" />
<property name="illegalPkgs" value="jersey.repackaged" />