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.

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

Conversation

brigaccess
Copy link
Contributor

When analyzing a Maven project with a dependency on opensymphony:quartz:1.6.0, ORT does not include it in the packages list of the analyzer, but it is still present in the dependency_graphs.Maven.packages. The reason for it turned out to be a lack of POM for the dependency in the Central (see [1]) causing an exception in MavenSupport.parsePackage call.

This PR fixes the issue by returning a coordinates-only Package from getOrElse block instead of null. Another way to fix this would be to return that Package from the MavenSupport.parsePackage itself. However, GradleDependencyHandler seems to rely on the exception thrown by it, and other (potential) consumers might want to do that as well.

[1] https://repo.maven.apache.org/maven2/opensymphony/quartz/1.6.0/

…tion

This allows the dependency to still be in the `packages` list of the report
even if POM resolution failed for some reason.

Signed-off-by: Igor Brovtsin <[email protected]>
@brigaccess brigaccess requested a review from a team as a code owner March 28, 2024 16:59
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.

Comment on lines +238 to +239
description shouldBe ""
homepageUrl shouldBe ""
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.

Comment on lines +228 to +233
with(pkg.id) {
type shouldBe "Maven"
namespace shouldBe artifact.groupId
name shouldBe artifact.artifactId
version shouldBe artifact.version
}
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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants