Skip to content

Commit dbff730

Browse files
RushabhBhansaliCopilot
RushabhBhansali
and
Copilot
authored
Fix Npm Transitive Dependency Calculation (#1617)
* Fix depedency duplication logic. Avoid duplicate parent-dependency pairs only. * added test * circular dependency test * removed redundant file * updated detector version * Add assertions to IsDependencyOfExplicitlyReferencedComponents test calls (#1618) * Initial plan * Add assertions to IsDependencyOfExplicitlyReferencedComponents calls in circular dependency test Co-authored-by: RushabhBhansali <24465841+RushabhBhansali@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: RushabhBhansali <24465841+RushabhBhansali@users.noreply.github.com> Co-authored-by: Rushabh <rbhansali@microsoft.com> --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: RushabhBhansali <24465841+RushabhBhansali@users.noreply.github.com>
1 parent 6a25122 commit dbff730

File tree

2 files changed

+242
-4
lines changed

2 files changed

+242
-4
lines changed

‎src/Microsoft.ComponentDetection.Detectors/npm/NpmLockfile3Detector.cs‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ public NpmLockfile3Detector(IPathUtilityService pathUtilityService)
3838

3939
public override string Id => "NpmLockfile3";
4040

41-
public override int Version => 2;
41+
public override int Version => 3;
4242

4343
protected override bool IsSupportedLockfileVersion(int lockfileVersion) => lockfileVersion == 3;
4444

@@ -91,7 +91,11 @@ protected override void ProcessLockfile(
9191
continue;
9292
}
9393

94-
var previouslyAddedComponents = new HashSet<string> { component.Id };
94+
// Track component-parent pairs instead of just component IDs
95+
var previouslyAddedComponentParents = new HashSet<(string ComponentId, string? ParentId)>
96+
{
97+
(component.Id, null),
98+
};
9599
var subQueue = new Queue<(string Path, PackageLockV3Package Package, TypedComponent Parent)>();
96100

97101
// Record the top-level component
@@ -107,12 +111,19 @@ protected override void ProcessLockfile(
107111
var subName = NpmComponentUtilities.GetModuleName(subPath);
108112

109113
var subComponent = this.CreateComponent(subName, subPackage.Version, subPackage.Integrity);
110-
if (subComponent is null || previouslyAddedComponents.Contains(subComponent.Id))
114+
if (subComponent is null)
115+
{
116+
continue;
117+
}
118+
119+
// Only skip if this exact component-parent relationship was already added
120+
var componentParentPair = (subComponent.Id, parentComponent.Id);
121+
if (previouslyAddedComponentParents.Contains(componentParentPair))
111122
{
112123
continue;
113124
}
114125

115-
previouslyAddedComponents.Add(subComponent.Id);
126+
previouslyAddedComponentParents.Add(componentParentPair);
116127

117128
this.RecordComponent(singleFileComponentRecorder, subComponent, subPackage.Dev ?? false, component, parentComponent.Id);
118129

‎test/Microsoft.ComponentDetection.Detectors.Tests/NpmLockfile3DetectorTests.cs‎

Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,4 +369,231 @@ public async Task TestNpmDetector_PackageLockMissingPackagesProperty_ShouldNotTh
369369
var detectedComponents = componentRecorder.GetDetectedComponents();
370370
detectedComponents.Should().BeEmpty(); // No dependencies should be detected since packages is missing
371371
}
372+
373+
[TestMethod]
374+
public async Task TestNpmDetector_SameComponentWithDifferentParents_BothPathsRecordedAsync()
375+
{
376+
// This test verifies that if we have both a->b->c and a->c dependency paths,
377+
// both relationships are recorded (c appears twice with different parents)
378+
var componentA = (Name: "componentA", Version: "1.0.0");
379+
var componentB = (Name: "componentB", Version: "1.0.0");
380+
var componentC = (Name: "componentC", Version: "1.0.0");
381+
382+
var packageLockJson = @"{{
383+
""name"": ""test"",
384+
""version"": ""0.0.0"",
385+
""lockfileVersion"": 3,
386+
""requires"": true,
387+
""packages"": {{
388+
"""": {{
389+
""name"": ""test"",
390+
""version"": ""0.0.0"",
391+
""dependencies"": {{
392+
""{0}"": ""{1}""
393+
}}
394+
}},
395+
""node_modules/{0}"": {{
396+
""version"": ""{1}"",
397+
""resolved"": ""https://registry.npmjs.org/{0}/-/{0}-{1}.tgz"",
398+
""integrity"": ""sha512-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="",
399+
""dependencies"": {{
400+
""{2}"": ""{3}"",
401+
""{4}"": ""{5}""
402+
}}
403+
}},
404+
""node_modules/{2}"": {{
405+
""version"": ""{3}"",
406+
""resolved"": ""https://registry.npmjs.org/{2}/-/{2}-{3}.tgz"",
407+
""integrity"": ""sha512-BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB="",
408+
""dependencies"": {{
409+
""{4}"": ""{5}""
410+
}}
411+
}},
412+
""node_modules/{4}"": {{
413+
""version"": ""{5}"",
414+
""resolved"": ""https://registry.npmjs.org/{4}/-/{4}-{5}.tgz"",
415+
""integrity"": ""sha512-CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC=""
416+
}}
417+
}}
418+
}}";
419+
420+
var packageLockTemplate = string.Format(
421+
packageLockJson,
422+
componentA.Name,
423+
componentA.Version,
424+
componentB.Name,
425+
componentB.Version,
426+
componentC.Name,
427+
componentC.Version);
428+
429+
var packageJson = @"{{
430+
""name"": ""test"",
431+
""version"": ""0.0.0"",
432+
""dependencies"": {{
433+
""{0}"": ""{1}""
434+
}}
435+
}}";
436+
437+
var packageJsonTemplate = string.Format(
438+
packageJson,
439+
componentA.Name,
440+
componentA.Version);
441+
442+
var (scanResult, componentRecorder) = await this.DetectorTestUtility
443+
.WithFile(this.packageLockJsonFileName, packageLockTemplate, this.packageLockJsonSearchPatterns)
444+
.WithFile(this.packageJsonFileName, packageJsonTemplate, this.packageJsonSearchPattern)
445+
.ExecuteDetectorAsync();
446+
447+
scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);
448+
449+
var detectedComponents = componentRecorder.GetDetectedComponents().ToList();
450+
detectedComponents.Should().HaveCount(3);
451+
452+
// Get component IDs
453+
var componentAId = detectedComponents.First(c => ((NpmComponent)c.Component).Name.Equals(componentA.Name)).Component.Id;
454+
var componentBId = detectedComponents.First(c => ((NpmComponent)c.Component).Name.Equals(componentB.Name)).Component.Id;
455+
var componentCId = detectedComponents.First(c => ((NpmComponent)c.Component).Name.Equals(componentC.Name)).Component.Id;
456+
457+
var dependencyGraph = componentRecorder.GetDependencyGraphsByLocation().Values.First();
458+
459+
// Verify a->b and a->c relationships exist
460+
var aDependencies = dependencyGraph.GetDependenciesForComponent(componentAId);
461+
aDependencies.Should().HaveCount(2);
462+
aDependencies.Should().Contain(componentBId);
463+
aDependencies.Should().Contain(componentCId);
464+
465+
// Verify b->c relationship exists
466+
var bDependencies = dependencyGraph.GetDependenciesForComponent(componentBId);
467+
bDependencies.Should().HaveCount(1);
468+
bDependencies.Should().Contain(componentCId);
469+
470+
// Verify c has no dependencies
471+
var cDependencies = dependencyGraph.GetDependenciesForComponent(componentCId);
472+
cDependencies.Should().BeEmpty();
473+
474+
// Verify that c appears as a child of both a and b
475+
var parentsOfC = dependencyGraph.GetAncestors(componentCId);
476+
parentsOfC.Should().HaveCount(2);
477+
parentsOfC.Should().Contain(componentAId);
478+
parentsOfC.Should().Contain(componentBId);
479+
}
480+
481+
[TestMethod]
482+
public async Task TestNpmDetector_CircularDependency_HandledGracefullyAsync()
483+
{
484+
// This test verifies that circular dependencies (a->b->c->a) are handled without infinite loops
485+
// NPM can have circular dependencies in rare cases with peer dependencies or symlinks
486+
var componentA = (Name: "componentA", Version: "1.0.0");
487+
var componentB = (Name: "componentB", Version: "1.0.0");
488+
var componentC = (Name: "componentC", Version: "1.0.0");
489+
490+
var packageLockJson = @"{{
491+
""name"": ""test"",
492+
""version"": ""0.0.0"",
493+
""lockfileVersion"": 3,
494+
""requires"": true,
495+
""packages"": {{
496+
"""": {{
497+
""name"": ""test"",
498+
""version"": ""0.0.0"",
499+
""dependencies"": {{
500+
""{0}"": ""{1}""
501+
}}
502+
}},
503+
""node_modules/{0}"": {{
504+
""version"": ""{1}"",
505+
""resolved"": ""https://registry.npmjs.org/{0}/-/{0}-{1}.tgz"",
506+
""integrity"": ""sha512-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="",
507+
""dependencies"": {{
508+
""{2}"": ""{3}""
509+
}}
510+
}},
511+
""node_modules/{2}"": {{
512+
""version"": ""{3}"",
513+
""resolved"": ""https://registry.npmjs.org/{2}/-/{2}-{3}.tgz"",
514+
""integrity"": ""sha512-BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB="",
515+
""dependencies"": {{
516+
""{4}"": ""{5}""
517+
}}
518+
}},
519+
""node_modules/{4}"": {{
520+
""version"": ""{5}"",
521+
""resolved"": ""https://registry.npmjs.org/{4}/-/{4}-{5}.tgz"",
522+
""integrity"": ""sha512-CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC="",
523+
""dependencies"": {{
524+
""{0}"": ""{1}""
525+
}}
526+
}}
527+
}}
528+
}}";
529+
530+
var packageLockTemplate = string.Format(
531+
packageLockJson,
532+
componentA.Name,
533+
componentA.Version,
534+
componentB.Name,
535+
componentB.Version,
536+
componentC.Name,
537+
componentC.Version);
538+
539+
var packageJson = @"{{
540+
""name"": ""test"",
541+
""version"": ""0.0.0"",
542+
""dependencies"": {{
543+
""{0}"": ""{1}""
544+
}}
545+
}}";
546+
547+
var packageJsonTemplate = string.Format(
548+
packageJson,
549+
componentA.Name,
550+
componentA.Version);
551+
552+
var (scanResult, componentRecorder) = await this.DetectorTestUtility
553+
.WithFile(this.packageLockJsonFileName, packageLockTemplate, this.packageLockJsonSearchPatterns)
554+
.WithFile(this.packageJsonFileName, packageJsonTemplate, this.packageJsonSearchPattern)
555+
.ExecuteDetectorAsync();
556+
557+
// The detector should handle circular dependencies gracefully without hanging or throwing
558+
scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);
559+
560+
var detectedComponents = componentRecorder.GetDetectedComponents().ToList();
561+
detectedComponents.Should().HaveCount(3);
562+
563+
// Get component IDs
564+
var componentAId = detectedComponents.First(c => ((NpmComponent)c.Component).Name.Equals(componentA.Name)).Component.Id;
565+
var componentBId = detectedComponents.First(c => ((NpmComponent)c.Component).Name.Equals(componentB.Name)).Component.Id;
566+
var componentCId = detectedComponents.First(c => ((NpmComponent)c.Component).Name.Equals(componentC.Name)).Component.Id;
567+
568+
var dependencyGraph = componentRecorder.GetDependencyGraphsByLocation().Values.First();
569+
570+
// Verify the circular dependency chain is recorded
571+
// A depends on B
572+
var aDependencies = dependencyGraph.GetDependenciesForComponent(componentAId);
573+
aDependencies.Should().Contain(componentBId);
574+
575+
// B depends on C
576+
var bDependencies = dependencyGraph.GetDependenciesForComponent(componentBId);
577+
bDependencies.Should().Contain(componentCId);
578+
579+
// C depends on A (circular)
580+
var cDependencies = dependencyGraph.GetDependenciesForComponent(componentCId);
581+
cDependencies.Should().Contain(componentAId);
582+
583+
// Verify all three components are properly registered
584+
componentRecorder.AssertAllExplicitlyReferencedComponents<NpmComponent>(
585+
componentAId,
586+
parentComponent => parentComponent.Name == componentA.Name);
587+
588+
// B should be a transitive dependency of A (which is explicitly referenced)
589+
componentRecorder.IsDependencyOfExplicitlyReferencedComponents<NpmComponent>(
590+
componentBId,
591+
parentComponent => parentComponent.Name == componentA.Name).Should().BeTrue();
592+
593+
// C should also be a transitive dependency of A (reachable via A->B->C)
594+
// Even with the circular dependency C->A, C is still reachable from the explicitly referenced A
595+
componentRecorder.IsDependencyOfExplicitlyReferencedComponents<NpmComponent>(
596+
componentCId,
597+
parentComponent => parentComponent.Name == componentA.Name).Should().BeTrue();
598+
}
372599
}

0 commit comments

Comments
 (0)