From aea873cc9278a6b70b0780e424863d6add13a9c5 Mon Sep 17 00:00:00 2001 From: Collin Shoop Date: Mon, 28 Jun 2021 13:10:07 -0400 Subject: [PATCH] storagedriver/s3: test refactor. fixed a bug in WalkFallback preventing ErrSkipDir from stopping gracefully --- registry/storage/driver/walk.go | 21 +-- registry/storage/driver/walk_test.go | 190 ++++++++++++--------------- 2 files changed, 97 insertions(+), 114 deletions(-) diff --git a/registry/storage/driver/walk.go b/registry/storage/driver/walk.go index 8ded5b8be..66c381b17 100644 --- a/registry/storage/driver/walk.go +++ b/registry/storage/driver/walk.go @@ -22,9 +22,14 @@ type WalkFn func(fileInfo FileInfo) error // to a directory, the directory will not be entered and Walk // will continue the traversal. If fileInfo refers to a normal file, processing stops func WalkFallback(ctx context.Context, driver StorageDriver, from string, f WalkFn) error { + err, _ := doWalkFallback(ctx, driver, from, f) + return err +} + +func doWalkFallback(ctx context.Context, driver StorageDriver, from string, f WalkFn) (error, bool) { children, err := driver.List(ctx, from) if err != nil { - return err + return err, false } sort.Stable(sort.StringSlice(children)) for _, child := range children { @@ -40,22 +45,22 @@ func WalkFallback(ctx context.Context, driver StorageDriver, from string, f Walk logrus.WithField("path", child).Infof("ignoring deleted path") continue default: - return err + return err, false } } err = f(fileInfo) if err == nil && fileInfo.IsDir() { - if err := WalkFallback(ctx, driver, child, f); err != nil { - return err + if err, ok := doWalkFallback(ctx, driver, child, f); err != nil || !ok { + return err, ok } } else if err == ErrSkipDir { - // Stop iteration if it's a file, otherwise noop if it's a directory + // noop for folders, will just skip if !fileInfo.IsDir() { - return nil + return nil, false // no error but stop iteration } } else if err != nil { - return err + return err, false } } - return nil + return nil, true } diff --git a/registry/storage/driver/walk_test.go b/registry/storage/driver/walk_test.go index c4abd9d06..2a3a4fb2b 100644 --- a/registry/storage/driver/walk_test.go +++ b/registry/storage/driver/walk_test.go @@ -2,7 +2,6 @@ package driver import ( "context" - "errors" "fmt" "strings" "testing" @@ -14,10 +13,10 @@ type changingFileSystem struct { keptFiles map[string]bool } -func (cfs *changingFileSystem) List(ctx context.Context, path string) ([]string, error) { +func (cfs *changingFileSystem) List(_ context.Context, _ string) ([]string, error) { return cfs.fileset, nil } -func (cfs *changingFileSystem) Stat(ctx context.Context, path string) (FileInfo, error) { +func (cfs *changingFileSystem) Stat(_ context.Context, path string) (FileInfo, error) { kept, ok := cfs.keptFiles[path] if ok && kept { return &FileInfoInternal{ @@ -31,6 +30,7 @@ func (cfs *changingFileSystem) Stat(ctx context.Context, path string) (FileInfo, type fileSystem struct { StorageDriver + // maps folder to list results fileset map[string][]string } @@ -81,120 +81,98 @@ func TestWalkFallback(t *testing.T) { "/folder2": {"/folder2/file1"}, }, } - expected := []string{ - "/file1", - "/folder1", - "/folder1/file1", - "/folder2", - "/folder2/file1", - } + noopFn := func(fileInfo FileInfo) error { return nil } - var walked []string - err := WalkFallback(context.Background(), d, "/", func(fileInfo FileInfo) error { - if fileInfo.IsDir() != d.isDir(fileInfo.Path()) { - t.Fatalf("fileInfo isDir not matching file system: expected %t actual %t", d.isDir(fileInfo.Path()), fileInfo.IsDir()) - } - walked = append(walked, fileInfo.Path()) - return nil - }) - if err != nil { - t.Fatalf(err.Error()) - } - compareWalked(t, expected, walked) -} - -// Walk is expected to skip directory on ErrSkipDir -func TestWalkFallbackSkipDirOnDir(t *testing.T) { - d := &fileSystem{ - fileset: map[string][]string{ - "/": {"/file1", "/folder1", "/folder2"}, - "/folder1": {"/folder1/file1"}, - "/folder2": {"/folder2/file1"}, + tcs := []struct { + name string + fn WalkFn + from string + expected []string + err bool + }{ + { + name: "walk all", + fn: noopFn, + expected: []string{ + "/file1", + "/folder1", + "/folder1/file1", + "/folder2", + "/folder2/file1", + }, + }, + { + name: "skip directory", + fn: func(fileInfo FileInfo) error { + if fileInfo.Path() == "/folder1" { + return ErrSkipDir + } + if strings.Contains(fileInfo.Path(), "/folder1") { + t.Fatalf("skipped dir %s and should not walk %s", "/folder1", fileInfo.Path()) + } + return nil + }, + expected: []string{ + "/file1", + "/folder1", // return ErrSkipDir, skip anything under /folder1 + // skip /folder1/file1 + "/folder2", + "/folder2/file1", + }, + }, + { + name: "stop early", + fn: func(fileInfo FileInfo) error { + if fileInfo.Path() == "/folder1/file1" { + return ErrSkipDir + } + return nil + }, + expected: []string{ + "/file1", + "/folder1", + "/folder1/file1", + // stop early + }, + }, + { + name: "from folder", + fn: noopFn, + expected: []string{ + "/folder1/file1", + }, + from: "/folder1", }, } - skipDir := "/folder1" - expected := []string{ - "/file1", - "/folder1", // return ErrSkipDir, skip anything under /folder1 - // skip /folder1/file1 - "/folder2", - "/folder2/file1", - } - var walked []string - err := WalkFallback(context.Background(), d, "/", func(fileInfo FileInfo) error { - walked = append(walked, fileInfo.Path()) - if fileInfo.Path() == skipDir { - return ErrSkipDir + for _, tc := range tcs { + var walked []string + if tc.from == "" { + tc.from = "/" } - if strings.Contains(fileInfo.Path(), skipDir) { - t.Fatalf("skipped dir %s and should not walk %s", skipDir, fileInfo.Path()) - } - return nil - }) - if err != nil { - t.Fatalf("expected Walk to not error %v", err) - } - compareWalked(t, expected, walked) -} - -func TestWalkFallbackSkipDirOnFile(t *testing.T) { - d := &fileSystem{ - fileset: map[string][]string{ - "/": {"/file1", "/file2", "/file3"}, - }, - } - skipFile := "/file2" - expected := []string{ - "/file1", - "/file2", // return ErrSkipDir, stop early + t.Run(tc.name, func(t *testing.T) { + err := WalkFallback(context.Background(), d, tc.from, func(fileInfo FileInfo) error { + walked = append(walked, fileInfo.Path()) + if fileInfo.IsDir() != d.isDir(fileInfo.Path()) { + t.Fatalf("fileInfo isDir not matching file system: expected %t actual %t", d.isDir(fileInfo.Path()), fileInfo.IsDir()) + } + return tc.fn(fileInfo) + }) + if tc.err && err == nil { + t.Fatalf("expected err") + } + if !tc.err && err != nil { + t.Fatalf(err.Error()) + } + compareWalked(t, tc.expected, walked) + }) } - var walked []string - err := WalkFallback(context.Background(), d, "/", func(fileInfo FileInfo) error { - walked = append(walked, fileInfo.Path()) - if fileInfo.Path() == skipFile { - return ErrSkipDir - } - return nil - }) - if err != nil { - t.Fatalf("expected Walk to not error %v", err) - } - compareWalked(t, expected, walked) -} - -// Walk is expected to skip directory on ErrSkipDir -func TestWalkFallbackErr(t *testing.T) { - d := &fileSystem{ - fileset: map[string][]string{ - "/": {"/file1", "/file2", "/file3"}, - }, - } - errFile := "/file2" - expected := []string{ - "/file1", - "/file2", // return ErrSkipDir, stop early - } - expectedErr := errors.New("foo") - - var walked []string - err := WalkFallback(context.Background(), d, "/", func(fileInfo FileInfo) error { - walked = append(walked, fileInfo.Path()) - if fileInfo.Path() == errFile { - return expectedErr - } - return nil - }) - if err != expectedErr { - t.Fatalf("unexpected err %v", err) - } - compareWalked(t, expected, walked) } func compareWalked(t *testing.T, expected, walked []string) { if len(walked) != len(expected) { - t.Fatalf("Mismatch number of fileInfo walked %d expected %d", len(walked), len(expected)) + t.Fatalf("Mismatch number of fileInfo walked %d expected %d; walked %s; expected %s;", len(walked), len(expected), walked, expected) } for i := range walked { if walked[i] != expected[i] {