Fix item deletion to properly clean up attachment files from storage (#1046)

* Initial plan

* Fix item deletion to properly clean up attachment files

Co-authored-by: tankerkiller125 <3457368+tankerkiller125@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: tankerkiller125 <3457368+tankerkiller125@users.noreply.github.com>

backend/internal/data/repo/repo_items.go

@@ -21,8 +21,9 @@ import (
 )
 
 type ItemsRepository struct {
-	db  *ent.Client
-	bus *eventbus.EventBus
+	db          *ent.Client
+	bus         *eventbus.EventBus
+	attachments *AttachmentRepo
 }
 
 type (
@@ -632,7 +633,32 @@ func (e *ItemsRepository) Create(ctx context.Context, gid uuid.UUID, data ItemCr
 }
 
 func (e *ItemsRepository) Delete(ctx context.Context, id uuid.UUID) error {
-	err := e.db.Item.DeleteOneID(id).Exec(ctx)
+	// Get the item with its group and attachments before deletion
+	itm, err := e.db.Item.Query().
+		Where(item.ID(id)).
+		WithGroup().
+		WithAttachments().
+		Only(ctx)
+	if err != nil {
+		return err
+	}
+
+	// Get the group ID for attachment deletion
+	var gid uuid.UUID
+	if itm.Edges.Group != nil {
+		gid = itm.Edges.Group.ID
+	}
+
+	// Delete all attachments (and their files) before deleting the item
+	for _, att := range itm.Edges.Attachments {
+		err := e.attachments.Delete(ctx, gid, id, att.ID)
+		if err != nil {
+			log.Err(err).Str("attachment_id", att.ID.String()).Msg("failed to delete attachment during item deletion")
+			// Continue with other attachments even if one fails
+		}
+	}
+
+	err = e.db.Item.DeleteOneID(id).Exec(ctx)
 	if err != nil {
 		return err
 	}
@@ -642,7 +668,28 @@ func (e *ItemsRepository) Delete(ctx context.Context, id uuid.UUID) error {
 }
 
 func (e *ItemsRepository) DeleteByGroup(ctx context.Context, gid, id uuid.UUID) error {
-	_, err := e.db.Item.
+	// Get the item with its attachments before deletion
+	itm, err := e.db.Item.Query().
+		Where(
+			item.ID(id),
+			item.HasGroupWith(group.ID(gid)),
+		).
+		WithAttachments().
+		Only(ctx)
+	if err != nil {
+		return err
+	}
+
+	// Delete all attachments (and their files) before deleting the item
+	for _, att := range itm.Edges.Attachments {
+		err := e.attachments.Delete(ctx, gid, id, att.ID)
+		if err != nil {
+			log.Err(err).Str("attachment_id", att.ID.String()).Msg("failed to delete attachment during item deletion")
+			// Continue with other attachments even if one fails
+		}
+	}
+
+	_, err = e.db.Item.
 		Delete().
 		Where(
 			item.ID(id),

backend/internal/data/repo/repo_items_test.go

@@ -2,12 +2,14 @@ package repo
 
 import (
 	"context"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"github.com/sysadminsmedia/homebox/backend/internal/data/ent/attachment"
 	"github.com/sysadminsmedia/homebox/backend/internal/data/types"
 )
 
@@ -317,3 +319,83 @@ func TestItemRepository_GetAllCustomFields(t *testing.T) {
 		assert.ElementsMatch(t, values[:1], results)
 	}
 }
+
+func TestItemsRepository_DeleteWithAttachments(t *testing.T) {
+	// Create an item with an attachment
+	item := useItems(t, 1)[0]
+
+	// Add an attachment to the item
+	attachment, err := tRepos.Attachments.Create(
+		context.Background(),
+		item.ID,
+		ItemCreateAttachment{
+			Title:   "test-attachment.txt",
+			Content: strings.NewReader("test content for attachment deletion"),
+		},
+		attachment.TypePhoto,
+		true,
+	)
+	require.NoError(t, err)
+	assert.NotNil(t, attachment)
+
+	// Verify the attachment exists
+	retrievedAttachment, err := tRepos.Attachments.Get(context.Background(), tGroup.ID, attachment.ID)
+	require.NoError(t, err)
+	assert.Equal(t, attachment.ID, retrievedAttachment.ID)
+
+	// Verify the attachment is linked to the item
+	itemWithAttachments, err := tRepos.Items.GetOne(context.Background(), item.ID)
+	require.NoError(t, err)
+	assert.Len(t, itemWithAttachments.Attachments, 1)
+	assert.Equal(t, attachment.ID, itemWithAttachments.Attachments[0].ID)
+
+	// Delete the item
+	err = tRepos.Items.Delete(context.Background(), item.ID)
+	require.NoError(t, err)
+
+	// Verify the item is deleted
+	_, err = tRepos.Items.GetOne(context.Background(), item.ID)
+	require.Error(t, err)
+
+	// Verify the attachment is also deleted
+	_, err = tRepos.Attachments.Get(context.Background(), tGroup.ID, attachment.ID)
+	require.Error(t, err)
+}
+
+func TestItemsRepository_DeleteByGroupWithAttachments(t *testing.T) {
+	// Create an item with an attachment
+	item := useItems(t, 1)[0]
+
+	// Add an attachment to the item
+	attachment, err := tRepos.Attachments.Create(
+		context.Background(),
+		item.ID,
+		ItemCreateAttachment{
+			Title:   "test-attachment-by-group.txt",
+			Content: strings.NewReader("test content for attachment deletion by group"),
+		},
+		attachment.TypePhoto,
+		true,
+	)
+	require.NoError(t, err)
+	assert.NotNil(t, attachment)
+
+	// Verify the attachment exists
+	retrievedAttachment, err := tRepos.Attachments.Get(context.Background(), tGroup.ID, attachment.ID)
+	require.NoError(t, err)
+	assert.Equal(t, attachment.ID, retrievedAttachment.ID)
+
+	// Delete the item using DeleteByGroup
+	err = tRepos.Items.DeleteByGroup(context.Background(), tGroup.ID, item.ID)
+	require.NoError(t, err)
+
+	// Verify the item is deleted
+	_, err = tRepos.Items.GetOneByGroup(context.Background(), tGroup.ID, item.ID)
+	require.Error(t, err)
+
+	// Verify the attachment is also deleted
+	_, err = tRepos.Attachments.Get(context.Background(), tGroup.ID, attachment.ID)
+	require.Error(t, err)
+}
+
+

backend/internal/data/repo/repos_all.go

@@ -21,14 +21,15 @@ type AllRepos struct {
 }
 
 func New(db *ent.Client, bus *eventbus.EventBus, storage config.Storage, pubSubConn string, thumbnail config.Thumbnail) *AllRepos {
+	attachments := &AttachmentRepo{db, storage, pubSubConn, thumbnail}
 	return &AllRepos{
 		Users:       &UserRepository{db},
 		AuthTokens:  &TokenRepository{db},
 		Groups:      NewGroupRepository(db),
 		Locations:   &LocationRepository{db, bus},
 		Labels:      &LabelRepository{db, bus},
-		Items:       &ItemsRepository{db, bus},
-		Attachments: &AttachmentRepo{db, storage, pubSubConn, thumbnail},
+		Items:       &ItemsRepository{db, bus, attachments},
+		Attachments: attachments,
 		MaintEntry:  &MaintenanceEntryRepository{db},
 		Notifiers:   NewNotifierRepository(db),
 	}