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

Node-related improvements #9732

Merged
merged 3 commits into from
Jan 14, 2025
Merged

Node-related improvements #9732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7553793
refactor(NpmDependencyHandler): Inline the only use of `readPackageJson`
sschuberth Jan 10, 2025
01a7f58
chore(node): Introduce a `typealias` for getting package details
sschuberth Jan 10, 2025
72c0d3a
refactor(node): Only pass required information to dependency handlers
sschuberth Jan 10, 2025
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
9 changes: 4 additions & 5 deletions plugins/package-managers/node/src/main/kotlin/NpmSupport.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,13 @@ internal fun parseVcsInfo(packageJson: PackageJson): VcsInfo {
)
}

typealias PackageDetailsFunction = (packageName: String) -> PackageJson?
Copy link
Member

Choose a reason for hiding this comment

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

Function is so generic, how about PackageDetailsProvider or GetPackageDetails?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had "Provider" initially, but compared to our other providers, like PackageCurationProvider, this is way more lightweight, which is why I went for "Function".

Also I didn't want to call it "GetPackageDetails", as that name IMO better fits for the variable / parameter name in line 146, and I didn't want to call the parameter and type basically the same, just with different capitalization.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would not be bad to call it the same. But it's also not so important.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, as somewhat of a compromise, would you like GetPackageDetailsFun better?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's eventually change this later on.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #9738.


/**
* Construct a [Package] by parsing its _package.json_ file and - if applicable - querying additional
* content via the `npm view` command. The result is a [Pair] with the raw identifier and the new package.
*/
internal fun parsePackage(
packageJsonFile: File,
getRemotePackageDetails: (packageName: String) -> PackageJson?
): Package {
internal fun parsePackage(packageJsonFile: File, getPackageDetails: PackageDetailsFunction): Package {
val packageJson = parsePackageJson(packageJsonFile)

// The "name" and "version" fields are only required if the package is going to be published, otherwise they are
Expand Down Expand Up @@ -179,7 +178,7 @@ internal fun parsePackage(
|| hash == Hash.NONE || vcsFromPackage == VcsInfo.EMPTY

if (hasIncompleteData) {
getRemotePackageDetails("$rawName@$version")?.let { details ->
getPackageDetails("$rawName@$version")?.let { details ->
if (description.isEmpty()) description = details.description.orEmpty()
if (homepageUrl.isEmpty()) homepageUrl = details.homepage.orEmpty()

Expand Down
2 changes: 1 addition & 1 deletion plugins/package-managers/node/src/main/kotlin/npm/Npm.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ class Npm(

private val legacyPeerDeps = options[OPTION_LEGACY_PEER_DEPS].toBoolean()
private val npmViewCache = mutableMapOf<String, PackageJson>()
private val handler = NpmDependencyHandler(this)
private val handler = NpmDependencyHandler(projectType, this::getRemotePackageDetails)
private val graphBuilder by lazy { DependencyGraphBuilder(handler) }

override fun resolveDependencies(definitionFile: File, labels: Map<String, String>): List<ProjectAnalyzerResult> =
Expand Down
22 changes: 12 additions & 10 deletions plugins/package-managers/node/src/main/kotlin/npm/NpmDependencyHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,28 @@
import org.ossreviewtoolkit.model.PackageLinkage
import org.ossreviewtoolkit.model.utils.DependencyHandler
import org.ossreviewtoolkit.plugins.packagemanagers.node.NodePackageManager
import org.ossreviewtoolkit.plugins.packagemanagers.node.PackageDetailsFunction
import org.ossreviewtoolkit.plugins.packagemanagers.node.PackageJson
import org.ossreviewtoolkit.plugins.packagemanagers.node.parsePackage
import org.ossreviewtoolkit.plugins.packagemanagers.node.parsePackageJson
import org.ossreviewtoolkit.plugins.packagemanagers.node.splitNamespaceAndName
import org.ossreviewtoolkit.utils.common.realFile

internal class NpmDependencyHandler(private val npm: Npm) : DependencyHandler<ModuleInfo> {
internal class NpmDependencyHandler(
private val projectType: String,
private val getPackageDetails: PackageDetailsFunction
) : DependencyHandler<ModuleInfo> {
private val packageJsonCache = mutableMapOf<File, PackageJson>()

override fun identifierFor(dependency: ModuleInfo): Identifier {
val type = npm.managerName.takeIf { dependency.isProject } ?: "NPM"
val type = if (dependency.isProject) projectType else "NPM"
val (namespace, name) = splitNamespaceAndName(dependency.name.orEmpty())
val version = if (dependency.isProject) {
readPackageJson(dependency.packageJsonFile).version.orEmpty()
val packageJson = packageJsonCache.getOrPut(dependency.packageJsonFile.realFile()) {
parsePackageJson(dependency.packageJsonFile)

Check warning on line 48 in plugins/package-managers/node/src/main/kotlin/npm/NpmDependencyHandler.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/npm/NpmDependencyHandler.kt#L47-L48 

Added lines #L47 - L48 were not covered by tests
}

packageJson.version.orEmpty()
} else {
dependency.version?.takeUnless { it.startsWith("link:") || it.startsWith("file:") }.orEmpty()
}
Expand All @@ -56,14 +64,8 @@

override fun createPackage(dependency: ModuleInfo, issues: MutableCollection<Issue>): Package? =
dependency.takeUnless { it.isProject || !it.isInstalled }?.let {
parsePackage(
packageJsonFile = it.packageJsonFile,
getRemotePackageDetails = npm::getRemotePackageDetails
)
parsePackage(it.packageJsonFile, getPackageDetails)

Check warning on line 67 in plugins/package-managers/node/src/main/kotlin/npm/NpmDependencyHandler.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/npm/NpmDependencyHandler.kt#L67 

Added line #L67 was not covered by tests
}

private fun readPackageJson(packageJsonFile: File): PackageJson =
packageJsonCache.getOrPut(packageJsonFile.realFile()) { parsePackageJson(packageJsonFile) }
}

private val ModuleInfo.isInstalled: Boolean get() = path != null
Expand Down
2 changes: 1 addition & 1 deletion plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
) = Pnpm(type, analysisRoot, analyzerConfig, repoConfig)
}

private val handler = PnpmDependencyHandler(this)
private val handler = PnpmDependencyHandler(projectType, this::getRemotePackageDetails)

Check warning on line 70 in plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt#L70 

Added line #L70 was not covered by tests
private val graphBuilder by lazy { DependencyGraphBuilder(handler) }
private val packageDetailsCache = mutableMapOf<String, PackageJson>()

Expand Down
13 changes: 7 additions & 6 deletions plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,17 @@
import org.ossreviewtoolkit.model.PackageLinkage
import org.ossreviewtoolkit.model.utils.DependencyHandler
import org.ossreviewtoolkit.plugins.packagemanagers.node.NodePackageManager
import org.ossreviewtoolkit.plugins.packagemanagers.node.PackageDetailsFunction
import org.ossreviewtoolkit.plugins.packagemanagers.node.PackageJson
import org.ossreviewtoolkit.plugins.packagemanagers.node.parsePackage
import org.ossreviewtoolkit.plugins.packagemanagers.node.parsePackageJson
import org.ossreviewtoolkit.plugins.packagemanagers.node.pnpm.ModuleInfo.Dependency
import org.ossreviewtoolkit.utils.common.realFile

internal class PnpmDependencyHandler(private val pnpm: Pnpm) : DependencyHandler<Dependency> {
internal class PnpmDependencyHandler(
private val projectType: String,
private val getPackageDetails: PackageDetailsFunction

Check warning on line 39 in plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt#L37-L39 

Added lines #L37 - L39 were not covered by tests
) : DependencyHandler<Dependency> {
private val workspaceModuleDirs = mutableSetOf<File>()
private val packageJsonCache = mutableMapOf<File, PackageJson>()

Expand All @@ -47,7 +51,7 @@
}

override fun identifierFor(dependency: Dependency): Identifier {
val type = pnpm.managerName.takeIf { dependency.isProject() } ?: "NPM"
val type = if (dependency.isProject()) projectType else "NPM"
val namespace = dependency.from.substringBeforeLast("/", "")
val name = dependency.from.substringAfterLast("/")
val version = if (dependency.isProject()) {
Expand All @@ -67,10 +71,7 @@

override fun createPackage(dependency: Dependency, issues: MutableCollection<Issue>): Package? =
dependency.takeUnless { it.isProject() || !it.isInstalled }?.let {
parsePackage(
packageJsonFile = it.packageJsonFile,
getRemotePackageDetails = pnpm::getRemotePackageDetails
)
parsePackage(it.packageJsonFile, getPackageDetails)

Check warning on line 74 in plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt#L74 

Added line #L74 was not covered by tests
}

private fun readPackageJson(packageJsonFile: File): PackageJson =
Expand Down
Loading