s3: fix prefix filtering and delimiter pagination (#11962)

test/s3/versioning/s3_listing_regression_test.go

@@ -0,0 +1,112 @@
+package s3api
+
+import (
+	"bytes"
+	"context"
+	"fmt"
+	"sort"
+	"testing"
+
+	"github.com/aws/aws-sdk-go-v2/aws"
+	"github.com/aws/aws-sdk-go-v2/service/s3"
+	"github.com/aws/aws-sdk-go-v2/service/s3/types"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Check both wire APIs with small pages. The directory walker alone does
+// not exercise prefix normalization, delimiter-marker adjustment, or tokens.
+func TestListingPrefixAndDelimiterPagination(t *testing.T) {
+	client := getS3Client(t)
+	ctx := context.Background()
+	for _, versioning := range []string{"", "Enabled", "Suspended"} {
+		t.Run("versioning="+versioning, func(t *testing.T) {
+			bucket := getNewBucketName()
+			createBucket(t, client, bucket)
+			defer deleteBucket(t, client, bucket)
+			if versioning != "" {
+				enableVersioning(t, client, bucket)
+				if versioning == "Suspended" {
+					_, err := client.PutBucketVersioning(ctx, &s3.PutBucketVersioningInput{
+						Bucket: aws.String(bucket), VersioningConfiguration: &types.VersioningConfiguration{Status: types.BucketVersioningStatusSuspended},
+					})
+					require.NoError(t, err)
+				}
+			}
+			for _, key := range []string{"a", "a-c", "a.txt", "a/b", "foo_bar", "foo_bar_baz", "foo_baz"} {
+				_, err := client.PutObject(ctx, &s3.PutObjectInput{Bucket: aws.String(bucket), Key: aws.String(key), Body: bytes.NewReader([]byte("x"))})
+				require.NoError(t, err)
+			}
+			for _, tc := range []struct {
+				name, prefix, delimiter, startAfter string
+				keys, prefixes                      []string
+			}{
+				{"trailing slash", "a/", "", "", []string{"a/b"}, nil},
+				{"trailing slash custom delimiter", "a/", "_", "", []string{"a/b"}, nil},
+				{"custom delimiter", "foo", "_", "", nil, []string{"foo_"}},
+				{"custom delimiter nested group", "foo_", "_", "", []string{"foo_bar", "foo_baz"}, []string{"foo_bar_"}},
+				{"marker names directory", "a", "", "a", []string{"a-c", "a.txt", "a/b"}, nil},
+				{"marker between name and slash", "a", "", "a.txt", []string{"a/b"}, nil},
+			} {
+				for _, v2 := range []bool{false, true} {
+					for _, pageSize := range []int32{1, 2, 1000} {
+						t.Run(fmt.Sprintf("%s/v2=%t/max=%d", tc.name, v2, pageSize), func(t *testing.T) {
+							var keys, prefixes []string
+							marker, token := tc.startAfter, ""
+							for page := 0; ; page++ {
+								require.Less(t, page, 10, "listing must terminate without repeating a prefix")
+								var objects []types.Object
+								var commonPrefixes []types.CommonPrefix
+								var truncated bool
+								if v2 {
+									in := &s3.ListObjectsV2Input{Bucket: aws.String(bucket), Prefix: aws.String(tc.prefix), Delimiter: aws.String(tc.delimiter), MaxKeys: aws.Int32(pageSize)}
+									if token != "" {
+										in.ContinuationToken = aws.String(token)
+									} else if marker != "" {
+										in.StartAfter = aws.String(marker)
+									}
+									out, err := client.ListObjectsV2(ctx, in)
+									require.NoError(t, err)
+									objects, commonPrefixes, truncated = out.Contents, out.CommonPrefixes, aws.ToBool(out.IsTruncated)
+									next := aws.ToString(out.NextContinuationToken)
+									if truncated {
+										require.NotEmpty(t, next)
+										require.NotEqual(t, token, next, "token must advance")
+									}
+									token = next
+								} else {
+									out, err := client.ListObjects(ctx, &s3.ListObjectsInput{
+										Bucket: aws.String(bucket), Prefix: aws.String(tc.prefix), Delimiter: aws.String(tc.delimiter), MaxKeys: aws.Int32(pageSize), Marker: aws.String(marker),
+									})
+									require.NoError(t, err)
+									objects, commonPrefixes, truncated = out.Contents, out.CommonPrefixes, aws.ToBool(out.IsTruncated)
+									next := aws.ToString(out.NextMarker)
+									if truncated {
+										if next == "" && len(objects) > 0 {
+											next = aws.ToString(objects[len(objects)-1].Key)
+										}
+										require.Greater(t, next, marker, "marker must advance")
+									}
+									marker = next
+								}
+								for _, object := range objects {
+									keys = append(keys, aws.ToString(object.Key))
+								}
+								for _, prefix := range commonPrefixes {
+									prefixes = append(prefixes, aws.ToString(prefix.Prefix))
+								}
+								if !truncated {
+									break
+								}
+							}
+							sort.Strings(keys)
+							sort.Strings(prefixes)
+							assert.Equal(t, tc.keys, keys)
+							assert.Equal(t, tc.prefixes, prefixes)
+						})
+					}
+				}
+			}
+		})
+	}
+}

weed/s3api/s3api_object_handlers_list.go

@@ -538,6 +538,18 @@ func (s3a *S3ApiServer) listFilerEntries(ctx context.Context, req listObjectsReq
 				if excludedKey != "" && !entry.IsDirectory && fmt.Sprintf("%s/%s", dir, entry.Name)[len(bucketPrefix):] == excludedKey {
 					return
 				}
+				// A delimiter marker excludes the entire folded prefix, not just
+				// the key bearing its name. This also applies to delimiters such as
+				// "_", whose groups are not directory blocks in the filer walk.
+				if delimiter != "" {
+					name := entry.Name
+					if entry.IsDirectory && !entry.IsPrefixObject() {
+						name += "/"
+					}
+					if folded := prefixForKey(dir, name); folded != "" && folded <= requestMarker {
+						return
+					}
+				}
 				dirName, entryName, _ := entryUrlEncode(dir, entry.Name, encodingTypeUrl)
 				if entry.IsDirectory {
 					if originalPrefix != "" {
@@ -1170,13 +1182,12 @@ func (s3a *S3ApiServer) doListFilerEntries(ctx context.Context, client filer_pb.
 			// we must stop processing siblings to avoid matching unrelated entries like "foo1000".
 			matchedPrefixDir := cursor.prefixEndsOnDelimiter && entry.Name == prefix && entry.IsDirectory
 			if cursor.prefixEndsOnDelimiter {
-				if entry.Name == prefix && entry.IsDirectory {
-					if delimiter != "/" {
-						cursor.prefixEndsOnDelimiter = false
-					}
-				} else {
+				if !matchedPrefixDir {
 					continue
 				}
+				// Keep the exact-directory filter until emitDir descends into
+				// the deferred block. Clearing it at the name position lets
+				// siblings like "a.txt" leak into a listing for "a/".
 			}
 			if entry.IsDirectory {
 				// glog.V(4).Infof("List Dir Entries %s, file: %s, maxKeys %d", dir, entry.Name, cursor.maxKeys)

weed/s3api/s3api_object_handlers_list_regression_test.go

@@ -0,0 +1,125 @@
+package s3api
+
+import (
+	"context"
+	"fmt"
+	"net/url"
+	"sort"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Exercise the response builder as well as the directory walker: deferred
+// directory resume markers can make the outer loop repeat prefixes.
+// Derive the expected response from S3 keys, independently of filer traversal.
+func TestListFilerEntriesListingRegression(t *testing.T) {
+	keys := []string{
+		"a", "a-c", "a.txt", "a/b", "a/c/d", "a0",
+		"boo/bar", "boo/baz", "boo/boo", "boo/boo2",
+		"foo_bar", "foo_bar_baz", "foo_baz", "logs/", "quux ab", "space dir/value", "z",
+	}
+	sort.Strings(keys)
+	prefixObject := newDir("a")
+	prefixObject.MarkPrefixObject()
+	file := func(name string) *filer_pb.Entry {
+		return &filer_pb.Entry{Name: name, Attributes: &filer_pb.FuseAttributes{}}
+	}
+	filer := &fakeBucketDeleteFiler{entriesByDir: map[string][]*filer_pb.Entry{
+		"/buckets/test": {
+			prefixObject, file("a-c"), file("a.txt"), file("a0"), newDir("boo"),
+			file("foo_bar"), file("foo_bar_baz"), file("foo_baz"),
+			directoryMarker("logs"), file("quux ab"), newDir("space dir"), file("z"),
+		},
+		"/buckets/test/a":         {file("b"), newDir("c")},
+		"/buckets/test/a/c":       {file("d")},
+		"/buckets/test/boo":       {file("bar"), file("baz"), file("boo"), file("boo2")},
+		"/buckets/test/logs":      {},
+		"/buckets/test/space dir": {file("value")},
+	}}
+	s3a := newFailoverTestServer(t, startFakeFiler(t, filer))
+	s3a.option.BucketsPath = "/buckets"
+	s3a.bucketConfigCache = NewBucketConfigCache(time.Minute)
+	for _, versioning := range []string{"", "Enabled", "Suspended"} {
+		t.Run("versioning="+versioning, func(t *testing.T) {
+			s3a.bucketConfigCache.Set("test", &BucketConfig{Name: "test", Versioning: versioning})
+			for _, encoded := range []bool{false, true} {
+				t.Run(fmt.Sprintf("encoded=%t", encoded), func(t *testing.T) {
+					testListingRegressionMatrix(t, s3a, keys, encoded)
+				})
+			}
+		})
+	}
+}
+
+func testListingRegressionMatrix(t *testing.T, s3a *S3ApiServer, keys []string, encoded bool) {
+	t.Helper()
+	for _, prefix := range []string{"", "a", "a/", "a/c/", "boo", "boo/", "boo/b", "foo", "foo_", "logs/", "space dir/", "missing/"} {
+		for _, delimiter := range []string{"", "/", "_"} {
+			for _, pageSize := range []uint16{1, 2, 1000} {
+				t.Run(fmt.Sprintf("prefix=%s/delimiter=%s/max=%d", prefix, delimiter, pageSize), func(t *testing.T) {
+					var wantKeys, wantPrefixes []string
+					prefixes := map[string]bool{}
+					for _, key := range keys {
+						if !strings.HasPrefix(key, prefix) {
+							continue
+						}
+						if delimiter != "" {
+							if i := strings.Index(strings.TrimPrefix(key, prefix), delimiter); i >= 0 {
+								prefixes[key[:len(prefix)+i+len(delimiter)]] = true
+								continue
+							}
+						}
+						wantKeys = append(wantKeys, key)
+					}
+					for p := range prefixes {
+						wantPrefixes = append(wantPrefixes, p)
+					}
+					sort.Strings(wantKeys)
+					sort.Strings(wantPrefixes)
+
+					var gotKeys, gotPrefixes []string
+					marker := ""
+					for page := 0; ; page++ {
+						require.Less(t, page, len(keys)+1, "listing did not terminate")
+						response, err := s3a.listFilerEntries(context.Background(), listObjectsRequest{
+							bucket: "test", prefix: prefix, delimiter: delimiter,
+							marker:        adjustMarkerForDelimiter(marker, prefix, delimiter),
+							requestMarker: marker, maxKeys: pageSize,
+							encodingTypeUrl: encoded,
+						})
+						require.NoError(t, err)
+						decode := func(value string) string {
+							if !encoded {
+								return value
+							}
+							decoded, err := url.QueryUnescape(value)
+							require.NoError(t, err)
+							return decoded
+						}
+						for _, entry := range response.Contents {
+							gotKeys = append(gotKeys, decode(entry.Key))
+						}
+						for _, entry := range response.CommonPrefixes {
+							gotPrefixes = append(gotPrefixes, decode(entry.Prefix))
+						}
+						if !response.IsTruncated {
+							break
+						}
+						require.Greater(t, response.NextMarker, marker, "pagination must advance")
+						marker = response.NextMarker
+					}
+					// Sorting retains duplicates, so repeated prefixes still fail.
+					sort.Strings(gotKeys)
+					sort.Strings(gotPrefixes)
+					assert.Equal(t, wantKeys, gotKeys)
+					assert.Equal(t, wantPrefixes, gotPrefixes)
+				})
+			}
+		}
+	}
+}