From ff6aceaeac7709657bcdb19e17c1446f93d8c417 Mon Sep 17 00:00:00 2001
From: Earl Warren <contact@earl-warren.org>
Date: Sat, 27 Jul 2024 15:29:53 +0200
Subject: [PATCH] feat: optimize the FindUnreferencedPackages package query

Replace a double select with a simple select.

The complication originates from the initial implementation which
deleted packages instead of selecting them. It was justified to
workaround a problem in MySQL. But it is just a waste of resources
when collecting a list of IDs.
---
 models/packages/package.go             | 22 ++++++++++------------
 services/packages/cleanup/cleanup.go   |  8 ++++----
 tests/integration/api_packages_test.go |  4 ++++
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/models/packages/package.go b/models/packages/package.go
index 65a257415..50717c695 100644
--- a/models/packages/package.go
+++ b/models/packages/package.go
@@ -280,19 +280,17 @@ func GetPackagesByType(ctx context.Context, ownerID int64, packageType Type) ([]
 }
 
 // FindUnreferencedPackages gets all packages without associated versions
-func FindUnreferencedPackages(ctx context.Context) ([]*Package, error) {
-	in := builder.
+func FindUnreferencedPackages(ctx context.Context) ([]int64, error) {
+	var pIDs []int64
+	if err := db.GetEngine(ctx).
 		Select("package.id").
-		From("package").
-		LeftJoin("package_version", "package_version.package_id = package.id").
-		Where(builder.Expr("package_version.id IS NULL"))
-
-	ps := make([]*Package, 0, 10)
-	return ps, db.GetEngine(ctx).
-		// double select workaround for MySQL
-		// https://stackoverflow.com/questions/4471277/mysql-delete-from-with-subquery-as-condition
-		Where(builder.In("package.id", builder.Select("id").From(in, "temp"))).
-		Find(&ps)
+		Table("package").
+		Join("LEFT", "package_version", "package_version.package_id = package.id").
+		Where("package_version.id IS NULL").
+		Find(&pIDs); err != nil {
+		return nil, err
+	}
+	return pIDs, nil
 }
 
 // HasOwnerPackages tests if a user/org has accessible packages
diff --git a/services/packages/cleanup/cleanup.go b/services/packages/cleanup/cleanup.go
index 5d5120c6a..5aba73099 100644
--- a/services/packages/cleanup/cleanup.go
+++ b/services/packages/cleanup/cleanup.go
@@ -154,15 +154,15 @@ func CleanupExpiredData(outerCtx context.Context, olderThan time.Duration) error
 		return err
 	}
 
-	ps, err := packages_model.FindUnreferencedPackages(ctx)
+	pIDs, err := packages_model.FindUnreferencedPackages(ctx)
 	if err != nil {
 		return err
 	}
-	for _, p := range ps {
-		if err := packages_model.DeleteAllProperties(ctx, packages_model.PropertyTypePackage, p.ID); err != nil {
+	for _, pID := range pIDs {
+		if err := packages_model.DeleteAllProperties(ctx, packages_model.PropertyTypePackage, pID); err != nil {
 			return err
 		}
-		if err := packages_model.DeletePackageByID(ctx, p.ID); err != nil {
+		if err := packages_model.DeletePackageByID(ctx, pID); err != nil {
 			return err
 		}
 	}
diff --git a/tests/integration/api_packages_test.go b/tests/integration/api_packages_test.go
index daf32e82f..9021d4e7f 100644
--- a/tests/integration/api_packages_test.go
+++ b/tests/integration/api_packages_test.go
@@ -479,6 +479,8 @@ func TestPackageCleanup(t *testing.T) {
 			AddBasicAuth(user.Name)
 		MakeRequest(t, req, http.StatusCreated)
 
+		unittest.AssertExistsAndLoadBean(t, &packages_model.Package{Name: "cleanup-test"})
+
 		pbs, err := packages_model.FindExpiredUnreferencedBlobs(db.DefaultContext, duration)
 		assert.NoError(t, err)
 		assert.NotEmpty(t, pbs)
@@ -493,6 +495,8 @@ func TestPackageCleanup(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Empty(t, pbs)
 
+		unittest.AssertNotExistsBean(t, &packages_model.Package{Name: "cleanup-test"})
+
 		_, err = packages_model.GetInternalVersionByNameAndVersion(db.DefaultContext, user.ID, packages_model.TypeContainer, "cleanup-test", container_model.UploadVersion)
 		assert.ErrorIs(t, err, packages_model.ErrPackageNotExist)
 	})