Add tests and more stable behavior when seeing Maven failure

framework/codemodder-base/src/main/java/io/codemodder/CodemodPackageUpdateResult.java

@@ -6,13 +6,16 @@
 import java.util.List;
 import java.util.Set;
 
-/** A */
+/** A model of a codemod's updating of packages. */
 public interface CodemodPackageUpdateResult {
 
+  /** A structured description of what we were able to do. */
   List<CodeTFPackageAction> packageActions();
 
+  /** The changes that were made to the manifest file. */
   List<CodeTFChangesetEntry> manifestChanges();
 
+  /** The set of files that we attempted to update, but failed. */
   Set<Path> filesFailedToChange();
 
   static CodemodPackageUpdateResult from(

plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/DefaultPOMDependencyUpdater.java

@@ -20,7 +20,7 @@
 import org.slf4j.LoggerFactory;
 
 /** POMDependencyUpdater is responsible for updating Maven POM files with new dependencies. */
-class DefaultPOMDependencyUpdater implements POMDependencyUpdater {
+final class DefaultPOMDependencyUpdater implements POMDependencyUpdater {
   private final PomFileFinder pomFileFinder;
 
   private Optional<Path> maybePomFile;
@@ -63,17 +63,19 @@ class DefaultPOMDependencyUpdater implements POMDependencyUpdater {
    * @param file The specific POM file to update.
    * @param dependencies The list of new dependencies to be added.
    * @return A DependencyUpdateResult containing information about the update process.
-   * @throws IOException If an I/O error occurs.
-   * @throws XMLStreamException If an error occurs during XML stream processing.
-   * @throws DocumentException If an error occurs while parsing the document.
-   * @throws URISyntaxException If there is an issue with the URI syntax.
    */
   @NotNull
   public DependencyUpdateResult execute(
-      final Path projectDir, final Path file, final List<DependencyGAV> dependencies)
-      throws IOException, XMLStreamException, DocumentException, URISyntaxException {
-    if (isEmptyPomFile(projectDir, file)) {
-      LOG.trace("Pom file was empty for {}", file);
+      final Path projectDir, final Path file, final List<DependencyGAV> dependencies) {
+
+    // if we can't find a pom, we want to skip this file
+    try {
+      if (isEmptyPomFile(projectDir, file)) {
+        LOG.trace("Pom file was empty for {}", file);
+        return DependencyUpdateResult.EMPTY_UPDATE;
+      }
+    } catch (IOException e) {
+      LOG.warn("Not all Maven dependencies could be found", e);
       return DependencyUpdateResult.EMPTY_UPDATE;
     }
 
@@ -84,7 +86,17 @@ public DependencyUpdateResult execute(
     skippedDependencies = new ArrayList<>();
     injectedDependencies = new ArrayList<>();
     erroredFiles = new LinkedHashSet<>();
-    foundDependenciesMapped = new AtomicReference<>(pomOperator.getAllFoundDependencies());
+
+    // get all the existing dependencies, if we're not able to find them, we can't update
+    Collection<DependencyGAV> allFoundDependencies;
+    try {
+      allFoundDependencies = pomOperator.getAllFoundDependencies();
+    } catch (IOException | DocumentException | XMLStreamException | URISyntaxException e) {
+      LOG.warn("Problem calculating all dependencies", e);
+      return DependencyUpdateResult.EMPTY_UPDATE;
+    }
+
+    foundDependenciesMapped = new AtomicReference<>(allFoundDependencies);
     LOG.trace("Beginning dependency set size: {}", foundDependenciesMapped.get().size());
 
     dependencies.forEach(
@@ -97,7 +109,6 @@ public DependencyUpdateResult execute(
             }
 
             final ProjectModel modifiedProjectModel = pomOperator.addDependency(newDependencyGAV);
-
             if (modifiedProjectModel == null) {
               LOG.trace("POM file didn't need modification or it failed?");
               return;
@@ -107,16 +118,14 @@ public DependencyUpdateResult execute(
 
             modifyPomFiles(projectDir, modifiedProjectModel, newDependencyGAV);
 
+            // recalculate the dependencies after our addition
             final Collection<DependencyGAV> newDependencySet =
                 pomOperator.getAllFoundDependencies();
-
             LOG.trace("New dependency set size: {}", newDependencySet.size());
-
             foundDependenciesMapped.set(newDependencySet);
-          } catch (DocumentException | IOException | URISyntaxException | XMLStreamException e) {
-            LOG.error("Unexpected problem getting on pom operator", e);
-            throw new MavenProvider.DependencyUpdateException(
-                "Failure while executing pom operator: ", e);
+
+          } catch (Exception e) {
+            LOG.error("Problem  getting on pom operator", e);
           }
         });
 

plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/MavenProvider.java

@@ -57,17 +57,21 @@ public void modify(final Path path, final byte[] contents) throws IOException {
   private final POMDependencyUpdater pomDependencyUpdater;
   private final PomFileFinder pomFileFinder;
 
+  MavenProvider(
+      final PomFileFinder pomFileFinder, final POMDependencyUpdater pomDependencyUpdater) {
+    this.pomFileFinder = Objects.requireNonNull(pomFileFinder);
+    this.pomDependencyUpdater = Objects.requireNonNull(pomDependencyUpdater);
+  }
+
   MavenProvider(
       final PomModifier pomModifier,
       final PomFileFinder pomFileFinder,
       final DependencyDescriptor dependencyDescriptor,
       final ArtifactInjectionPositionFinder positionFinder) {
-    Objects.requireNonNull(pomModifier);
-    Objects.requireNonNull(pomFileFinder);
-    this.pomFileFinder = pomFileFinder;
-    this.pomDependencyUpdater =
+    this(
+        pomFileFinder,
         new DefaultPOMDependencyUpdater(
-            new CodeTFGenerator(positionFinder, dependencyDescriptor), pomFileFinder, pomModifier);
+            new CodeTFGenerator(positionFinder, dependencyDescriptor), pomFileFinder, pomModifier));
   }
 
   MavenProvider(
@@ -113,20 +117,17 @@ public Optional<Path> findForFile(final Path projectDir, final Path file) throws
     }
   }
 
+  /**
+   * This method must not throw exception -- it should capture failures in its model and bubble up
+   * normal results.
+   */
   @Override
   public DependencyUpdateResult updateDependencies(
       final Path projectDir, final Path file, final List<DependencyGAV> dependencies) {
-    try {
-      String dependenciesStr =
-          dependencies.stream().map(DependencyGAV::toString).collect(Collectors.joining(","));
-      LOG.trace("Updating dependencies for {} in {}: {}", file, projectDir, dependenciesStr);
-      final DependencyUpdateResult dependencyUpdateResult =
-          pomDependencyUpdater.execute(projectDir, file, dependencies);
-      LOG.trace("Dependency update result: {}", dependencyUpdateResult);
-      return dependencyUpdateResult;
-    } catch (Exception e) {
-      throw new DependencyUpdateException("Failure when updating dependencies", e);
-    }
+    String dependenciesStr =
+        dependencies.stream().map(DependencyGAV::toString).collect(Collectors.joining(","));
+    LOG.trace("Updating dependencies for {} in {}: {}", file, projectDir, dependenciesStr);
+    return pomDependencyUpdater.execute(projectDir, file, dependencies);
   }
 
   @Override

plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/POMDependencyUpdater.java

@@ -2,15 +2,10 @@
 
 import io.codemodder.DependencyGAV;
 import io.codemodder.DependencyUpdateResult;
-import java.io.IOException;
-import java.net.URISyntaxException;
 import java.nio.file.Path;
 import java.util.List;
-import javax.xml.stream.XMLStreamException;
-import org.dom4j.DocumentException;
 
 interface POMDependencyUpdater {
 
-  DependencyUpdateResult execute(Path projectDir, Path file, List<DependencyGAV> dependencies)
-      throws IOException, XMLStreamException, DocumentException, URISyntaxException;
+  DependencyUpdateResult execute(Path projectDir, Path file, List<DependencyGAV> dependencies);
 }

plugins/codemodder-plugin-maven/src/test/java/io/codemodder/plugins/maven/DefaultPOMDependencyUpdaterTest.java

@@ -0,0 +1,99 @@
+package io.codemodder.plugins.maven;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.*;
+
+import io.codemodder.DependencyDescriptor;
+import io.codemodder.DependencyGAV;
+import io.codemodder.DependencyUpdateResult;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.Optional;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+final class DefaultPOMDependencyUpdaterTest {
+
+  private ArtifactInjectionPositionFinder positionFinder;
+  private CodeTFGenerator codeTFGenerator;
+  private DependencyDescriptor dependencyDescriptor;
+  private PomFileFinder pomFileFinder;
+  private MavenProvider.PomModifier pomModifier;
+  private Path projectDir;
+  private Path pomPath;
+  private DefaultPOMDependencyUpdater updater;
+
+  @BeforeEach
+  void setup(@TempDir Path tempDir) throws IOException {
+    positionFinder = mock(ArtifactInjectionPositionFinder.class);
+    dependencyDescriptor = DependencyDescriptor.createMarkdownDescriptor();
+    pomFileFinder = mock(PomFileFinder.class);
+    pomModifier = mock(MavenProvider.PomModifier.class);
+    this.projectDir = tempDir;
+    build();
+  }
+
+  void build() throws IOException {
+    pomPath = projectDir.resolve("pom.xml");
+
+    if (!Files.exists(pomPath)) {
+      Files.createFile(pomPath);
+    }
+    Files.writeString(
+        pomPath,
+        """
+                <project>
+                    <modelVersion>4.0.0</modelVersion>
+                    <groupId>io.codemodder</groupId>
+                    <artifactId>test</artifactId>
+                    <version>1.0.0</version>
+                    <dependencies>
+
+                    </dependencies>
+                </project>
+                """);
+
+    when(pomFileFinder.findForFile(any(), any())).thenReturn(Optional.ofNullable(pomPath));
+    codeTFGenerator = new CodeTFGenerator(positionFinder, dependencyDescriptor);
+    when(positionFinder.find(any(), any())).thenReturn(7);
+    updater = new DefaultPOMDependencyUpdater(codeTFGenerator, pomFileFinder, pomModifier);
+  }
+
+  @Test
+  void it_works() {
+    DependencyUpdateResult result =
+        updater.execute(projectDir, pomPath, List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER));
+    assertThat(result.erroredFiles()).isEmpty();
+    assertThat(result.injectedPackages()).containsExactly(DependencyGAV.OWASP_XSS_JAVA_ENCODER);
+    assertThat(result.skippedPackages()).isEmpty();
+    assertThat(result.packageChanges()).hasSize(1);
+  }
+
+  @Test
+  void it_doesnt_propagate_update_exceptions_when_no_pom_found() throws IOException {
+    when(pomFileFinder.findForFile(any(), any()))
+        .thenThrow(new IOException("blows up during finding"));
+    DependencyUpdateResult result =
+        updater.execute(projectDir, pomPath, List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER));
+    assertThat(result.erroredFiles()).isEmpty();
+    assertThat(result.injectedPackages()).isEmpty();
+    assertThat(result.skippedPackages()).isEmpty();
+    assertThat(result.packageChanges()).isEmpty();
+  }
+
+  @Test
+  void it_doesnt_propagate_update_exceptions_when_pom_io_error() throws IOException {
+    build();
+    doThrow(new IOException("blows up during modification")).when(pomModifier).modify(any(), any());
+    DependencyUpdateResult result =
+        updater.execute(projectDir, pomPath, List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER));
+    assertThat(result.erroredFiles()).hasSize(1);
+    assertThat(result.injectedPackages()).isEmpty();
+    assertThat(result.skippedPackages()).isEmpty();
+    assertThat(result.packageChanges()).isEmpty();
+  }
+}

plugins/codemodder-plugin-maven/src/test/java/io/codemodder/plugins/maven/MavenProviderTest.java

@@ -153,7 +153,7 @@ void it_returns_changeset_when_no_issues() throws IOException {
 
     // we've updated all the poms, so we merge this with the pre-existing one change
     List<CodeTFChangesetEntry> changes = result.packageChanges();
-    assertThat(changes.size()).isEqualTo(2);
+    assertThat(changes).hasSize(2);
 
     // we injected all the dependencies, total success!
     assertThat(result.injectedPackages()).containsOnly(marsDependency1, marsDependency2);
@@ -204,15 +204,15 @@ void it_places_library_facts_in_correct_pom_place(
 
     // we only injected the toolkit -- verify that
     List<DependencyGAV> injectedPackages = result.injectedPackages();
-    assertThat(injectedPackages.size()).isEqualTo(1);
+    assertThat(injectedPackages).hasSize(1);
     DependencyGAV injectedPackage = injectedPackages.get(0);
     assertThat(injectedPackage).isEqualTo(DependencyGAV.JAVA_SECURITY_TOOLKIT);
 
     List<CodeTFChangesetEntry> changesets = result.packageChanges();
-    assertThat(changesets.size()).isEqualTo(1);
+    assertThat(changesets).hasSize(1);
     CodeTFChangesetEntry change = changesets.get(0);
     List<CodeTFChange> changes = change.getChanges();
-    assertThat(changes.size()).isEqualTo(1);
+    assertThat(changes).hasSize(1);
     CodeTFChange lineChange = changes.get(0);
     assertThat(lineChange.getDescription()).contains("License: ");
     assertThat(lineChange.getLineNumber()).isEqualTo(libraryFactsLineTarget);
@@ -402,7 +402,7 @@ void it_updates_poms_correctly() throws IOException {
         provider.updateDependencies(
             projectDir, module1Pom, List.of(marsDependency1, marsDependency2, venusDependency));
 
-    assertThat(result.packageChanges().size()).isEqualTo(3);
+    assertThat(result.packageChanges()).hasSize(3);
 
     CodeTFChangesetEntry changesetEntry = result.packageChanges().iterator().next();
     assertThat(changesetEntry.getPath()).isEqualTo("module1/pom.xml");
@@ -423,8 +423,7 @@ void it_finds_correct_poms() throws IOException {
             Pair.of(jspFile, Optional.of(rootPom)),
             Pair.of(irrelevantFile, Optional.of(rootPom)))) {
       Optional<Path> pom = pomFinder.findForFile(this.projectDir, pair.getLeft());
-      assertThat(pom.isPresent()).isTrue();
-      assertThat(pom.get()).isEqualTo(pair.getRight().get());
+      assertThat(pom).contains(pair.getRight().get());
     }
   }