Merge pull request #184 from AndreyKostov/storagedriver-s3-fix-empty-root

Fix S3 driver's list when the root directory is either "" or "/"

docs/storage/driver/s3/s3.go

@@ -587,6 +587,15 @@ func (d *driver) List(path string) ([]string, error) {
 	if path != "/" && path[len(path)-1] != '/' {
 		path = path + "/"
 	}
+
+	// This is to cover for the cases when the rootDirectory of the driver is either "" or "/".
+	// In those cases, there is no root prefix to replace and we must actually add a "/" to all
+	// results in order to keep them as valid paths as recognized by storagedriver.PathRegexp
+	prefix := ""
+	if d.s3Path("") == "" {
+		prefix = "/"
+	}
+
 	listResponse, err := d.Bucket.List(d.s3Path(path), "/", "", listMax)
 	if err != nil {
 		return nil, err
@@ -597,11 +606,11 @@ func (d *driver) List(path string) ([]string, error) {
 
 	for {
 		for _, key := range listResponse.Contents {
-			files = append(files, strings.Replace(key.Key, d.s3Path(""), "", 1))
+			files = append(files, strings.Replace(key.Key, d.s3Path(""), prefix, 1))
 		}
 
 		for _, commonPrefix := range listResponse.CommonPrefixes {
-			directories = append(directories, strings.Replace(commonPrefix[0:len(commonPrefix)-1], d.s3Path(""), "", 1))
+			directories = append(directories, strings.Replace(commonPrefix[0:len(commonPrefix)-1], d.s3Path(""), prefix, 1))
 		}
 
 		if listResponse.IsTruncated {

docs/storage/driver/s3/s3_test.go

@@ -16,6 +16,8 @@ import (
 // Hook up gocheck into the "go test" runner.
 func Test(t *testing.T) { check.TestingT(t) }
 
+type S3DriverConstructor func(rootDirectory string) (*Driver, error)
+
 func init() {
 	accessKey := os.Getenv("AWS_ACCESS_KEY")
 	secretKey := os.Getenv("AWS_SECRET_KEY")
@@ -30,7 +32,7 @@ func init() {
 	}
 	defer os.Remove(root)
 
-	s3DriverConstructor := func(region aws.Region) (storagedriver.StorageDriver, error) {
+	s3DriverConstructor := func(rootDirectory string) (*Driver, error) {
 		encryptBool := false
 		if encrypt != "" {
 			encryptBool, err = strconv.ParseBool(encrypt)
@@ -47,7 +49,7 @@ func init() {
 			}
 		}
 
-		v4AuthBool := true
+		v4AuthBool := false
 		if v4auth != "" {
 			v4AuthBool, err = strconv.ParseBool(v4auth)
 			if err != nil {
@@ -59,12 +61,12 @@ func init() {
 			accessKey,
 			secretKey,
 			bucket,
-			region,
+			aws.GetRegion(region),
 			encryptBool,
 			secureBool,
 			v4AuthBool,
 			minChunkSize,
-			root,
+			rootDirectory,
 		}
 
 		return New(parameters)
@@ -78,14 +80,18 @@ func init() {
 		return ""
 	}
 
-	// for _, region := range aws.Regions {
+	driverConstructor := func() (storagedriver.StorageDriver, error) {
-	// 	if region == aws.USGovWest {
+		return s3DriverConstructor(root)
-	// 		continue
+	}
-	// 	}
+
+	testsuites.RegisterInProcessSuite(driverConstructor, skipCheck)
+
+	// s3Constructor := func() (*Driver, error) {
+	// 	return s3DriverConstructor(aws.GetRegion(region))
+	// }
+
+	RegisterS3DriverSuite(s3DriverConstructor, skipCheck)
 
-	testsuites.RegisterInProcessSuite(func() (storagedriver.StorageDriver, error) {
-		return s3DriverConstructor(aws.GetRegion(region))
-	}, skipCheck)
 	// testsuites.RegisterIPCSuite(driverName, map[string]string{
 	// 	"accesskey": accessKey,
 	// 	"secretkey": secretKey,
@@ -95,3 +101,50 @@ func init() {
 	// }, skipCheck)
 	// }
 }
+
+func RegisterS3DriverSuite(s3DriverConstructor S3DriverConstructor, skipCheck testsuites.SkipCheck) {
+	check.Suite(&S3DriverSuite{
+		Constructor: s3DriverConstructor,
+		SkipCheck:   skipCheck,
+	})
+}
+
+type S3DriverSuite struct {
+	Constructor S3DriverConstructor
+	testsuites.SkipCheck
+}
+
+func (suite *S3DriverSuite) SetUpSuite(c *check.C) {
+	if reason := suite.SkipCheck(); reason != "" {
+		c.Skip(reason)
+	}
+}
+
+func (suite *S3DriverSuite) TestEmptyRootList(c *check.C) {
+	validRoot, err := ioutil.TempDir("", "driver-")
+	c.Assert(err, check.IsNil)
+	defer os.Remove(validRoot)
+
+	rootedDriver, err := suite.Constructor(validRoot)
+	c.Assert(err, check.IsNil)
+	emptyRootDriver, err := suite.Constructor("")
+	c.Assert(err, check.IsNil)
+	slashRootDriver, err := suite.Constructor("/")
+	c.Assert(err, check.IsNil)
+
+	filename := "/test"
+	contents := []byte("contents")
+	err = rootedDriver.PutContent(filename, contents)
+	c.Assert(err, check.IsNil)
+	defer rootedDriver.Delete(filename)
+
+	keys, err := emptyRootDriver.List("/")
+	for _, path := range keys {
+		c.Assert(storagedriver.PathRegexp.MatchString(path), check.Equals, true)
+	}
+
+	keys, err = slashRootDriver.List("/")
+	for _, path := range keys {
+		c.Assert(storagedriver.PathRegexp.MatchString(path), check.Equals, true)
+	}
+}