-
Notifications
You must be signed in to change notification settings - Fork 382
Yarn2: Improve dependency handling #11972
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,8 @@ package org.ossreviewtoolkit.plugins.packagemanagers.node.yarn2 | |
|
|
||
| import java.io.File | ||
|
|
||
| import org.apache.logging.log4j.kotlin.logger | ||
|
|
||
| import org.ossreviewtoolkit.model.Identifier | ||
| import org.ossreviewtoolkit.model.Issue | ||
| import org.ossreviewtoolkit.model.Package | ||
|
|
@@ -31,6 +33,9 @@ import org.ossreviewtoolkit.plugins.packagemanagers.node.NodePackageManagerType | |
| import org.ossreviewtoolkit.plugins.packagemanagers.node.PackageJson | ||
| import org.ossreviewtoolkit.plugins.packagemanagers.node.parsePackage | ||
|
|
||
| import org.semver4j.Semver | ||
| import org.semver4j.range.RangeListFactory | ||
|
|
||
| internal class Yarn2DependencyHandler( | ||
| private val moduleInfoResolver: ModuleInfoResolver | ||
| ) : DependencyHandler<PackageInfo> { | ||
|
|
@@ -65,7 +70,7 @@ internal class Yarn2DependencyHandler( | |
| ) | ||
|
|
||
| override fun dependenciesFor(dependency: PackageInfo): List<PackageInfo> = | ||
| dependency.children.dependencies.map { packageInfoForLocator.getValue(it.realLocator) } | ||
| dependency.children.dependencies.map(this::packageInfoFor) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please drop |
||
|
|
||
| override fun linkageFor(dependency: PackageInfo): PackageLinkage = | ||
| if (dependency.isProject) PackageLinkage.PROJECT_DYNAMIC else PackageLinkage.DYNAMIC | ||
|
|
@@ -76,11 +81,68 @@ internal class Yarn2DependencyHandler( | |
|
|
||
| return parsePackage(packageJson, moduleInfoResolver) | ||
| } | ||
|
|
||
| /** | ||
| * Obtain the [PackageInfo] object for the given [dependency]. | ||
| * | ||
| * Try the `realLocator` first to correctly handle virtual packages. If that fails, try to construct the real | ||
| * locator from the virtual package's actual resolved version (handles virtual packages whose `children.version` | ||
| * was overridden by Yarn's `resolutions` feature). | ||
| * | ||
| * If both targeted lookups fail, fall back to searching the map for all installed non-virtual, non-project | ||
| * versions of the same module by name. This handles the case where Yarn's `resolutions` feature (or similar | ||
| * mechanisms) cause a non-virtual dependency locator to reference a version that is not present in the map, | ||
| * while a different version of the same module was actually installed. If exactly one candidate is found, it | ||
| * is used. If multiple candidates are found, the semver range from the [dependency]'s descriptor is used to | ||
| * narrow down the candidates. If after all fallbacks the result is still not unique, an exception is thrown. | ||
| */ | ||
| internal fun packageInfoFor(dependency: PackageInfo.Dependency): PackageInfo { | ||
| // Direct lookup by the real locator. | ||
| packageInfoForLocator[dependency.realLocator]?.let { return it } | ||
|
|
||
| // Fallback for virtual packages: derive the real locator from the virtual package's resolved version. | ||
| packageInfoForLocator[dependency.locator]?.let { virtualInfo -> | ||
| val moduleName = Locator.parse(dependency.locator).moduleName | ||
| packageInfoForLocator["$moduleName@npm:${virtualInfo.children.version}"]?.let { return it } | ||
| } | ||
|
|
||
| // Fallback for version mismatches caused by Yarn's `resolutions` feature: find installed versions of the | ||
| // same module by name, ignoring the exact version in the locator. | ||
| val moduleName = Locator.parse(dependency.realLocator).moduleName | ||
| val candidates = packageInfoForLocator.values.filter { | ||
| it.moduleName == moduleName && !it.isProject && !it.isVirtual | ||
| } | ||
|
|
||
| if (candidates.size == 1) { | ||
| return candidates.single().also { | ||
| logger.debug { | ||
| "Resolved locator '${dependency.realLocator}' to '${it.value}' via module name lookup." | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (candidates.size > 1) { | ||
| candidates.matchVersionRange(dependency)?.let { return it } | ||
|
|
||
| error( | ||
| "Could not unambiguously resolve locator '${dependency.realLocator}'. Found ${candidates.size} " + | ||
| "installed versions of module '$moduleName': ${candidates.map { it.value }}." | ||
| ) | ||
| } | ||
|
|
||
| error( | ||
| "Could not find a PackageInfo for locator '${dependency.realLocator}'. No entry for module " + | ||
| "'$moduleName' exists in ${packageInfoForLocator.keys}." | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| internal val PackageInfo.isProject: Boolean | ||
| get() = Locator.parse(value).isProject | ||
|
|
||
| internal val PackageInfo.isVirtual: Boolean | ||
| get() = Locator.parse(value).isVirtual | ||
|
|
||
| internal val PackageInfo.moduleName: String | ||
| // TODO: Handle patched packages different than non-patched ones. | ||
| // Patch packages have locators as e.g. the following, where the first component ends with "@patch". | ||
|
|
@@ -113,4 +175,34 @@ internal data class Locator( | |
|
|
||
| val isProject: Boolean = remainder.startsWith("workspace:") || | ||
| (remainder.startsWith("virtual:") && "#workspace:" in remainder) | ||
|
|
||
| val isVirtual: Boolean = remainder.startsWith("virtual:") && !isProject | ||
| } | ||
|
|
||
| /** | ||
| * Try to find a single [PackageInfo] from this collection that matches the given [dependency] taking semantic | ||
| * version ranges into account. | ||
| */ | ||
| private fun Collection<PackageInfo>.matchVersionRange(dependency: PackageInfo.Dependency): PackageInfo? { | ||
| val descriptorRemainder = Locator.parse(dependency.descriptor).remainder | ||
| if (descriptorRemainder.startsWith("npm:")) { | ||
| val rangeSpec = descriptorRemainder.removePrefix("npm:") | ||
|
Comment on lines
+188
to
+189
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer using |
||
| val range = runCatching { RangeListFactory.create(rangeSpec) }.getOrNull() | ||
| if (range != null) { | ||
|
Comment on lines
+190
to
+191
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer |
||
| val matchingCandidates = filter { candidate -> | ||
| Semver.coerce(candidate.children.version)?.let { range.isSatisfiedBy(it) } == true | ||
| } | ||
|
|
||
| if (matchingCandidates.size == 1) { | ||
| return matchingCandidates.single().also { | ||
|
Comment on lines
+196
to
+197
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer using |
||
| logger.debug { | ||
| "Resolved locator '${dependency.realLocator}' to '${it.value}' via semver range " + | ||
| "matching on descriptor '${dependency.descriptor}'." | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,8 +19,14 @@ | |
|
|
||
| package org.ossreviewtoolkit.plugins.packagemanagers.node.yarn2 | ||
|
|
||
| import io.kotest.assertions.throwables.shouldThrow | ||
| import io.kotest.core.spec.style.WordSpec | ||
| import io.kotest.matchers.shouldBe | ||
| import io.kotest.matchers.string.shouldContain | ||
|
|
||
| import java.io.File | ||
|
|
||
| import org.ossreviewtoolkit.plugins.packagemanagers.node.ModuleInfoResolver | ||
|
|
||
| class Yarn2DependencyHandlerTest : WordSpec({ | ||
| "Locator.parse()" should { | ||
|
|
@@ -59,4 +65,170 @@ class Yarn2DependencyHandlerTest : WordSpec({ | |
| locator.isProject shouldBe true | ||
| } | ||
| } | ||
|
|
||
| "Locator.isVirtual" should { | ||
| "return true for a virtual npm package" { | ||
| val locator = Locator.parse( | ||
| "cookie@virtual:abc123def456abc123def456abc123def456abc123def456abc123def456abc123de#npm:1.0.2" | ||
| ) | ||
|
|
||
| locator.isVirtual shouldBe true | ||
| } | ||
|
|
||
| "return false for a real npm package" { | ||
| Locator.parse("cookie@npm:1.0.2").isVirtual shouldBe false | ||
| } | ||
|
|
||
| "return false for a workspace project" { | ||
| Locator.parse("myapp@workspace:.").isVirtual shouldBe false | ||
| } | ||
|
|
||
| "return false for a virtual workspace project" { | ||
| val locator = Locator.parse( | ||
| "@failing/package-with-lightningcss@virtual:f87a972e7ee54256c6d8f979d7f3914b32522893226eba595e4ef" + | ||
| "e4ecc641a239c6d88e01eccc6f32db30829d6ac493bfc98cb406a9b0d6059ee4112c084" + | ||
| "3da9#workspace:packages/spark" | ||
| ) | ||
|
|
||
| locator.isVirtual shouldBe false | ||
| } | ||
| } | ||
|
|
||
| "packageInfoFor()" should { | ||
| "resolve a dependency via its realLocator" { | ||
| val info = packageInfo("cookie@npm:1.0.2", "1.0.2") | ||
| val handler = handlerWith(mapOf("cookie@npm:1.0.2" to info)) | ||
|
|
||
| handler.packageInfoFor(dep("cookie@npm:1.0.2")) shouldBe info | ||
| } | ||
|
|
||
| "resolve a virtual dependency via its realLocator" { | ||
| val info = packageInfo("cookie@npm:1.0.2", "1.0.2") | ||
| val virtualLocator = | ||
| "cookie@virtual:abc123def456abc123def456abc123def456abc123def456abc123def456abc123de#npm:1.0.2" | ||
| val handler = handlerWith(mapOf("cookie@npm:1.0.2" to info)) | ||
|
|
||
| handler.packageInfoFor(dep(virtualLocator)) shouldBe info | ||
| } | ||
|
|
||
| "use the virtual package fallback when the realLocator is not in the map" { | ||
| // This handles virtual packages whose children.version was overridden by Yarn's resolutions feature. | ||
| // The virtual locator encodes version 1.0.2, but children.version reflects the resolved version 1.1.1. | ||
| val virtualLocator = | ||
| "cookie@virtual:abc123def456abc123def456abc123def456abc123def456abc123def456abc123de#npm:1.0.2" | ||
| val virtualInfo = packageInfo(virtualLocator, "1.1.1") | ||
| val resolvedInfo = packageInfo("cookie@npm:1.1.1", "1.1.1") | ||
| val handler = handlerWith( | ||
| mapOf( | ||
| virtualLocator to virtualInfo, | ||
| "cookie@npm:1.1.1" to resolvedInfo | ||
| ) | ||
| ) | ||
|
|
||
| handler.packageInfoFor(dep(virtualLocator)) shouldBe resolvedInfo | ||
| } | ||
|
|
||
| "use the module name fallback when only a different version is installed" { | ||
| // This handles the case where Yarn's resolutions feature causes a non-virtual dependency locator | ||
| // to reference a version that is not present in the map. | ||
| val resolvedInfo = packageInfo("cookie@npm:1.1.1", "1.1.1") | ||
| val handler = handlerWith(mapOf("cookie@npm:1.1.1" to resolvedInfo)) | ||
|
|
||
| handler.packageInfoFor(dep("cookie@npm:1.0.2")) shouldBe resolvedInfo | ||
| } | ||
|
|
||
| "ignore virtual packages in the module name fallback" { | ||
| val virtualLocator = | ||
| "cookie@virtual:abc123def456abc123def456abc123def456abc123def456abc123def456abc123de#npm:1.1.1" | ||
| val resolvedInfo = packageInfo("cookie@npm:1.1.1", "1.1.1") | ||
| val virtualInfo = packageInfo(virtualLocator, "1.1.1") | ||
| val handler = handlerWith( | ||
| mapOf( | ||
| "cookie@npm:1.1.1" to resolvedInfo, | ||
| virtualLocator to virtualInfo | ||
| ) | ||
| ) | ||
|
|
||
| handler.packageInfoFor(dep("cookie@npm:1.0.2")) shouldBe resolvedInfo | ||
| } | ||
|
|
||
| "throw when no entry for the module is found" { | ||
| val handler = handlerWith(mapOf("other@npm:1.0.0" to packageInfo("other@npm:1.0.0", "1.0.0"))) | ||
|
|
||
| val exception = shouldThrow<IllegalStateException> { | ||
| handler.packageInfoFor(dep("cookie@npm:1.0.2")) | ||
| } | ||
|
|
||
| exception.message shouldContain "cookie" | ||
| } | ||
|
|
||
| "throw when multiple real versions of the module are found" { | ||
| val handler = handlerWith( | ||
| mapOf( | ||
| "cookie@npm:1.0.0" to packageInfo("cookie@npm:1.0.0", "1.0.0"), | ||
| "cookie@npm:1.1.1" to packageInfo("cookie@npm:1.1.1", "1.1.1") | ||
| ) | ||
| ) | ||
|
|
||
| val exception = shouldThrow<IllegalStateException> { | ||
| handler.packageInfoFor(dep("cookie@npm:1.0.2")) | ||
| } | ||
|
|
||
| exception.message shouldContain "2" | ||
| exception.message shouldContain "cookie" | ||
| } | ||
|
|
||
| "use the semver range from the descriptor to disambiguate multiple candidates" { | ||
| // Two real versions are installed. The descriptor's range matches only one of them. | ||
| val info100 = packageInfo("cookie@npm:1.0.0", "1.0.0") | ||
| val info200 = packageInfo("cookie@npm:2.0.0", "2.0.0") | ||
| // Descriptor "cookie@npm:^1.0.0" matches 1.0.0 but not 2.0.0. | ||
| val dependency = PackageInfo.Dependency(descriptor = "cookie@npm:^1.0.0", locator = "cookie@npm:1.0.2") | ||
| val handler = handlerWith(mapOf("cookie@npm:1.0.0" to info100, "cookie@npm:2.0.0" to info200)) | ||
|
|
||
| handler.packageInfoFor(dependency) shouldBe info100 | ||
| } | ||
|
|
||
| "throw when the semver range from the descriptor still matches multiple candidates" { | ||
| // Both installed versions satisfy the descriptor range. | ||
| val info440 = packageInfo("debug@npm:4.4.0", "4.4.0") | ||
| val info443 = packageInfo("debug@npm:4.4.3", "4.4.3") | ||
| // Descriptor "debug@npm:^4.3.0" matches both 4.4.0 and 4.4.3. | ||
| val dependency = PackageInfo.Dependency(descriptor = "debug@npm:^4.3.0", locator = "debug@npm:4.3.6") | ||
| val handler = handlerWith(mapOf("debug@npm:4.4.0" to info440, "debug@npm:4.4.3" to info443)) | ||
|
|
||
| val exception = shouldThrow<IllegalStateException> { | ||
| handler.packageInfoFor(dependency) | ||
| } | ||
|
|
||
| exception.message shouldContain "2" | ||
| exception.message shouldContain "debug" | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| private val WORKING_DIR = File(".") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be safer / better practice to use a temporary directory here? |
||
|
|
||
| /** | ||
| * Create a minimal [PackageInfo] for the given [locator] and [version]. | ||
| */ | ||
| private fun packageInfo(locator: String, version: String, deps: List<PackageInfo.Dependency> = emptyList()) = | ||
| PackageInfo( | ||
| value = locator, | ||
| children = PackageInfo.Children(version = version, dependencies = deps) | ||
| ) | ||
|
|
||
| /** | ||
| * Create a [PackageInfo.Dependency] with the given [locator]. The descriptor is set to a dummy value. | ||
| */ | ||
| private fun dep(locator: String) = PackageInfo.Dependency(descriptor = "dummy@npm:^1.2.3", locator = locator) | ||
|
|
||
| /** | ||
| * Create a [Yarn2DependencyHandler] with the given [packageInfoForLocator] map set via | ||
| * [Yarn2DependencyHandler.setContext]. | ||
| */ | ||
| private fun handlerWith(packageInfoForLocator: Map<String, PackageInfo>): Yarn2DependencyHandler { | ||
| val resolver = ModuleInfoResolver { _, _ -> emptySet() } | ||
| resolver.workingDir = WORKING_DIR | ||
| return Yarn2DependencyHandler(resolver).apply { setContext(WORKING_DIR, emptyMap(), packageInfoForLocator) } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Let me ensure I understand this sentence correctly. Are you saying, if a project transitively depends on package "foo" both in version "1.0.0" and "2.0.0", then only either version will be resolved?
If so, that would contradict my understand of the Node ecosystem's capability to indeed support multiple versions of the same package in the transitive tree.
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 have rephrased the commit message to hopefully make it clearer. The problem with the current implementation is that it looks up
PackageInfoobjects using a key which contains a specific version. In this project, however, there are references to dependencies with slightly different versions.