Skip to content
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

fix(maven): return coordinates-only Package on parsePackage exception #8471

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(maven): return coordinates-only Package on parsePackage exception #8471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 20 additions & 1 deletion plugins/package-managers/maven/src/main/kotlin/utils/MavenDependencyHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.Issue
import org.ossreviewtoolkit.model.Package
import org.ossreviewtoolkit.model.PackageLinkage
import org.ossreviewtoolkit.model.RemoteArtifact
import org.ossreviewtoolkit.model.VcsInfo
import org.ossreviewtoolkit.model.createAndLogIssue
import org.ossreviewtoolkit.model.utils.DependencyHandler
import org.ossreviewtoolkit.plugins.packagemanagers.maven.Maven
Expand Down Expand Up @@ -112,7 +114,24 @@ class MavenDependencyHandler(
message = "Could not get package information for dependency '" +
"${dependency.artifact.identifier()}': ${e.collectMessages()}"
)
}.getOrNull()
}.getOrElse {
Package(
id = dependency.artifact.run {
Identifier(
type = "Maven",
namespace = groupId,
name = artifactId,
version = version
)
},
binaryArtifact = RemoteArtifact.EMPTY,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we shouldn't be able to at least get the binary artifact here... I mean, apparently software with a dependency on opensymphony:quartz:1.6.0 does build, so the binary can be retrieved, and we should be able to get its coordinates here.

Maybe instead of this fallback here, can we narrow down which part of parsePackage() fails, and try to get other data from it on a more fine-granular basis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we narrow down which part of parsePackage() fails

The Maven call (MavenSupport.kt#698 => MavenSupport.kt#715) raises ProjectBuildingException. Not sure if it's reasonable to break the contract of parsePackage rethrowing these. Same goes for requestRemoteArtifact in MavenDependencyHandler (given that it is a private method).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

try to get other data from it on a more fine-granular basis

I've checked the exception and it doesn't seem like there's much to extract from it, except for the fact that the cause is ArtifactResolutionException. This could be used to make a conditional that tries to fetch the binary, I guess?

Another way would be to try and fetch the remote artifact before the POM, but it's probably a bad idea given how rare the case of POMless dependency is.

declaredLicenses = emptySet(),
description = "",
homepageUrl = "",
sourceArtifact = RemoteArtifact.EMPTY,
vcs = VcsInfo.EMPTY
)
}
}

/**
Expand Down
24 changes: 22 additions & 2 deletions plugins/package-managers/maven/src/test/kotlin/utils/MavenDependencyHandlerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import io.kotest.matchers.collections.beEmpty
import io.kotest.matchers.collections.containExactly
import io.kotest.matchers.collections.haveSize
import io.kotest.matchers.nulls.beNull
import io.kotest.matchers.nulls.shouldNotBeNull
import io.kotest.matchers.should
import io.kotest.matchers.shouldBe
import io.kotest.matchers.string.contain
Expand All @@ -46,7 +47,9 @@ import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.Issue
import org.ossreviewtoolkit.model.Package
import org.ossreviewtoolkit.model.PackageLinkage
import org.ossreviewtoolkit.model.RemoteArtifact
import org.ossreviewtoolkit.model.Severity
import org.ossreviewtoolkit.model.VcsInfo

class MavenDependencyHandlerTest : WordSpec({
beforeSpec {
Expand Down Expand Up @@ -204,7 +207,7 @@ class MavenDependencyHandlerTest : WordSpec({
handler.linkageFor(dependency) shouldBe PackageLinkage.PROJECT_DYNAMIC
}

"handle an exception from MavenSupport" {
"return coordinates-only Package when an exception is raised from MavenSupport" {
val exception = ProjectBuildingException(
"BrokenProject", "Cannot parse pom.",
IOException("General failure when reading hard disk.")
Expand All @@ -219,7 +222,24 @@ class MavenDependencyHandlerTest : WordSpec({
every { dependency.repositories } returns repos
every { handler.support.parsePackage(artifact, repos) } throws exception

handler.createPackage(dependency, issues) should beNull()
val pkg = handler.createPackage(dependency, issues)
pkg.shouldNotBeNull()

with(pkg.id) {
type shouldBe "Maven"
namespace shouldBe artifact.groupId
name shouldBe artifact.artifactId
version shouldBe artifact.version
}
Comment on lines +228 to +233
Copy link
Member

Choose a reason for hiding this comment

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

Maybe nest this in the below?


with(pkg) {
binaryArtifact shouldBe RemoteArtifact.EMPTY
declaredLicenses should beEmpty()
description shouldBe ""
homepageUrl shouldBe ""
Comment on lines +238 to +239
Copy link
Member

Choose a reason for hiding this comment

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

This should be should beEmpty() similar to like one line above, but it requires to import either version via an alias to avoid a name clash. Probably import the line above like import io.kotest.matchers.collections.beEmpty as beEmptyCollection.

sourceArtifact shouldBe RemoteArtifact.EMPTY
vcs shouldBe VcsInfo.EMPTY
}

issues should haveSize(1)
with(issues[0]) {
Expand Down
Loading