From 397a1c6f3e28b4d94573ec4cf28b17032f781fa8 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Oct 2025 08:55:15 -0400 Subject: [PATCH] Fix: Return error to UI when attachment upload fails due to storage misconfiguration (#1045) * Initial plan * Fix attachment upload error handling to return errors to UI Co-authored-by: tankerkiller125 <3457368+tankerkiller125@users.noreply.github.com> * Final verification: All tests pass and code builds successfully 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> --- .../services/service_items_attachments.go | 1 + .../service_items_attachments_test.go | 51 +++++++++++++++++++ .../data/repo/repo_item_attachments.go | 5 +- .../data/repo/repo_items_search_test.go | 22 ++++---- 4 files changed, 65 insertions(+), 14 deletions(-) diff --git a/backend/internal/core/services/service_items_attachments.go b/backend/internal/core/services/service_items_attachments.go index 4085d90d..4ce0decc 100644 --- a/backend/internal/core/services/service_items_attachments.go +++ b/backend/internal/core/services/service_items_attachments.go @@ -50,6 +50,7 @@ func (svc *ItemService) AttachmentAdd(ctx Context, itemID uuid.UUID, filename st _, err = svc.repo.Attachments.Create(ctx, itemID, repo.ItemCreateAttachment{Title: filename, Content: file}, attachmentType, primary) if err != nil { log.Err(err).Msg("failed to create attachment") + return repo.ItemOut{}, err } return svc.repo.Items.GetOneByGroup(ctx, ctx.GID, itemID) diff --git a/backend/internal/core/services/service_items_attachments_test.go b/backend/internal/core/services/service_items_attachments_test.go index 16560fd3..1b7cb416 100644 --- a/backend/internal/core/services/service_items_attachments_test.go +++ b/backend/internal/core/services/service_items_attachments_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/sysadminsmedia/homebox/backend/internal/data/repo" + "github.com/sysadminsmedia/homebox/backend/internal/sys/config" ) func TestItemService_AddAttachment(t *testing.T) { @@ -60,3 +61,53 @@ func TestItemService_AddAttachment(t *testing.T) { require.NoError(t, err) assert.Equal(t, contents, string(bts)) } + +func TestItemService_AddAttachment_InvalidStorage(t *testing.T) { + // Create a service with an invalid storage path to simulate the issue + svc := &ItemService{ + repo: tRepos, + filepath: "/nonexistent/path/that/should/not/exist", + } + + // Create a temporary repo with invalid storage config + invalidRepos := repo.New(tClient, tbus, config.Storage{ + PrefixPath: "/", + ConnString: "file:///nonexistent/directory/that/does/not/exist", + }, "mem://{{ .Topic }}", config.Thumbnail{ + Enabled: false, + Width: 0, + Height: 0, + }) + + svc.repo = invalidRepos + + loc, err := invalidRepos.Locations.Create(context.Background(), tGroup.ID, repo.LocationCreate{ + Description: "test", + Name: "test-invalid", + }) + require.NoError(t, err) + assert.NotNil(t, loc) + + itmC := repo.ItemCreate{ + Name: fk.Str(10), + Description: fk.Str(10), + LocationID: loc.ID, + } + + itm, err := invalidRepos.Items.Create(context.Background(), tGroup.ID, itmC) + require.NoError(t, err) + assert.NotNil(t, itm) + t.Cleanup(func() { + err := invalidRepos.Items.Delete(context.Background(), itm.ID) + require.NoError(t, err) + }) + + contents := fk.Str(1000) + reader := strings.NewReader(contents) + + // Attempt to add attachment with invalid storage - should return an error + _, err = svc.AttachmentAdd(tCtx, itm.ID, "testfile.txt", "attachment", false, reader) + + // This should return an error now (after the fix) + assert.Error(t, err, "AttachmentAdd should return an error when storage is invalid") +} diff --git a/backend/internal/data/repo/repo_item_attachments.go b/backend/internal/data/repo/repo_item_attachments.go index eb6f8391..0cd2feed 100644 --- a/backend/internal/data/repo/repo_item_attachments.go +++ b/backend/internal/data/repo/repo_item_attachments.go @@ -218,9 +218,8 @@ func (r *AttachmentRepo) Create(ctx context.Context, itemID uuid.UUID, doc ItemC // Upload the file to the storage bucket path, err := r.UploadFile(ctx, itemGroup, doc) if err != nil { - err := tx.Rollback() - if err != nil { - return nil, err + if rollbackErr := tx.Rollback(); rollbackErr != nil { + return nil, rollbackErr } return nil, err } diff --git a/backend/internal/data/repo/repo_items_search_test.go b/backend/internal/data/repo/repo_items_search_test.go index cb7accf3..e2156815 100644 --- a/backend/internal/data/repo/repo_items_search_test.go +++ b/backend/internal/data/repo/repo_items_search_test.go @@ -3,18 +3,18 @@ package repo import ( "testing" - "github.com/sysadminsmedia/homebox/backend/pkgs/textutils" "github.com/stretchr/testify/assert" + "github.com/sysadminsmedia/homebox/backend/pkgs/textutils" ) func TestItemsRepository_AccentInsensitiveSearch(t *testing.T) { // Test cases for accent-insensitive search testCases := []struct { - name string - itemName string - searchQuery string - shouldMatch bool - description string + name string + itemName string + searchQuery string + shouldMatch bool + description string }{ { name: "Spanish accented item, search without accents", @@ -155,25 +155,25 @@ func TestItemsRepository_AccentInsensitiveSearch(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Test the normalization logic used in the repository normalizedSearch := textutils.NormalizeSearchQuery(tc.searchQuery) - + // This simulates what happens in the repository // The original search would find exact matches (case-insensitive) // The normalized search would find accent-insensitive matches - + // Test that our normalization works as expected if tc.shouldMatch { // If it should match, then either the original query should match // or the normalized query should match when applied to the stored data assert.NotEqual(t, "", normalizedSearch, "Normalized search should not be empty") - + // The key insight is that we're searching with both the original and normalized queries // So "electrónica" will be found when searching for "electronica" because: // 1. Original search: "electronica" doesn't match "electrónica" // 2. Normalized search: "electronica" matches the normalized version - t.Logf("✓ %s: Item '%s' should be found with search '%s' (normalized: '%s')", + t.Logf("✓ %s: Item '%s' should be found with search '%s' (normalized: '%s')", tc.description, tc.itemName, tc.searchQuery, normalizedSearch) } else { - t.Logf("✗ %s: Item '%s' should NOT be found with search '%s' (normalized: '%s')", + t.Logf("✗ %s: Item '%s' should NOT be found with search '%s' (normalized: '%s')", tc.description, tc.itemName, tc.searchQuery, normalizedSearch) } })