Skip to content

Commit f97ff44

Browse files
jasonpaulosjasonpaulos
committed
Address review feedback
1 parent 8b46f92 commit f97ff44

2 files changed

Lines changed: 145 additions & 11 deletions

File tree

‎src/Microsoft.ComponentDetection.Orchestrator/Services/GraphTranslation/DefaultGraphTranslationService.cs‎

Lines changed: 51 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ public ScanResult GenerateScanResultFromProcessingResult(
4545
if (settings.FilterBaseImageComponents)
4646
{
4747
componentsToOutput = FilterOutBaseImageComponents(componentsToOutput, detectorProcessingResult.ContainersDetailsMap);
48+
PruneFilteredComponentsFromGraphs(dependencyGraphs, componentsToOutput);
4849
}
4950

5051
return new DefaultGraphScanResult
@@ -74,10 +75,20 @@ internal static List<DetectedComponent> FilterOutBaseImageComponents(
7475
return components;
7576
}
7677

77-
return components.Where(component => !IsExclusivelyFromBaseImage(component, containerDetailsMap)).ToList();
78+
// Build an indexed lookup: containerDetailId → (layerIndex → DockerLayer)
79+
var layerLookup = new Dictionary<int, Dictionary<int, DockerLayer>>();
80+
foreach (var (id, details) in containerDetailsMap)
81+
{
82+
if (details.Layers != null)
83+
{
84+
layerLookup[id] = details.Layers.ToDictionary(l => l.LayerIndex);
85+
}
86+
}
87+
88+
return components.Where(component => !IsExclusivelyFromBaseImage(component, layerLookup)).ToList();
7889
}
7990

80-
private static bool IsExclusivelyFromBaseImage(DetectedComponent component, Dictionary<int, ContainerDetails> containerDetailsMap)
91+
private static bool IsExclusivelyFromBaseImage(DetectedComponent component, Dictionary<int, Dictionary<int, DockerLayer>> layerLookup)
8192
{
8293
// Components without container layer references are not from a container scan - keep them.
8394
if (component.ContainerLayerIds == null || component.ContainerLayerIds.Count == 0)
@@ -87,18 +98,15 @@ private static bool IsExclusivelyFromBaseImage(DetectedComponent component, Dict
8798

8899
foreach (var (containerDetailId, layerIndices) in component.ContainerLayerIds)
89100
{
90-
if (!containerDetailsMap.TryGetValue(containerDetailId, out var containerDetails) || containerDetails.Layers == null)
101+
if (!layerLookup.TryGetValue(containerDetailId, out var layersByIndex))
91102
{
92103
// If we can't resolve the container details, assume it's not a base image component.
93104
return false;
94105
}
95106

96-
var layersList = containerDetails.Layers.ToList();
97-
98107
foreach (var layerIndex in layerIndices)
99108
{
100-
var layer = layersList.FirstOrDefault(l => l.LayerIndex == layerIndex);
101-
if (layer == null || !layer.IsBaseImage)
109+
if (!layersByIndex.TryGetValue(layerIndex, out var layer) || !layer.IsBaseImage)
102110
{
103111
// Layer not found or not from base image. Keep this component.
104112
return false;
@@ -109,6 +117,42 @@ private static bool IsExclusivelyFromBaseImage(DetectedComponent component, Dict
109117
return true;
110118
}
111119

120+
/// <summary>
121+
/// Removes component IDs from dependency graphs that are no longer present in the output components list.
122+
/// </summary>
123+
private static void PruneFilteredComponentsFromGraphs(DependencyGraphCollection graphs, List<DetectedComponent> retainedComponents)
124+
{
125+
if (graphs == null || graphs.Count == 0)
126+
{
127+
return;
128+
}
129+
130+
var retainedIds = new HashSet<string>(retainedComponents.Select(c => c.Component.Id));
131+
132+
foreach (var graphWithMetadata in graphs.Values)
133+
{
134+
var graph = graphWithMetadata.Graph;
135+
136+
// Remove nodes that are no longer in retained components.
137+
var idsToRemove = graph.Keys.Where(id => !retainedIds.Contains(id)).ToList();
138+
foreach (var id in idsToRemove)
139+
{
140+
graph.Remove(id);
141+
}
142+
143+
// Remove references to removed IDs from remaining nodes' dependency sets.
144+
foreach (var edges in graph.Values)
145+
{
146+
edges?.RemoveWhere(id => !retainedIds.Contains(id));
147+
}
148+
149+
// Clean up metadata sets.
150+
graphWithMetadata.ExplicitlyReferencedComponentIds?.RemoveWhere(id => !retainedIds.Contains(id));
151+
graphWithMetadata.DevelopmentDependencies?.RemoveWhere(id => !retainedIds.Contains(id));
152+
graphWithMetadata.Dependencies?.RemoveWhere(id => !retainedIds.Contains(id));
153+
}
154+
}
155+
112156
private static ConcurrentHashSet<string> MergeTargetFrameworks(ConcurrentHashSet<string> left, ConcurrentHashSet<string> right)
113157
{
114158
if (left == null && right == null)

‎test/Microsoft.ComponentDetection.Orchestrator.Tests/Services/DefaultGraphTranslationServiceTests.cs‎

Lines changed: 94 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -568,7 +568,7 @@ public void GenerateScanResult_MultipleRichAndBare_BareGraphDataAbsorbedByAllRic
568568
[TestMethod]
569569
public void FilterBaseImageComponents_RemovesComponentsExclusivelyFromBaseImageLayers()
570570
{
571-
var singleFileRecorder = this.componentRecorder.CreateSingleFileComponentRecorder(Path.Join(this.sourceDirectory.FullName, "/file1"));
571+
var singleFileRecorder = this.componentRecorder.CreateSingleFileComponentRecorder(Path.Join(this.sourceDirectory.FullName, "file1"));
572572

573573
var baseImageComponent = new DetectedComponent(new NpmComponent("base-pkg", "1.0.0"), containerDetailsId: 1, containerLayerId: 0);
574574
singleFileRecorder.RegisterUsage(baseImageComponent);
@@ -598,7 +598,7 @@ public void FilterBaseImageComponents_RemovesComponentsExclusivelyFromBaseImageL
598598
[TestMethod]
599599
public void FilterBaseImageComponents_RetainsComponentsWithMixedLayers()
600600
{
601-
var singleFileRecorder = this.componentRecorder.CreateSingleFileComponentRecorder(Path.Join(this.sourceDirectory.FullName, "/file1"));
601+
var singleFileRecorder = this.componentRecorder.CreateSingleFileComponentRecorder(Path.Join(this.sourceDirectory.FullName, "file1"));
602602

603603
var mixedComponent = new DetectedComponent(new NpmComponent("mixed-pkg", "1.0.0"), containerDetailsId: 1, containerLayerId: 0);
604604
mixedComponent.ContainerLayerIds[1] = [0, 1];
@@ -630,7 +630,7 @@ public void FilterBaseImageComponents_RetainsComponentsWithMixedLayers()
630630
[TestMethod]
631631
public void FilterBaseImageComponents_RetainsComponentsWithNoContainerReferences()
632632
{
633-
var singleFileRecorder = this.componentRecorder.CreateSingleFileComponentRecorder(Path.Join(this.sourceDirectory.FullName, "/file1"));
633+
var singleFileRecorder = this.componentRecorder.CreateSingleFileComponentRecorder(Path.Join(this.sourceDirectory.FullName, "file1"));
634634

635635
var filesystemComponent = new DetectedComponent(new NpmComponent("fs-pkg", "2.0.0"));
636636
singleFileRecorder.RegisterUsage(filesystemComponent);
@@ -661,7 +661,7 @@ public void FilterBaseImageComponents_RetainsComponentsWithNoContainerReferences
661661
[TestMethod]
662662
public void FilterBaseImageComponents_NoOpWhenFlagIsDisabled()
663663
{
664-
var singleFileRecorder = this.componentRecorder.CreateSingleFileComponentRecorder(Path.Join(this.sourceDirectory.FullName, "/file1"));
664+
var singleFileRecorder = this.componentRecorder.CreateSingleFileComponentRecorder(Path.Join(this.sourceDirectory.FullName, "file1"));
665665

666666
var baseImageComponent = new DetectedComponent(new NpmComponent("base-pkg", "1.0.0"), containerDetailsId: 1, containerLayerId: 0);
667667
singleFileRecorder.RegisterUsage(baseImageComponent);
@@ -688,4 +688,94 @@ public void FilterBaseImageComponents_NoOpWhenFlagIsDisabled()
688688
result.ComponentsFound.Should().HaveCount(1);
689689
((NpmComponent)result.ComponentsFound.Single().Component).Name.Should().Be("base-pkg");
690690
}
691+
692+
[TestMethod]
693+
public void FilterBaseImageComponents_PrunesFilteredComponentsFromDependencyGraphs()
694+
{
695+
var filePath = Path.Join(this.sourceDirectory.FullName, "file1");
696+
var singleFileRecorder = this.componentRecorder.CreateSingleFileComponentRecorder(filePath);
697+
698+
// Register a retained (non-base-image) component and a base-image component with a dependency edge.
699+
var retainedComponent = new DetectedComponent(new NpmComponent("app-pkg", "1.0.0"), containerDetailsId: 1, containerLayerId: 1);
700+
var baseImageComponent = new DetectedComponent(new NpmComponent("base-pkg", "2.0.0"), containerDetailsId: 1, containerLayerId: 0);
701+
702+
singleFileRecorder.RegisterUsage(retainedComponent, isExplicitReferencedDependency: true);
703+
singleFileRecorder.RegisterUsage(baseImageComponent, parentComponentId: retainedComponent.Component.Id);
704+
705+
var containerDetailsMap = new Dictionary<int, ContainerDetails>
706+
{
707+
[1] = new ContainerDetails
708+
{
709+
Id = 1,
710+
Layers = [new DockerLayer { LayerIndex = 0, IsBaseImage = true }, new DockerLayer { LayerIndex = 1, IsBaseImage = false }],
711+
},
712+
};
713+
714+
var processingResult = new DetectorProcessingResult
715+
{
716+
ResultCode = ProcessingResultCode.Success,
717+
ContainersDetailsMap = containerDetailsMap,
718+
ComponentRecorders = [(this.componentDetectorMock.Object, this.componentRecorder)],
719+
};
720+
721+
var result = (DefaultGraphScanResult)this.serviceUnderTest.GenerateScanResultFromProcessingResult(
722+
processingResult, new ScanSettings { SourceDirectory = this.sourceDirectory, FilterBaseImageComponents = true });
723+
724+
// Only the non-base-image component should remain.
725+
result.ComponentsFound.Should().HaveCount(1);
726+
((NpmComponent)result.ComponentsFound.Single().Component).Name.Should().Be("app-pkg");
727+
728+
// The dependency graph should not reference the filtered component.
729+
var graphEntry = result.DependencyGraphs.Should().ContainSingle().Which;
730+
var graph = graphEntry.Value.Graph;
731+
graph.Should().ContainKey(retainedComponent.Component.Id);
732+
graph.Should().NotContainKey(baseImageComponent.Component.Id);
733+
734+
// The retained component's edge set should not reference the removed component.
735+
var retainedEdges = graph[retainedComponent.Component.Id];
736+
retainedEdges?.Should().NotContain(baseImageComponent.Component.Id);
737+
738+
// Metadata sets should also be cleaned.
739+
graphEntry.Value.ExplicitlyReferencedComponentIds.Should().NotContain(baseImageComponent.Component.Id);
740+
}
741+
742+
[TestMethod]
743+
public void FilterBaseImageComponents_DependencyGraphsUnchangedWhenFlagDisabled()
744+
{
745+
var filePath = Path.Join(this.sourceDirectory.FullName, "file1");
746+
var singleFileRecorder = this.componentRecorder.CreateSingleFileComponentRecorder(filePath);
747+
748+
var retainedComponent = new DetectedComponent(new NpmComponent("app-pkg", "1.0.0"), containerDetailsId: 1, containerLayerId: 1);
749+
var baseImageComponent = new DetectedComponent(new NpmComponent("base-pkg", "2.0.0"), containerDetailsId: 1, containerLayerId: 0);
750+
751+
singleFileRecorder.RegisterUsage(retainedComponent, isExplicitReferencedDependency: true);
752+
singleFileRecorder.RegisterUsage(baseImageComponent, parentComponentId: retainedComponent.Component.Id);
753+
754+
var containerDetailsMap = new Dictionary<int, ContainerDetails>
755+
{
756+
[1] = new ContainerDetails
757+
{
758+
Id = 1,
759+
Layers = [new DockerLayer { LayerIndex = 0, IsBaseImage = true }, new DockerLayer { LayerIndex = 1, IsBaseImage = false }],
760+
},
761+
};
762+
763+
var processingResult = new DetectorProcessingResult
764+
{
765+
ResultCode = ProcessingResultCode.Success,
766+
ContainersDetailsMap = containerDetailsMap,
767+
ComponentRecorders = [(this.componentDetectorMock.Object, this.componentRecorder)],
768+
};
769+
770+
var result = (DefaultGraphScanResult)this.serviceUnderTest.GenerateScanResultFromProcessingResult(
771+
processingResult, new ScanSettings { SourceDirectory = this.sourceDirectory, FilterBaseImageComponents = false });
772+
773+
// Both components should be present.
774+
result.ComponentsFound.Should().HaveCount(2);
775+
776+
// The dependency graph should still contain both component IDs.
777+
var graph = result.DependencyGraphs.Single().Value.Graph;
778+
graph.Should().ContainKey(retainedComponent.Component.Id);
779+
graph.Should().ContainKey(baseImageComponent.Component.Id);
780+
}
691781
}

0 commit comments

Comments
 (0)