Skip to content

Adding guard when annotating linkage errors #2196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
suztomo merged 4 commits into from
Aug 20, 2021
Merged

Adding guard when annotating linkage errors #2196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb99668
Adding guard when annotating linkage errors
suztomo Aug 20, 2021
470ffaf
Test that fails
suztomo Aug 20, 2021
6224def
Fixed LinkageCheckTask to check dependencies of pom artifacts
suztomo Aug 20, 2021
0284b91
Unnested if-statement
suztomo Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 82 additions & 52 deletions ...c/main/java/com/google/cloud/tools/opensource/classpath/LinkageProblemCauseAnnotator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.cloud.tools.opensource.dependencies.DependencyPath;
import com.google.common.collect.ImmutableList;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
import java.util.logging.Logger;
import org.eclipse.aether.artifact.Artifact;

/** Annotates {@link LinkageProblem}s with {@link LinkageProblemCause}s. */
public final class LinkageProblemCauseAnnotator {
private static final Logger logger =
Logger.getLogger(LinkageProblemCauseAnnotator.class.getName());

private LinkageProblemCauseAnnotator() {}

Expand All @@ -35,74 +39,100 @@ private LinkageProblemCauseAnnotator() {}
* @param classPathBuilder class path builder to resolve dependency graphs
* @param rootResult the class path used for generating the linkage problems
* @param linkageProblems linkage problems to annotate
* @throws IOException when there is a problem reading JAR files
*/
public static void annotate(
ClassPathBuilder classPathBuilder,
ClassPathResult rootResult,
Iterable<LinkageProblem> linkageProblems)
throws IOException {
Iterable<LinkageProblem> linkageProblems) {
checkNotNull(classPathBuilder);
checkNotNull(rootResult);
checkNotNull(linkageProblems);

Map<Artifact, ClassPathResult> cache = new HashMap<>();
Map<Artifact, ClassPathResult> artifactResolutionCache = new HashMap<>();
for (LinkageProblem linkageProblem : linkageProblems) {
ClassFile sourceClass = linkageProblem.getSourceClass();
ClassPathEntry sourceEntry = sourceClass.getClassPathEntry();
// Annotating linkage errors is a nice-to-have feature for Linkage Checker plugins. Let's not
// fail the entire process if there are problems, such as classPathBuilder unable to resolve
// one artifact or to return correct dependency tree.
try {
annotateProblem(classPathBuilder, rootResult, artifactResolutionCache, linkageProblem);
} catch (Exception ex) {
logger.warning("Failed to annotate: " + linkageProblem);
linkageProblem.setCause(UnknownCause.getInstance());
}
}
}

Artifact sourceArtifact = sourceEntry.getArtifact();
private static void annotateProblem(
ClassPathBuilder classPathBuilder,
ClassPathResult rootResult,
Map<Artifact, ClassPathResult> artifactResolutionCache,
LinkageProblem linkageProblem)
throws IOException {
ClassFile sourceClass = linkageProblem.getSourceClass();
ClassPathEntry sourceEntry = sourceClass.getClassPathEntry();

ClassPathResult subtreeResult = cache.get(sourceArtifact);
if (subtreeResult == null) {
// Resolves the dependency graph with the source artifact at the root.
subtreeResult = classPathBuilder.resolveWithMaven(sourceArtifact);
cache.put(sourceArtifact, subtreeResult);
}
Artifact sourceArtifact = sourceEntry.getArtifact();

Symbol symbol = linkageProblem.getSymbol();
ClassPathEntry entryInSubtree = subtreeResult.findEntryBySymbol(symbol);
if (entryInSubtree == null) {
linkageProblem.setCause(UnknownCause.getInstance());
} else {
Artifact artifactInSubtree = entryInSubtree.getArtifact();
DependencyPath pathToSourceEntry = rootResult.getDependencyPaths(sourceEntry).get(0);

@suztomo suztomo Aug 20, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A user reported that this threw ArrayIndexOutOfBoundsException.

DependencyPath pathFromSourceEntryToUnselectedEntry =
subtreeResult.getDependencyPaths(entryInSubtree).get(0);
DependencyPath pathToUnselectedEntry =
pathToSourceEntry.concat(pathFromSourceEntryToUnselectedEntry);
ClassPathResult subtreeResult = artifactResolutionCache.get(sourceArtifact);
if (subtreeResult == null) {
// Resolves the dependency graph with the source artifact at the root.
subtreeResult = classPathBuilder.resolveWithMaven(sourceArtifact);
artifactResolutionCache.put(sourceArtifact, subtreeResult);
}

ClassPathEntry selectedEntry =
rootResult.findEntryById(
artifactInSubtree.getGroupId(), artifactInSubtree.getArtifactId());
if (selectedEntry != null) {
Artifact selectedArtifact = selectedEntry.getArtifact();
if (!selectedArtifact.getVersion().equals(artifactInSubtree.getVersion())) {
// Different version of that artifact is selected in rootResult
linkageProblem.setCause(
new DependencyConflict(
linkageProblem,
rootResult.getDependencyPaths(selectedEntry).get(0),
pathToUnselectedEntry));
} else {
// A linkage error was already there when sourceArtifact was built.
linkageProblem.setCause(UnknownCause.getInstance());
}
} else {
// No artifact that matches groupId and artifactId in rootResult.
Symbol symbol = linkageProblem.getSymbol();
ClassPathEntry entryInSubtree = subtreeResult.findEntryBySymbol(symbol);
if (entryInSubtree == null) {
linkageProblem.setCause(UnknownCause.getInstance());

@dzou dzou Aug 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more clear to early return and exit the method if entryInSubtree == null and then unindent everything in the else-block. (Fewer levels of indentation).

if (entryInSubtree == null) {
  linkageProblem.setCause(UnknownCause.getInstance());
  return;
}

@suztomo suztomo Aug 20, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea. Updated.

return;
}
Artifact artifactInSubtree = entryInSubtree.getArtifact();
ImmutableList<DependencyPath> dependencyPathsToSource =
rootResult.getDependencyPaths(sourceEntry);
if (dependencyPathsToSource.isEmpty()) {
// When an artifact is excluded, it's possible to have no dependency path to sourceEntry.
linkageProblem.setCause(UnknownCause.getInstance());
return;
}
DependencyPath pathToSourceEntry = dependencyPathsToSource.get(0);
DependencyPath pathFromSourceEntryToUnselectedEntry =
subtreeResult.getDependencyPaths(entryInSubtree).get(0);
DependencyPath pathToUnselectedEntry =
pathToSourceEntry.concat(pathFromSourceEntryToUnselectedEntry);

// Checking exclusion elements in the dependency path
Artifact excludingArtifact =
pathToSourceEntry.findExclusion(
artifactInSubtree.getGroupId(), artifactInSubtree.getArtifactId());
if (excludingArtifact != null) {
linkageProblem.setCause(
new ExcludedDependency(pathToUnselectedEntry, excludingArtifact));
} else {
linkageProblem.setCause(new MissingDependency(pathToUnselectedEntry));
}
ClassPathEntry selectedEntry =
rootResult.findEntryById(
artifactInSubtree.getGroupId(), artifactInSubtree.getArtifactId());
if (selectedEntry != null) {
Artifact selectedArtifact = selectedEntry.getArtifact();
if (!selectedArtifact.getVersion().equals(artifactInSubtree.getVersion())) {
// Different version of that artifact is selected in rootResult
ImmutableList<DependencyPath> pathToSelectedArtifact =
rootResult.getDependencyPaths(selectedEntry);
if (pathToSelectedArtifact.isEmpty()) {
linkageProblem.setCause(UnknownCause.getInstance());
return;
}
linkageProblem.setCause(
new DependencyConflict(
linkageProblem, pathToSelectedArtifact.get(0), pathToUnselectedEntry));
} else {
// A linkage error was already there when sourceArtifact was built.
linkageProblem.setCause(UnknownCause.getInstance());
}
} else {
// No artifact that matches groupId and artifactId in rootResult.

// Checking exclusion elements in the dependency path
Artifact excludingArtifact =
pathToSourceEntry.findExclusion(
artifactInSubtree.getGroupId(), artifactInSubtree.getArtifactId());
if (excludingArtifact != null) {
linkageProblem.setCause(new ExcludedDependency(pathToUnselectedEntry, excludingArtifact));
} else {
linkageProblem.setCause(new MissingDependency(pathToUnselectedEntry));
}
}
}

}
22 changes: 22 additions & 0 deletions ...st/java/com/google/cloud/tools/opensource/classpath/LinkageProblemCauseAnnotatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,4 +198,26 @@ public void testAnnotate_dependencyInSpringRepository() throws IOException, Repo
"org.reactivestreams:reactive-streams:1.0.3",
Artifacts.toCoordinates(conflict.getPathToArtifactThruSource().getLeaf()));
}

@Test
public void testAnnotate_missingArtifactInTree() throws IOException, RepositoryException {
// The dependency tree of the google-api-client does not contain class "com.Foo"
ClassPathBuilder builder = new ClassPathBuilder();
ClassPathResult classPathResult =
builder.resolve(
ImmutableList.of(new DefaultArtifact("com.google.api-client:google-api-client:1.27.0")),
false,
DependencyMediation.MAVEN);

ClassPathEntry googleApiClient = classPathResult.getClassPath().get(0);
ClassNotFoundProblem dummyProblem =
new ClassNotFoundProblem(
new ClassFile(googleApiClient, "com.Foo"), new ClassSymbol("com.Bar"));

LinkageProblemCauseAnnotator.annotate(
classPathBuilder, classPathResult, ImmutableSet.of(dummyProblem));

LinkageProblemCause cause = dummyProblem.getCause();
assertTrue(cause instanceof UnknownCause);
}
}
34 changes: 34 additions & 0 deletions ...alTest/groovy/com/google/cloud/tools/dependencies/gradle/BuildStatusFunctionalTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,4 +224,38 @@ class BuildStatusFunctionalTest extends Specification {
result.output.count("Circular dependency for: com.fasterxml.jackson:jackson-bom:2.12.1") == 1
result.task(":linkageCheck").outcome == TaskOutcome.FAILED
}

def "can handle artifacts with pom packaging"() {
buildFile << """
repositories {
mavenCentral()
}

dependencies {
// This artifact is pom-packaging and does not have JAR artifacts
compile 'org.eclipse.jetty.aggregate:jetty-all:9.4.7.v20170914'
}

linkageChecker {
configurations = ['compile']
}
"""

when:
def result = GradleRunner.create()
.withProjectDir(testProjectDir.root)
.withArguments('linkageCheck', '--stacktrace')
.withPluginClasspath()
.buildAndFail()

then:
result.output.contains("Task :linkageCheck")
result.output.contains(
"The valid symbol is in org.eclipse.jetty.alpn:alpn-api:jar:1.1.3.v20160715 at "+
"org.eclipse.jetty.aggregate:jetty-all:9.4.7.v20170914 (compile) / "+
"org.eclipse.jetty:jetty-alpn-client:9.4.7.v20170914 (compile) / "+
"org.eclipse.jetty.alpn:alpn-api:1.1.3.v20160715 (provided) but it was not selected "+
"because the path contains a provided-scope dependency")
result.task(":linkageCheck").outcome == TaskOutcome.FAILED
}
}
33 changes: 23 additions & 10 deletions gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.MultimapBuilder;
import java.io.File;
import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;
Expand Down Expand Up @@ -314,6 +315,20 @@ private String dependencyPathToArtifacts(
return output.toString();
}

private static Artifact artifactWithPomFrom(ResolvedDependency resolvedDependency) {
ModuleVersionIdentifier moduleVersionId = resolvedDependency.getModule().getId();
DefaultArtifact artifact =
new DefaultArtifact(
moduleVersionId.getGroup(),
moduleVersionId.getName(),
null,
"pom",
moduleVersionId.getVersion(),
null,
(File) null); // no JAR file
return artifact;
}

private static Artifact artifactFrom(
ResolvedDependency resolvedDependency, ResolvedArtifact resolvedArtifact) {
ModuleVersionIdentifier moduleVersionId = resolvedDependency.getModule().getId();
Expand Down Expand Up @@ -357,6 +372,7 @@ private DependencyGraph createDependencyGraph(ResolvedConfiguration configuratio
PathToNode<ResolvedDependency> item = queue.poll();
ResolvedDependency node = item.getNode();

// Never null
DependencyPath parentPath = item.getParentPath();

// If there are multiple artifacts (with different classifiers) in this node, then the path is
Expand All @@ -372,18 +388,15 @@ private DependencyGraph createDependencyGraph(ResolvedConfiguration configuratio
getLogger()
.warn(
"The dependency node " + node.getName() + " does not have any artifact. Skipping.");
continue;
path = parentPath.append(new Dependency(artifactWithPomFrom(node), "compile"));
} else {
// For artifacts with classifiers, there can be multiple resolved artifacts for one node
for (ResolvedArtifact artifact : moduleArtifacts) {
path = parentPath.append(dependencyFrom(node, artifact));
graph.addPath(path);
}
}

// For artifacts with classifiers, there can be multiple resolved artifacts for one node
for (ResolvedArtifact artifact : moduleArtifacts) {
// parentPath is null for the first item

@suztomo suztomo Aug 20, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the first item's parentPath is actually set line 352. It's never null.

path =
parentPath == null
? new DependencyPath(artifactFrom(node, artifact))
: parentPath.append(dependencyFrom(node, artifact));
graph.addPath(path);
}

for (ResolvedDependency child : node.getChildren()) {
if (visited.add(child)) {
Expand Down