fix(maven): return coordinates-only Package on parsePackage exception#8471
fix(maven): return coordinates-only Package on parsePackage exception#8471brigaccess wants to merge 1 commit intooss-review-toolkit:mainfrom
Package on parsePackage exception#8471Conversation
…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 <brigaccess@gmail.com>
| version = version | ||
| ) | ||
| }, | ||
| binaryArtifact = RemoteArtifact.EMPTY, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| description shouldBe "" | ||
| homepageUrl shouldBe "" |
There was a problem hiding this comment.
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.
| with(pkg.id) { | ||
| type shouldBe "Maven" | ||
| namespace shouldBe artifact.groupId | ||
| name shouldBe artifact.artifactId | ||
| version shouldBe artifact.version | ||
| } |
There was a problem hiding this comment.
Maybe nest this in the below?
2 participants
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 thedependency_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 inMavenSupport.parsePackagecall.This PR fixes the issue by returning a coordinates-only
PackagefromgetOrElseblock instead ofnull. Another way to fix this would be to return thatPackagefrom theMavenSupport.parsePackageitself. However,GradleDependencyHandlerseems 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/