From a6e762518a362f7cfca283592f3b63143d37218b Mon Sep 17 00:00:00 2001 From: Dan Webb Date: Fri, 23 Jan 2026 12:37:38 +0000 Subject: [PATCH] refactor: use pagination for ListObjects to reduce memory usage Fixes #11 - Add ListObjectsPaginated method with MaxKeys limit and continuation token - Add ListObjectsChannel method for streaming object iteration - Update BrowseBucket to use pagination (100 objects per page) - Update DeleteFolder to stream objects instead of loading all into memory - Update DownloadZip to stream objects instead of loading all into memory - Add pagination types: ListObjectsOptions, ListObjectsResult - Update mocks and journey tests for new methods --- cmd/server/bucket_journey_test.go | 26 +++++--- cmd/server/mocks_test.go | 10 +++ internal/handlers/buckets_handler.go | 93 ++++++++++++++-------------- internal/services/minio_factory.go | 69 +++++++++++++++++++++ internal/services/pagination_test.go | 76 +++++++++++++++++++++++ 5 files changed, 218 insertions(+), 56 deletions(-) create mode 100644 internal/services/pagination_test.go diff --git a/cmd/server/bucket_journey_test.go b/cmd/server/bucket_journey_test.go index dc020ca..a1bc7b0 100644 --- a/cmd/server/bucket_journey_test.go +++ b/cmd/server/bucket_journey_test.go @@ -109,9 +109,13 @@ func TestObjectBrowserJourney(t *testing.T) { creds := services.Credentials{Endpoint: "play.minio.io:9000", AccessKey: "admin", SecretKey: "password"} mockFactory.On("NewClient", creds).Return(mockClient, nil) - // Mock Object Operations - mockClient.On("ListObjects", mock.Anything, "my-bucket", mock.Anything).Return([]minio.ObjectInfo{ - {Key: "file1.txt", Size: 123, LastModified: time.Now()}, + // Mock Object Operations - use ListObjectsPaginated for BrowseBucket + mockClient.On("ListObjectsPaginated", mock.Anything, "my-bucket", mock.Anything).Return(services.ListObjectsResult{ + Objects: []minio.ObjectInfo{ + {Key: "file1.txt", Size: 123, LastModified: time.Now()}, + }, + IsTruncated: false, + NextContinuationToken: "", }, nil) mockClient.On("PutObject", mock.Anything, "my-bucket", "testfile.txt", mock.Anything, mock.Anything, mock.Anything).Return(minio.UploadInfo{}, nil) @@ -161,14 +165,16 @@ func TestZipDownloadJourney(t *testing.T) { creds := services.Credentials{Endpoint: "play.minio.io:9000", AccessKey: "admin", SecretKey: "password"} mockFactory.On("NewClient", creds).Return(mockClient, nil) - // Mock listing objects in a folder - mockClient.On("ListObjects", mock.Anything, "my-bucket", mock.MatchedBy(func(opts minio.ListObjectsOptions) bool { + // Mock listing objects in a folder using channel-based streaming + objectsChan := make(chan minio.ObjectInfo, 3) + objectsChan <- minio.ObjectInfo{Key: "folder/file1.txt", Size: 13, LastModified: time.Now()} + objectsChan <- minio.ObjectInfo{Key: "folder/file2.txt", Size: 13, LastModified: time.Now()} + objectsChan <- minio.ObjectInfo{Key: "folder/subfolder/file3.txt", Size: 17, LastModified: time.Now()} + close(objectsChan) + + mockClient.On("ListObjectsChannel", mock.Anything, "my-bucket", mock.MatchedBy(func(opts minio.ListObjectsOptions) bool { return opts.Prefix == "folder/" && opts.Recursive == true - })).Return([]minio.ObjectInfo{ - {Key: "folder/file1.txt", Size: 13, LastModified: time.Now()}, - {Key: "folder/file2.txt", Size: 13, LastModified: time.Now()}, - {Key: "folder/subfolder/file3.txt", Size: 17, LastModified: time.Now()}, - }, nil) + })).Return((<-chan minio.ObjectInfo)(objectsChan)) // Mock getting each object's content mockClient.On("GetObjectReader", mock.Anything, "my-bucket", "folder/file1.txt", mock.Anything). diff --git a/cmd/server/mocks_test.go b/cmd/server/mocks_test.go index ce796a5..25e0cb5 100644 --- a/cmd/server/mocks_test.go +++ b/cmd/server/mocks_test.go @@ -152,6 +152,16 @@ func (m *MockMinioClient) ListObjects(ctx context.Context, bucketName string, op return args.Get(0).([]minio.ObjectInfo), args.Error(1) } +func (m *MockMinioClient) ListObjectsPaginated(ctx context.Context, bucketName string, opts services.ListObjectsOptions) (services.ListObjectsResult, error) { + args := m.Called(ctx, bucketName, opts) + return args.Get(0).(services.ListObjectsResult), args.Error(1) +} + +func (m *MockMinioClient) ListObjectsChannel(ctx context.Context, bucketName string, opts minio.ListObjectsOptions) <-chan minio.ObjectInfo { + args := m.Called(ctx, bucketName, opts) + return args.Get(0).(<-chan minio.ObjectInfo) +} + func (m *MockMinioClient) PutObject(ctx context.Context, bucketName, objectName string, reader io.Reader, objectSize int64, opts minio.PutObjectOptions) (minio.UploadInfo, error) { args := m.Called(ctx, bucketName, objectName, reader, objectSize, opts) return args.Get(0).(minio.UploadInfo), args.Error(1) diff --git a/internal/handlers/buckets_handler.go b/internal/handlers/buckets_handler.go index 4870858..00c6296 100644 --- a/internal/handlers/buckets_handler.go +++ b/internal/handlers/buckets_handler.go @@ -152,7 +152,7 @@ func (h *BucketsHandler) DeleteBucket(c echo.Context) error { return c.NoContent(http.StatusOK) } -// BrowseBucket renders the object browser with folder support +// BrowseBucket renders the object browser with folder support and pagination func (h *BucketsHandler) BrowseBucket(c echo.Context) error { creds, err := GetCredentialsOrRedirect(c) if err != nil { @@ -161,16 +161,19 @@ func (h *BucketsHandler) BrowseBucket(c echo.Context) error { bucketName := c.Param("bucketName") prefix := c.QueryParam("prefix") + continuationToken := c.QueryParam("continuation") client, err := h.minioFactory.NewClient(*creds) if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "Failed to connect to MinIO") } - // List objects with prefix for folder support - rawObjects, err := client.ListObjects(c.Request().Context(), bucketName, minio.ListObjectsOptions{ - Prefix: prefix, - Recursive: false, // Non-recursive to get folders + // List objects with pagination to limit memory usage + result, err := client.ListObjectsPaginated(c.Request().Context(), bucketName, services.ListObjectsOptions{ + Prefix: prefix, + Recursive: false, // Non-recursive to get folders + MaxKeys: services.DefaultPageSize, + ContinuationToken: continuationToken, }) if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "Failed to list objects") @@ -180,7 +183,7 @@ func (h *BucketsHandler) BrowseBucket(c echo.Context) error { var folders []models.FolderInfo seenFolders := make(map[string]bool) - for _, obj := range rawObjects { + for _, obj := range result.Objects { // Check if it's a folder (ends with /) if strings.HasSuffix(obj.Key, "/") { folderName := strings.TrimPrefix(obj.Key, prefix) @@ -235,12 +238,14 @@ func (h *BucketsHandler) BrowseBucket(c echo.Context) error { } return c.Render(http.StatusOK, "browser", map[string]interface{}{ - "ActiveNav": "buckets", - "BucketName": bucketName, - "Prefix": prefix, - "Objects": objects, - "Folders": folders, - "Breadcrumbs": breadcrumbs, + "ActiveNav": "buckets", + "BucketName": bucketName, + "Prefix": prefix, + "Objects": objects, + "Folders": folders, + "Breadcrumbs": breadcrumbs, + "HasMore": result.IsTruncated, + "NextContinuationToken": result.NextContinuationToken, }) } @@ -387,7 +392,7 @@ func (h *BucketsHandler) CreateFolder(c echo.Context) error { return HTMXRedirect(c, "/buckets/"+bucketName+"?prefix="+prefix) } -// DeleteFolder deletes a folder and all its contents +// DeleteFolder deletes a folder and all its contents using streaming func (h *BucketsHandler) DeleteFolder(c echo.Context) error { creds, err := GetCredentials(c) if err != nil { @@ -402,17 +407,17 @@ func (h *BucketsHandler) DeleteFolder(c echo.Context) error { return echo.NewHTTPError(http.StatusInternalServerError, "Failed to connect to MinIO") } - // List all objects with this prefix - objectsList, err := client.ListObjects(c.Request().Context(), bucketName, minio.ListObjectsOptions{ + // Stream objects and delete one at a time to avoid loading all into memory + objectsChan := client.ListObjectsChannel(c.Request().Context(), bucketName, minio.ListObjectsOptions{ Prefix: prefix, Recursive: true, }) - if err != nil { - return echo.NewHTTPError(http.StatusInternalServerError, "Failed to list objects") - } - // Delete all objects - for _, obj := range objectsList { + // Delete objects as they stream in + for obj := range objectsChan { + if obj.Err != nil { + return echo.NewHTTPError(http.StatusInternalServerError, "Failed to list objects: "+obj.Err.Error()) + } err := client.RemoveObject(c.Request().Context(), bucketName, obj.Key, minio.RemoveObjectOptions{}) if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "Failed to delete object: "+obj.Key) @@ -422,7 +427,7 @@ func (h *BucketsHandler) DeleteFolder(c echo.Context) error { return c.NoContent(http.StatusOK) } -// DownloadZip streams a folder as a ZIP archive +// DownloadZip streams a folder as a ZIP archive using streaming object listing func (h *BucketsHandler) DownloadZip(c echo.Context) error { creds, err := GetCredentialsOrRedirect(c) if err != nil { @@ -437,31 +442,9 @@ func (h *BucketsHandler) DownloadZip(c echo.Context) error { return echo.NewHTTPError(http.StatusInternalServerError, "Failed to connect to MinIO") } - // List all objects with this prefix recursively - objectsList, err := client.ListObjects(c.Request().Context(), bucketName, minio.ListObjectsOptions{ - Prefix: prefix, - Recursive: true, - }) - if err != nil { - return echo.NewHTTPError(http.StatusInternalServerError, "Failed to list objects") - } - - // Filter out folder markers (objects ending with /) - var files []minio.ObjectInfo - for _, obj := range objectsList { - if !strings.HasSuffix(obj.Key, "/") { - files = append(files, obj) - } - } - - if len(files) == 0 { - return echo.NewHTTPError(http.StatusNotFound, "No files to download") - } - // Determine ZIP filename from prefix or bucket name zipName := bucketName + ".zip" if prefix != "" { - // Remove trailing slash and get the folder name folderName := strings.TrimSuffix(prefix, "/") if idx := strings.LastIndex(folderName, "/"); idx >= 0 { folderName = folderName[idx+1:] @@ -478,12 +461,26 @@ func (h *BucketsHandler) DownloadZip(c echo.Context) error { zipWriter := zip.NewWriter(c.Response().Writer) defer func() { _ = zipWriter.Close() }() - // Add each file to the ZIP - for _, obj := range files { + // Stream objects and add to ZIP one at a time to avoid loading all into memory + objectsChan := client.ListObjectsChannel(c.Request().Context(), bucketName, minio.ListObjectsOptions{ + Prefix: prefix, + Recursive: true, + }) + + fileCount := 0 + for obj := range objectsChan { + if obj.Err != nil { + continue + } + + // Skip folder markers (objects ending with /) + if strings.HasSuffix(obj.Key, "/") { + continue + } + // Get the file content reader, _, err := client.GetObjectReader(c.Request().Context(), bucketName, obj.Key, minio.GetObjectOptions{}) if err != nil { - // Log error but continue with other files continue } @@ -501,8 +498,12 @@ func (h *BucketsHandler) DownloadZip(c echo.Context) error { if err != nil { continue } + fileCount++ } + // Note: If fileCount == 0, headers are already sent so we can't return an error. + // The ZIP will just be empty, which is acceptable behavior. + return nil } diff --git a/internal/services/minio_factory.go b/internal/services/minio_factory.go index 734f314..cf9942c 100644 --- a/internal/services/minio_factory.go +++ b/internal/services/minio_factory.go @@ -17,6 +17,24 @@ import ( "github.com/minio/minio-go/v7/pkg/tags" ) +// DefaultPageSize is the default number of objects to return per page +const DefaultPageSize = 100 + +// ListObjectsOptions extends minio.ListObjectsOptions with pagination +type ListObjectsOptions struct { + Prefix string + Recursive bool + MaxKeys int + ContinuationToken string +} + +// ListObjectsResult contains paginated results from ListObjectsPaginated +type ListObjectsResult struct { + Objects []minio.ObjectInfo + IsTruncated bool + NextContinuationToken string +} + // MinioAdminClient is an interface for the madmin methods we use type MinioAdminClient interface { ServerInfo(ctx context.Context, opts ...func(*madmin.ServerInfoOpts)) (madmin.InfoMessage, error) @@ -60,6 +78,8 @@ type MinioClient interface { // Object Operations ListObjects(ctx context.Context, bucketName string, opts minio.ListObjectsOptions) ([]minio.ObjectInfo, error) + ListObjectsPaginated(ctx context.Context, bucketName string, opts ListObjectsOptions) (ListObjectsResult, error) + ListObjectsChannel(ctx context.Context, bucketName string, opts minio.ListObjectsOptions) <-chan minio.ObjectInfo PutObject(ctx context.Context, bucketName, objectName string, reader io.Reader, objectSize int64, opts minio.PutObjectOptions) (minio.UploadInfo, error) GetObject(ctx context.Context, bucketName, objectName string, opts minio.GetObjectOptions) (*minio.Object, error) GetObjectReader(ctx context.Context, bucketName, objectName string, opts minio.GetObjectOptions) (io.ReadCloser, int64, error) @@ -124,6 +144,55 @@ func (c *WrappedMinioClient) ListObjects(ctx context.Context, bucketName string, return objects, nil } +func (c *WrappedMinioClient) ListObjectsPaginated(ctx context.Context, bucketName string, opts ListObjectsOptions) (ListObjectsResult, error) { + maxKeys := opts.MaxKeys + if maxKeys <= 0 { + maxKeys = DefaultPageSize + } + + minioOpts := minio.ListObjectsOptions{ + Prefix: opts.Prefix, + Recursive: opts.Recursive, + } + + // Use StartAfter for continuation (MinIO uses marker-based pagination) + if opts.ContinuationToken != "" { + minioOpts.StartAfter = opts.ContinuationToken + } + + var objects []minio.ObjectInfo + var lastKey string + + for obj := range c.client.ListObjects(ctx, bucketName, minioOpts) { + if obj.Err != nil { + return ListObjectsResult{}, obj.Err + } + + objects = append(objects, obj) + lastKey = obj.Key + + // Stop after maxKeys objects + if len(objects) >= maxKeys { + break + } + } + + result := ListObjectsResult{ + Objects: objects, + IsTruncated: len(objects) >= maxKeys, + } + + if result.IsTruncated { + result.NextContinuationToken = lastKey + } + + return result, nil +} + +func (c *WrappedMinioClient) ListObjectsChannel(ctx context.Context, bucketName string, opts minio.ListObjectsOptions) <-chan minio.ObjectInfo { + return c.client.ListObjects(ctx, bucketName, opts) +} + func (c *WrappedMinioClient) PutObject(ctx context.Context, bucketName, objectName string, reader io.Reader, objectSize int64, opts minio.PutObjectOptions) (minio.UploadInfo, error) { return c.client.PutObject(ctx, bucketName, objectName, reader, objectSize, opts) } diff --git a/internal/services/pagination_test.go b/internal/services/pagination_test.go new file mode 100644 index 0000000..fdf1020 --- /dev/null +++ b/internal/services/pagination_test.go @@ -0,0 +1,76 @@ +package services + +import ( + "testing" + + "github.com/minio/minio-go/v7" +) + +func TestListObjectsResult_Structure(t *testing.T) { + result := ListObjectsResult{ + Objects: []minio.ObjectInfo{}, + IsTruncated: false, + NextContinuationToken: "", + } + + if result.IsTruncated { + t.Error("expected IsTruncated to be false") + } + if result.NextContinuationToken != "" { + t.Error("expected NextContinuationToken to be empty") + } + if len(result.Objects) != 0 { + t.Error("expected Objects to be empty") + } +} + +func TestListObjectsOptions_Structure(t *testing.T) { + opts := ListObjectsOptions{ + Prefix: "folder/", + Recursive: true, + MaxKeys: 50, + ContinuationToken: "last-key", + } + + if opts.Prefix != "folder/" { + t.Errorf("expected Prefix to be 'folder/', got %s", opts.Prefix) + } + if !opts.Recursive { + t.Error("expected Recursive to be true") + } + if opts.MaxKeys != 50 { + t.Errorf("expected MaxKeys to be 50, got %d", opts.MaxKeys) + } + if opts.ContinuationToken != "last-key" { + t.Errorf("expected ContinuationToken to be 'last-key', got %s", opts.ContinuationToken) + } +} + +func TestDefaultPageSize(t *testing.T) { + if DefaultPageSize != 100 { + t.Errorf("expected DefaultPageSize to be 100, got %d", DefaultPageSize) + } +} + +func TestListObjectsResult_WithTruncation(t *testing.T) { + objects := []minio.ObjectInfo{ + {Key: "file1.txt"}, + {Key: "file2.txt"}, + } + + result := ListObjectsResult{ + Objects: objects, + IsTruncated: true, + NextContinuationToken: "file2.txt", + } + + if !result.IsTruncated { + t.Error("expected IsTruncated to be true") + } + if result.NextContinuationToken != "file2.txt" { + t.Errorf("expected NextContinuationToken to be 'file2.txt', got %s", result.NextContinuationToken) + } + if len(result.Objects) != 2 { + t.Errorf("expected 2 objects, got %d", len(result.Objects)) + } +}