From 5d33382e3056afd5b75b1775255e0f3033833f6c Mon Sep 17 00:00:00 2001 From: Camilo Hollanda Date: Tue, 14 Jul 2026 18:11:23 -0300 Subject: [PATCH] =?UTF-8?q?feat:=20bulk=20actions=20=E2=80=94=20recursivel?= =?UTF-8?q?y=20delete=20folders=20(key=20prefixes)=20(#68)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(backend,frontend): recursive delete of key prefixes in bulk actions Bulk object selection previously only supported deleting individually listed object keys — folders ("prefixes") could not be selected or deleted, leaving no way to remove a directory and all of its contents. Backend: - Add S3Service.DeleteObjectsByPrefix, which recursively lists every object under a prefix and batch-deletes them, returning the count. - Extend the delete-multiple endpoint to accept a "prefixes" array alongside "keys"; keys are batch-deleted and each prefix is deleted recursively. Response now reports the total objects removed. Frontend: - Enable folder checkboxes and add a per-folder "Delete folder" action. - Select-all now covers both files and folders. - Route all bulk/folder deletes through a confirmation dialog that spells out the file/folder counts and warns that folders are removed recursively (previously bulk delete fired with no confirmation). - api/hook send "prefixes"; optimistic update drops objects under any deleted prefix. * fix(backend): validate delete prefixes and count actual removals Addresses maintainer review on the recursive prefix-delete endpoint: - Reject blank/whitespace-only prefixes with a 400 instead of a 500, and normalize each prefix to have a trailing "/" so "photos/2024" can no longer also delete siblings like "photos/2024-old/..." on this irreversible public endpoint. - DeleteMultipleObjects now returns the number of objects actually removed (requested keys minus failures) rather than assuming every requested key was deleted; the handler sums real counts across the keys and prefix paths. Draining the full RemoveObjects error channel also fixes a potential sender-goroutine leak on early return. - Add tests: blank-prefix -> 400, prefix trailing-slash normalization, and S3Service.DeleteObjectsByPrefix (list-then-delete, empty prefix, no-match, and list-error propagation). * fix(frontend): align select-all with the active search filter The header "select all" checkbox derived its checked state from the filtered (searched) rows, but handleSelectAll operated on the full, unfiltered object list — so with a search active it selected hidden items and the checkbox state disagreed with the selection. ObjectsTable now passes the keys of the currently visible (filtered) rows to onSelectAll, and its checked state reflects whether every visible row is selected. handleSelectAll toggles only those visible rows, leaving any off-screen selection intact. * fix(frontend): scope select-all to the visible page After merging upstream's client-side deep-search pagination, the rendered rows are pageObjects (one page slice) while select-all still operated on filteredObjects (every match across hidden pages). Scope the header checkbox's state and its select-all action to pageObjects so one click never selects off-screen rows for a destructive bulk delete. In normal/prefix browsing pageObjects === filteredObjects, so behavior there is unchanged. --------- Authored-by: Camilo Hollanda <775409+prem-prakash@users.noreply.github.com> --- backend/internal/handlers/objects.go | 69 ++++++-- backend/internal/handlers/objects_test.go | 129 +++++++++++++- backend/internal/models/responses.go | 7 +- backend/internal/services/interfaces.go | 3 +- backend/internal/services/mocks/mocks_test.go | 8 +- backend/internal/services/mocks/s3_mock.go | 15 +- backend/internal/services/s3.go | 71 ++++++-- backend/internal/services/s3_minio_test.go | 108 +++++++++++- .../components/buckets/ObjectBrowserView.tsx | 158 +++++++++++++++--- .../src/components/buckets/ObjectsTable.tsx | 68 ++++++-- frontend/src/hooks/useApi.ts | 4 +- frontend/src/hooks/useBucketObjects.ts | 22 ++- frontend/src/lib/api.ts | 6 +- 13 files changed, 581 insertions(+), 87 deletions(-) diff --git a/backend/internal/handlers/objects.go b/backend/internal/handlers/objects.go index cc1ef94..56bd2c7 100644 --- a/backend/internal/handlers/objects.go +++ b/backend/internal/handlers/objects.go @@ -519,8 +519,8 @@ func (h *ObjectHandler) GetPresignedURL(c fiber.Ctx) error { // @Tags Objects // @Accept json // @Produce json -// @Param bucket path string true "Name of the bucket containing the objects" -// @Param request body object{keys=[]string,prefix=string} true "List of object keys to delete and optional prefix for path context" +// @Param bucket path string true "Name of the bucket containing the objects" +// @Param request body object{keys=[]string,prefixes=[]string} true "Object keys to delete and/or folder prefixes to delete recursively" // @Success 200 {object} models.APIResponse{data=models.ObjectDeleteMultipleResponse} "Successfully deleted the objects" // @Failure 400 {object} models.APIResponse{error=models.APIError} "Invalid request parameters" // @Failure 404 {object} models.APIResponse{error=models.APIError} "Bucket not found" @@ -537,10 +537,11 @@ func (h *ObjectHandler) DeleteMultipleObjects(c fiber.Ctx) error { ) } - // Parse request body to get keys and optional prefix + // Parse request body. "keys" are concrete objects to delete; "prefixes" are + // folders to delete recursively (every object stored under the prefix). var req struct { - Keys []string `json:"keys"` - Prefix string `json:"prefix,omitempty"` + Keys []string `json:"keys"` + Prefixes []string `json:"prefixes,omitempty"` } if err := c.Bind().JSON(&req); err != nil { return c.Status(fiber.StatusBadRequest).JSON( @@ -548,23 +549,61 @@ func (h *ObjectHandler) DeleteMultipleObjects(c fiber.Ctx) error { ) } - if len(req.Keys) == 0 { + if len(req.Keys) == 0 && len(req.Prefixes) == 0 { return c.Status(fiber.StatusBadRequest).JSON( - models.ErrorResponse(models.ErrCodeBadRequest, "At least one key is required"), + models.ErrorResponse(models.ErrCodeBadRequest, "At least one key or prefix is required"), ) } - // Delete multiple objects - if err := h.s3Service.DeleteMultipleObjects(ctx, bucketName, req.Keys); err != nil { - return c.Status(fiber.StatusInternalServerError).JSON( - models.ErrorResponse(models.ErrCodeDeleteFailed, "Failed to delete objects: "+err.Error()), - ) + // Validate and normalize folder prefixes before running an irreversible + // recursive delete on a public endpoint. A blank prefix would match the + // entire bucket, and a prefix without a trailing slash (e.g. "photos/2024") + // would also match sibling keys such as "photos/2024-old/...". Reject blanks + // with a 4XX and force a trailing slash so a prefix only ever deletes the + // objects inside its own folder. + prefixes := make([]string, 0, len(req.Prefixes)) + for _, p := range req.Prefixes { + trimmed := strings.TrimSpace(p) + if trimmed == "" { + return c.Status(fiber.StatusBadRequest).JSON( + models.ErrorResponse(models.ErrCodeBadRequest, "Prefix must not be blank"), + ) + } + if !strings.HasSuffix(trimmed, "/") { + trimmed += "/" + } + prefixes = append(prefixes, trimmed) + } + + deleted := 0 + + // Delete the individually selected objects in a single batch call. + if len(req.Keys) > 0 { + n, err := h.s3Service.DeleteMultipleObjects(ctx, bucketName, req.Keys) + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON( + models.ErrorResponse(models.ErrCodeDeleteFailed, "Failed to delete objects: "+err.Error()), + ) + } + deleted += n + } + + // Recursively delete every object under each selected folder prefix. + for _, prefix := range prefixes { + n, err := h.s3Service.DeleteObjectsByPrefix(ctx, bucketName, prefix) + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON( + models.ErrorResponse(models.ErrCodeDeleteFailed, "Failed to delete folder "+prefix+": "+err.Error()), + ) + } + deleted += n } response := models.ObjectDeleteMultipleResponse{ - Bucket: bucketName, - Deleted: len(req.Keys), - Keys: req.Keys, + Bucket: bucketName, + Deleted: deleted, + Keys: req.Keys, + Prefixes: prefixes, } return c.JSON(models.SuccessResponse(response)) diff --git a/backend/internal/handlers/objects_test.go b/backend/internal/handlers/objects_test.go index 2c7b783..00b72f8 100644 --- a/backend/internal/handlers/objects_test.go +++ b/backend/internal/handlers/objects_test.go @@ -565,11 +565,11 @@ func TestUploadObject_ServiceError500(t *testing.T) { func TestDeleteMultipleObjects_Success(t *testing.T) { app, s3 := newObjectsTestApp(t) - s3.DeleteMultipleObjectsFn = func(_ context.Context, bucket string, keys []string) error { + s3.DeleteMultipleObjectsFn = func(_ context.Context, bucket string, keys []string) (int, error) { if bucket != "b1" || len(keys) != 3 { t.Errorf("args = (%q, %v)", bucket, keys) } - return nil + return len(keys), nil } body, _ := json.Marshal(map[string]any{"keys": []string{"a", "b", "c"}}) req := httptest.NewRequest(http.MethodPost, "/buckets/b1/objects/delete-multiple", bytes.NewReader(body)) @@ -591,6 +591,81 @@ func TestDeleteMultipleObjects_Success(t *testing.T) { } } +func TestDeleteMultipleObjects_Prefixes_Recursive(t *testing.T) { + app, s3 := newObjectsTestApp(t) + s3.DeleteObjectsByPrefixFn = func(_ context.Context, bucket, prefix string) (int, error) { + if bucket != "b1" || prefix != "docs/" { + t.Errorf("args = (%q, %q)", bucket, prefix) + } + return 4, nil + } + body, _ := json.Marshal(map[string]any{"prefixes": []string{"docs/"}}) + req := httptest.NewRequest(http.MethodPost, "/buckets/b1/objects/delete-multiple", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp, err := app.Test(req) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d", resp.StatusCode) + } + var out struct { + Data models.ObjectDeleteMultipleResponse `json:"data"` + } + decodeJSON(t, resp.Body, &out) + if out.Data.Deleted != 4 { + t.Errorf("Deleted = %d, want 4", out.Data.Deleted) + } +} + +func TestDeleteMultipleObjects_KeysAndPrefixes(t *testing.T) { + app, s3 := newObjectsTestApp(t) + s3.DeleteMultipleObjectsFn = func(_ context.Context, _ string, keys []string) (int, error) { + if len(keys) != 2 { + t.Errorf("keys = %v", keys) + } + return len(keys), nil + } + s3.DeleteObjectsByPrefixFn = func(_ context.Context, _, _ string) (int, error) { return 3, nil } + body, _ := json.Marshal(map[string]any{"keys": []string{"a", "b"}, "prefixes": []string{"docs/"}}) + req := httptest.NewRequest(http.MethodPost, "/buckets/b1/objects/delete-multiple", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp, err := app.Test(req) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d", resp.StatusCode) + } + var out struct { + Data models.ObjectDeleteMultipleResponse `json:"data"` + } + decodeJSON(t, resp.Body, &out) + if out.Data.Deleted != 5 { + t.Errorf("Deleted = %d, want 5 (2 keys + 3 under prefix)", out.Data.Deleted) + } +} + +func TestDeleteMultipleObjects_PrefixError500(t *testing.T) { + app, s3 := newObjectsTestApp(t) + s3.DeleteObjectsByPrefixFn = func(_ context.Context, _, _ string) (int, error) { + return 0, errors.New("boom") + } + body, _ := json.Marshal(map[string]any{"prefixes": []string{"docs/"}}) + req := httptest.NewRequest(http.MethodPost, "/buckets/b1/objects/delete-multiple", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp, err := app.Test(req) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", resp.StatusCode) + } +} + func TestDeleteMultipleObjects_EmptyKeys400(t *testing.T) { app, _ := newObjectsTestApp(t) body, _ := json.Marshal(map[string]any{"keys": []string{}}) @@ -606,6 +681,54 @@ func TestDeleteMultipleObjects_EmptyKeys400(t *testing.T) { } } +func TestDeleteMultipleObjects_BlankPrefix400(t *testing.T) { + // A blank/whitespace-only prefix must be rejected with a 4XX before any + // delete is attempted — it would otherwise target the whole bucket. + for _, prefix := range []string{"", " "} { + app, s3 := newObjectsTestApp(t) + s3.DeleteObjectsByPrefixFn = func(_ context.Context, _, _ string) (int, error) { + t.Errorf("DeleteObjectsByPrefix must not be called for blank prefix %q", prefix) + return 0, nil + } + body, _ := json.Marshal(map[string]any{"prefixes": []string{prefix}}) + req := httptest.NewRequest(http.MethodPost, "/buckets/b1/objects/delete-multiple", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp, err := app.Test(req) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("prefix %q: status = %d, want 400", prefix, resp.StatusCode) + } + } +} + +func TestDeleteMultipleObjects_PrefixNormalizedToTrailingSlash(t *testing.T) { + // A prefix without a trailing slash must be normalized so it only deletes + // its own folder ("photos/2024/"), not siblings like "photos/2024-old/". + app, s3 := newObjectsTestApp(t) + var gotPrefix string + s3.DeleteObjectsByPrefixFn = func(_ context.Context, _, prefix string) (int, error) { + gotPrefix = prefix + return 1, nil + } + body, _ := json.Marshal(map[string]any{"prefixes": []string{"photos/2024"}}) + req := httptest.NewRequest(http.MethodPost, "/buckets/b1/objects/delete-multiple", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp, err := app.Test(req) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200", resp.StatusCode) + } + if gotPrefix != "photos/2024/" { + t.Errorf("prefix passed to service = %q, want %q", gotPrefix, "photos/2024/") + } +} + func TestDeleteMultipleObjects_MalformedJSON400(t *testing.T) { app, _ := newObjectsTestApp(t) req := httptest.NewRequest(http.MethodPost, "/buckets/b1/objects/delete-multiple", strings.NewReader("{not-json")) @@ -622,7 +745,7 @@ func TestDeleteMultipleObjects_MalformedJSON400(t *testing.T) { func TestDeleteMultipleObjects_ServiceError500(t *testing.T) { app, s3 := newObjectsTestApp(t) - s3.DeleteMultipleObjectsFn = func(_ context.Context, _ string, _ []string) error { return errors.New("boom") } + s3.DeleteMultipleObjectsFn = func(_ context.Context, _ string, _ []string) (int, error) { return 0, errors.New("boom") } body, _ := json.Marshal(map[string]any{"keys": []string{"a"}}) req := httptest.NewRequest(http.MethodPost, "/buckets/b1/objects/delete-multiple", bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") diff --git a/backend/internal/models/responses.go b/backend/internal/models/responses.go index 50b4ca6..57c8572 100644 --- a/backend/internal/models/responses.go +++ b/backend/internal/models/responses.go @@ -151,9 +151,10 @@ type PresignedURLResponse struct { } type ObjectDeleteMultipleResponse struct { - Bucket string `json:"bucket"` - Deleted int `json:"deleted"` - Keys []string `json:"keys"` + Bucket string `json:"bucket"` + Deleted int `json:"deleted"` + Keys []string `json:"keys"` + Prefixes []string `json:"prefixes,omitempty"` } // UserListResponse represents a list of users/keys diff --git a/backend/internal/services/interfaces.go b/backend/internal/services/interfaces.go index 5c2d902..8795204 100644 --- a/backend/internal/services/interfaces.go +++ b/backend/internal/services/interfaces.go @@ -56,7 +56,8 @@ type S3Storage interface { DeleteObject(ctx context.Context, bucketName, key string) error GetObjectMetadata(ctx context.Context, bucketName, key string) (*models.ObjectInfo, error) GetPresignedURL(ctx context.Context, bucketName, key string, expiresIn time.Duration) (string, error) - DeleteMultipleObjects(ctx context.Context, bucketName string, keys []string) error + DeleteMultipleObjects(ctx context.Context, bucketName string, keys []string) (int, error) + DeleteObjectsByPrefix(ctx context.Context, bucketName, prefix string) (int, error) UploadMultipleObjects(ctx context.Context, bucketName string, files []struct { Key string Body io.Reader diff --git a/backend/internal/services/mocks/mocks_test.go b/backend/internal/services/mocks/mocks_test.go index db34ecc..388d8e4 100644 --- a/backend/internal/services/mocks/mocks_test.go +++ b/backend/internal/services/mocks/mocks_test.go @@ -115,7 +115,7 @@ func TestS3Mock_UnconfiguredMethodsReturnSentinel(t *testing.T) { if _, err := m.GetPresignedURL(ctx, "b", "k", time.Minute); err == nil { t.Error("GetPresignedURL: want error") } - if err := m.DeleteMultipleObjects(ctx, "b", []string{"k"}); err == nil { + if _, err := m.DeleteMultipleObjects(ctx, "b", []string{"k"}); err == nil { t.Error("DeleteMultipleObjects: want error") } // UploadMultipleObjects has no error channel; it must return a result slice @@ -163,7 +163,7 @@ func TestS3Mock_ConfiguredFnsAreInvoked(t *testing.T) { GetPresignedURLFn: func(_ context.Context, _, _ string, _ time.Duration) (string, error) { return "http://x", nil }, - DeleteMultipleObjectsFn: func(_ context.Context, _ string, _ []string) error { return nil }, + DeleteMultipleObjectsFn: func(_ context.Context, _ string, keys []string) (int, error) { return len(keys), nil }, UploadMultipleObjectsFn: func(_ context.Context, _ string, files []struct { Key string Body io.Reader @@ -201,8 +201,8 @@ func TestS3Mock_ConfiguredFnsAreInvoked(t *testing.T) { if u, err := m.GetPresignedURL(ctx, "b", "k", time.Minute); err != nil || u == "" { t.Errorf("GetPresignedURL = (%q, %v)", u, err) } - if err := m.DeleteMultipleObjects(ctx, "b", []string{"k"}); err != nil { - t.Errorf("DeleteMultipleObjects: %v", err) + if n, err := m.DeleteMultipleObjects(ctx, "b", []string{"k"}); err != nil || n != 1 { + t.Errorf("DeleteMultipleObjects = (%d, %v)", n, err) } results := m.UploadMultipleObjects(ctx, "b", []struct { Key string diff --git a/backend/internal/services/mocks/s3_mock.go b/backend/internal/services/mocks/s3_mock.go index ab9ea7f..ce640ac 100644 --- a/backend/internal/services/mocks/s3_mock.go +++ b/backend/internal/services/mocks/s3_mock.go @@ -32,7 +32,8 @@ type S3Mock struct { DeleteObjectFn func(ctx context.Context, bucketName, key string) error GetObjectMetadataFn func(ctx context.Context, bucketName, key string) (*models.ObjectInfo, error) GetPresignedURLFn func(ctx context.Context, bucketName, key string, expiresIn time.Duration) (string, error) - DeleteMultipleObjectsFn func(ctx context.Context, bucketName string, keys []string) error + DeleteMultipleObjectsFn func(ctx context.Context, bucketName string, keys []string) (int, error) + DeleteObjectsByPrefixFn func(ctx context.Context, bucketName, prefix string) (int, error) UploadMultipleObjectsFn func(ctx context.Context, bucketName string, files []struct { Key string Body io.Reader @@ -120,14 +121,22 @@ func (m *S3Mock) GetPresignedURL(ctx context.Context, bucketName, key string, ex return m.GetPresignedURLFn(ctx, bucketName, key, expiresIn) } -func (m *S3Mock) DeleteMultipleObjects(ctx context.Context, bucketName string, keys []string) error { +func (m *S3Mock) DeleteMultipleObjects(ctx context.Context, bucketName string, keys []string) (int, error) { m.record("DeleteMultipleObjects", bucketName, keys) if m.DeleteMultipleObjectsFn == nil { - return s3NotConfigured("DeleteMultipleObjects") + return 0, s3NotConfigured("DeleteMultipleObjects") } return m.DeleteMultipleObjectsFn(ctx, bucketName, keys) } +func (m *S3Mock) DeleteObjectsByPrefix(ctx context.Context, bucketName, prefix string) (int, error) { + m.record("DeleteObjectsByPrefix", bucketName, prefix) + if m.DeleteObjectsByPrefixFn == nil { + return 0, s3NotConfigured("DeleteObjectsByPrefix") + } + return m.DeleteObjectsByPrefixFn(ctx, bucketName, prefix) +} + func (m *S3Mock) UploadMultipleObjects(ctx context.Context, bucketName string, files []struct { Key string Body io.Reader diff --git a/backend/internal/services/s3.go b/backend/internal/services/s3.go index 8234e8f..cc0a569 100644 --- a/backend/internal/services/s3.go +++ b/backend/internal/services/s3.go @@ -604,16 +604,23 @@ func (s *S3Service) GetObjectMetadata(ctx context.Context, bucketName, key strin }, nil } -// DeleteMultipleObjects deletes multiple objects from a bucket -func (s *S3Service) DeleteMultipleObjects(ctx context.Context, bucketName string, keys []string) error { +// DeleteMultipleObjects deletes multiple objects from a bucket and returns the +// number of objects that were removed (requested keys minus any that failed). +// +// Note: S3/MinIO batch delete is idempotent — removing a key that does not +// exist succeeds and is not reported on the error channel, so it counts toward +// the returned total. The count therefore reflects "keys the delete operation +// did not fail on", which is the strongest signal obtainable without a +// per-key existence check. +func (s *S3Service) DeleteMultipleObjects(ctx context.Context, bucketName string, keys []string) (int, error) { if len(keys) == 0 { - return nil + return 0, nil } // Get bucket-specific MinIO client client, err := s.getMinioClient(ctx, bucketName, OpWrite) if err != nil { - return fmt.Errorf("failed to get MinIO client for bucket %s: %w", bucketName, err) + return 0, fmt.Errorf("failed to get MinIO client for bucket %s: %w", bucketName, err) } // Create channel for objects to delete @@ -629,17 +636,61 @@ func (s *S3Service) DeleteMultipleObjects(ctx context.Context, bucketName string } }() - // Call MinIO RemoveObjects API (batch delete) + // Call MinIO RemoveObjects API (batch delete). RemoveObjects only surfaces + // the objects it FAILED to delete, so we drain the whole channel (which also + // avoids leaking the sender goroutine) and count failures. errorCh := client.RemoveObjects(ctx, bucketName, objectsCh, minio.RemoveObjectsOptions{}) - // Check for errors - for err := range errorCh { - if err.Err != nil { - return fmt.Errorf("failed to delete object %s from bucket %s: %w", err.ObjectName, bucketName, err.Err) + failed := 0 + var firstErr error + for rerr := range errorCh { + if rerr.Err != nil { + failed++ + if firstErr == nil { + firstErr = fmt.Errorf("failed to delete object %s from bucket %s: %w", rerr.ObjectName, bucketName, rerr.Err) + } } } - return nil + if firstErr != nil { + return len(keys) - failed, firstErr + } + + return len(keys), nil +} + +// DeleteObjectsByPrefix recursively deletes every object stored under the given +// prefix (i.e. a "folder"), including the directory marker itself. It returns +// the number of objects that were deleted. +func (s *S3Service) DeleteObjectsByPrefix(ctx context.Context, bucketName, prefix string) (int, error) { + if prefix == "" { + return 0, fmt.Errorf("prefix is required for recursive delete") + } + + // Get bucket-specific MinIO client + client, err := s.getMinioClient(ctx, bucketName, OpWrite) + if err != nil { + return 0, fmt.Errorf("failed to get MinIO client for bucket %s: %w", bucketName, err) + } + + // List every object under the prefix recursively (no delimiter), so nested + // folders are flattened into their concrete keys. + keys := make([]string, 0) + for obj := range client.ListObjects(ctx, bucketName, minio.ListObjectsOptions{ + Prefix: prefix, + Recursive: true, + }) { + if obj.Err != nil { + return 0, fmt.Errorf("failed to list objects under prefix %s in bucket %s: %w", prefix, bucketName, obj.Err) + } + keys = append(keys, obj.Key) + } + + if len(keys) == 0 { + return 0, nil + } + + return s.DeleteMultipleObjects(ctx, bucketName, keys) } // GetPresignedURL generates a pre-signed URL for temporary access to an object diff --git a/backend/internal/services/s3_minio_test.go b/backend/internal/services/s3_minio_test.go index 5bc7b1a..361aed3 100644 --- a/backend/internal/services/s3_minio_test.go +++ b/backend/internal/services/s3_minio_test.go @@ -274,8 +274,8 @@ func TestS3_DeleteMultipleObjects_EmptyKeysIsNoop(t *testing.T) { }) s3 := newS3TestService(t, h) - if err := s3.DeleteMultipleObjects(context.Background(), "whatever", nil); err != nil { - t.Fatalf("empty keys should return nil, got %v", err) + if n, err := s3.DeleteMultipleObjects(context.Background(), "whatever", nil); err != nil || n != 0 { + t.Fatalf("empty keys should return (0, nil), got (%d, %v)", n, err) } if called { t.Error("S3 handler was invoked for empty-keys call") @@ -289,12 +289,114 @@ func TestS3_DeleteMultipleObjects_ServerErrorPropagates(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) defer cancel() - err := s3.DeleteMultipleObjects(ctx, "b-TestS3_DeleteMultipleObjects_ServerErrorPropagates", []string{"a", "b"}) + _, err := s3.DeleteMultipleObjects(ctx, "b-TestS3_DeleteMultipleObjects_ServerErrorPropagates", []string{"a", "b"}) if err == nil { t.Fatal("expected error, got nil") } } +// s3PrefixDeleteHandler serves a ListObjectsV2 response (GET) from listBody and +// a successful multi-object DeleteResult (POST /{bucket}?delete), recording how +// many batch-delete requests were made. +func s3PrefixDeleteHandler(listBody string, deletePosts *int) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + *deletePosts++ + w.Header().Set("Content-Type", "application/xml") + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, ``) + return + } + // Any GET is treated as a ListObjectsV2 request. + w.Header().Set("Content-Type", "application/xml") + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, listBody) + }) +} + +func TestS3_DeleteObjectsByPrefix_EmptyPrefixIsError(t *testing.T) { + // A blank prefix must be rejected before any network call — it would + // otherwise match (and delete) every object in the bucket. + called := false + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + s3ErrorXML(w, http.StatusInternalServerError, "ShouldNotHappen", "") + }) + s3 := newS3TestService(t, h) + + n, err := s3.DeleteObjectsByPrefix(context.Background(), "b-TestS3_DeleteObjectsByPrefix_EmptyPrefixIsError", "") + if err == nil { + t.Fatal("empty prefix should return an error") + } + if n != 0 { + t.Errorf("count = %d, want 0", n) + } + if called { + t.Error("no S3 request should be made for an empty prefix") + } +} + +func TestS3_DeleteObjectsByPrefix_ListsThenDeletes(t *testing.T) { + contents := []struct { + Key string + Size int64 + LastModified string + ETag string + }{ + {Key: "docs/a"}, {Key: "docs/b"}, {Key: "docs/sub/c"}, + } + listBody := listBucketResultXML("b", false, "", contents, nil) + deletePosts := 0 + s3 := newS3TestService(t, s3PrefixDeleteHandler(listBody, &deletePosts)) + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + + n, err := s3.DeleteObjectsByPrefix(ctx, "b-TestS3_DeleteObjectsByPrefix_ListsThenDeletes", "docs/") + if err != nil { + t.Fatalf("DeleteObjectsByPrefix: %v", err) + } + if n != 3 { + t.Errorf("deleted = %d, want 3 (all objects listed under the prefix)", n) + } + if deletePosts == 0 { + t.Error("expected a batch-delete request to be made") + } +} + +func TestS3_DeleteObjectsByPrefix_NoObjectsReturnsZero(t *testing.T) { + listBody := listBucketResultXML("b", false, "", nil, nil) + deletePosts := 0 + s3 := newS3TestService(t, s3PrefixDeleteHandler(listBody, &deletePosts)) + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + + n, err := s3.DeleteObjectsByPrefix(ctx, "b-TestS3_DeleteObjectsByPrefix_NoObjectsReturnsZero", "empty/") + if err != nil { + t.Fatalf("DeleteObjectsByPrefix: %v", err) + } + if n != 0 { + t.Errorf("deleted = %d, want 0", n) + } + if deletePosts != 0 { + t.Errorf("no batch-delete should be made when nothing matches, got %d", deletePosts) + } +} + +func TestS3_DeleteObjectsByPrefix_ListErrorPropagates(t *testing.T) { + h, _ := errS3Handler(http.StatusForbidden, "AccessDenied") + s3 := newS3TestService(t, h) + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + + _, err := s3.DeleteObjectsByPrefix(ctx, "b-TestS3_DeleteObjectsByPrefix_ListErrorPropagates", "docs/") + if err == nil { + t.Fatal("expected the list error to propagate, got nil") + } +} + func TestS3_GetPresignedURL_ReturnsURLWithoutServerCall(t *testing.T) { // Presign is purely local (no network round-trip). Any handler suffices. called := false diff --git a/frontend/src/components/buckets/ObjectBrowserView.tsx b/frontend/src/components/buckets/ObjectBrowserView.tsx index ef82886..df44c51 100644 --- a/frontend/src/components/buckets/ObjectBrowserView.tsx +++ b/frontend/src/components/buckets/ObjectBrowserView.tsx @@ -5,6 +5,7 @@ import {Input} from '@/components/ui/input'; import {ObjectsTable} from './ObjectsTable'; import {CreateDirectoryDialog} from './CreateDirectoryDialog'; import {DeleteObjectDialog} from './DeleteObjectDialog'; +import {ConfirmDialog} from '@/components/ui/confirm-dialog'; import {UploadProgress} from './UploadProgress'; import {ArrowLeft, ChevronRight, FolderPlus, Home, RotateCwIcon, ScanSearch, Search, Trash, Upload} from 'lucide-react'; import {getBreadcrumbs} from '@/lib/file-utils'; @@ -28,7 +29,7 @@ interface ObjectBrowserViewProps { onUploadFiles?: (files: File[]) => Promise; uploadTasks: UploadTask[]; onDeleteObject?: (key: string) => Promise; - onDeleteMultipleObjects?: (keys: string[]) => Promise; + onDeleteMultipleObjects?: (keys: string[], prefixes?: string[]) => Promise; onCreateDirectory?: (name: string) => Promise; onRefresh: () => Promise; onPageChange: (token?: string) => void; @@ -72,6 +73,10 @@ export function ObjectBrowserView({ const [selectedObject, setSelectedObject] = useState(null); const [createDirDialogOpen, setCreateDirDialogOpen] = useState(false); const [selectedFileKeys, setSelectedFileKeys] = useState>(new Set()); + const [selectedFolderKeys, setSelectedFolderKeys] = useState>(new Set()); + // Holds the keys/prefixes awaiting confirmation in the bulk-delete dialog. + const [pendingDelete, setPendingDelete] = useState<{ keys: string[]; prefixes: string[] } | null>(null); + const [bulkDeleting, setBulkDeleting] = useState(false); const { getRootProps, getInputProps, isDragActive } = useDropzone({ onDrop: async (acceptedFiles, _fileRejections, event) => { @@ -133,33 +138,88 @@ export function ObjectBrowserView({ }); }; + const selectedCount = selectedFileKeys.size + selectedFolderKeys.size; + + const toggleInSet = (set: Set, key: string) => { + const next = new Set(set); + if (next.has(key)) { + next.delete(key); + } else { + next.add(key); + } + return next; + }; + const handleToggleFileSelection = (key: string) => { - const newSelected = new Set(selectedFileKeys); - if (newSelected.has(key)) { - newSelected.delete(key); - } else { - newSelected.add(key); - } - setSelectedFileKeys(newSelected); + setSelectedFileKeys(prev => toggleInSet(prev, key)); }; - const handleSelectAllFiles = () => { - const fileKeys = objects - .filter(obj => !obj.isFolder) - .map(obj => obj.key); + const handleToggleFolderSelection = (key: string) => { + setSelectedFolderKeys(prev => toggleInSet(prev, key)); + }; - if (selectedFileKeys.size === fileKeys.length && fileKeys.length > 0) { - setSelectedFileKeys(new Set()); + // Select/deselect the currently visible (filtered) rows. The table passes the + // keys it is actually showing so this stays aligned with the search filter + // instead of operating on the full, unfiltered object list. + const handleSelectAll = (fileKeys: string[], folderKeys: string[]) => { + const allVisibleSelected = + fileKeys.length + folderKeys.length > 0 && + fileKeys.every(k => selectedFileKeys.has(k)) && + folderKeys.every(k => selectedFolderKeys.has(k)); + + if (allVisibleSelected) { + // Drop only the visible rows, leaving any off-screen selection intact. + setSelectedFileKeys(prev => { + const next = new Set(prev); + fileKeys.forEach(k => next.delete(k)); + return next; + }); + setSelectedFolderKeys(prev => { + const next = new Set(prev); + folderKeys.forEach(k => next.delete(k)); + return next; + }); } else { - setSelectedFileKeys(new Set(fileKeys)); + setSelectedFileKeys(prev => new Set([...prev, ...fileKeys])); + setSelectedFolderKeys(prev => new Set([...prev, ...folderKeys])); } }; - const handleBulkDeleteFiles = async () => { - if (!onDeleteMultipleObjects || selectedFileKeys.size === 0) return; + // Open the confirmation dialog for the current multi-selection. + const handleRequestBulkDelete = () => { + if (selectedCount === 0) return; + setPendingDelete({ + keys: Array.from(selectedFileKeys), + prefixes: Array.from(selectedFolderKeys), + }); + }; - await onDeleteMultipleObjects(Array.from(selectedFileKeys)); - setSelectedFileKeys(new Set()); + // Open the confirmation dialog for a single folder (recursive delete). + const handleDeleteFolder = (folderKey: string) => { + setPendingDelete({ keys: [], prefixes: [folderKey] }); + }; + + const handleConfirmBulkDelete = async () => { + if (!pendingDelete || !onDeleteMultipleObjects) return; + + setBulkDeleting(true); + const success = await onDeleteMultipleObjects(pendingDelete.keys, pendingDelete.prefixes); + setBulkDeleting(false); + + if (success) { + // Drop the deleted folders/files from the live selection. + setSelectedFileKeys(prev => { + const next = new Set(prev); + pendingDelete.keys.forEach(k => next.delete(k)); + return next; + }); + setSelectedFolderKeys(prev => { + const next = new Set(prev); + pendingDelete.prefixes.forEach(k => next.delete(k)); + return next; + }); + setPendingDelete(null); + } }; const handleDeleteObject = async (key: string): Promise => { @@ -237,14 +297,14 @@ export function ObjectBrowserView({
- {onDeleteMultipleObjects && selectedFileKeys.size > 0 && ( + {onDeleteMultipleObjects && selectedCount > 0 && ( )} {onUploadFiles && ( @@ -387,6 +447,7 @@ export function ObjectBrowserView({ filterQuery={filterQuery} deepSearch={deepSearch} selectedFileKeys={selectedFileKeys} + selectedFolderKeys={selectedFolderKeys} isDragActive={isDragActive} isLoading={isLoading && !isRefreshing && !isNavigating} isTruncated={isTruncated} @@ -397,8 +458,10 @@ export function ObjectBrowserView({ setSelectedObject(obj); setDeleteObjectDialogOpen(true); } : undefined} + onDeleteFolder={onDeleteMultipleObjects ? (obj) => handleDeleteFolder(obj.key) : undefined} onToggleFileSelection={handleToggleFileSelection} - onSelectAllFiles={handleSelectAllFiles} + onToggleFolderSelection={handleToggleFolderSelection} + onSelectAll={handleSelectAll} onPageChange={onPageChange} onItemsPerPageChange={onItemsPerPageChange} initialPageToken={initialPageToken} @@ -424,6 +487,53 @@ export function ObjectBrowserView({ object={selectedObject} onDeleteObject={handleDeleteObject} /> + + {/* Bulk / Folder Delete Confirmation */} + { + if (!open && !bulkDeleting) setPendingDelete(null); + }} + title={getBulkDeleteTitle(pendingDelete)} + description={getBulkDeleteDescription(pendingDelete)} + confirmLabel="Delete" + loading={bulkDeleting} + onConfirm={handleConfirmBulkDelete} + />
); } + +// Builds a concise title summarising what the bulk-delete dialog will remove. +function getBulkDeleteTitle(pending: { keys: string[]; prefixes: string[] } | null): string { + if (!pending) return 'Delete items?'; + const { keys, prefixes } = pending; + const total = keys.length + prefixes.length; + if (keys.length === 0 && prefixes.length === 1) { + return 'Delete folder?'; + } + return `Delete ${total} item${total !== 1 ? 's' : ''}?`; +} + +// Spells out the file/folder counts and warns that folders are removed recursively. +function getBulkDeleteDescription( + pending: { keys: string[]; prefixes: string[] } | null, +): string { + if (!pending) return ''; + const { keys, prefixes } = pending; + const parts: string[] = []; + if (keys.length > 0) { + parts.push(`${keys.length} file${keys.length !== 1 ? 's' : ''}`); + } + if (prefixes.length > 0) { + parts.push(`${prefixes.length} folder${prefixes.length !== 1 ? 's' : ''}`); + } + const summary = parts.join(' and '); + + if (prefixes.length > 0) { + return `This will permanently delete ${summary}. Every object stored inside the selected folder${ + prefixes.length !== 1 ? 's' : '' + } will be removed recursively.`; + } + return `This will permanently delete ${summary}.`; +} diff --git a/frontend/src/components/buckets/ObjectsTable.tsx b/frontend/src/components/buckets/ObjectsTable.tsx index 07362e6..641addd 100644 --- a/frontend/src/components/buckets/ObjectsTable.tsx +++ b/frontend/src/components/buckets/ObjectsTable.tsx @@ -25,15 +25,22 @@ interface ObjectsTableProps { filterQuery: string; deepSearch: boolean; selectedFileKeys: Set; + selectedFolderKeys: Set; isDragActive: boolean; isLoading?: boolean; isTruncated?: boolean; nextContinuationToken?: string; itemsPerPage: number; onNavigateToFolder: (key: string) => void; + // Optional so the parent can withhold them when the user lacks delete + // permission; canDelete (below) is derived from onDeleteObject. onDeleteObject?: (object: S3Object) => void; + onDeleteFolder?: (object: S3Object) => void; onToggleFileSelection: (key: string) => void; - onSelectAllFiles: () => void; + onToggleFolderSelection: (key: string) => void; + // Receives the keys of the currently *visible* (filtered) rows so selection + // stays aligned with what the search is actually showing. + onSelectAll: (fileKeys: string[], folderKeys: string[]) => void; onPageChange: (token?: string) => void; onItemsPerPageChange: (count: number) => void; initialPageToken?: string; @@ -51,6 +58,7 @@ export function ObjectsTable({ filterQuery, deepSearch, selectedFileKeys, + selectedFolderKeys, isDragActive, isLoading = false, isTruncated = false, @@ -58,8 +66,10 @@ export function ObjectsTable({ itemsPerPage, onNavigateToFolder, onDeleteObject, + onDeleteFolder, onToggleFileSelection, - onSelectAllFiles, + onToggleFolderSelection, + onSelectAll, onPageChange, onItemsPerPageChange, initialPageToken, @@ -218,12 +228,23 @@ export function ObjectsTable({ {canDelete && ( )} @@ -277,10 +298,9 @@ export function ObjectsTable({ {obj.isFolder ? ( onToggleFolderSelection(obj.key)} + aria-label={`Select folder ${obj.key} (deletes its contents recursively)`} /> ) : ( - {!obj.isFolder && ( + {obj.isFolder ? ( + + + + + + onNavigateToFolder(obj.key)}> + + Open + + {onDeleteFolder && ( + <> + + onDeleteFolder(obj)} + > + + Delete folder + + + )} + + + ) : (