Remediator API and adjacent classes refactoring (#455)

Adds a new API for the remediator/searcher pattern we use for codemods.
The aim is to eliminate a lot of duplicate/similar-ish code around the
remediators. Also converts the Sonar JNDI codemod to use the new API.

Classes from the previous API are still there, just renamed with the
`Legacy` prefix.

Author: andrecsilva

core-codemods/src/main/java/io/codemodder/codemods/SonarJNDIInjectionCodemod.java

@@ -7,11 +7,13 @@
 import io.codemodder.providers.sonar.RuleIssue;
 import io.codemodder.providers.sonar.SonarRemediatingJavaParserChanger;
 import io.codemodder.remediation.GenericRemediationMetadata;
+import io.codemodder.remediation.Remediator;
 import io.codemodder.remediation.jndiinjection.JNDIInjectionRemediator;
 import io.codemodder.sonar.model.Issue;
 import io.codemodder.sonar.model.SonarFinding;
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 import javax.inject.Inject;
 
 /** This codemod knows how to fix JNDI vulnerabilities found by sonar. */
@@ -22,15 +24,15 @@
     importance = Importance.HIGH)
 public final class SonarJNDIInjectionCodemod extends SonarRemediatingJavaParserChanger {
 
-  private final JNDIInjectionRemediator remediator;
   private final RuleIssue issues;
+  private final Remediator<Issue> remediator;
 
   @Inject
   public SonarJNDIInjectionCodemod(
       @ProvidedSonarScan(ruleId = "javasecurity:S2078") final RuleIssue issues) {
     super(GenericRemediationMetadata.JNDI.reporter(), issues);
     this.issues = Objects.requireNonNull(issues);
-    this.remediator = JNDIInjectionRemediator.DEFAULT;
+    this.remediator = new JNDIInjectionRemediator<>();
   }
 
   @Override
@@ -52,7 +54,13 @@ public CodemodFileScanningResult visit(
         issuesForFile,
         SonarFinding::getKey,
         i -> i.getTextRange() != null ? i.getTextRange().getStartLine() : i.getLine(),
-        i -> i.getTextRange() != null ? i.getTextRange().getEndLine() : null,
-        i -> i.getTextRange() != null ? i.getTextRange().getStartOffset() : null);
+        i ->
+            i.getTextRange() != null
+                ? Optional.of(i.getTextRange().getEndLine())
+                : Optional.empty(),
+        i ->
+            i.getTextRange() != null
+                ? Optional.of(i.getTextRange().getStartOffset() + 1)
+                : Optional.empty());
   }
 }

core-codemods/src/main/java/io/codemodder/codemods/remediators/ssrf/DefaultSSRFRemediator.java

@@ -15,9 +15,9 @@
 import io.codemodder.codetf.DetectorRule;
 import io.codemodder.codetf.FixedFinding;
 import io.codemodder.codetf.UnfixedFinding;
-import io.codemodder.remediation.FixCandidate;
-import io.codemodder.remediation.FixCandidateSearchResults;
-import io.codemodder.remediation.FixCandidateSearcher;
+import io.codemodder.remediation.LegacyFixCandidate;
+import io.codemodder.remediation.LegacyFixCandidateSearchResults;
+import io.codemodder.remediation.LegacyFixCandidateSearcher;
 import io.codemodder.remediation.MethodOrConstructor;
 import io.github.pixee.security.HostValidator;
 import io.github.pixee.security.Urls;
@@ -42,17 +42,17 @@ public <T> CodemodFileScanningResult remediateAll(
 
     // search for new URL() calls on those lines, assuming the tool points there -- there are plenty
     // of more signatures to chase down
-    FixCandidateSearcher<T> searcher =
-        new FixCandidateSearcher.Builder<T>()
+    LegacyFixCandidateSearcher<T> searcher =
+        new LegacyFixCandidateSearcher.Builder<T>()
             .withMatcher(mce -> mce.isConstructorForType("URL"))
             .withMatcher(mce -> !mce.getArguments().isEmpty())
             .build();
 
     // Matches calls like RestTemplate.exchange(...)
     // Doesn't actually check that the `exchange` call scope is actually of type RestTemplate
     // This is left for the detectors, for now
-    FixCandidateSearcher<T> rtSearcher =
-        new FixCandidateSearcher.Builder<T>()
+    LegacyFixCandidateSearcher<T> rtSearcher =
+        new LegacyFixCandidateSearcher.Builder<T>()
             // is method with name
             .withMatcher(mce -> mce.isMethodCallWithName("exchange"))
             // has a scope
@@ -153,7 +153,7 @@ private boolean hardenRT(final CompilationUnit cu, final MethodCallExpr call) {
    * issues, and a fixer predicate, that returns true if the change is successful.
    */
   private <T> Pair<List<CodemodChange>, List<UnfixedFinding>> searchAndFix(
-      final FixCandidateSearcher<T> searcher,
+      final LegacyFixCandidateSearcher<T> searcher,
       final BiPredicate<CompilationUnit, MethodOrConstructor> fixer,
       final CompilationUnit cu,
       final String path,
@@ -166,7 +166,7 @@ private <T> Pair<List<CodemodChange>, List<UnfixedFinding>> searchAndFix(
     List<CodemodChange> changes = new ArrayList<>();
     List<UnfixedFinding> unfixedFindings = new ArrayList<>();
 
-    FixCandidateSearchResults<T> results =
+    LegacyFixCandidateSearchResults<T> results =
         searcher.search(
             cu,
             path,
@@ -177,7 +177,7 @@ private <T> Pair<List<CodemodChange>, List<UnfixedFinding>> searchAndFix(
             getEndLine,
             getStartColumn);
 
-    for (FixCandidate<T> candidate : results.fixCandidates()) {
+    for (LegacyFixCandidate<T> candidate : results.fixCandidates()) {
       MethodOrConstructor call = candidate.call();
       List<T> issues = candidate.issues();
       if (fixer.test(cu, call)) {

core-codemods/src/main/java/io/codemodder/codemods/remediators/weakrandom/DefaultWeakRandomRemediator.java

@@ -50,7 +50,7 @@ public <T> CodemodFileScanningResult remediateAll(
                 detectorRule,
                 path,
                 getLine.apply(issue),
-                RemediationMessages.multipleCallsFound));
+                RemediationMessages.multipleNodesFound));
         continue;
       } else if (unsafeRandoms.isEmpty()) {
         unfixedFindings.add(
@@ -59,7 +59,7 @@ public <T> CodemodFileScanningResult remediateAll(
                 detectorRule,
                 path,
                 getLine.apply(issue),
-                RemediationMessages.noCallsAtThatLocation));
+                RemediationMessages.noNodesAtThatLocation));
         continue;
       }
 

framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultFixCandidateSearcher.java

@@ -3,23 +3,23 @@
 import com.github.javaparser.Position;
 import com.github.javaparser.ast.CompilationUnit;
 import com.github.javaparser.ast.Node;
-import com.github.javaparser.ast.expr.MethodCallExpr;
-import com.github.javaparser.ast.expr.ObjectCreationExpr;
 import io.codemodder.codetf.DetectorRule;
 import io.codemodder.codetf.UnfixedFinding;
 import java.util.*;
 import java.util.function.Function;
 import java.util.function.Predicate;
 import org.jetbrains.annotations.VisibleForTesting;
 
+/**
+ * Maps issues of type T to relevant nodes in the AST. Relevant nodes are decided with matchers.
+ *
+ * @param <T>
+ */
 final class DefaultFixCandidateSearcher<T> implements FixCandidateSearcher<T> {
 
-  private final List<Predicate<MethodOrConstructor>> matchers;
-  private final String methodName;
+  private final List<Predicate<Node>> matchers;
 
-  DefaultFixCandidateSearcher(
-      final String methodName, final List<Predicate<MethodOrConstructor>> matchers) {
-    this.methodName = methodName;
+  DefaultFixCandidateSearcher(final List<Predicate<Node>> matchers) {
     this.matchers = matchers;
   }
 
@@ -31,61 +31,58 @@ public FixCandidateSearchResults<T> search(
       final List<T> issuesForFile,
       final Function<T, String> getKey,
       final Function<T, Integer> getStartLine,
-      final Function<T, Integer> getEndLine,
-      final Function<T, Integer> getColumn) {
+      final Function<T, Optional<Integer>> getEndLine,
+      final Function<T, Optional<Integer>> getColumn) {
 
     List<UnfixedFinding> unfixedFindings = new ArrayList<>();
 
-    List<MethodOrConstructor> calls =
+    List<Node> nodes =
         cu.findAll(Node.class).stream()
-            // limit to just the interesting nodes for us
-            .filter(n -> n instanceof MethodCallExpr || n instanceof ObjectCreationExpr)
-            // turn them into our convenience type
-            .map(MethodOrConstructor::new)
-            // don't find calls we may have added -- you can pick these out by them not having a
-            // range yet
-            .filter(MethodOrConstructor::hasRange)
-            // filter by method name if one is provided in the search
-            .filter(mce -> methodName == null || mce.isMethodCallWithName(methodName))
             // filter by matchers
-            .filter(mce -> matchers.stream().allMatch(m -> m.test(mce)))
+            .filter(n -> matchers.stream().allMatch(m -> m.test(n)))
             .toList();
 
-    Map<MethodOrConstructor, List<T>> fixCandidateToIssueMapping = new HashMap<>();
+    Map<Node, List<T>> fixCandidateToIssueMapping = new HashMap<>();
 
     for (T issue : issuesForFile) {
       String findingId = getKey.apply(issue);
       int issueStartLine = getStartLine.apply(issue);
-      Integer issueEndLine = getEndLine.apply(issue);
-      Integer column = getColumn.apply(issue);
-      List<MethodOrConstructor> callsForIssue =
-          calls.stream()
-              .filter(MethodOrConstructor::hasRange)
+      Optional<Integer> maybeEndLine = getEndLine.apply(issue);
+      Optional<Integer> maybeColumn = getColumn.apply(issue);
+      List<Node> nodesForIssue =
+          nodes.stream()
               .filter(
-                  mce -> {
-                    int callStartLine = mce.getRange().begin.line;
-                    return matches(issueStartLine, issueEndLine, callStartLine);
+                  n -> {
+                    int nodeStartLine = n.getRange().orElseThrow().begin.line;
+                    // if issue end line info is present, match the node line with the issue range
+                    return maybeEndLine
+                        .map(issueEndLine -> matches(issueStartLine, nodeStartLine, issueEndLine))
+                        .orElse(matches(issueStartLine, nodeStartLine));
                   })
+              // if column info is present, check if the column is contained in the node's range
+              .filter(
+                  n ->
+                      maybeColumn
+                          .map(
+                              column ->
+                                  n.getRange()
+                                      .orElseThrow()
+                                      .contains(new Position(issueStartLine, column)))
+                          .orElse(true))
               .toList();
-      if (callsForIssue.size() > 1 && column != null) {
-        callsForIssue =
-            callsForIssue.stream()
-                .filter(mce -> mce.getRange().contains(new Position(issueStartLine, column)))
-                .toList();
-      }
-      if (callsForIssue.isEmpty()) {
+      if (nodesForIssue.isEmpty()) {
         unfixedFindings.add(
             new UnfixedFinding(
-                findingId, rule, path, issueStartLine, RemediationMessages.noCallsAtThatLocation));
+                findingId, rule, path, issueStartLine, RemediationMessages.noNodesAtThatLocation));
         continue;
-      } else if (callsForIssue.size() > 1) {
+      } else if (nodesForIssue.size() > 1) {
         unfixedFindings.add(
             new UnfixedFinding(
-                findingId, rule, path, issueStartLine, RemediationMessages.multipleCallsFound));
+                findingId, rule, path, issueStartLine, RemediationMessages.multipleNodesFound));
         continue;
       }
-      MethodOrConstructor call = callsForIssue.get(0);
-      fixCandidateToIssueMapping.computeIfAbsent(call, k -> new ArrayList<>()).add(issue);
+      Node node = nodesForIssue.get(0);
+      fixCandidateToIssueMapping.computeIfAbsent(node, k -> new ArrayList<>()).add(issue);
     }
 
     List<FixCandidate<T>> fixCandidates =
@@ -108,13 +105,15 @@ public List<FixCandidate<T>> fixCandidates() {
 
   @VisibleForTesting
   static boolean matches(
-      final int issueStartLine, final Integer issueEndLine, final int startCallLine) {
-    // if the issue spans multiple lines, the call must be within that range
-    if (issueEndLine != null) {
-      return isInBetween(startCallLine, issueStartLine, issueEndLine);
-    }
-    // if the issue is on a single line, the call must be on that line
-    return startCallLine == issueStartLine;
+      final int issueStartLine, final int startNodeLine, final int issueEndLine) {
+    // if the issue spans multiple lines, the node must be within that range
+    return matches(issueStartLine, startNodeLine)
+        && isInBetween(startNodeLine, issueStartLine, issueEndLine);
+  }
+
+  static boolean matches(final int issueStartLine, final int startNodeLine) {
+    // if the issue is on a single line, the node must be on that line
+    return startNodeLine == issueStartLine;
   }
 
   private static boolean isInBetween(int number, int lowerBound, int upperBound) {

framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultLegacyFixCandidateSearcher.java

@@ -0,0 +1,124 @@
+package io.codemodder.remediation;
+
+import com.github.javaparser.Position;
+import com.github.javaparser.ast.CompilationUnit;
+import com.github.javaparser.ast.Node;
+import com.github.javaparser.ast.expr.MethodCallExpr;
+import com.github.javaparser.ast.expr.ObjectCreationExpr;
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.codetf.UnfixedFinding;
+import java.util.*;
+import java.util.function.Function;
+import java.util.function.Predicate;
+import org.jetbrains.annotations.VisibleForTesting;
+
+@Deprecated
+final class DefaultLegacyFixCandidateSearcher<T> implements LegacyFixCandidateSearcher<T> {
+
+  private final List<Predicate<MethodOrConstructor>> matchers;
+  private final String methodName;
+
+  DefaultLegacyFixCandidateSearcher(
+      final String methodName, final List<Predicate<MethodOrConstructor>> matchers) {
+    this.methodName = methodName;
+    this.matchers = matchers;
+  }
+
+  @Override
+  public LegacyFixCandidateSearchResults<T> search(
+      final CompilationUnit cu,
+      final String path,
+      final DetectorRule rule,
+      final List<T> issuesForFile,
+      final Function<T, String> getKey,
+      final Function<T, Integer> getStartLine,
+      final Function<T, Integer> getEndLine,
+      final Function<T, Integer> getColumn) {
+
+    List<UnfixedFinding> unfixedFindings = new ArrayList<>();
+
+    List<MethodOrConstructor> calls =
+        cu.findAll(Node.class).stream()
+            // limit to just the interesting nodes for us
+            .filter(n -> n instanceof MethodCallExpr || n instanceof ObjectCreationExpr)
+            // turn them into our convenience type
+            .map(MethodOrConstructor::new)
+            // don't find calls we may have added -- you can pick these out by them not having a
+            // range yet
+            .filter(MethodOrConstructor::hasRange)
+            // filter by method name if one is provided in the search
+            .filter(mce -> methodName == null || mce.isMethodCallWithName(methodName))
+            // filter by matchers
+            .filter(mce -> matchers.stream().allMatch(m -> m.test(mce)))
+            .toList();
+
+    Map<MethodOrConstructor, List<T>> fixCandidateToIssueMapping = new HashMap<>();
+
+    for (T issue : issuesForFile) {
+      String findingId = getKey.apply(issue);
+      int issueStartLine = getStartLine.apply(issue);
+      Integer issueEndLine = getEndLine.apply(issue);
+      Integer column = getColumn.apply(issue);
+      List<MethodOrConstructor> callsForIssue =
+          calls.stream()
+              .filter(MethodOrConstructor::hasRange)
+              .filter(
+                  mce -> {
+                    int callStartLine = mce.getRange().begin.line;
+                    return matches(issueStartLine, issueEndLine, callStartLine);
+                  })
+              .toList();
+      if (callsForIssue.size() > 1 && column != null) {
+        callsForIssue =
+            callsForIssue.stream()
+                .filter(mce -> mce.getRange().contains(new Position(issueStartLine, column)))
+                .toList();
+      }
+      if (callsForIssue.isEmpty()) {
+        unfixedFindings.add(
+            new UnfixedFinding(
+                findingId, rule, path, issueStartLine, RemediationMessages.noNodesAtThatLocation));
+        continue;
+      } else if (callsForIssue.size() > 1) {
+        unfixedFindings.add(
+            new UnfixedFinding(
+                findingId, rule, path, issueStartLine, RemediationMessages.multipleNodesFound));
+        continue;
+      }
+      MethodOrConstructor call = callsForIssue.get(0);
+      fixCandidateToIssueMapping.computeIfAbsent(call, k -> new ArrayList<>()).add(issue);
+    }
+
+    List<LegacyFixCandidate<T>> legacyFixCandidates =
+        fixCandidateToIssueMapping.entrySet().stream()
+            .map(entry -> new LegacyFixCandidate<>(entry.getKey(), entry.getValue()))
+            .toList();
+
+    return new LegacyFixCandidateSearchResults<T>() {
+      @Override
+      public List<UnfixedFinding> unfixableFindings() {
+        return unfixedFindings;
+      }
+
+      @Override
+      public List<LegacyFixCandidate<T>> fixCandidates() {
+        return legacyFixCandidates;
+      }
+    };
+  }
+
+  @VisibleForTesting
+  static boolean matches(
+      final int issueStartLine, final Integer issueEndLine, final int startCallLine) {
+    // if the issue spans multiple lines, the call must be within that range
+    if (issueEndLine != null) {
+      return isInBetween(startCallLine, issueStartLine, issueEndLine);
+    }
+    // if the issue is on a single line, the call must be on that line
+    return startCallLine == issueStartLine;
+  }
+
+  private static boolean isInBetween(int number, int lowerBound, int upperBound) {
+    return number >= lowerBound && number <= upperBound;
+  }
+}