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 + + + )} + + + ) : (