fix: [AH-1197]: fix failing OCI delete image and artifact (#3741)

* feat: [AH-1197]: update SQL migration queries for sqlite * feat: [AH-1197]: fix failing tests * feat: [AH-1197]: delete manifest on delete oci image * feat: [AH-1197]: fix failing nodes because of foreign key constraint * feat: [AH-1197]: remove unused functions * feat: [AH-1197]: hard delete artifact and images for oci flow as well * fix: [AH-1197]: fix failing OCI delete image and artifact
2025-05-05 15:32:56 +00:00 · 2025-04-30 20:31:14 +00:00 · 2025-04-30 20:31:14 +00:00 · ebe634e5df
commit ebe634e5df
parent 988f6eff9b
14 changed files with 187 additions and 20 deletions
--- a/app/store/database/migrate/postgres/0111_alter_table_nodes.down.sql
+++ b/app/store/database/migrate/postgres/0111_alter_table_nodes.down.sql
@ -0,0 +1,6 @@
+ALTER TABLE nodes DROP CONSTRAINT IF EXISTS nodes_node_parent_id_fkey;
+
+ALTER TABLE nodes
+ADD CONSTRAINT nodes_node_parent_id_fkey
+FOREIGN KEY (node_parent_id) 
+REFERENCES nodes(node_id);
--- a/app/store/database/migrate/postgres/0111_alter_table_nodes.up.sql
+++ b/app/store/database/migrate/postgres/0111_alter_table_nodes.up.sql
@ -0,0 +1,7 @@
+ALTER TABLE nodes DROP CONSTRAINT IF EXISTS nodes_node_parent_id_fkey;
+
+ALTER TABLE nodes
+ADD CONSTRAINT nodes_node_parent_id_fkey
+FOREIGN KEY (node_parent_id) 
+REFERENCES nodes(node_id)
+ON DELETE CASCADE;
--- a/app/store/database/migrate/sqlite/0111_alter_table_nodes.down.sql
+++ b/app/store/database/migrate/sqlite/0111_alter_table_nodes.down.sql
@ -0,0 +1,39 @@
+ALTER TABLE nodes RENAME TO nodes_old;
+
+CREATE TABLE nodes (
+                       node_id              TEXT PRIMARY KEY,
+                       node_name            TEXT    NOT NULL,
+                       node_parent_id       TEXT REFERENCES nodes (node_id),
+                       node_registry_id     INTEGER NOT NULL REFERENCES registries (registry_id),
+                       node_is_file         BOOLEAN NOT NULL,
+                       node_path            TEXT    NOT NULL,
+                       node_generic_blob_id TEXT REFERENCES generic_blobs (generic_blob_id),
+                       node_created_at      INTEGER NOT NULL,
+                       node_created_by      INTEGER NOT NULL,
+                       CONSTRAINT unique_nodes UNIQUE (node_name, node_parent_id)
+);
+
+INSERT INTO nodes (
+    node_id,
+    node_name,
+    node_parent_id,
+    node_registry_id,
+    node_is_file,
+    node_path,
+    node_generic_blob_id,
+    node_created_at,
+    node_created_by
+)
+SELECT
+    node_id,
+    node_name,
+    node_parent_id,
+    node_registry_id,
+    node_is_file,
+    node_path,
+    node_generic_blob_id,
+    node_created_at,
+    node_created_by
+FROM nodes_old;
+
+DROP TABLE nodes_old;
--- a/app/store/database/migrate/sqlite/0111_alter_table_nodes.up.sql
+++ b/app/store/database/migrate/sqlite/0111_alter_table_nodes.up.sql
@ -0,0 +1,39 @@
+ALTER TABLE nodes RENAME TO nodes_old;
+
+CREATE TABLE nodes (
+                       node_id              TEXT PRIMARY KEY,
+                       node_name            TEXT    NOT NULL,
+                       node_parent_id       TEXT REFERENCES nodes (node_id) ON DELETE CASCADE,
+                       node_registry_id     INTEGER NOT NULL REFERENCES registries (registry_id),
+                       node_is_file         BOOLEAN NOT NULL,
+                       node_path            TEXT    NOT NULL,
+                       node_generic_blob_id TEXT REFERENCES generic_blobs (generic_blob_id),
+                       node_created_at      INTEGER NOT NULL,
+                       node_created_by      INTEGER NOT NULL,
+                       CONSTRAINT unique_nodes UNIQUE (node_name, node_parent_id)
+);
+
+INSERT INTO nodes (
+    node_id,
+    node_name,
+    node_parent_id,
+    node_registry_id,
+    node_is_file,
+    node_path,
+    node_generic_blob_id,
+    node_created_at,
+    node_created_by
+)
+SELECT
+    node_id,
+    node_name,
+    node_parent_id,
+    node_registry_id,
+    node_is_file,
+    node_path,
+    node_generic_blob_id,
+    node_created_at,
+    node_created_by
+FROM nodes_old;
+
+DROP TABLE nodes_old;
--- a/cmd/gitness/wire_gen.go
+++ b/cmd/gitness/wire_gen.go
@ -527,7 +527,7 @@ func initSystem(ctx context.Context, config *types.Config) (*server.System, erro
 	}
 	registryHelper := rpm.LocalRegistryHelperProvider(fileManager, artifactRepository)
 	indexService := index.ProvideService(registryHelper, lockerLocker)
-	apiHandler := router.APIHandlerProvider(registryRepository, upstreamProxyConfigRepository, fileManager, tagRepository, manifestRepository, cleanupPolicyRepository, imageRepository, storageDriver, spaceFinder, transactor, authenticator, provider, authorizer, auditService, artifactRepository, webhooksRepository, webhooksExecutionRepository, service2, spacePathStore, reporter10, downloadStatRepository, indexService, config)
+	apiHandler := router.APIHandlerProvider(registryRepository, upstreamProxyConfigRepository, fileManager, tagRepository, manifestRepository, cleanupPolicyRepository, imageRepository, storageDriver, spaceFinder, transactor, authenticator, provider, authorizer, auditService, artifactRepository, webhooksRepository, webhooksExecutionRepository, service2, spacePathStore, reporter10, downloadStatRepository, indexService, config, registryBlobRepository)
 	mavenDBStore := maven.DBStoreProvider(registryRepository, imageRepository, artifactRepository, spaceStore, bandwidthStatRepository, downloadStatRepository, nodesRepository, upstreamProxyConfigRepository)
 	mavenLocalRegistry := maven.LocalRegistryProvider(mavenDBStore, transactor, fileManager)
 	mavenController := maven.ProvideProxyController(mavenLocalRegistry, secretService, spaceFinder)
--- a/registry/app/api/controller/metadata/controller.go
+++ b/registry/app/api/controller/metadata/controller.go
@ -54,6 +54,7 @@ type APIController struct {
 	DownloadStatRepository       store.DownloadStatRepository
 	RegistryIndexService         index.Service
 	SetupDetailsAuthHeaderPrefix string
+	RegistryBlobStore            store.RegistryBlobRepository
 }

 func NewAPIController(
@ -81,6 +82,7 @@ func NewAPIController(
 	downloadStatRepository store.DownloadStatRepository,
 	registryIndexService index.Service,
 	setupDetailsAuthHeaderPrefix string,
+	registryBlobStore store.RegistryBlobRepository,
 ) *APIController {
 	return &APIController{
 		fileManager:                  fileManager,
@ -107,5 +109,6 @@ func NewAPIController(
 		DownloadStatRepository:       downloadStatRepository,
 		RegistryIndexService:         registryIndexService,
 		SetupDetailsAuthHeaderPrefix: setupDetailsAuthHeaderPrefix,
+		RegistryBlobStore:            registryBlobStore,
 	}
 }
--- a/registry/app/api/controller/metadata/create_registry_test.go
+++ b/registry/app/api/controller/metadata/create_registry_test.go
@ -265,6 +265,7 @@ func TestCreateRegistry(t *testing.T) {
 					nil, // downloadStatRepository.
 					nil, // registryIndexService - not needed for this test.
 					"",
+					nil, // registryBlobStore - not needed for this test.
 				)
 			},
 		},
@ -337,6 +338,7 @@ func TestCreateRegistry(t *testing.T) {
 					nil, //
 					nil, // downloadStatRepository.
 					"",
+					nil, // registryIndexService - not needed for this test.
 				)
 			},
 		},
--- a/registry/app/api/controller/metadata/delete_artifact.go
+++ b/registry/app/api/controller/metadata/delete_artifact.go
@ -136,10 +136,22 @@ func (c *APIController) deleteOCIImage(
 ) error {
 	err := c.tx.WithTx(
 		ctx, func(ctx context.Context) error {
-			// Delete tags linked to the image
-			err := c.TagStore.DeleteTagsByImageName(ctx, regInfo.RegistryID, artifactName)
+			// Delete manifests linked to the image
+			_, err := c.ManifestStore.DeleteManifestByImageName(ctx, regInfo.RegistryID, artifactName)
 			if err != nil {
-				return fmt.Errorf("failed to delete artifact: %w", err)
+				return fmt.Errorf("failed to delete manifests: %w", err)
+			}
+
+			// Delete registry blobs linked to the image
+			_, err = c.RegistryBlobStore.UnlinkBlobByImageName(ctx, regInfo.RegistryID, artifactName)
+			if err != nil {
+				return fmt.Errorf("failed to delete registry blobs: %w", err)
+			}
+
+			// Delete Artifacts linked to image
+			err = c.ArtifactStore.DeleteByImageNameAndRegistryID(ctx, regInfo.RegistryID, artifactName)
+			if err != nil {
+				return fmt.Errorf("failed to delete versions: %w", err)
 			}

 			// Delete image
--- a/registry/app/api/controller/metadata/delete_artifacts_version.go
+++ b/registry/app/api/controller/metadata/delete_artifacts_version.go
@ -82,7 +82,7 @@ func (c *APIController) DeleteArtifactVersion(ctx context.Context, r artifact.De
 	versionName := string(r.Version)
 	registryName := repoEntity.Name

-	image, err := c.ImageStore.GetByRepoAndName(ctx, regInfo.ParentID, regInfo.RegistryIdentifier, artifactName)
+	imageInfo, err := c.ImageStore.GetByName(ctx, regInfo.RegistryID, artifactName)
 	if err != nil {
 		//nolint:nilerr
 		return artifact.DeleteArtifactVersion404JSONResponse{
@ -92,16 +92,6 @@ func (c *APIController) DeleteArtifactVersion(ctx context.Context, r artifact.De
 		}, nil
 	}

-	_, err = c.ArtifactStore.GetByName(ctx, image.ID, versionName)
-	if err != nil {
-		//nolint:nilerr
-		return artifact.DeleteArtifactVersion404JSONResponse{
-			NotFoundJSONResponse: artifact.NotFoundJSONResponse(
-				*GetErrorResponse(http.StatusNotFound, "version doesn't exist with this key"),
-			),
-		}, nil
-	}
-
 	switch regInfo.PackageType {
 	case artifact.PackageTypeDOCKER:
 		err = c.deleteTagWithAudit(ctx, regInfo, registryName, session.Principal, artifactName,
@ -110,17 +100,17 @@ func (c *APIController) DeleteArtifactVersion(ctx context.Context, r artifact.De
 		err = c.deleteTagWithAudit(ctx, regInfo, registryName, session.Principal, artifactName,
 			versionName)
 	case artifact.PackageTypeNPM:
-		err = c.deleteVersion(ctx, regInfo, artifactName, versionName)
+		err = c.deleteVersion(ctx, regInfo, imageInfo, artifactName, versionName)
 	case artifact.PackageTypeMAVEN:
-		err = c.deleteVersion(ctx, regInfo, artifactName, versionName)
+		err = c.deleteVersion(ctx, regInfo, imageInfo, artifactName, versionName)
 	case artifact.PackageTypePYTHON:
-		err = c.deleteVersion(ctx, regInfo, artifactName, versionName)
+		err = c.deleteVersion(ctx, regInfo, imageInfo, artifactName, versionName)
 	case artifact.PackageTypeGENERIC:
-		err = c.deleteVersion(ctx, regInfo, artifactName, versionName)
+		err = c.deleteVersion(ctx, regInfo, imageInfo, artifactName, versionName)
 	case artifact.PackageTypeNUGET:
 		err = fmt.Errorf("delete version not supported for nuget")
 	case artifact.PackageTypeRPM:
-		err = c.deleteVersion(ctx, regInfo, artifactName, versionName)
+		err = c.deleteVersion(ctx, regInfo, imageInfo, artifactName, versionName)
 		if err != nil {
 			break
 		}
@ -177,9 +167,15 @@ func (c *APIController) deleteTagWithAudit(
 func (c *APIController) deleteVersion(
 	ctx context.Context,
 	regInfo *registryTypes.RegistryRequestBaseInfo,
+	imageInfo *registryTypes.Image,
 	artifactName string,
 	versionName string,
 ) error {
+	_, err := c.ArtifactStore.GetByName(ctx, imageInfo.ID, versionName)
+	if err != nil {
+		return fmt.Errorf("version doesn't exist with for image %v", imageInfo.Name)
+	}
+
 	// get the file path based on package type
 	filePath, err := utils.GetFilePath(regInfo.PackageType, artifactName, versionName)
 	if err != nil {
--- a/registry/app/api/router/harness/route.go
+++ b/registry/app/api/router/harness/route.go
@ -81,6 +81,7 @@ func NewAPIHandler(
 	downloadStatRepository store.DownloadStatRepository,
 	registryIndexService index.Service,
 	gitnessConfig *types.Config,
+	registryBlobsDao store.RegistryBlobRepository,
 ) APIHandler {
 	r := chi.NewRouter()
 	r.Use(audit.Middleware())
@ -112,6 +113,7 @@ func NewAPIHandler(
 		downloadStatRepository,
 		registryIndexService,
 		gitnessConfig.Registry.SetupDetailsAuthHeaderPrefix,
+		registryBlobsDao,
 	)

 	handler := artifact.NewStrictHandler(apiController, []artifact.StrictMiddlewareFunc{})
--- a/registry/app/api/router/wire.go
+++ b/registry/app/api/router/wire.go
@ -81,6 +81,7 @@ func APIHandlerProvider(
 	downloadStatRepository store.DownloadStatRepository,
 	registryIndexService index.Service,
 	gitnessConfig *types.Config,
+	registryBlobsDao store.RegistryBlobRepository,
 ) harness.APIHandler {
 	return harness.NewAPIHandler(
 		repoDao,
@ -107,6 +108,7 @@ func APIHandlerProvider(
 		downloadStatRepository,
 		registryIndexService,
 		gitnessConfig,
+		registryBlobsDao,
 	)
 }

--- a/registry/app/store/database.go
+++ b/registry/app/store/database.go
@ -125,6 +125,10 @@ type ManifestRepository interface {
 		ctx context.Context, repoID int64,
 		imageName string, d digest.Digest,
 	) (bool, error)
+	DeleteManifestByImageName(
+		ctx context.Context, repoID int64,
+		imageName string,
+	) (bool, error)
 	ListManifestsBySubject(
 		ctx context.Context, repoID int64,
 		id int64,
@ -402,6 +406,11 @@ type RegistryBlobRepository interface {
 		ctx context.Context, imageName string,
 		registry *types.Registry, blobID int64,
 	) (bool, error)
+
+	UnlinkBlobByImageName(
+		ctx context.Context, registryID int64,
+		imageName string,
+	) (bool, error)
 }

 type ImageRepository interface {
--- a/registry/app/store/database/manifest.go
+++ b/registry/app/store/database/manifest.go
@ -388,6 +388,32 @@ func (dao manifestDao) DeleteManifest(
 	return count == 1, nil
 }

+func (dao manifestDao) DeleteManifestByImageName(
+	ctx context.Context, repoID int64,
+	imageName string,
+) (bool, error) {
+	stmt := database.Builder.Delete("manifests").
+		Where(
+			"manifest_registry_id = ? AND manifest_image_name = ?",
+			repoID, imageName,
+		)
+
+	toSQL, args, err := stmt.ToSql()
+	if err != nil {
+		return false, fmt.Errorf("failed to convert manifest query to sql: %w", err)
+	}
+
+	db := dbtx.GetAccessor(ctx, dao.sqlDB)
+
+	r, err := db.ExecContext(ctx, toSQL, args...)
+	if err != nil {
+		return false, database.ProcessSQLErrorf(ctx, err, "the delete query failed")
+	}
+
+	count, _ := r.RowsAffected()
+	return count > 0, nil
+}
+
 func (dao manifestDao) FindManifestByID(
 	ctx context.Context,
 	registryID,
--- a/registry/app/store/database/registry_blobs.go
+++ b/registry/app/store/database/registry_blobs.go
@ -127,6 +127,30 @@ func (r registryBlobDao) UnlinkBlob(
 	return affected == 1, err
 }

+func (r registryBlobDao) UnlinkBlobByImageName(
+	ctx context.Context, registryID int64,
+	imageName string,
+) (bool, error) {
+	stmt := databaseg.Builder.Delete("registry_blobs").
+		Where("rblob_registry_id = ? AND rblob_image_name = ?",
+			registryID, imageName)
+
+	sql, args, err := stmt.ToSql()
+	if err != nil {
+		return false, fmt.Errorf("failed to convert purge registry query to sql: %w", err)
+	}
+
+	db := dbtx.GetAccessor(ctx, r.db)
+
+	result, err := db.ExecContext(ctx, sql, args...)
+	if err != nil {
+		return false, databaseg.ProcessSQLErrorf(ctx, err, "error unlinking blobs")
+	}
+
+	affected, err := result.RowsAffected()
+	return affected > 0, err
+}
+
 func mapToInternalRegistryBlob(
 	ctx context.Context, registryID int64, blobID int64,
 	imageName string,