-
Notifications
You must be signed in to change notification settings - Fork 72
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.
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
Adding guard when annotating linkage errors #2196
Conversation
linkageProblem.setCause(UnknownCause.getInstance()); | ||
} else { | ||
Artifact artifactInSubtree = entryInSubtree.getArtifact(); | ||
DependencyPath pathToSourceEntry = rootResult.getDependencyPaths(sourceEntry).get(0); |
There was a problem hiding this comment.
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.
// Different version of that artifact is selected in rootResult | ||
ImmutableList<DependencyPath> pathToSelectedArtifact = | ||
rootResult.getDependencyPaths(selectedEntry); | ||
if (pathToSelectedArtifact.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another guard to avoid ArrayIndexOutOfBoundsException
Artifact artifactInSubtree = entryInSubtree.getArtifact(); | ||
ImmutableList<DependencyPath> dependencyPathsToSource = | ||
rootResult.getDependencyPaths(sourceEntry); | ||
if (dependencyPathsToSource.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main purpose of this PR.
} | ||
|
||
// For artifacts with classifiers, there can be multiple resolved artifacts for one node | ||
for (ResolvedArtifact artifact : moduleArtifacts) { | ||
// parentPath is null for the first item |
There was a problem hiding this comment.
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.
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 when there are problems, such as classPathBuilder unable to resolve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// fail the entire process when there are problems, such as classPathBuilder unable to resolve | |
// fail the entire process if there are problems, such as classPathBuilder unable to resolve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Symbol symbol = linkageProblem.getSymbol(); | ||
ClassPathEntry entryInSubtree = subtreeResult.findEntryBySymbol(symbol); | ||
if (entryInSubtree == null) { | ||
linkageProblem.setCause(UnknownCause.getInstance()); |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea. Updated.
artifactInSubtree.getGroupId(), artifactInSubtree.getArtifactId()); | ||
if (selectedEntry != null) { | ||
Artifact selectedArtifact = selectedEntry.getArtifact(); | ||
if (!selectedArtifact.getVersion().equals(artifactInSubtree.getVersion())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could probably combine the guard clause you created here with the other error condition:
ClassPathEntry selectedEntry =
rootResult.findEntryById(
artifactInSubtree.getGroupId(), artifactInSubtree.getArtifactId());
if (selectedEntry != null) {
Artifact selectedArtifact = selectedEntry.getArtifact();
ImmutableList<DependencyPath> pathToSelectedArtifact =
rootResult.getDependencyPaths(selectedEntry);
if (pathToSelectedArtifact.isEmpty() || !selectedArtifact.getVersion().equals(artifactInSubtree.getVersion()) {
linkageProblem.setCause(UnknownCause.getInstance());
return;
}
linkageProblem.setCause(
new DependencyConflict(
linkageProblem, pathToSelectedArtifact.get(0), pathToUnselectedEntry));
} else {
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be logically the same but the current if-statement delivers better meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
A user reported that pom-packaging artifact is not working correctly.