Merge pull request #1227 from stevvooe/walk-sorted

storage: enforce sorted traversal during Walk

docs/storage/walk.go

@@ -3,6 +3,7 @@ package storage
 import (
 	"errors"
 	"fmt"
+	"sort"
 
 	"github.com/docker/distribution/context"
 	storageDriver "github.com/docker/distribution/registry/storage/driver"
@@ -26,7 +27,12 @@ func Walk(ctx context.Context, driver storageDriver.StorageDriver, from string,
 	if err != nil {
 		return err
 	}
+	sort.Stable(sort.StringSlice(children))
 	for _, child := range children {
+		// TODO(stevvooe): Calling driver.Stat for every entry is quite
+		// expensive when running against backends with a slow Stat
+		// implementation, such as s3. This is very likely a serious
+		// performance bottleneck.
 		fileInfo, err := driver.Stat(ctx, child)
 		if err != nil {
 			return err
@@ -38,7 +44,9 @@ func Walk(ctx context.Context, driver storageDriver.StorageDriver, from string,
 		}
 
 		if fileInfo.IsDir() && !skipDir {
-			Walk(ctx, driver, child, f)
+			if err := Walk(ctx, driver, child, f); err != nil {
+				return err
+			}
 		}
 	}
 	return nil

docs/storage/walk_test.go

@@ -2,6 +2,7 @@ package storage
 
 import (
 	"fmt"
+	"sort"
 	"testing"
 
 	"github.com/docker/distribution/context"
@@ -11,14 +12,7 @@ import (
 
 func testFS(t *testing.T) (driver.StorageDriver, map[string]string, context.Context) {
 	d := inmemory.New()
-	c := []byte("")
 	ctx := context.Background()
-	if err := d.PutContent(ctx, "/a/b/c/d", c); err != nil {
-		t.Fatalf("Unable to put to inmemory fs")
-	}
-	if err := d.PutContent(ctx, "/a/b/c/e", c); err != nil {
-		t.Fatalf("Unable to put to inmemory fs")
-	}
 
 	expected := map[string]string{
 		"/a":       "dir",
@@ -26,6 +20,22 @@ func testFS(t *testing.T) (driver.StorageDriver, map[string]string, context.Cont
 		"/a/b/c":   "dir",
 		"/a/b/c/d": "file",
 		"/a/b/c/e": "file",
+		"/a/b/f":   "dir",
+		"/a/b/f/g": "file",
+		"/a/b/f/h": "file",
+		"/a/b/f/i": "file",
+		"/z":       "dir",
+		"/z/y":     "file",
+	}
+
+	for p, typ := range expected {
+		if typ != "file" {
+			continue
+		}
+
+		if err := d.PutContent(ctx, p, []byte(p)); err != nil {
+			t.Fatalf("unable to put content into fixture: %v", err)
+		}
 	}
 
 	return d, expected, ctx
@@ -41,19 +51,26 @@ func TestWalkErrors(t *testing.T) {
 		t.Error("Expected invalid root err")
 	}
 
+	errEarlyExpected := fmt.Errorf("Early termination")
+
 	err = Walk(ctx, d, "/", func(fileInfo driver.FileInfo) error {
 		// error on the 2nd file
 		if fileInfo.Path() == "/a/b" {
-			return fmt.Errorf("Early termination")
+			return errEarlyExpected
 		}
+
 		delete(expected, fileInfo.Path())
 		return nil
 	})
 	if len(expected) != fileCount-1 {
 		t.Error("Walk failed to terminate with error")
 	}
-	if err != nil {
+	if err != errEarlyExpected {
-		t.Error(err.Error())
+		if err == nil {
+			t.Fatalf("expected an error due to early termination")
+		} else {
+			t.Error(err.Error())
+		}
 	}
 
 	err = Walk(ctx, d, "/nonexistant", func(fileInfo driver.FileInfo) error {
@@ -67,6 +84,7 @@ func TestWalkErrors(t *testing.T) {
 
 func TestWalk(t *testing.T) {
 	d, expected, ctx := testFS(t)
+	var traversed []string
 	err := Walk(ctx, d, "/", func(fileInfo driver.FileInfo) error {
 		filePath := fileInfo.Path()
 		filetype, ok := expected[filePath]
@@ -82,13 +100,26 @@ func TestWalk(t *testing.T) {
 			if filetype != "file" {
 				t.Errorf("Unexpected file type: %q", filePath)
 			}
+
+			// each file has its own path as the contents. If the length
+			// doesn't match the path length, fail.
+			if fileInfo.Size() != int64(len(fileInfo.Path())) {
+				t.Fatalf("unexpected size for %q: %v != %v",
+					fileInfo.Path(), fileInfo.Size(), len(fileInfo.Path()))
+			}
 		}
 		delete(expected, filePath)
+		traversed = append(traversed, filePath)
 		return nil
 	})
 	if len(expected) > 0 {
 		t.Errorf("Missed files in walk: %q", expected)
 	}
+
+	if !sort.StringsAreSorted(traversed) {
+		t.Errorf("result should be sorted: %v", traversed)
+	}
+
 	if err != nil {
 		t.Fatalf(err.Error())
 	}