Skip to content

Commit f6a33a4

Browse files
Hubtrick-GitHubtrick-Git
committed
implemented code review changes and refactored the code deeper
1 parent 64ed014 commit f6a33a4

10 files changed

Lines changed: 59 additions & 68 deletions

‎database/migrations/20260518100544_remove_obsolete_indexes_and_columns_from_component_dependencies.up.sql‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ DROP INDEX IF EXISTS dependency_purl_idx;
1616
-- remove the current id column and replace it with a composite key on all columns to make enforce deduplication on a data level and reduce complexity (also on indexes)
1717

1818
-- currently component_id can be NULL if its a root node; but primary keys cannot contain NULL values, so we replace it with a ROOT constant
19-
INSERT INTO public.components VALUES('ROOT','',NULL,NULL,NULL); -- add a ROOT component to the components table to be referenced
19+
INSERT INTO public.components (id, name, version, purl, cpe)
20+
VALUES('ROOT','',NULL,NULL,NULL) ON CONFLICT (id) DO NOTHING; -- add a ROOT component to the components table to be referenced
21+
2022
UPDATE public.component_dependencies SET component_id = 'ROOT' WHERE component_id IS NULL; -- use an explicit value for ROOT component dependencies instead of NULL
2123

2224
ALTER TABLE public.component_dependencies DROP CONSTRAINT component_dependencies_pkey, DROP COLUMN id; -- drop primary key constraint and the primary key column

‎database/models/component_model.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ type ComponentDependency struct {
6969
// this means, that the dependency graph between people using the same library might differ, since they use it differently
7070
// we use edges, which provide the information, that a component is used by another component in one asset
7171
Component Component `json:"component" gorm:"foreignKey:ComponentID;references:ID;constraint:OnDelete:CASCADE;"`
72-
ComponentID *string `json:"componentPurl" gorm:"column:component_id;index:component_purl_idx"` // will be ROOT, for direct dependencies
72+
ComponentID string `json:"componentPurl" gorm:"column:component_id;index:component_purl_idx"` // will be ROOT for direct dependencies
7373
Dependency Component `json:"dependency" gorm:"foreignKey:DependencyID;references:ID;constraint:OnDelete:CASCADE;"`
7474
DependencyID string `json:"dependencyPurl" gorm:"column:dependency_id;index:dependency_purl_idx"`
7575

@@ -92,7 +92,7 @@ func (c ComponentDependencyNode) GetID() string {
9292
func (c ComponentDependency) ToNodes() []ComponentDependencyNode {
9393
// a component dependency represents an edge in the dependency tree
9494
// thus we can represent it as two nodes
95-
return []ComponentDependencyNode{{ID: utils.SafeDereference(c.ComponentID)}, {ID: c.DependencyID}}
95+
return []ComponentDependencyNode{{ID: c.ComponentID}, {ID: c.DependencyID}}
9696
}
9797

9898
func resolveLicense(component ComponentDependency, componentLicenseOverwrites map[string]string) cyclonedx.Licenses {
@@ -241,7 +241,7 @@ func (c ComponentDependency) GetID() string {
241241
return c.DependencyID
242242
}
243243

244-
func (c ComponentDependency) GetDependentID() *string {
244+
func (c ComponentDependency) GetDependentID() string {
245245
return c.ComponentID
246246
}
247247

‎database/repositories/component_repository.go‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -148,8 +148,8 @@ func (c *componentRepository) LoadComponentsWithProject(ctx context.Context, tx
148148
componentDependencies[i].Dependency.License = &license
149149
componentDependencies[i].Dependency.IsLicenseOverwritten = true
150150
}
151-
if component.ComponentID != nil && *component.ComponentID != "ROOT" {
152-
if license, ok := isPurlOverwrittenMap[*component.ComponentID]; ok {
151+
if component.ComponentID != "ROOT" {
152+
if license, ok := isPurlOverwrittenMap[component.ComponentID]; ok {
153153
componentDependencies[i].Component.License = &license
154154
componentDependencies[i].Component.IsLicenseOverwritten = true
155155
}
@@ -197,15 +197,9 @@ func (c *componentRepository) HandleStateDiff(ctx context.Context, tx *gorm.DB,
197197
if len(diff.RemovedEdges) > 0 {
198198
var valueClauses []string
199199
for _, edge := range diff.RemovedEdges {
200-
var componentID string
201-
if edge[0] == normalize.GraphRootNodeID {
202-
componentID = "NULL"
203-
} else {
204-
componentID = fmt.Sprintf("'%s'", strings.ReplaceAll(edge[0], "'", "''"))
205-
}
206-
200+
escapedComp := strings.ReplaceAll(edge[0], "'", "''")
207201
escapedDep := strings.ReplaceAll(edge[1], "'", "''")
208-
valueClauses = append(valueClauses, fmt.Sprintf("(%s, '%s')", componentID, escapedDep))
202+
valueClauses = append(valueClauses, fmt.Sprintf("('%s', '%s')", escapedComp, escapedDep))
209203
}
210204
// Join the value clauses with commas
211205
values := strings.Join(valueClauses, ",")
@@ -244,7 +238,7 @@ func (c *componentRepository) HandleStateDiff(ctx context.Context, tx *gorm.DB,
244238
componentDependency := models.ComponentDependency{
245239
AssetID: assetVersion.AssetID,
246240
AssetVersionName: assetVersion.Name,
247-
ComponentID: utils.Ptr(componentID),
241+
ComponentID: componentID,
248242
DependencyID: c2.Component.PackageURL,
249243
}
250244

‎integrations/commonint/integration_helper_test.go‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -398,10 +398,10 @@ func TestRenderPathToComponent(t *testing.T) {
398398
t.Run("Everything works as expeted with a non empty component list", func(t *testing.T) {
399399
// Create a chain of actual components (all with pkg: prefix) to have a path with edges
400400
components := []models.ComponentDependency{
401-
{ComponentID: utils.Ptr("ROOT"), DependencyID: "artifact:test-artifact", Dependency: models.Component{ID: "artifact:test-artifact"}}, // root --> artifact
402-
{ComponentID: utils.Ptr("artifact:test-artifact"), DependencyID: "sbom:test@test-artifact", Dependency: models.Component{ID: "sbom:test@test-artifact"}}, // artifact -> sbom
403-
{ComponentID: utils.Ptr("sbom:test@test-artifact"), DependencyID: "pkg:npm/root-dep@1.0.0", Dependency: models.Component{ID: "pkg:npm/root-dep@1.0.0"}}, // sbom -> root-dep (component)
404-
{ComponentID: utils.Ptr("pkg:npm/root-dep@1.0.0"), DependencyID: "pkg:npm/test-package@1.0.0", Dependency: models.Component{ID: "pkg:npm/test-package@1.0.0"}}, // root-dep -> test-package (component)
401+
{ComponentID: "ROOT", DependencyID: "artifact:test-artifact", Dependency: models.Component{ID: "artifact:test-artifact"}}, // root --> artifact
402+
{ComponentID: "artifact:test-artifact", DependencyID: "sbom:test@test-artifact", Dependency: models.Component{ID: "sbom:test@test-artifact"}}, // artifact -> sbom
403+
{ComponentID: "sbom:test@test-artifact", DependencyID: "pkg:npm/root-dep@1.0.0", Dependency: models.Component{ID: "pkg:npm/root-dep@1.0.0"}}, // sbom -> root-dep (component)
404+
{ComponentID: "pkg:npm/root-dep@1.0.0", DependencyID: "pkg:npm/test-package@1.0.0", Dependency: models.Component{ID: "pkg:npm/test-package@1.0.0"}}, // root-dep -> test-package (component)
405405
}
406406
componentRepository := mocks.NewComponentRepository(t)
407407
componentRepository.On("LoadComponents", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(components, nil)
@@ -423,10 +423,10 @@ func TestRenderPathToComponent(t *testing.T) {
423423
t.Run("should escape @ symbols", func(t *testing.T) {
424424
// Create a chain of actual components to verify @ escaping in mermaid output
425425
components := []models.ComponentDependency{
426-
{ComponentID: utils.Ptr("ROOT"), DependencyID: "artifact:test-artifact", Dependency: models.Component{ID: "artifact:test-artifact"}}, // root --> artifact
427-
{ComponentID: utils.Ptr("artifact:test-artifact"), DependencyID: "sbom:test@test-artifact", Dependency: models.Component{ID: "sbom:test@test-artifact"}}, // artifact -> sbom
428-
{ComponentID: utils.Ptr("sbom:test@test-artifact"), DependencyID: "pkg:npm/root-dep@1.0.0", Dependency: models.Component{ID: "pkg:npm/root-dep@1.0.0"}}, // sbom -> root-dep (component)
429-
{ComponentID: utils.Ptr("pkg:npm/root-dep@1.0.0"), DependencyID: "pkg:npm/test-package@1.0.0", Dependency: models.Component{ID: "pkg:npm/test-package@1.0.0"}}, // root-dep -> test-package (component)
426+
{ComponentID: "ROOT", DependencyID: "artifact:test-artifact", Dependency: models.Component{ID: "artifact:test-artifact"}}, // root --> artifact
427+
{ComponentID: "artifact:test-artifact", DependencyID: "sbom:test@test-artifact", Dependency: models.Component{ID: "sbom:test@test-artifact"}}, // artifact -> sbom
428+
{ComponentID: "sbom:test@test-artifact", DependencyID: "pkg:npm/root-dep@1.0.0", Dependency: models.Component{ID: "pkg:npm/root-dep@1.0.0"}}, // sbom -> root-dep (component)
429+
{ComponentID: "pkg:npm/root-dep@1.0.0", DependencyID: "pkg:npm/test-package@1.0.0", Dependency: models.Component{ID: "pkg:npm/test-package@1.0.0"}}, // root-dep -> test-package (component)
430430
}
431431
componentRepository := mocks.NewComponentRepository(t)
432432
componentRepository.On("LoadComponents", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(components, nil)
@@ -452,9 +452,9 @@ func TestRenderPathToComponent(t *testing.T) {
452452
// The graph requires an artifact and sbom info-source node above the component
453453
// so that FindAllComponentOnlyPathsToPURL can terminate the BFS correctly.
454454
components := []models.ComponentDependency{
455-
{ComponentID: utils.Ptr("ROOT"), DependencyID: "artifact:test-artifact", Dependency: models.Component{ID: "artifact:test-artifact"}},
456-
{ComponentID: utils.Ptr("artifact:test-artifact"), DependencyID: "sbom:test@test-artifact", Dependency: models.Component{ID: "sbom:test@test-artifact"}},
457-
{ComponentID: utils.Ptr("sbom:test@test-artifact"), DependencyID: "pkg:npm/single@1.0.0", Dependency: models.Component{ID: "pkg:npm/single@1.0.0"}},
455+
{ComponentID: "ROOT", DependencyID: "artifact:test-artifact", Dependency: models.Component{ID: "artifact:test-artifact"}},
456+
{ComponentID: "artifact:test-artifact", DependencyID: "sbom:test@test-artifact", Dependency: models.Component{ID: "sbom:test@test-artifact"}},
457+
{ComponentID: "sbom:test@test-artifact", DependencyID: "pkg:npm/single@1.0.0", Dependency: models.Component{ID: "pkg:npm/single@1.0.0"}},
458458
}
459459
componentRepository := mocks.NewComponentRepository(t)
460460
componentRepository.On("LoadComponents", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(components, nil)
@@ -711,12 +711,12 @@ func TestTicketContentBitwiseReproducibility(t *testing.T) {
711711
// Previously, map iteration randomness caused the two path edges to appear
712712
// in non-deterministic order in the Mermaid output.
713713
components := []models.ComponentDependency{
714-
{ComponentID: utils.Ptr("ROOT"), DependencyID: "artifact:art", Dependency: models.Component{ID: "artifact:art"}},
715-
{ComponentID: utils.Ptr("artifact:art"), DependencyID: "sbom:s@art", Dependency: models.Component{ID: "sbom:s@art"}},
716-
{ComponentID: utils.Ptr("sbom:s@art"), DependencyID: "pkg:npm/route-b@1.0", Dependency: models.Component{ID: "pkg:npm/route-b@1.0"}},
717-
{ComponentID: utils.Ptr("sbom:s@art"), DependencyID: "pkg:npm/route-a@1.0", Dependency: models.Component{ID: "pkg:npm/route-a@1.0"}},
718-
{ComponentID: utils.Ptr("pkg:npm/route-a@1.0"), DependencyID: "pkg:npm/target@1.0", Dependency: models.Component{ID: "pkg:npm/target@1.0"}},
719-
{ComponentID: utils.Ptr("pkg:npm/route-b@1.0"), DependencyID: "pkg:npm/target@1.0", Dependency: models.Component{ID: "pkg:npm/target@1.0"}},
714+
{ComponentID: "ROOT", DependencyID: "artifact:art", Dependency: models.Component{ID: "artifact:art"}},
715+
{ComponentID: "artifact:art", DependencyID: "sbom:s@art", Dependency: models.Component{ID: "sbom:s@art"}},
716+
{ComponentID: "sbom:s@art", DependencyID: "pkg:npm/route-b@1.0", Dependency: models.Component{ID: "pkg:npm/route-b@1.0"}},
717+
{ComponentID: "sbom:s@art", DependencyID: "pkg:npm/route-a@1.0", Dependency: models.Component{ID: "pkg:npm/route-a@1.0"}},
718+
{ComponentID: "pkg:npm/route-a@1.0", DependencyID: "pkg:npm/target@1.0", Dependency: models.Component{ID: "pkg:npm/target@1.0"}},
719+
{ComponentID: "pkg:npm/route-b@1.0", DependencyID: "pkg:npm/target@1.0", Dependency: models.Component{ID: "pkg:npm/target@1.0"}},
720720
}
721721

722722
assetID := uuid.New()

‎mocks/mock_GraphComponent.go‎

Lines changed: 6 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎normalize/sbom_graph.go‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1831,7 +1831,7 @@ func ParseGraphNodeID(id string) (prefix, name string) {
18311831
// GraphComponent represents a component that can be loaded from the database.
18321832
type GraphComponent interface {
18331833
GetID() string
1834-
GetDependentID() *string
1834+
GetDependentID() string
18351835
ToCdxComponent(componentLicenseOverwrites map[string]string) (cdx.Component, error)
18361836
}
18371837

@@ -1845,10 +1845,8 @@ func SBOMGraphFromComponents[T GraphComponent](components []T, licenseOverwrites
18451845
// Build dependency map: parent -> children
18461846
dependencyMap := make(map[string][]string, len(components))
18471847
for _, c := range components {
1848-
var parentID string
1849-
if c.GetDependentID() != nil {
1850-
parentID = *c.GetDependentID()
1851-
} else {
1848+
parentID := c.GetDependentID()
1849+
if parentID == "" {
18521850
parentID = GraphRootNodeID
18531851
}
18541852
dependencyMap[parentID] = append(dependencyMap[parentID], c.GetID())

‎tests/component_service_integration_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ func TestGetAndSaveLicenseInformation(t *testing.T) {
9797
err = f.DB.Create(&models.ComponentDependency{
9898
AssetVersionName: assetVersion.Name,
9999
AssetID: assetVersion.AssetID,
100-
ComponentID: utils.Ptr("ROOT"),
100+
ComponentID: "ROOT",
101101
DependencyID: artifactRoot,
102102
}).Error
103103
assert.NoError(t, err)
@@ -107,21 +107,21 @@ func TestGetAndSaveLicenseInformation(t *testing.T) {
107107
{
108108
AssetVersionName: assetVersion.Name,
109109
AssetID: assetVersion.AssetID,
110-
ComponentID: &artifactRoot,
110+
ComponentID: artifactRoot,
111111
DependencyID: componentWithInvalidLicense.ID,
112112
Dependency: componentWithInvalidLicense,
113113
},
114114
{
115115
AssetVersionName: assetVersion.Name,
116116
AssetID: assetVersion.AssetID,
117-
ComponentID: &artifactRoot,
117+
ComponentID: artifactRoot,
118118
DependencyID: componentWithValidLicense.ID,
119119
Dependency: componentWithValidLicense,
120120
},
121121
{
122122
AssetVersionName: assetVersion.Name,
123123
AssetID: assetVersion.AssetID,
124-
ComponentID: &artifactRoot,
124+
ComponentID: artifactRoot,
125125
DependencyID: componentWithoutLicense.ID,
126126
Dependency: componentWithoutLicense,
127127
},
@@ -219,7 +219,7 @@ func TestGetAndSaveLicenseInformation(t *testing.T) {
219219
err = f.DB.Create(&models.ComponentDependency{
220220
AssetVersionName: assetVersion.Name,
221221
AssetID: assetVersion.AssetID,
222-
ComponentID: utils.Ptr("ROOT"),
222+
ComponentID: "ROOT",
223223
DependencyID: artifactRoot,
224224
}).Error
225225
assert.NoError(t, err)
@@ -228,7 +228,7 @@ func TestGetAndSaveLicenseInformation(t *testing.T) {
228228
componentDep := models.ComponentDependency{
229229
AssetVersionName: assetVersion.Name,
230230
AssetID: assetVersion.AssetID,
231-
ComponentID: &artifactRoot,
231+
ComponentID: artifactRoot,
232232
DependencyID: componentWithInvalidLicense.ID,
233233
Dependency: componentWithInvalidLicense,
234234
}

‎tests/daemon_pipeline_test.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func createSBOMStructure(f *TestFixture, asset models.Asset, assetVersion models
5151
artifactRootDep := models.ComponentDependency{
5252
AssetID: asset.ID,
5353
AssetVersionName: assetVersion.Name,
54-
ComponentID: utils.Ptr("ROOT"),
54+
ComponentID: "ROOT",
5555
DependencyID: artifactRoot,
5656
}
5757
if err := f.DB.Create(&artifactRootDep).Error; err != nil {
@@ -62,7 +62,7 @@ func createSBOMStructure(f *TestFixture, asset models.Asset, assetVersion models
6262
infoSourceDep := models.ComponentDependency{
6363
AssetID: asset.ID,
6464
AssetVersionName: assetVersion.Name,
65-
ComponentID: &artifactRoot,
65+
ComponentID: artifactRoot,
6666
DependencyID: infoSourceID,
6767
}
6868
if err := f.DB.Create(&infoSourceDep).Error; err != nil {
@@ -74,7 +74,7 @@ func createSBOMStructure(f *TestFixture, asset models.Asset, assetVersion models
7474
componentDependency := models.ComponentDependency{
7575
AssetID: asset.ID,
7676
AssetVersionName: assetVersion.Name,
77-
ComponentID: &infoSourceID,
77+
ComponentID: infoSourceID,
7878
DependencyID: purl,
7979
}
8080
if err := f.DB.Create(&componentDependency).Error; err != nil {
@@ -101,7 +101,7 @@ func createVEXStructure(f *TestFixture, asset models.Asset, assetVersion models.
101101
vexInfoSourceDep := models.ComponentDependency{
102102
AssetID: asset.ID,
103103
AssetVersionName: assetVersion.Name,
104-
ComponentID: &artifactRoot,
104+
ComponentID: artifactRoot,
105105
DependencyID: vexInfoSourceID,
106106
}
107107
if err := f.DB.Create(&vexInfoSourceDep).Error; err != nil {
@@ -114,7 +114,7 @@ func createVEXStructure(f *TestFixture, asset models.Asset, assetVersion models.
114114
componentDependency := models.ComponentDependency{
115115
AssetID: asset.ID,
116116
AssetVersionName: assetVersion.Name,
117-
ComponentID: &vexInfoSourceID,
117+
ComponentID: vexInfoSourceID,
118118
DependencyID: purl,
119119
}
120120
if err := f.DB.Create(&componentDependency).Error; err != nil {
@@ -179,7 +179,7 @@ func TestDaemonPipelineEndToEnd(t *testing.T) {
179179
artifactRootDep := models.ComponentDependency{
180180
AssetID: asset.ID,
181181
AssetVersionName: assetVersion.Name,
182-
ComponentID: utils.Ptr("ROOT"),
182+
ComponentID: "ROOT",
183183
DependencyID: artifactRoot,
184184
}
185185
err = f.DB.Create(&artifactRootDep).Error
@@ -189,7 +189,7 @@ func TestDaemonPipelineEndToEnd(t *testing.T) {
189189
componentDependency := models.ComponentDependency{
190190
AssetID: asset.ID,
191191
AssetVersionName: assetVersion.Name,
192-
ComponentID: &artifactRoot,
192+
ComponentID: artifactRoot,
193193
DependencyID: "pkg:npm/test-package@1.0.0",
194194
Dependency: component,
195195
}
@@ -580,7 +580,7 @@ func TestDaemonPipelineScanAssetDetectVulns(t *testing.T) {
580580
artifactRootDep := models.ComponentDependency{
581581
AssetID: asset.ID,
582582
AssetVersionName: assetVersion.Name,
583-
ComponentID: utils.Ptr("ROOT"),
583+
ComponentID: "ROOT",
584584
DependencyID: artifactRoot,
585585
}
586586
err = f.DB.Create(&artifactRootDep).Error
@@ -590,7 +590,7 @@ func TestDaemonPipelineScanAssetDetectVulns(t *testing.T) {
590590
componentDependency := models.ComponentDependency{
591591
AssetID: asset.ID,
592592
AssetVersionName: assetVersion.Name,
593-
ComponentID: &artifactRoot,
593+
ComponentID: artifactRoot,
594594
DependencyID: "pkg:npm/vulnerable-package@2.0.0",
595595
Dependency: component,
596596
}
@@ -772,7 +772,7 @@ func TestDaemonPipelineRiskCalculation(t *testing.T) {
772772
artifactRootDep := models.ComponentDependency{
773773
AssetID: asset.ID,
774774
AssetVersionName: assetVersion.Name,
775-
ComponentID: utils.Ptr("ROOT"),
775+
ComponentID: "ROOT",
776776
DependencyID: artifactRoot,
777777
}
778778
err = f.DB.Create(&artifactRootDep).Error
@@ -782,7 +782,7 @@ func TestDaemonPipelineRiskCalculation(t *testing.T) {
782782
componentDependency := models.ComponentDependency{
783783
AssetID: asset.ID,
784784
AssetVersionName: assetVersion.Name,
785-
ComponentID: &artifactRoot,
785+
ComponentID: artifactRoot,
786786
DependencyID: "pkg:npm/risk-test-package@1.0.0",
787787
Dependency: component,
788788
}

0 commit comments

Comments
 (0)