Implementing file search, pagination, bulk ops & URL S3 instance routing #83
Conversation
enzodjabali
force-pushed
the
file-search-pagination-bulk-ops-and-url-s3-instance-routing
branch
from
3c8eeb7 to
edecd60
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors S3 instance selection to be URL-based and enhances the bucket object UI with search, sorting, pagination, and bulk operations.
Changes:
- Replace global “current instance” switching with instance selection encoded in the URL (e.g.
/{instance}/buckets/...), updating routes and templates accordingly. - Extend bucket views (single- and multi-instance) to support search, sorting by key/size/owner/last-modified, and pagination, including “show all”.
- Add bulk delete and bulk download (ZIP) operations for objects, including UI checkboxes and corresponding HTTP handlers.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
web/template/layout.html.tmpl |
Adjusts S3 instance dropdown to use anchor links to /{instance}/buckets and guards dropdown toggling with null checks, aligning the UI with URL-based instance selection. |
web/template/buckets.html.tmpl |
Routes bucket links and bucket-creation POSTs through /{CurrentS3.Name}/... paths and updates the connection error tip to reflect the new instance list UX. |
web/template/bucket.html.tmpl |
Reworks the bucket page to be instance-aware in links (/{CurrentS3.Name}/buckets/...), and adds search, sortable headers, pagination controls, and UI for bulk object actions (download/delete). |
main.go |
Reconfigures HTTP routing so the root redirects to the first instance’s /buckets and all bucket/API endpoints are namespaced by {instance}, adding routes for bulk delete and bulk download. |
internal/app/s3manager/s3.go |
Extends the S3 interface with a RemoveObjects method to support streaming bulk deletions. |
internal/app/s3manager/multi_s3_manager.go |
Removes global current-instance state, adds GetInstance (by ID or name), and deprecates the old current-instance accessors in favor of URL-based selection. |
internal/app/s3manager/manager_handlers.go |
Updates manager-backed handlers to resolve the instance from the {instance} route var, enhances the bucket view with search/sort/pagination for multi-instance, and adds manager-aware bulk delete/download handlers. |
internal/app/s3manager/instance_handlers.go |
Simplifies the instances API to just list configured instances, removing the server-side “switch instance” endpoint. |
internal/app/s3manager/bulk_operations.go |
Introduces HTTP handlers for bulk object deletion using RemoveObjects and for on-the-fly ZIP creation to download multiple objects. |
internal/app/s3manager/bucket_view.go |
Enhances the single-instance bucket view with search, sorting, and pagination, and wires template helper functions needed by the updated bucket.html.tmpl. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // HandleBulkDeleteObjects deletes multiple objects from a bucket. | ||
| func HandleBulkDeleteObjects(s3 S3) http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| bucketName := mux.Vars(r)["bucketName"] | ||
|
|
||
| var req BulkDeleteRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| handleHTTPError(w, fmt.Errorf("error parsing request: %w", err)) | ||
| return | ||
| } | ||
|
|
||
| if len(req.Keys) == 0 { | ||
| http.Error(w, "no keys provided", http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| // Create a channel for objects to delete | ||
| objectsCh := make(chan minio.ObjectInfo) | ||
|
|
||
| // Send object names to the channel | ||
| go func() { | ||
| defer close(objectsCh) | ||
| for _, key := range req.Keys { | ||
| objectsCh <- minio.ObjectInfo{Key: key} | ||
| } | ||
| }() | ||
|
|
||
| // Remove objects | ||
| errorCh := s3.RemoveObjects(r.Context(), bucketName, objectsCh, minio.RemoveObjectsOptions{}) | ||
|
|
||
| // Check for errors | ||
| for err := range errorCh { | ||
| if err.Err != nil { | ||
| handleHTTPError(w, fmt.Errorf("error removing object %s: %w", err.ObjectName, err.Err)) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusOK) | ||
| w.Write([]byte(`{"success": true}`)) | ||
| } | ||
| } | ||
|
|
||
| // HandleBulkDownloadObjects downloads multiple objects as a ZIP archive. | ||
| func HandleBulkDownloadObjects(s3 S3) http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| bucketName := mux.Vars(r)["bucketName"] | ||
|
|
||
| // Parse the form to get the keys | ||
| if err := r.ParseForm(); err != nil { | ||
| handleHTTPError(w, fmt.Errorf("error parsing form: %w", err)) | ||
| return | ||
| } | ||
|
|
||
| keysJSON := r.FormValue("keys") | ||
| var keys []string | ||
| if err := json.Unmarshal([]byte(keysJSON), &keys); err != nil { | ||
| handleHTTPError(w, fmt.Errorf("error parsing keys: %w", err)) | ||
| return | ||
| } | ||
|
|
||
| if len(keys) == 0 { | ||
| http.Error(w, "no keys provided", http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| // Set headers for ZIP download | ||
| timestamp := time.Now().Format("20060102-150405") | ||
| zipFilename := fmt.Sprintf("%s-%s.zip", bucketName, timestamp) | ||
| w.Header().Set("Content-Type", "application/zip") | ||
| w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=\"%s\"", zipFilename)) | ||
|
|
||
| // Create a new ZIP writer | ||
| zipWriter := zip.NewWriter(w) | ||
| defer zipWriter.Close() | ||
|
|
||
| // Add each object to the ZIP | ||
| for _, key := range keys { | ||
| // Get the object from S3 | ||
| object, err := s3.GetObject(r.Context(), bucketName, key, minio.GetObjectOptions{}) | ||
| if err != nil { | ||
| // Log error but continue with other files | ||
| continue | ||
| } | ||
|
|
||
| // Get object info to check if it's valid | ||
| _, err = object.Stat() | ||
| if err != nil { | ||
| object.Close() | ||
| continue | ||
| } | ||
|
|
||
| // Create a file in the ZIP | ||
| zipFile, err := zipWriter.Create(key) | ||
| if err != nil { | ||
| object.Close() | ||
| continue | ||
| } | ||
|
|
||
| // Copy the object content to the ZIP file | ||
| _, err = io.Copy(zipFile, object) | ||
| object.Close() | ||
| if err != nil { | ||
| // Error writing to ZIP, but we can't return HTTP error at this point | ||
| continue | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new bulk delete and bulk download handlers introduce non-trivial server behavior (streaming deletes via RemoveObjects and on-the-fly ZIP creation) but currently have no tests. Given that other S3 handlers in this package are covered by unit tests, please add tests that exercise successful bulk operations and common error cases (invalid/missing keys, S3 errors) to guard against regressions.
| PutObject(ctx context.Context, bucketName, objectName string, reader io.Reader, objectSize int64, opts minio.PutObjectOptions) (minio.UploadInfo, error) | ||
| RemoveBucket(ctx context.Context, bucketName string) error | ||
| RemoveObject(ctx context.Context, bucketName, objectName string, opts minio.RemoveObjectOptions) error | ||
| RemoveObjects(ctx context.Context, bucketName string, objectsCh <-chan minio.ObjectInfo, opts minio.RemoveObjectsOptions) <-chan minio.RemoveObjectError |
There was a problem hiding this comment.
Adding RemoveObjects to the S3 interface (here) means all existing implementations, including internal/app/s3manager/mocks/s3.go, must be updated to define this method; currently the mock S3Mock does not implement RemoveObjects, so the _ s3manager.S3 = &S3Mock{} assertion will fail to compile. Please extend the mock and any other concrete implementations to satisfy the updated interface before merging.
| RemoveObjects(ctx context.Context, bucketName string, objectsCh <-chan minio.ObjectInfo, opts minio.RemoveObjectsOptions) <-chan minio.RemoveObjectError |
| // BulkDownloadRequest represents the request body for bulk download | ||
| type BulkDownloadRequest struct { | ||
| Keys []string `json:"keys"` | ||
| } | ||
|
|
There was a problem hiding this comment.
BulkDownloadRequest is declared but never used, which adds noise and may confuse future maintainers about the supported request formats for bulk downloads. Consider removing this type or wiring it into a JSON-based bulk-download endpoint so it reflects actual behavior.
| // BulkDownloadRequest represents the request body for bulk download | |
| type BulkDownloadRequest struct { | |
| Keys []string `json:"keys"` | |
| } |
web/template/buckets.html.tmpl
Outdated
| {{ range $bucket := .Buckets }} | ||
| <div class="col l6 m12"> | ||
| <a href="{{$.RootURL}}/buckets/{{ $bucket.Name }}" style="color:black;"> | ||
| <a href="{{$.RootURL}}/{{$.CurrentS3.Name}}/buckets/{{ $bucket.Name }}" style="color:black;"> |
There was a problem hiding this comment.
This template now hardcodes {{$.CurrentS3.Name}} into the bucket link, but the single-instance handler HandleBucketsView still renders buckets.html.tmpl with a page data struct that does not have a CurrentS3 field. When HandleBucketsView is used (e.g. in existing tests or single-backend deployments), executing this template will cause a runtime template error (can't evaluate field CurrentS3), so either extend that handler's page data to include CurrentS3 or avoid referencing CurrentS3 when it is not available.
| <a href="{{$.RootURL}}/{{$.CurrentS3.Name}}/buckets/{{ $bucket.Name }}" style="color:black;"> | |
| <a href="{{$.RootURL}}/buckets/{{ $bucket.Name }}" style="color:black;"> |
web/template/bucket.html.tmpl
Outdated
| {{ end }} | ||
| <div style="margin-top: 20px;"> | ||
| <a href="{{$.RootURL}}/buckets" class="waves-effect waves-green btn"> | ||
| <a href="{{$.RootURL}}/{{$.CurrentS3.Name}}/buckets" class="waves-effect waves-green btn"> |
There was a problem hiding this comment.
The updated bucket view template uses {{$.CurrentS3.Name}} in several navigation links, but HandleBucketView (the non-multi-instance handler) still executes this template with a page data struct that lacks CurrentS3. This will cause template execution to fail for callers using HandleBucketView directly (including bucket_view_test.go), so either add CurrentS3/S3Instances fields to that handler's page data or guard these links so they do not depend on fields that may be absent.
| <a href="{{$.RootURL}}/{{$.CurrentS3.Name}}/buckets" class="waves-effect waves-green btn"> | |
| <a href="{{$.RootURL}}/buckets" class="waves-effect waves-green btn"> |
enzodjabali
force-pushed
the
file-search-pagination-bulk-ops-and-url-s3-instance-routing
branch
2 times, most recently
from
c7af603 to
f02b027
Compare
enzodjabali
force-pushed
the
file-search-pagination-bulk-ops-and-url-s3-instance-routing
branch
from
f02b027 to
ef0ca4a
Compare
|
This change sounds like something that would be perfect for a current client of mine. An automated check seems to have failed when merging the pull request ("password required" when logging in to Docker Hub. Not sure what needs to be done to fix this. @enzodjabali: Is there anything I can do to help? At the very least you should know that people are rooting for you. :-) |
2 participants
Hi @cloudlena, here's a PR with handy file management upgrades + fix for the global S3 instance
currentIDissue from PR #73.Changes
While working on this PR I realised that in my previous PR #73, I used a global
currentIDin theMultiS3Managerstruct. This meant all access to s3manager shared the same selected instance.I refactored this to use URL-based routing where each instance has its own URL path (e.g.,
/my-first-instance/buckets/my-bucket).Then this PR implements better file handling in buckets:
Added search bar for files in S3 buckets
Implemented pagination for file listing
Added checkboxes for multi-select: bulk download/delete files
Sort file listing by Key, Size, Owner & Last Modified
Switched to URL-based S3 instance routing
Cheers :)