From 9e68b295e5a2ac48fa8aedbfe1a800bb85c75d20 Mon Sep 17 00:00:00 2001 From: Arshan Dabirsiaghi Date: Sun, 24 Nov 2024 14:50:31 -0500 Subject: [PATCH] Add tests and more stable behavior when seeing Maven failure (#476) --- .../CodemodPackageUpdateResult.java | 5 +- .../maven/DefaultPOMDependencyUpdater.java | 43 ++++---- .../plugins/maven/MavenProvider.java | 33 ++++--- .../plugins/maven/POMDependencyUpdater.java | 7 +- .../DefaultPOMDependencyUpdaterTest.java | 99 +++++++++++++++++++ .../plugins/maven/MavenProviderTest.java | 13 ++- 6 files changed, 153 insertions(+), 47 deletions(-) create mode 100644 plugins/codemodder-plugin-maven/src/test/java/io/codemodder/plugins/maven/DefaultPOMDependencyUpdaterTest.java diff --git a/framework/codemodder-base/src/main/java/io/codemodder/CodemodPackageUpdateResult.java b/framework/codemodder-base/src/main/java/io/codemodder/CodemodPackageUpdateResult.java index 4bb498cdf..7f4dc2c54 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/CodemodPackageUpdateResult.java +++ b/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 packageActions(); + /** The changes that were made to the manifest file. */ List manifestChanges(); + /** The set of files that we attempted to update, but failed. */ Set filesFailedToChange(); static CodemodPackageUpdateResult from( diff --git a/plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/DefaultPOMDependencyUpdater.java b/plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/DefaultPOMDependencyUpdater.java index 8a2e4742e..0108c229e 100644 --- a/plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/DefaultPOMDependencyUpdater.java +++ b/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 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 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 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 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 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); } }); diff --git a/plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/MavenProvider.java b/plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/MavenProvider.java index 646f5dbf3..72dce755f 100644 --- a/plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/MavenProvider.java +++ b/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 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 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 diff --git a/plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/POMDependencyUpdater.java b/plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/POMDependencyUpdater.java index 038e6b0f7..f1b9685fe 100644 --- a/plugins/codemodder-plugin-maven/src/main/java/io/codemodder/plugins/maven/POMDependencyUpdater.java +++ b/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 dependencies) - throws IOException, XMLStreamException, DocumentException, URISyntaxException; + DependencyUpdateResult execute(Path projectDir, Path file, List dependencies); } diff --git a/plugins/codemodder-plugin-maven/src/test/java/io/codemodder/plugins/maven/DefaultPOMDependencyUpdaterTest.java b/plugins/codemodder-plugin-maven/src/test/java/io/codemodder/plugins/maven/DefaultPOMDependencyUpdaterTest.java new file mode 100644 index 000000000..6be8b0282 --- /dev/null +++ b/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, + """ + + 4.0.0 + io.codemodder + test + 1.0.0 + + + + + """); + + 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(); + } +} diff --git a/plugins/codemodder-plugin-maven/src/test/java/io/codemodder/plugins/maven/MavenProviderTest.java b/plugins/codemodder-plugin-maven/src/test/java/io/codemodder/plugins/maven/MavenProviderTest.java index 3fe5b7e3b..1193edbca 100644 --- a/plugins/codemodder-plugin-maven/src/test/java/io/codemodder/plugins/maven/MavenProviderTest.java +++ b/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 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 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 changesets = result.packageChanges(); - assertThat(changesets.size()).isEqualTo(1); + assertThat(changesets).hasSize(1); CodeTFChangesetEntry change = changesets.get(0); List 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 pom = pomFinder.findForFile(this.projectDir, pair.getLeft()); - assertThat(pom.isPresent()).isTrue(); - assertThat(pom.get()).isEqualTo(pair.getRight().get()); + assertThat(pom).contains(pair.getRight().get()); } }