serve s3: support listing with delimiters other than "/"

Listings treated any delimiter as "/", so a listing with a delimiter
such as "a" or "." returned the keys and directories rather than the
keys rolled up into common prefixes at the delimiter. A delimiter of
whitespace was ignored altogether.

Now any delimiter is supported. Delimiters other than "/" are found by
walking the directory tree below the prefix, skipping directories
whose names contain the delimiter as all their keys roll up into the
same common prefix.

A prefix starting with "/" now matches nothing, as no key can start
with "/", rather than listing the whole bucket.

cmd/serve/s3/backend.go

@@ -272,18 +272,13 @@ func (b *s3Backend) ListBucket(ctx context.Context, bucket string, prefix *gofak
 		prefix = emptyPrefix
 	}
 
-	// workaround
-	if strings.TrimSpace(prefix.Prefix) == "" {
-		prefix.HasPrefix = false
-	}
-	if strings.TrimSpace(prefix.Delimiter) == "" {
-		prefix.HasDelimiter = false
-	}
-
 	response := gofakes3.NewObjectList()
-	path, remaining := prefixParser(prefix)
+	// Keys can't start with "/" so such a prefix matches nothing
+	if strings.HasPrefix(prefix.Prefix, "/") {
+		return response, nil
+	}
 
-	err = b.listPage(_vfs, bucket, path, remaining, prefix.HasDelimiter, page, response)
+	err = b.listPage(_vfs, bucket, prefix.Prefix, prefix.Delimiter, page, response)
 	if err == gofakes3.ErrNoSuchKey {
 		// AWS just returns an empty list
 		response = gofakes3.NewObjectList()

cmd/serve/s3/list.go

@@ -38,6 +38,13 @@ type lister struct {
 	keys      int    // keys added to the page so far
 	lastKey   string // the last key added to the page
 	dirsRead  int    // directories read, for debug logging
+
+	// With a delimiter other than "/", which can't be found by
+	// listing directories, the whole tree is walked and the keys are
+	// rolled up into common prefixes.
+	prefix     string // the prefix of the listing
+	delimiter  string // the delimiter, or "" if none or "/"
+	lastPrefix string // the last common prefix added to the page
 }
 
 // newLister makes a lister which fills response with at most one page of the
@@ -128,6 +135,65 @@ func (l *lister) addObject(key string, entry vfs.Node) error {
 	return nil
 }
 
+// commonPrefix returns the common prefix key rolls up into, if the
+// listing has a delimiter other than "/" and key has it after the
+// listing's prefix.
+func (l *lister) commonPrefix(key string) (prefix string, ok bool) {
+	if l.delimiter == "" || !strings.HasPrefix(key, l.prefix) {
+		return "", false
+	}
+	i := strings.Index(key[len(l.prefix):], l.delimiter)
+	if i < 0 {
+		return "", false
+	}
+	return key[:len(l.prefix)+i+len(l.delimiter)], true
+}
+
+// addRolledUp adds the common prefix a key rolls up into to the page
+// unless it has been added already, returning errPageFull if the page
+// is now complete.
+//
+// The keys rolling up into a common prefix are contiguous in the
+// listing, so it only needs comparing with the last one.
+func (l *lister) addRolledUp(prefix string) error {
+	if prefix == l.lastPrefix || l.skipKey(prefix) {
+		return nil
+	}
+	if err := l.addCommonPrefix(prefix); err != nil {
+		return err
+	}
+	l.lastPrefix = prefix
+	return nil
+}
+
+// hasKeys reports whether the directory fp in the bucket has any keys
+// below it.
+func (l *lister) hasKeys(fdPath string) bool {
+	fp, err := bucketDirPath(l.bucket, fdPath)
+	if err != nil {
+		return false
+	}
+	dirEntries, err := getDirEntries(fp, l.vfs)
+	if err != nil {
+		return false
+	}
+	for _, entry := range dirEntries {
+		if isTempObject(entry.Name()) {
+			continue
+		}
+		if !entry.IsDir() || l.hasKeys(path.Join(fdPath, entry.Name())) {
+			return true
+		}
+	}
+	return false
+}
+
+// isTempObject reports whether name is a temporary object of an
+// in-progress upload, which is hidden from listings.
+func isTempObject(name string) bool {
+	return strings.HasPrefix(name, tempObjectPrefix) || strings.HasPrefix(name, legacyMultipartUploadPrefix)
+}
+
 // list walks the directory fdPath, adding the entries whose leaf name starts
 // with name to the page.
 //
@@ -159,7 +225,7 @@ func (l *lister) list(fdPath, name string, addPrefix bool) error {
 		object := entry.Name()
 
 		// Hide the temporary objects of in-progress uploads
-		if strings.HasPrefix(object, tempObjectPrefix) || strings.HasPrefix(object, legacyMultipartUploadPrefix) {
+		if isTempObject(object) {
 			continue
 		}
 
@@ -184,6 +250,17 @@ func (l *lister) list(fdPath, name string, addPrefix bool) error {
 			if l.skipDir(prefixWithTrailingSlash) {
 				continue
 			}
+			// If the delimiter is in the directory's path every
+			// key below it rolls up into the same common prefix,
+			// so the directory needn't be read.
+			if prefix, ok := l.commonPrefix(prefixWithTrailingSlash); ok {
+				if l.hasKeys(objectPath) {
+					if err := l.addRolledUp(prefix); err != nil {
+						return err
+					}
+				}
+				continue
+			}
 			err := l.list(objectPath, "", false)
 			if errors.Is(err, gofakes3.ErrNoSuchKey) {
 				// The directory went away while we were listing it, so
@@ -193,6 +270,12 @@ func (l *lister) list(fdPath, name string, addPrefix bool) error {
 				return err
 			}
 		} else {
+			if prefix, ok := l.commonPrefix(objectPath); ok {
+				if err := l.addRolledUp(prefix); err != nil {
+					return err
+				}
+				continue
+			}
 			if l.skipKey(objectPath) {
 				continue
 			}
@@ -204,16 +287,25 @@ func (l *lister) list(fdPath, name string, addPrefix bool) error {
 	return nil
 }
 
-// listPage fills response with one page of the listing of the directory
-// fdPath in bucket, as described by page.
+// listPage fills response with one page of the listing of bucket with
+// prefix and delimiter, as described by page.
 //
-// Only entries whose leaf name starts with name are listed. If addPrefix is
-// set, subdirectories are reported as common prefixes, otherwise the whole
-// subtree below fdPath is listed.
-//
-// It returns gofakes3.ErrNoSuchKey if fdPath is not a directory.
-func (b *s3Backend) listPage(_vfs *vfs.VFS, bucket, fdPath, name string, addPrefix bool, page gofakes3.ListBucketPage, response *gofakes3.ObjectList) error {
+// It returns gofakes3.ErrNoSuchKey if the directory the prefix is in
+// doesn't exist.
+func (b *s3Backend) listPage(_vfs *vfs.VFS, bucket, prefix, delimiter string, page gofakes3.ListBucketPage, response *gofakes3.ObjectList) error {
 	l := newLister(b, _vfs, bucket, page, response)
+	// The prefix is split into the directory to list and the start of
+	// the names in it to list.
+	fdPath, name := "", prefix
+	if i := strings.LastIndexByte(prefix, '/'); i >= 0 {
+		fdPath, name = prefix[:i], prefix[i+1:]
+	}
+	// A "/" delimiter is found by listing directories, any other by
+	// walking the whole tree.
+	addPrefix := delimiter == "/"
+	if delimiter != "" && delimiter != "/" {
+		l.prefix, l.delimiter = prefix, delimiter
+	}
 	err := l.list(fdPath, name, addPrefix)
 	if err != nil && !errors.Is(err, errPageFull) {
 		return err

cmd/serve/s3/list_test.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"path/filepath"
 	"slices"
+	"strings"
 	"testing"
 
 	"github.com/rclone/gofakes3"
@@ -263,6 +264,74 @@ func TestListSkipDir(t *testing.T) {
 	}
 }
 
+// TestListAnyDelimiter checks listings with any delimiter, and with a
+// prefix, against the keys an S3 listing of the same flat keyspace has,
+// whatever the page size.
+func TestListAnyDelimiter(t *testing.T) {
+	files := []string{
+		"a.txt", "a/b/c", "a/bc", "a0",
+		"b%ar", "b ar", "b.ar", "bar", "bazar", "baz/qux", "baz/q ux/x",
+		"cab", "c/a/b/z", "c/ab", "dir/sub/file", "foo",
+	}
+	var paths []string
+	for _, file := range files {
+		paths = append(paths, "bucket/"+file)
+	}
+	b := newListBackend(t, paths...)
+	// An empty directory has no keys, so mustn't be a common prefix
+	// for delimiters other than "/"
+	require.NoError(t, os.MkdirAll(filepath.Join(b.s.f.Root(), "bucket", "empty", "abc"), 0777))
+
+	// want returns the keys and common prefixes S3 lists
+	want := func(prefix, delimiter string) (keys []string) {
+		seen := map[string]bool{}
+		keySpace := files
+		if delimiter == "/" {
+			// Listing directories with "/" shows empty ones too, as
+			// it always has
+			keySpace = append(slices.Clone(files), "empty/abc/")
+		}
+		for _, key := range keySpace {
+			if !strings.HasPrefix(key, prefix) {
+				continue
+			}
+			if delimiter != "" {
+				if i := strings.Index(key[len(prefix):], delimiter); i >= 0 {
+					key = key[:len(prefix)+i+len(delimiter)]
+				}
+			}
+			if !seen[key] {
+				seen[key] = true
+				keys = append(keys, key)
+			}
+		}
+		slices.Sort(keys)
+		return keys
+	}
+
+	for _, prefix := range []string{"", "b", "ba", "baz/", "c", "c/a", "e", "x"} {
+		for _, delimiter := range []string{"", "/", "a", "ba", " ", ".", "%", "/a", "b/", "z"} {
+			t.Run(fmt.Sprintf("prefix=%q,delimiter=%q", prefix, delimiter), func(t *testing.T) {
+				expected := want(prefix, delimiter)
+				for _, maxKeys := range []int{1, 2, 3, 1000} {
+					got := listPages(t, b, listPrefix(prefix, delimiter), maxKeys)
+					assert.Equal(t, expected, got, "max-keys %d", maxKeys)
+				}
+			})
+		}
+	}
+}
+
+// TestListPrefixLeadingSlash checks a prefix starting with "/" matches
+// nothing, as no key starts with "/".
+func TestListPrefixLeadingSlash(t *testing.T) {
+	b := newListBackend(t, "bucket/a/b", "bucket/c")
+	keys, _ := listOnce(t, b, listPrefix("/", ""), gofakes3.ListBucketPage{MaxKeys: 1000})
+	assert.Empty(t, keys)
+	keys, _ = listOnce(t, b, listPrefix("/a", "/"), gofakes3.ListBucketPage{MaxKeys: 1000})
+	assert.Empty(t, keys)
+}
+
 // TestListETagQuoted checks the ETags in listings are quoted as on S3
 // and in HEAD responses.
 func TestListETagQuoted(t *testing.T) {

cmd/serve/s3/serve_s3.md

@@ -441,10 +441,13 @@ When using `PutObject` or `DeleteObject`, rclone will automatically
 create or clean up empty folders. If you don't want to clean up empty
 folders automatically, use `--no-cleanup`.
 
-When using `ListObjects`, rclone treats any delimiter as `/`. Without a
-delimiter the listing is recursive: rclone walks the directory tree
-below the prefix, reading only as many directories as it needs to fill
-each page.
+When using `ListObjects` with the delimiter `/`, rclone lists the
+directory the prefix is in, showing its subdirectories as common
+prefixes, including empty ones. Without a delimiter the listing is
+recursive: rclone walks the directory tree below the prefix, reading
+only as many directories as it needs to fill each page. Any other
+delimiter is supported too, but needs the whole directory tree below
+the prefix walking, unless the delimiter is in the directory names.
 
 Versioning is not currently supported.
 

cmd/serve/s3/utils.go

@@ -130,14 +130,6 @@ func bucketDirPath(bucketName, dirName string) (string, error) {
 	return bucketObjectPath(bucketName, dirName)
 }
 
-func prefixParser(p *gofakes3.Prefix) (path, remaining string) {
-	idx := strings.LastIndexByte(p.Prefix, '/')
-	if idx < 0 {
-		return "", p.Prefix
-	}
-	return p.Prefix[:idx], p.Prefix[idx+1:]
-}
-
 // mkdirRecursive makes the directory path and any of its parents which
 // don't exist, returning the top-most directory it made, or "" if path
 // existed already.