Skip to content

Commit 4c82892

Browse files
refactor: precompute package graph; extract chainBuilder dfs
Address PR review comments on #296: - Precompute pkgByUID and parentsByUID once per result via buildPackageGraph, avoiding O(N*M) reconstruction inside the vulnerability loop. - buildDependencyChains now takes a targetUID + packageGraph; the call site uses vuln.PkgIdentifier.UID (with PURL fallback). - Extract the DFS into a chainBuilder struct with nodeName/tryRecord helpers, dropping the closure's cyclomatic complexity below the threshold. - tryRecord enforces maxDependencyChains for the cycle-fallback branch too, preventing one extra chain past the cap. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
1 parent 1342cba commit 4c82892

2 files changed

Lines changed: 102 additions & 72 deletions

File tree

internal/tool/tool.go

Lines changed: 84 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,9 @@ func (t codacyTrivy) getVulnerabilities(ctx context.Context, report ptypes.Repor
196196
lineNumberByPurl[pkg.Identifier.PURL.ToString()] = lineNumber
197197
}
198198

199+
// Precompute the package graph once per result; reused for every vulnerability's chain lookup.
200+
graph := buildPackageGraph(result.Packages)
201+
199202
// Ensure Trivy only produces results with severities matching the specified patterns.
200203
// Due to the way we invoke Trivy, this won't happen by simply setting it in the config.
201204
if err := tresult.FilterResult(ctx, &result, tresult.IgnoreConfig{}, tresult.FilterOptions{Severities: trivySeverities}); err != nil {
@@ -233,7 +236,11 @@ func (t codacyTrivy) getVulnerabilities(ctx context.Context, report ptypes.Repor
233236
return nil, err
234237
}
235238

236-
chains := buildDependencyChains(purl, result.Packages)
239+
targetUID := vuln.PkgIdentifier.UID
240+
if targetUID == "" {
241+
targetUID = graph.uidByPURL[purl]
242+
}
243+
chains := buildDependencyChains(targetUID, graph)
237244
extraFields, err := json.Marshal(map[string]any{
238245
"dependenciesChains": chains,
239246
"CVE": vuln.VulnerabilityID,
@@ -515,75 +522,98 @@ func unencodeComponents(bom *cdx.BOM) {
515522
}
516523
}
517524

518-
// buildDependencyChains enumerates root-to-vulnerable chains for a vulnerable PURL via DFS.
519-
// Each chain is ordered root-most first, vulnerable package last.
520-
// DependsOn in Trivy lists a package's children (what it depends on). To find parents
521-
// (what depends on the vulnerable package), we build an inverted map keyed by UID.
522-
// Limits: max maxDependencyChains chains; per-chain length capped at maxDependencyChainLen (tail kept).
523-
func buildDependencyChains(targetPURL string, packages []ftypes.Package) [][]string {
524-
// Map from UID to package for name resolution.
525-
pkgByUID := make(map[string]ftypes.Package, len(packages))
526-
// Inverted map: child UID -> parent UIDs (packages that depend on this child).
527-
parentsByUID := make(map[string][]string)
528-
targetUID := ""
525+
// packageGraph holds precomputed lookups over a result's packages. Building it once
526+
// per result avoids O(N*M) reconstruction inside the vulnerability loop.
527+
type packageGraph struct {
528+
// pkgByUID resolves a UID to its package (for display-name lookup).
529+
pkgByUID map[string]ftypes.Package
530+
// parentsByUID is the inverted dependency map: child UID -> UIDs that depend on it.
531+
// DependsOn in Trivy lists a package's children; we invert it to walk upward.
532+
parentsByUID map[string][]string
533+
// uidByPURL is a fallback for vulnerabilities that carry a PURL but no UID.
534+
uidByPURL map[string]string
535+
}
529536

537+
func buildPackageGraph(packages []ftypes.Package) packageGraph {
538+
g := packageGraph{
539+
pkgByUID: make(map[string]ftypes.Package, len(packages)),
540+
parentsByUID: make(map[string][]string),
541+
uidByPURL: make(map[string]string, len(packages)),
542+
}
530543
for _, pkg := range packages {
531544
uid := pkg.Identifier.UID
532-
pkgByUID[uid] = pkg
533-
if pkg.Identifier.PURL != nil && pkg.Identifier.PURL.ToString() == targetPURL {
534-
targetUID = uid
545+
g.pkgByUID[uid] = pkg
546+
if pkg.Identifier.PURL != nil {
547+
g.uidByPURL[pkg.Identifier.PURL.ToString()] = uid
535548
}
536549
for _, depUID := range pkg.DependsOn {
537-
parentsByUID[depUID] = append(parentsByUID[depUID], uid)
550+
g.parentsByUID[depUID] = append(g.parentsByUID[depUID], uid)
538551
}
539552
}
553+
return g
554+
}
540555

541-
if targetUID == "" {
556+
// buildDependencyChains enumerates root-to-vulnerable chains for the package identified
557+
// by targetUID via DFS. Each chain is ordered root-most first, vulnerable package last.
558+
// Limits: max maxDependencyChains chains; per-chain length capped at maxDependencyChainLen (tail kept).
559+
func buildDependencyChains(targetUID string, graph packageGraph) [][]string {
560+
if _, ok := graph.pkgByUID[targetUID]; !ok {
542561
return nil
543562
}
563+
b := chainBuilder{graph: graph}
564+
b.dfs(targetUID, nil, map[string]bool{})
565+
return b.out
566+
}
544567

545-
var out [][]string
568+
// chainBuilder isolates DFS state and helpers so the traversal stays modular and
569+
// each method has a low cyclomatic complexity.
570+
type chainBuilder struct {
571+
graph packageGraph
572+
out [][]string
573+
}
546574

547-
var dfs func(currentUID string, path []string, visited map[string]bool)
548-
dfs = func(currentUID string, path []string, visited map[string]bool) {
549-
if len(out) >= maxDependencyChains {
550-
return
551-
}
552-
if visited[currentUID] {
553-
return
554-
}
555-
visited[currentUID] = true
556-
defer delete(visited, currentUID)
557-
558-
pkg, ok := pkgByUID[currentUID]
559-
var name string
560-
if ok && pkg.Identifier.PURL != nil {
561-
name = purlPrettyPrint(*pkg.Identifier.PURL)
562-
} else {
563-
name = currentUID
564-
}
565-
path = append([]string{name}, path...)
575+
func (b *chainBuilder) nodeName(uid string) string {
576+
if pkg, ok := b.graph.pkgByUID[uid]; ok && pkg.Identifier.PURL != nil {
577+
return purlPrettyPrint(*pkg.Identifier.PURL)
578+
}
579+
return uid
580+
}
581+
582+
// tryRecord appends a chain unless the per-vulnerability chain cap is reached.
583+
// Returns true when a chain was recorded.
584+
func (b *chainBuilder) tryRecord(chain []string) bool {
585+
if len(b.out) >= maxDependencyChains {
586+
return false
587+
}
588+
b.out = append(b.out, trimChainTail(chain, maxDependencyChainLen))
589+
return true
590+
}
591+
592+
func (b *chainBuilder) dfs(currentUID string, path []string, visited map[string]bool) {
593+
if visited[currentUID] {
594+
return
595+
}
596+
visited[currentUID] = true
597+
defer delete(visited, currentUID)
566598

567-
parents := parentsByUID[currentUID]
568-
if len(parents) == 0 {
569-
out = append(out, trimChainTail(path, maxDependencyChainLen))
599+
path = append([]string{b.nodeName(currentUID)}, path...)
600+
601+
parents := b.graph.parentsByUID[currentUID]
602+
if len(parents) == 0 {
603+
b.tryRecord(path)
604+
return
605+
}
606+
before := len(b.out)
607+
for _, parentUID := range parents {
608+
if len(b.out) >= maxDependencyChains {
570609
return
571610
}
572-
before := len(out)
573-
for _, parentUID := range parents {
574-
if len(out) >= maxDependencyChains {
575-
return
576-
}
577-
dfs(parentUID, path, visited)
578-
}
579-
// All parent branches were blocked by cycle detection — treat current node as root.
580-
if len(out) == before {
581-
out = append(out, trimChainTail(path, maxDependencyChainLen))
582-
}
611+
b.dfs(parentUID, path, visited)
612+
}
613+
// All parent branches were blocked by cycle detection — treat current node as root.
614+
if len(b.out) == before {
615+
b.tryRecord(path)
583616
}
584-
585-
dfs(targetUID, nil, map[string]bool{})
586-
return out
587617
}
588618

589619
func trimChainTail(chain []string, max int) []string {

internal/tool/tool_test.go

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -925,9 +925,9 @@ func newPURL(pkgType, namespace, name, version string) *packageurl.PackageURL {
925925

926926
func TestBuildDependencyChains(t *testing.T) {
927927
type testData struct {
928-
packages []ftypes.Package
929-
targetPURL string
930-
expected [][]string
928+
packages []ftypes.Package
929+
targetUID string
930+
expected [][]string
931931
}
932932

933933
vulnPURL := newPURL("npm", "", "vuln-pkg", "1.0.0")
@@ -947,33 +947,33 @@ func TestBuildDependencyChains(t *testing.T) {
947947
packages: []ftypes.Package{
948948
makePkg(vulnUID, vulnPURL),
949949
},
950-
targetPURL: vulnUID,
951-
expected: [][]string{{"npm/vuln-pkg@1.0.0"}},
950+
targetUID: vulnUID,
951+
expected: [][]string{{"npm/vuln-pkg@1.0.0"}},
952952
},
953953
"single transitive chain — one parent": {
954954
packages: []ftypes.Package{
955955
makePkg(vulnUID, vulnPURL),
956956
makePkg(parentUID, parentPURL, vulnUID),
957957
},
958-
targetPURL: vulnUID,
959-
expected: [][]string{{"npm/parent-pkg@2.0.0", "npm/vuln-pkg@1.0.0"}},
958+
targetUID: vulnUID,
959+
expected: [][]string{{"npm/parent-pkg@2.0.0", "npm/vuln-pkg@1.0.0"}},
960960
},
961961
"deep transitive chain — two ancestors": {
962962
packages: []ftypes.Package{
963963
makePkg(vulnUID, vulnPURL),
964964
makePkg(parentUID, parentPURL, vulnUID),
965965
makePkg(grandparentUID, grandparentPURL, parentUID),
966966
},
967-
targetPURL: vulnUID,
968-
expected: [][]string{{"npm/grandparent-pkg@3.0.0", "npm/parent-pkg@2.0.0", "npm/vuln-pkg@1.0.0"}},
967+
targetUID: vulnUID,
968+
expected: [][]string{{"npm/grandparent-pkg@3.0.0", "npm/parent-pkg@2.0.0", "npm/vuln-pkg@1.0.0"}},
969969
},
970970
"multiple paths to root — two chains": {
971971
packages: []ftypes.Package{
972972
makePkg(vulnUID, vulnPURL),
973973
makePkg(sibling1UID, sibling1PURL, vulnUID),
974974
makePkg(sibling2UID, sibling2PURL, vulnUID),
975975
},
976-
targetPURL: vulnUID,
976+
targetUID: vulnUID,
977977
expected: [][]string{
978978
{"npm/sibling-1@1.0.0", "npm/vuln-pkg@1.0.0"},
979979
{"npm/sibling-2@1.0.0", "npm/vuln-pkg@1.0.0"},
@@ -983,16 +983,16 @@ func TestBuildDependencyChains(t *testing.T) {
983983
packages: []ftypes.Package{
984984
makePkg(parentUID, parentPURL, vulnUID),
985985
},
986-
targetPURL: vulnUID,
987-
expected: nil,
986+
targetUID: vulnUID,
987+
expected: nil,
988988
},
989989
"package without PURL — falls back to UID as name": {
990990
packages: []ftypes.Package{
991991
makePkg(vulnUID, vulnPURL),
992992
makePkg("no-purl-root", nil, vulnUID),
993993
},
994-
targetPURL: vulnUID,
995-
expected: [][]string{{"no-purl-root", "npm/vuln-pkg@1.0.0"}},
994+
targetUID: vulnUID,
995+
expected: [][]string{{"no-purl-root", "npm/vuln-pkg@1.0.0"}},
996996
},
997997
"max chains limit — stops at 10": {
998998
packages: func() []ftypes.Package {
@@ -1003,7 +1003,7 @@ func TestBuildDependencyChains(t *testing.T) {
10031003
}
10041004
return pkgs
10051005
}(),
1006-
targetPURL: vulnUID,
1006+
targetUID: vulnUID,
10071007
expected: func() [][]string {
10081008
var chains [][]string
10091009
for i := 0; i < maxDependencyChains; i++ {
@@ -1016,7 +1016,7 @@ func TestBuildDependencyChains(t *testing.T) {
10161016

10171017
for testName, testData := range testSet {
10181018
t.Run(testName, func(t *testing.T) {
1019-
result := buildDependencyChains(testData.targetPURL, testData.packages)
1019+
result := buildDependencyChains(testData.targetUID, buildPackageGraph(testData.packages))
10201020
assert.Equal(t, testData.expected, result)
10211021
})
10221022
}
@@ -1037,7 +1037,7 @@ func TestBuildDependencyChains_CycleTerminates(t *testing.T) {
10371037
makePkg(sibling2UID, sibling2PURL, vulnUID, sibling1UID),
10381038
}
10391039

1040-
result := buildDependencyChains(vulnUID, packages)
1040+
result := buildDependencyChains(vulnUID, buildPackageGraph(packages))
10411041

10421042
// Cycle guard must prevent infinite loop; exactly 2 finite chains produced
10431043
assert.Len(t, result, 2)
@@ -1062,7 +1062,7 @@ func TestBuildDependencyChains_ChainLengthTrimmed(t *testing.T) {
10621062
prevUID = uid
10631063
}
10641064

1065-
result := buildDependencyChains(vulnUID, packages)
1065+
result := buildDependencyChains(vulnUID, buildPackageGraph(packages))
10661066
if assert.Len(t, result, 1) {
10671067
chain := result[0]
10681068
assert.Len(t, chain, maxDependencyChainLen, "chain should be trimmed to maxDependencyChainLen")

0 commit comments

Comments
 (0)