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

Do not register local and link components in pnpm9 detector #1331

Merged
merged 2 commits into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,6 @@ public static class PnpmConstants
public const string PnpmFileDependencyPath = "file:";

public const string PnpmLinkDependencyPath = "link:";
public const string PnpmHttpDependencyPath = "http:";
public const string PnpmHttpsDependencyPath = "https:";
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,19 @@ public bool IsLocalDependency(KeyValuePair<string, string> dependency)
}

/// <summary>
/// Parse a pnpm path of the form "/package-name/version".
/// Parse a pnpm path of the form "/package-name/version and create an npm component".
/// </summary>
/// <param name="pnpmPackagePath">a pnpm path of the form "/package-name/version".</param>
/// <returns>Data parsed from path.</returns>
public abstract DetectedComponent CreateDetectedComponentFromPnpmPath(string pnpmPackagePath);

/// <summary>
/// Parse a pnpm path of the form "/package-name/version into a packageName and Version.
/// </summary>
/// <param name="pnpmPackagePath">a pnpm path of the form "/package-name/version".</param>
/// <returns>Data parsed from path.</returns>
public abstract (string FullPackageName, string PackageVersion) ExtractNameAndVersionFromPnpmPackagePath(string pnpmPackagePath);

public virtual string ReconstructPnpmDependencyPath(string dependencyName, string dependencyVersion)
{
if (dependencyVersion.StartsWith('/'))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public override DetectedComponent CreateDetectedComponentFromPnpmPath(string pnp
return new DetectedComponent(new NpmComponent(parentName, parentVersion));
}

private (string Name, string Version) ExtractNameAndVersionFromPnpmPackagePath(string pnpmPackagePath)
public override (string FullPackageName, string PackageVersion) ExtractNameAndVersionFromPnpmPackagePath(string pnpmPackagePath)
{
var pnpmComponentDefSections = pnpmPackagePath.Trim('/').Split('/');
(var packageVersion, var indexVersionIsAt) = this.GetPackageVersion(pnpmComponentDefSections);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ public override DetectedComponent CreateDetectedComponentFromPnpmPath(string pnp

// Strip parenthesized suffices from package. These hold peed dep related information that is unneeded here.
// An example of a dependency path with these: /webpack-cli@4.10.0(webpack-bundle-analyzer@4.10.1)(webpack-dev-server@4.6.0)(webpack@5.89.0)
var (normalizedPackageName, packageVersion) = this.ExtractNameAndVersionFromPnpmPackagePath(pnpmPackagePath);
return new DetectedComponent(new NpmComponent(normalizedPackageName, packageVersion));
}

public override (string FullPackageName, string PackageVersion) ExtractNameAndVersionFromPnpmPackagePath(string pnpmPackagePath)
{
var fullPackageNameAndVersion = pnpmPackagePath.Split("(")[0];

var packageNameParts = fullPackageNameAndVersion.Split("@");
Expand All @@ -37,6 +43,6 @@ public override DetectedComponent CreateDetectedComponentFromPnpmPath(string pnp
// It is unclear if real packages could have a name starting with `/`, so avoid `TrimStart` that just in case.
var normalizedPackageName = fullPackageName[1..];

return new DetectedComponent(new NpmComponent(normalizedPackageName, packageVersion));
return (normalizedPackageName, packageVersion);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,23 @@ public class PnpmV9ParsingUtilities<T> : PnpmParsingUtilitiesBase<T>
where T : PnpmYaml
{
public override DetectedComponent CreateDetectedComponentFromPnpmPath(string pnpmPackagePath)
{
/*
* The format is documented at https://github.com/pnpm/spec/blob/master/dependency-path.md.
* At the writing it does not seem to reflect changes which were made in lock file format v9:
* See https://github.com/pnpm/spec/issues/5.
* In general, the spec sheet for the v9 lockfile is not published, so parsing of this lockfile was emperically determined.
* see https://github.com/pnpm/spec/issues/6
*/

// Strip parenthesized suffices from package. These hold peed dep related information that is unneeded here.
// An example of a dependency path with these: /webpack-cli@4.10.0(webpack-bundle-analyzer@4.10.1)(webpack-dev-server@4.6.0)(webpack@5.89.0)
(var fullPackageName, var packageVersion) = this.ExtractNameAndVersionFromPnpmPackagePath(pnpmPackagePath);

return new DetectedComponent(new NpmComponent(fullPackageName, packageVersion));
}

public override (string FullPackageName, string PackageVersion) ExtractNameAndVersionFromPnpmPackagePath(string pnpmPackagePath)
{
/*
* The format is documented at https://github.com/pnpm/spec/blob/master/dependency-path.md.
Expand All @@ -28,7 +45,7 @@ public override DetectedComponent CreateDetectedComponentFromPnpmPath(string pnp
// Version is section after last `@`.
var packageVersion = packageNameParts[^1];

return new DetectedComponent(new NpmComponent(fullPackageName, packageVersion));
return (fullPackageName, packageVersion);
}

/// <summary>
Expand Down
35 changes: 22 additions & 13 deletions src/Microsoft.ComponentDetection.Detectors/pnpm/Pnpm9Detector.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,8 @@ public void RecordDependencyGraphFromFile(string yamlFileContent, ISingleFileCom
// Ignore "file:" as these are local packages.
// Such local packages should only be referenced at the top level (via ProcessDependencyList) which also skips them or from other local packages (which this skips).
// There should be no cases where a non-local package references a local package, so skipping them here should not result in failed lookups below when adding all the graph references.
if (pnpmDependencyPath.StartsWith(PnpmConstants.PnpmFileDependencyPath))
{
continue;
}
var (packageName, packageVersion) = this.pnpmParsingUtilities.ExtractNameAndVersionFromPnpmPackagePath(pnpmDependencyPath);
var isFileOrLink = this.IsFileOrLink(packageVersion) || this.IsFileOrLink(pnpmDependencyPath);

var dependencyPath = pnpmDependencyPath;
if (pnpmDependencyPath.StartsWith('/'))
Expand All @@ -48,7 +46,10 @@ public void RecordDependencyGraphFromFile(string yamlFileContent, ISingleFileCom
// It should get registered again with with additional information (what depended on it) later,
// but registering it now ensures nothing is missed due to a limitation in dependency traversal
// like skipping local dependencies which might have transitively depended on this.
singleFileComponentRecorder.RegisterUsage(parentDetectedComponent);
if (!isFileOrLink)
{
singleFileComponentRecorder.RegisterUsage(parentDetectedComponent);
}
}

// now that the components dictionary is populated, add direct dependencies of the current file/project setting isExplicitReferencedDependency to true
Expand All @@ -70,23 +71,31 @@ private void ProcessDependencyList(ISingleFileComponentRecorder singleFileCompon
{
foreach (var (name, dep) in dependencies ?? Enumerable.Empty<KeyValuePair<string, PnpmYamlV9Dependency>>())
{
// Ignore "file:" and "link:" as these are local packages.
if (dep.Version.StartsWith(PnpmConstants.PnpmLinkDependencyPath) || dep.Version.StartsWith(PnpmConstants.PnpmFileDependencyPath))
FernandoRojo marked this conversation as resolved.
Show resolved Hide resolved
{
continue;
}

var pnpmDependencyPath = this.pnpmParsingUtilities.ReconstructPnpmDependencyPath(name, dep.Version);
var (component, package) = components[pnpmDependencyPath];

// Lockfile v9 apparently removed the tagging of dev dependencies in the lockfile, so we revert to using the dependency tree to establish dev dependency state.
// At this point, the root dependencies are marked according to which dependency group they are declared in the lockfile itself.
singleFileComponentRecorder.RegisterUsage(component, isExplicitReferencedDependency: true, isDevelopmentDependency: isDevelopmentDependency);
// Ignore "file:" and "link:" as these are local packages.
var isFileOrLink = this.IsFileOrLink(dep.Version);
if (!isFileOrLink)
{
singleFileComponentRecorder.RegisterUsage(component, isExplicitReferencedDependency: true, isDevelopmentDependency: isDevelopmentDependency);
}

var seenDependencies = new HashSet<string>();
this.ProcessIndirectDependencies(singleFileComponentRecorder, components, component.Component.Id, package.Dependencies, isDevelopmentDependency, seenDependencies);
this.ProcessIndirectDependencies(singleFileComponentRecorder, components, isFileOrLink ? null : component.Component.Id, package.Dependencies, isDevelopmentDependency, seenDependencies);
FernandoRojo marked this conversation as resolved.
Show resolved Hide resolved
}
}

private bool IsFileOrLink(string packagePath)
{
return packagePath.StartsWith(PnpmConstants.PnpmLinkDependencyPath) ||
packagePath.StartsWith(PnpmConstants.PnpmFileDependencyPath) ||
packagePath.StartsWith(PnpmConstants.PnpmHttpDependencyPath) ||
packagePath.StartsWith(PnpmConstants.PnpmHttpsDependencyPath);
}

private void ProcessIndirectDependencies(ISingleFileComponentRecorder singleFileComponentRecorder, Dictionary<string, (DetectedComponent C, Package P)> components, string parentComponentId, Dictionary<string, string> dependencies, bool isDevDependency, HashSet<string> seenDependencies)
{
// Now that the `components` dictionary is populated, make another pass of all components, registering all the dependency edges in the graph.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,144 @@ public async Task TestPnpmDetector_V9_GoodLockVersion_SkipsFileAndLinkDependenci
parentComponent => parentComponent.Name == "sampleDependency");
}

[TestMethod]
public async Task TestPnpmDetector_V9_GoodLockVersion_FileAndLinkDependenciesAreNotRegistered()
{
var yamlFile = @"
lockfileVersion: '9.0'
FernandoRojo marked this conversation as resolved.
Show resolved Hide resolved
settings:
autoInstallPeers: true
excludeLinksFromLockfile: false
importers:
FernandoRojo marked this conversation as resolved.
Show resolved Hide resolved
.:
dependencies:
sampleDependency:
specifier: ^1.1.1
version: 1.1.1
sampleFileDependency:
specifier: file:../test
version: file:../test
SampleLinkDependency:
specifier: workspace:*
version: link:SampleLinkDependency
packages:
sampleDependency@1.1.1:
resolution: {integrity: sha512-zRpUiDwd/xk6ADqPMATG8vc9VPrkck7T07OIx0gnjmJAnHnTVXNQG3vfvWNuiZIkwu9KrKdA1iJKfsfTVxE6NA==}
sampleIndirectDependency2@2.2.2:
resolution: {integrity: sha512-FQN4MRfuJeHf7cBbBMJFXhKSDq+2kAArBlmRBvcvFE5BB1HZKXtSFASDhdlz9zOYwxh8lDdnvmMOe/+5cdoEdg==}
engines: {node: '>= 0.8'}
sampleIndirectDependency@3.3.3:
resolution: {integrity: sha512-ZySD7Nf91aLB0RxL4KGrKHBXl7Eds1DAmEdcoVawXnLD7SDhpNgtuII2aAkg7a7QS41jxPSZ17p4VdGnMHk3MQ==}
engines: {node: '>=0.4.0'}

snapshots:
sampleDependency@1.1.1:
dependencies:
sampleIndirectDependency: 3.3.3
sampleIndirectDependency2: 2.2.2
'file://../sampleFile': 'link:../\\'
sampleIndirectDependency2@2.2.2: {}
sampleIndirectDependency@3.3.3: {}
sampleFileDependency@file:../test': {}
";

var (scanResult, componentRecorder) = await this.DetectorTestUtility
.WithFile("pnpm-lock.yaml", yamlFile)
.ExecuteDetectorAsync();

scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);

var detectedComponents = componentRecorder.GetDetectedComponents();
detectedComponents.Should().HaveCount(3);
var npmComponents = detectedComponents.Select(x => new { Component = x.Component as NpmComponent, DetectedComponent = x });
npmComponents.Should().Contain(x => x.Component.Name == "sampleDependency" && x.Component.Version == "1.1.1");
npmComponents.Should().Contain(x => x.Component.Name == "sampleIndirectDependency2" && x.Component.Version == "2.2.2");
npmComponents.Should().Contain(x => x.Component.Name == "sampleIndirectDependency" && x.Component.Version == "3.3.3");

var noDevDependencyComponent = npmComponents.First(x => x.Component.Name == "sampleDependency");
var indirectDependencyComponent2 = npmComponents.First(x => x.Component.Name == "sampleIndirectDependency2");
var indirectDependencyComponent = npmComponents.First(x => x.Component.Name == "sampleIndirectDependency");

componentRecorder.GetEffectiveDevDependencyValue(noDevDependencyComponent.Component.Id).Should().BeFalse();
componentRecorder.GetEffectiveDevDependencyValue(indirectDependencyComponent2.Component.Id).Should().BeFalse();
componentRecorder.GetEffectiveDevDependencyValue(indirectDependencyComponent.Component.Id).Should().BeFalse();
componentRecorder.AssertAllExplicitlyReferencedComponents<NpmComponent>(
indirectDependencyComponent.Component.Id,
parentComponent => parentComponent.Name == "sampleDependency");
componentRecorder.AssertAllExplicitlyReferencedComponents<NpmComponent>(
indirectDependencyComponent2.Component.Id,
parentComponent => parentComponent.Name == "sampleDependency");
}

[TestMethod]
public async Task TestPnpmDetector_V9_GoodLockVersion_HttpDependenciesAreNotRegistered()
{
var yamlFile = @"
lockfileVersion: '9.0'
settings:
autoInstallPeers: true
excludeLinksFromLockfile: false
importers:
.:
dependencies:
sampleDependency:
specifier: ^1.1.1
version: 1.1.1
sampleHttpDependency:
specifier: https://samplePackage/tar.gz/32f550d3b3bdb1b781aabe100683311cd982c98e
version: sample@https://samplePackage/tar.gz/32f550d3b3bdb1b781aabe100683311cd982c98e
SampleLinkDependency:
specifier: workspace:*
version: link:SampleLinkDependency
packages:
sampleDependency@1.1.1:
resolution: {integrity: sha512-zRpUiDwd/xk6ADqPMATG8vc9VPrkck7T07OIx0gnjmJAnHnTVXNQG3vfvWNuiZIkwu9KrKdA1iJKfsfTVxE6NA==}
sampleIndirectDependency2@2.2.2:
resolution: {integrity: sha512-FQN4MRfuJeHf7cBbBMJFXhKSDq+2kAArBlmRBvcvFE5BB1HZKXtSFASDhdlz9zOYwxh8lDdnvmMOe/+5cdoEdg==}
engines: {node: '>= 0.8'}
sampleIndirectDependency@3.3.3:
resolution: {integrity: sha512-ZySD7Nf91aLB0RxL4KGrKHBXl7Eds1DAmEdcoVawXnLD7SDhpNgtuII2aAkg7a7QS41jxPSZ17p4VdGnMHk3MQ==}
engines: {node: '>=0.4.0'}

snapshots:
sampleDependency@1.1.1:
dependencies:
sampleIndirectDependency: 3.3.3
sampleIndirectDependency2: 2.2.2
'file://../sampleFile': 'link:../\\'
sampleIndirectDependency2@2.2.2: {}
sampleIndirectDependency@3.3.3: {}
sampleHttpDependency@https://samplePackage/tar.gz/32f550d3b3bdb1b781aabe100683311cd982c98e': {}
";

var (scanResult, componentRecorder) = await this.DetectorTestUtility
.WithFile("pnpm-lock.yaml", yamlFile)
.ExecuteDetectorAsync();

scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);

var detectedComponents = componentRecorder.GetDetectedComponents();
detectedComponents.Should().HaveCount(3);
var npmComponents = detectedComponents.Select(x => new { Component = x.Component as NpmComponent, DetectedComponent = x });
npmComponents.Should().Contain(x => x.Component.Name == "sampleDependency" && x.Component.Version == "1.1.1");
npmComponents.Should().Contain(x => x.Component.Name == "sampleIndirectDependency2" && x.Component.Version == "2.2.2");
npmComponents.Should().Contain(x => x.Component.Name == "sampleIndirectDependency" && x.Component.Version == "3.3.3");

var noDevDependencyComponent = npmComponents.First(x => x.Component.Name == "sampleDependency");
var indirectDependencyComponent2 = npmComponents.First(x => x.Component.Name == "sampleIndirectDependency2");
var indirectDependencyComponent = npmComponents.First(x => x.Component.Name == "sampleIndirectDependency");

componentRecorder.GetEffectiveDevDependencyValue(noDevDependencyComponent.Component.Id).Should().BeFalse();
componentRecorder.GetEffectiveDevDependencyValue(indirectDependencyComponent2.Component.Id).Should().BeFalse();
componentRecorder.GetEffectiveDevDependencyValue(indirectDependencyComponent.Component.Id).Should().BeFalse();
componentRecorder.AssertAllExplicitlyReferencedComponents<NpmComponent>(
indirectDependencyComponent.Component.Id,
parentComponent => parentComponent.Name == "sampleDependency");
componentRecorder.AssertAllExplicitlyReferencedComponents<NpmComponent>(
indirectDependencyComponent2.Component.Id,
parentComponent => parentComponent.Name == "sampleDependency");
}

[TestMethod]
public async Task TestPnpmDetector_V9_GoodLockVersion_MissingSnapshotsSuccess()
{
Expand Down
Loading