Merge pull request #1628 from l3montree-dev/1627-dependency-vulnerabilities-are-not-removed-after-deleting-the-related-artifact

fix artifact deletion logic and add tests for dependency vulnerability handling after delete artifacts

Author: timbastin

database/repositories/artifact_repository.go

@@ -54,15 +54,21 @@ func (r *artifactRepository) ReadArtifact(name string, assetVersionName string,
 	return artifact, err
 }
 
-func (r *artifactRepository) DeleteArtifact(assetID uuid.UUID, assetVersionName string, artifactName string) error {
-	err := r.db.Where("artifact_name = ? AND asset_version_name = ? AND asset_id = ?", artifactName, assetVersionName, assetID).Delete(&models.Artifact{}).Error
+func (r *artifactRepository) DeleteArtifact(tx *gorm.DB, assetID uuid.UUID, assetVersionName string, artifactName string) error {
+
+	db := r.db
+	if tx != nil {
+		db = tx
+	}
+
+	err := db.Where("artifact_name = ? AND asset_version_name = ? AND asset_id = ?", artifactName, assetVersionName, assetID).Delete(&models.Artifact{}).Error
 	if err != nil {
 		return err
 	}
 
 	r.FireAndForget(func() {
 		sql := CleanupOrphanedRecordsSQL
-		err = r.db.Exec(sql).Error
+		err = db.Exec(sql).Error
 		if err != nil {
 			slog.Error("Failed to clean up orphaned records after deleting artifact", "err", err)
 		}

mocks/mock_ArtifactRepository.go

@@ -74,7 +74,7 @@ func (s *ArtifactService) DeleteArtifact(assetID uuid.UUID, assetVersionName str
 		}
 
 		// Delete the artifact record itself
-		err = s.artifactRepository.GetDB(tx).Where("asset_id = ? AND asset_version_name = ? AND artifact_name = ?", assetID, assetVersionName, artifactName).Delete(&models.Artifact{}).Error
+		err = s.artifactRepository.DeleteArtifact(tx, assetID, assetVersionName, artifactName)
 		if err != nil {
 			slog.Error("failed to delete artifact record", "assetID", assetID, "assetVersionName", assetVersionName, "artifactName", artifactName, "error", err)
 			return err

services/artifact_service.go

@@ -136,7 +136,7 @@ type ArtifactRepository interface {
 	utils.Repository[string, models.Artifact, DB]
 	GetByAssetIDAndAssetVersionName(assetID uuid.UUID, assetVersionName string) ([]models.Artifact, error)
 	ReadArtifact(name string, assetVersionName string, assetID uuid.UUID) (models.Artifact, error)
-	DeleteArtifact(assetID uuid.UUID, assetVersionName string, artifactName string) error
+	DeleteArtifact(tx DB, assetID uuid.UUID, assetVersionName string, artifactName string) error
 	GetAllArtifactAffectedByDependencyVuln(tx DB, vulnID string) ([]models.Artifact, error)
 	GetByAssetVersions(assetID uuid.UUID, assetVersionNames []string) ([]models.Artifact, error)
 }

shared/common_interfaces.go

@@ -3,7 +3,6 @@ package tests
 import (
 	"net/http/httptest"
 	"testing"
-	"time"
 
 	"github.com/l3montree-dev/devguard/database/models"
 	"github.com/l3montree-dev/devguard/dtos"
@@ -124,9 +123,6 @@ func TestArtifactControllerDeleteArtifact(t *testing.T) {
 			err := f.App.ArtifactController.DeleteArtifact(ctx)
 			assert.NoError(t, err)
 
-			// Wait for Fire and Forget to complete
-			time.Sleep(500 * time.Millisecond)
-
 			// Verify artifact was deleted
 			var deletedArtifact models.Artifact
 			err = f.DB.First(&deletedArtifact, "artifact_name = ? AND asset_id = ?", artifact1.ArtifactName, asset.ID).Error
@@ -216,5 +212,153 @@ func TestArtifactControllerDeleteArtifact(t *testing.T) {
 			err = f.DB.First(&deletedArtifact, "artifact_name = ? AND asset_id = ?", artifact3.ArtifactName, asset.ID).Error
 			assert.Error(t, err, "Artifact should be deleted")
 		})
+
+		t.Run("should delete dependency vuln when it only belongs to the deleted artifact", func(t *testing.T) {
+			// Create a new artifact for this test
+			artifactSingle := models.Artifact{
+				ArtifactName:     "artifact-single-vuln",
+				AssetVersionName: assetVersion.Name,
+				AssetID:          asset.ID,
+			}
+			assert.NoError(t, f.DB.Create(&artifactSingle).Error)
+
+			// Create a component for this test
+			componentSingle := models.Component{
+				ID: "pkg:npm/single-artifact-vuln-package@1.0.0",
+			}
+			assert.NoError(t, f.DB.Create(&componentSingle).Error)
+
+			// Create a vulnerability that only belongs to this artifact
+			vulnSingle := models.DependencyVuln{
+				Vulnerability: models.Vulnerability{
+					AssetID:          asset.ID,
+					AssetVersionName: assetVersion.Name,
+					State:            dtos.VulnStateOpen,
+				},
+				CVEID:             cve.CVE,
+				ComponentPurl:     componentSingle.ID,
+				VulnerabilityPath: []string{"root", "artifact:artifact-single-vuln", componentSingle.ID},
+				Artifacts: []models.Artifact{
+					artifactSingle,
+				},
+			}
+			assert.NoError(t, f.DB.Create(&vulnSingle).Error)
+
+			// Verify vulnerability exists before deletion
+			var vulnBefore models.DependencyVuln
+			assert.NoError(t, f.DB.First(&vulnBefore, "id = ?", vulnSingle.ID).Error)
+
+			// Create echo context for delete request
+			e := echo.New()
+			req := httptest.NewRequest("DELETE", "/", nil)
+			rec := httptest.NewRecorder()
+			ctx := e.NewContext(req, rec)
+
+			// Set context values
+			ctx.Set("organization", org)
+			ctx.Set("project", project)
+			ctx.Set("asset", asset)
+			ctx.Set("assetVersion", assetVersion)
+			ctx.Set("artifact", artifactSingle)
+			ctx.Set("session", NewSessionMock("test-user"))
+
+			// Execute delete
+			err := f.App.ArtifactController.DeleteArtifact(ctx)
+			assert.NoError(t, err)
+			assert.Equal(t, 200, rec.Code)
+
+			// Verify artifact was deleted
+			var deletedArtifact models.Artifact
+			err = f.DB.First(&deletedArtifact, "artifact_name = ? AND asset_id = ?", artifactSingle.ArtifactName, asset.ID).Error
+			assert.Error(t, err, "Artifact should be deleted")
+
+			// Verify the dependency vulnerability was also deleted
+			var vulnAfter models.DependencyVuln
+			err = f.DB.First(&vulnAfter, "id = ?", vulnSingle.ID).Error
+			assert.Error(t, err, "Dependency vulnerability should be deleted when its only artifact is deleted")
+		})
+
+		t.Run("should keep dependency vuln but remove artifact relation when vuln belongs to multiple artifacts", func(t *testing.T) {
+			// Create two artifacts for this test
+			artifactMulti1 := models.Artifact{
+				ArtifactName:     "artifact-multi-1",
+				AssetVersionName: assetVersion.Name,
+				AssetID:          asset.ID,
+			}
+			assert.NoError(t, f.DB.Create(&artifactMulti1).Error)
+
+			artifactMulti2 := models.Artifact{
+				ArtifactName:     "artifact-multi-2",
+				AssetVersionName: assetVersion.Name,
+				AssetID:          asset.ID,
+			}
+			assert.NoError(t, f.DB.Create(&artifactMulti2).Error)
+
+			// Create a component for this test
+			componentMulti := models.Component{
+				ID: "pkg:npm/multi-artifact-vuln-package@2.0.0",
+			}
+			assert.NoError(t, f.DB.Create(&componentMulti).Error)
+
+			// Create a vulnerability that belongs to both artifacts
+			vulnMulti := models.DependencyVuln{
+				Vulnerability: models.Vulnerability{
+					AssetID:          asset.ID,
+					AssetVersionName: assetVersion.Name,
+					State:            dtos.VulnStateOpen,
+				},
+				CVEID:             cve.CVE,
+				ComponentPurl:     componentMulti.ID,
+				VulnerabilityPath: []string{"root", "artifact:artifact-multi-1", componentMulti.ID},
+				Artifacts: []models.Artifact{
+					artifactMulti1,
+					artifactMulti2,
+				},
+			}
+			assert.NoError(t, f.DB.Create(&vulnMulti).Error)
+
+			// Verify vulnerability exists with two artifacts before deletion
+			var vulnBefore models.DependencyVuln
+			assert.NoError(t, f.DB.Preload("Artifacts").First(&vulnBefore, "id = ?", vulnMulti.ID).Error)
+			assert.Len(t, vulnBefore.Artifacts, 2, "Vulnerability should have 2 artifacts before deletion")
+
+			// Create echo context for delete request - delete artifact 1
+			e := echo.New()
+			req := httptest.NewRequest("DELETE", "/", nil)
+			rec := httptest.NewRecorder()
+			ctx := e.NewContext(req, rec)
+
+			// Set context values
+			ctx.Set("organization", org)
+			ctx.Set("project", project)
+			ctx.Set("asset", asset)
+			ctx.Set("assetVersion", assetVersion)
+			ctx.Set("artifact", artifactMulti1)
+			ctx.Set("session", NewSessionMock("test-user"))
+
+			// Execute delete
+			err := f.App.ArtifactController.DeleteArtifact(ctx)
+			assert.NoError(t, err)
+			assert.Equal(t, 200, rec.Code)
+
+			// Verify artifact1 was deleted
+			var deletedArtifact models.Artifact
+			err = f.DB.First(&deletedArtifact, "artifact_name = ? AND asset_id = ?", artifactMulti1.ArtifactName, asset.ID).Error
+			assert.Error(t, err, "Artifact1 should be deleted")
+
+			// Verify artifact2 still exists
+			var remainingArtifact models.Artifact
+			err = f.DB.First(&remainingArtifact, "artifact_name = ? AND asset_id = ?", artifactMulti2.ArtifactName, asset.ID).Error
+			assert.NoError(t, err, "Artifact2 should still exist")
+
+			// Verify the dependency vulnerability still exists
+			var vulnAfter models.DependencyVuln
+			err = f.DB.Preload("Artifacts").First(&vulnAfter, "id = ?", vulnMulti.ID).Error
+			assert.NoError(t, err, "Dependency vulnerability should still exist when it has remaining artifacts")
+
+			// Verify the vulnerability now only has one artifact (the remaining one)
+			assert.Len(t, vulnAfter.Artifacts, 1, "Vulnerability should have only 1 artifact after deletion")
+			assert.Equal(t, artifactMulti2.ArtifactName, vulnAfter.Artifacts[0].ArtifactName, "Remaining artifact should be artifact-multi-2")
+		})
 	})
 }