Fix ListBucket in s3bolt backend to honor page.Marker and page.MaxKeys#114
Fix ListBucket in s3bolt backend to honor page.Marker and page.MaxKeys#114Aliexe-code wants to merge 2 commits intojohannesboyne:masterfrom
Conversation
7f04192 to
c022794
Compare
Fixes johannesboyne#66 The s3bolt backend now properly supports pagination with the marker parameter and maxKeys limit, matching the behavior of the s3mem backend. Changes: - Remove ErrInternalPageNotImplemented error for non-empty pages - Implement page.Marker support using c.Seek() to jump to the marker - Implement page.MaxKeys support with proper IsTruncated and NextMarker - Add comprehensive tests for marker-based pagination - Refactor tests with helper functions to reduce code duplication
c022794 to
9169291
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #114 +/- ##
==========================================
+ Coverage 49.35% 51.70% +2.34%
==========================================
Files 47 47
Lines 5155 4000 -1155
==========================================
- Hits 2544 2068 -476
+ Misses 2299 1615 -684
- Partials 312 317 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
johannesboyne
left a comment
There was a problem hiding this comment.
@Aliexe-code - thanks a lot for the contribution! Just some minor remarks - see comments.
For testing, these tests could be added
package s3bolt
import (
"os"
"strings"
"testing"
"github.com/johannesboyne/gofakes3"
)
func mkBackend(t *testing.T, objs []string) (*Backend, func()) {
t.Helper()
f, err := os.CreateTemp("", "pr114-*.db")
if err != nil { t.Fatal(err) }
_ = f.Close()
b, err := NewFile(f.Name())
if err != nil { t.Fatal(err) }
if err := b.CreateBucket("b"); err != nil { t.Fatal(err) }
for _, o := range objs {
if _, err := b.PutObject("b", o, nil, strings.NewReader(o), int64(len(o)), nil); err != nil { t.Fatal(err) }
}
return b, func(){ _ = os.Remove(f.Name()) }
}
func TestRepro_NextMarkerEmptyWithDelimiter(t *testing.T) {
b, done := mkBackend(t, []string{"a/1", "a/2", "a/3", "b/1"})
defer done()
p := &gofakes3.Prefix{HasDelimiter: true, Delimiter: "/"}
out, err := b.ListBucket("b", p, gofakes3.ListBucketPage{MaxKeys:1})
if err != nil { t.Fatal(err) }
if !out.IsTruncated { t.Fatalf("expected truncated") }
if out.NextMarker == "" { t.Fatalf("expected non-empty next marker, got empty") }
}
func TestRepro_TruncatedShouldRespectPrefixFilter(t *testing.T) {
b, done := mkBackend(t, []string{"x/1", "x/2", "y/1"})
defer done()
p := &gofakes3.Prefix{HasPrefix:true, Prefix:"x/"}
out, err := b.ListBucket("b", p, gofakes3.ListBucketPage{MaxKeys:2})
if err != nil { t.Fatal(err) }
if out.IsTruncated { t.Fatalf("expected not truncated when no more matching keys") }
}
func TestRepro_DuplicateCommonPrefixCountsTowardMaxKeys(t *testing.T) {
b, done := mkBackend(t, []string{"a/1", "a/2", "a/3", "b/1"})
defer done()
p := &gofakes3.Prefix{HasDelimiter:true, Delimiter:"/"}
out, err := b.ListBucket("b", p, gofakes3.ListBucketPage{MaxKeys:2})
if err != nil { t.Fatal(err) }
if len(out.CommonPrefixes) != 2 {
t.Fatalf("expected 2 common prefixes (a/, b/), got %d", len(out.CommonPrefixes))
}
}
backend/s3bolt/backend.go
Outdated
| cnt++ | ||
| if page.MaxKeys > 0 && cnt >= page.MaxKeys { | ||
| nextK, _ := c.Next() |
There was a problem hiding this comment.
IsTruncated is computed by checking whether any later key exists (c.Next()), not whether any later key matches the requested prefix/delimiter filter. So, I assume paginated calls can return IsTruncated=true even when there are no more matching results.
| LastModified: gofakes3.NewContentTime(b.LastModified.UTC()), | ||
| } | ||
| objects.Add(item) | ||
| lastKey = key |
There was a problem hiding this comment.
When a page is truncated on CommonPrefixes entries, NextMarker can be empty because lastKey is only set for object contents, not for common-prefix matches, so clients cannot reliably request the next page (especially for delimiter-based listing).
backend/s3bolt/backend.go
Outdated
| lastKey = key | ||
| } | ||
|
|
||
| cnt++ |
There was a problem hiding this comment.
cnt increments for every matching key, including duplicates that collapse into the same CommonPrefix. This violates each rolled-up prefix counts once behavior and causes early truncation.
- Fix cnt increment to only count unique items (not duplicate CommonPrefix entries) - Fix IsTruncated logic to properly check prefix/delimiter filter for remaining keys - Fix NextMarker for CommonPrefixes by setting lastKey to MatchedPart - Add reproduction tests for delimiter pagination edge cases Fixes review comments from johannesboyne on PR johannesboyne#114
|



Fixes #66
The s3bolt backend now properly supports pagination with the marker parameter and maxKeys limit, matching the behavior of the s3mem backend.
Changes: