Skip to content

Commit

Permalink
Add tests and more stable behavior when seeing Maven failure (#476)
Browse files Browse the repository at this point in the history
  • Loading branch information
nahsra authored Nov 24, 2024
1 parent e261a7f commit 9e68b29
Show file tree
Hide file tree
Showing 6 changed files with 153 additions and 47 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand All @@ -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(
Expand All @@ -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;
Expand All @@ -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);
}
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Original file line number Diff line number Diff line change
@@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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");
Expand All @@ -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());
}
}

Expand Down

0 comments on commit 9e68b29

Please sign in to comment.