From b55922effe027f2708fd528bb697194acb58e9e5 Mon Sep 17 00:00:00 2001 From: Karthic Rao Date: Thu, 17 Mar 2016 06:00:22 +0530 Subject: [PATCH] Fix for Istruncated set to true under certain conditions. Optimizing List Objects by using binary sort to discard entries in cases where prefix or marker is set. Adding test coverage to ListObjects. Adding benchmark to ListObjects. --- pkg/fs/api_suite_test.go | 2 - pkg/fs/dir.go | 108 +++-- pkg/fs/fs-bucket-listobjects.go | 51 ++- pkg/fs/fs-bucket-listobjects_test.go | 611 +++++++++++++++++++++++++++ pkg/fs/fs-bucket_test.go | 2 +- 5 files changed, 725 insertions(+), 49 deletions(-) create mode 100644 pkg/fs/fs-bucket-listobjects_test.go diff --git a/pkg/fs/api_suite_test.go b/pkg/fs/api_suite_test.go index f7e3ca31f..ecf2864fd 100644 --- a/pkg/fs/api_suite_test.go +++ b/pkg/fs/api_suite_test.go @@ -24,7 +24,6 @@ import ( "encoding/xml" "math/rand" "strconv" - "time" "gopkg.in/check.v1" ) @@ -216,7 +215,6 @@ func testPaging(c *check.C, create func() Filesystem) { c.Assert(len(result.Objects), check.Equals, 1) c.Assert(result.Prefixes[0], check.Equals, "this/is/also/") } - time.Sleep(time.Second) // check delimited results with delimiter without prefix { diff --git a/pkg/fs/dir.go b/pkg/fs/dir.go index 7bbc0194c..bc2fe21a1 100644 --- a/pkg/fs/dir.go +++ b/pkg/fs/dir.go @@ -26,11 +26,11 @@ import ( ) const ( - // listObjectsLimit - maximum list objects limit + // ListObjectsLimit - maximum list objects limit. listObjectsLimit = 1000 ) -// isDirEmpty - returns whether given directory is empty or not +// IsDirEmpty - returns whether given directory is empty or not. func isDirEmpty(dirname string) (status bool, err error) { f, err := os.Open(dirname) if err == nil { @@ -44,7 +44,7 @@ func isDirEmpty(dirname string) (status bool, err error) { return } -// isDirExist - returns whether given directory is exist or not +// IsDirExist - returns whether given directory is exist or not. func isDirExist(dirname string) (status bool, err error) { fi, err := os.Lstat(dirname) if err == nil { @@ -54,11 +54,17 @@ func isDirExist(dirname string) (status bool, err error) { return } -// byName implements sort.Interface for sorting os.FileInfo list +// ByName implements sort.Interface for sorting os.FileInfo list. type byName []os.FileInfo -func (f byName) Len() int { return len(f) } -func (f byName) Swap(i, j int) { f[i], f[j] = f[j], f[i] } +func (f byName) Len() int { + return len(f) +} + +func (f byName) Swap(i, j int) { + f[i], f[j] = f[j], f[i] +} + func (f byName) Less(i, j int) bool { n1 := f[i].Name() if f[i].IsDir() { @@ -73,7 +79,7 @@ func (f byName) Less(i, j int) bool { return n1 < n2 } -// ObjectInfo - object info +// ObjectInfo - object info. type ObjectInfo struct { Bucket string Name string @@ -85,31 +91,61 @@ type ObjectInfo struct { Err error } -// readDir - read 'scanDir' directory. It returns list of ObjectInfo where -// each object name is appended with 'namePrefix' -func readDir(scanDir, namePrefix string) (objInfos []ObjectInfo) { +// Using sort.Search() internally to jump to the file entry containing the prefix. +func searchFileInfos(fileInfos []os.FileInfo, x string) int { + processFunc := func(i int) bool { + return fileInfos[i].Name() >= x + } + return sort.Search(len(fileInfos), processFunc) +} + +// ReadDir - read 'scanDir' directory. It returns list of ObjectInfo. +// Each object name is appended with 'namePrefix'. +func readDir(scanDir, namePrefix, queryPrefix string, isFirst bool) (objInfos []ObjectInfo) { f, err := os.Open(scanDir) if err != nil { objInfos = append(objInfos, ObjectInfo{Err: err}) return } - fis, err := f.Readdir(-1) if err != nil { f.Close() objInfos = append(objInfos, ObjectInfo{Err: err}) return } - - // Close the directory + // Close the directory. f.Close() - // Sort files by Name. sort.Sort(byName(fis)) - // Populate []ObjectInfo from []FileInfo + var prefixIndex int + // Searching for entries with objectName containing prefix. + // Binary search is used for efficient search. + if queryPrefix != "" && isFirst { + prefixIndex = searchFileInfos(fis, queryPrefix) + if prefixIndex == len(fis) { + return + } + + if !strings.HasPrefix(fis[prefixIndex].Name(), queryPrefix) { + return + } + fis = fis[prefixIndex:] + + } + + // Populate []ObjectInfo from []FileInfo. for _, fi := range fis { name := fi.Name() + if queryPrefix != "" && isFirst { + // If control is here then there is a queryPrefix, and there are objects which satisfies the prefix. + // Since the result is sorted, the object names which satisfies query prefix would be stored one after the other. + // Push the objectInfo only if its contains the prefix. + // This ensures that the channel containing object Info would only has objects with the given queryPrefix. + if !strings.HasPrefix(name, queryPrefix) { + return + } + } size := fi.Size() modTime := fi.ModTime() isDir := fi.Mode().IsDir() @@ -122,7 +158,8 @@ func readDir(scanDir, namePrefix string) (objInfos []ObjectInfo) { // For directories explicitly end with '/'. if isDir { name += "/" - size = 0 // Size is set to '0' for directories explicitly. + // Size is set to '0' for directories explicitly. + size = 0 } if fi.Mode()&os.ModeSymlink == os.ModeSymlink { @@ -150,12 +187,13 @@ func readDir(scanDir, namePrefix string) (objInfos []ObjectInfo) { Size: size, IsDir: isDir, }) + } return } -// ObjectInfoChannel - object info channel +// ObjectInfoChannel - object info channel. type ObjectInfoChannel struct { ch <-chan ObjectInfo objInfo *ObjectInfo @@ -170,7 +208,7 @@ func (oic *ObjectInfoChannel) Read() (ObjectInfo, bool) { } if oic.objInfo == nil { - // first read + // First read. if oi, ok := <-oic.ch; ok { oic.objInfo = &oi } else { @@ -183,7 +221,7 @@ func (oic *ObjectInfoChannel) Read() (ObjectInfo, bool) { status := true oic.objInfo = nil - // read once more to know whether it was last read + // Read once more to know whether it was last read. if oi, ok := <-oic.ch; ok { oic.objInfo = &oi } else { @@ -193,7 +231,7 @@ func (oic *ObjectInfoChannel) Read() (ObjectInfo, bool) { return retObjInfo, status } -// IsClosed - return whether channel is closed or not +// IsClosed - return whether channel is closed or not. func (oic ObjectInfoChannel) IsClosed() bool { if oic.objInfo != nil { return false @@ -202,7 +240,7 @@ func (oic ObjectInfoChannel) IsClosed() bool { return oic.closed } -// IsTimedOut - return whether channel is closed due to timeout +// IsTimedOut - return whether channel is closed due to timeout. func (oic ObjectInfoChannel) IsTimedOut() bool { if oic.timedOut { return true @@ -220,19 +258,19 @@ func (oic ObjectInfoChannel) IsTimedOut() bool { } } -// treeWalk - walk into 'scanDir' recursively when 'recursive' is true. -// It uses 'bucketDir' to get name prefix for object name. -func treeWalk(scanDir, bucketDir string, recursive bool) ObjectInfoChannel { +// TreeWalk - walk into 'scanDir' recursively when 'recursive' is true. +// It uses 'bucketDir' to get name prefix for object name. +func treeWalk(scanDir, bucketDir string, recursive bool, queryPrefix string) ObjectInfoChannel { objectInfoCh := make(chan ObjectInfo, listObjectsLimit) timeoutCh := make(chan struct{}, 1) - // goroutine - retrieves directory entries, makes ObjectInfo and sends into the channel + // goroutine - retrieves directory entries, makes ObjectInfo and sends into the channel. go func() { defer close(objectInfoCh) defer close(timeoutCh) - // send function - returns true if ObjectInfo is sent - // within (time.Second * 15) else false on time-out + // Send function - returns true if ObjectInfo is sent. + // Within (time.Second * 15) else false on time-out. send := func(oi ObjectInfo) bool { timer := time.After(time.Second * 15) select { @@ -246,11 +284,14 @@ func treeWalk(scanDir, bucketDir string, recursive bool) ObjectInfoChannel { namePrefix := strings.Replace(filepath.ToSlash(scanDir), filepath.ToSlash(bucketDir), "", 1) if strings.HasPrefix(namePrefix, "/") { - /* remove beginning "/" */ + // Remove forward slash ("/") from beginning. namePrefix = namePrefix[1:] } - - for objInfos := readDir(scanDir, namePrefix); len(objInfos) > 0; { + // The last argument (isFisrt), is set to `true` only during the first run of the function. + // This makes sure that the sub-directories inside the prefixDir are recursed + // without being asserted for prefix in the object name. + isFirst := true + for objInfos := readDir(scanDir, namePrefix, queryPrefix, isFirst); len(objInfos) > 0; { var objInfo ObjectInfo objInfo, objInfos = objInfos[0], objInfos[1:] if !send(objInfo) { @@ -259,14 +300,15 @@ func treeWalk(scanDir, bucketDir string, recursive bool) ObjectInfoChannel { if objInfo.IsDir && recursive { scanDir := filepath.Join(bucketDir, filepath.FromSlash(objInfo.Name)) - namePrefix = strings.Replace(filepath.ToSlash(scanDir), filepath.ToSlash(bucketDir), "", 1) + if strings.HasPrefix(namePrefix, "/") { /* remove beginning "/" */ namePrefix = namePrefix[1:] } - - objInfos = append(readDir(scanDir, namePrefix), objInfos...) + // The last argument is set to false in the further calls to readdir. + isFirst = false + objInfos = append(readDir(scanDir, namePrefix, queryPrefix, isFirst), objInfos...) } } }() diff --git a/pkg/fs/fs-bucket-listobjects.go b/pkg/fs/fs-bucket-listobjects.go index e770ff46a..20c43109a 100644 --- a/pkg/fs/fs-bucket-listobjects.go +++ b/pkg/fs/fs-bucket-listobjects.go @@ -20,6 +20,7 @@ import ( "fmt" "net/url" "os" + "path" "path/filepath" "strings" @@ -30,6 +31,7 @@ import ( // maxKeys number of objects per call. func (fs Filesystem) ListObjects(bucket, prefix, marker, delimiter string, maxKeys int) (ListObjectsResult, *probe.Error) { result := ListObjectsResult{} + var queryPrefix string // Input validation. if !IsValidBucketName(bucket) { @@ -40,8 +42,10 @@ func (fs Filesystem) ListObjects(bucket, prefix, marker, delimiter string, maxKe if status, err := isDirExist(filepath.Join(fs.path, bucket)); !status { if err == nil { + // File exists, but its not a directory. return result, probe.NewError(BucketNotFound{Bucket: bucket}) } else if os.IsNotExist(err) { + // File does not exist. return result, probe.NewError(BucketNotFound{Bucket: bucket}) } else { return result, probe.NewError(err) @@ -60,8 +64,9 @@ func (fs Filesystem) ListObjects(bucket, prefix, marker, delimiter string, maxKe } if !strings.HasPrefix(marker, prefix) { - return result, probe.NewError(fmt.Errorf("marker '%s' and prefix '%s' do not match", marker, prefix)) + return result, probe.NewError(fmt.Errorf("Invalid combination of marker '%s' and prefix '%s'", marker, prefix)) } + } // Return empty response for a valid request when maxKeys is 0. @@ -85,17 +90,37 @@ func (fs Filesystem) ListObjects(bucket, prefix, marker, delimiter string, maxKe prefixDir := filepath.Dir(filepath.FromSlash(prefix)) rootDir := filepath.Join(bucketDir, prefixDir) + // Maximum 1000 objects returned in a single to listObjects. + // Further calls will set right marker value to continue reading the rest of the objectList. + // popListObjectCh returns nil if the call to ListObject is done for the first time. + // On further calls to ListObjects to retrive more objects within the timeout period, + // popListObjectCh returns the channel from which rest of the objects can be retrieved. objectInfoCh := fs.popListObjectCh(ListObjectParams{bucket, delimiter, marker, prefix}) if objectInfoCh == nil { - ch := treeWalk(rootDir, bucketDir, recursive) + if prefix != "" { + // queryPrefix variable is set to value of the prefix to be searched. + // If prefix contains directory hierarchy queryPrefix is set to empty string, + // this ensure that all objects inside the prefixDir is listed. + // Otherwise the base name is extracted from path.Base and it'll be will be set to Querystring. + // if prefix = /Asia/India/, queryPrefix will be set to empty string(""), so that all objects in prefixDir are listed. + // if prefix = /Asia/India/summerpics , Querystring will be set to "summerpics", + // so those all objects with the prefix "summerpics" inside the /Asia/India/ prefix folder gets listed. + if prefix[len(prefix)-1:] == "/" { + queryPrefix = "" + } else { + queryPrefix = path.Base(prefix) + } + } + ch := treeWalk(rootDir, bucketDir, recursive, queryPrefix) objectInfoCh = &ch } nextMarker := "" for i := 0; i < maxKeys; { + objInfo, ok := objectInfoCh.Read() if !ok { - // closed channel + // Closed channel. return result, nil } @@ -113,18 +138,18 @@ func (fs Filesystem) ListObjects(bucket, prefix, marker, delimiter string, maxKe // Add the bucket. objInfo.Bucket = bucket - - if strings.HasPrefix(objInfo.Name, prefix) { - if objInfo.Name > marker { - if objInfo.IsDir { - result.Prefixes = append(result.Prefixes, objInfo.Name) - } else { - result.Objects = append(result.Objects, objInfo) - } - nextMarker = objInfo.Name - i++ + // In case its not the first call to ListObjects (before timeout), + // The result is already inside the buffered channel. + if objInfo.Name > marker { + if objInfo.IsDir { + result.Prefixes = append(result.Prefixes, objInfo.Name) + } else { + result.Objects = append(result.Objects, objInfo) } + nextMarker = objInfo.Name + i++ } + } if !objectInfoCh.IsClosed() { diff --git a/pkg/fs/fs-bucket-listobjects_test.go b/pkg/fs/fs-bucket-listobjects_test.go new file mode 100644 index 000000000..a1fc837d4 --- /dev/null +++ b/pkg/fs/fs-bucket-listobjects_test.go @@ -0,0 +1,611 @@ +/* + * Minio Cloud Storage, (C) 2015-2016 Minio, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package fs + +import ( + "bytes" + "fmt" + "io/ioutil" + "os" + "path/filepath" + "strconv" + "strings" + "testing" +) + +func TestListObjects(t *testing.T) { + + // Make a temporary directory to use as the filesystem. + directory, e := ioutil.TempDir("", "minio-list-object-test") + if e != nil { + t.Fatal(e) + } + defer os.RemoveAll(directory) + + // Create the filesystem. + fs, err := New(directory, 0) + if err != nil { + t.Fatal(err) + } + // This bucket is used for testing ListObject operations. + err = fs.MakeBucket("test-bucket-list-object") + if err != nil { + t.Fatal(err) + } + // Will not store any objects in this bucket, + // Its to test ListObjects on an empty bucket. + err = fs.MakeBucket("empty-bucket") + if err != nil { + t.Fatal(err) + } + + tmpfile, e := ioutil.TempFile("", "simple-file.txt") + if e != nil { + t.Fatal(e) + } + defer os.Remove(tmpfile.Name()) // clean up + + _, err = fs.CreateObject("test-bucket-list-object", "Asia-maps", "", int64(len("asia-maps")), bytes.NewBufferString("asia-maps"), nil) + if err != nil { + t.Fatal(e) + } + + _, err = fs.CreateObject("test-bucket-list-object", "Asia/India/India-summer-photos-1", "", int64(len("contentstring")), bytes.NewBufferString("contentstring"), nil) + if err != nil { + t.Fatal(e) + } + + _, err = fs.CreateObject("test-bucket-list-object", "Asia/India/Karnataka/Bangalore/Koramangala/pics", "", int64(len("contentstring")), bytes.NewBufferString("contentstring"), nil) + if err != nil { + t.Fatal(e) + } + + for i := 0; i < 2; i++ { + key := "newPrefix" + strconv.Itoa(i) + _, err = fs.CreateObject("test-bucket-list-object", key, "", int64(len(key)), bytes.NewBufferString(key), nil) + if err != nil { + t.Fatal(err) + } + } + _, err = fs.CreateObject("test-bucket-list-object", "newzen/zen/recurse/again/again/again/pics", "", int64(len("recurse")), bytes.NewBufferString("recurse"), nil) + if err != nil { + t.Fatal(e) + } + + for i := 0; i < 3; i++ { + key := "obj" + strconv.Itoa(i) + _, err = fs.CreateObject("test-bucket-list-object", key, "", int64(len(key)), bytes.NewBufferString(key), nil) + if err != nil { + t.Fatal(err) + } + } + + // Formualting the result data set to be expected from ListObjects call inside the tests, + // This will be used in testCases and used for asserting the correctness of ListObjects output in the tests. + + resultCases := []ListObjectsResult{ + // ListObjectsResult-0. + // Testing for listing all objects in the bucket, (testCase 20,21,22). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "Asia-maps"}, + ObjectInfo{Name: "Asia/India/India-summer-photos-1"}, + ObjectInfo{Name: "Asia/India/Karnataka/Bangalore/Koramangala/pics"}, + ObjectInfo{Name: "newPrefix0"}, + ObjectInfo{Name: "newPrefix1"}, + ObjectInfo{Name: "newzen/zen/recurse/again/again/again/pics"}, + ObjectInfo{Name: "obj0"}, + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-1. + // Used for asserting the truncated case, (testCase 23). + ListObjectsResult{ + IsTruncated: true, + Objects: []ObjectInfo{ + ObjectInfo{Name: "Asia-maps"}, + ObjectInfo{Name: "Asia/India/India-summer-photos-1"}, + ObjectInfo{Name: "Asia/India/Karnataka/Bangalore/Koramangala/pics"}, + ObjectInfo{Name: "newPrefix0"}, + ObjectInfo{Name: "newPrefix1"}, + }, + }, + // ListObjectsResult-2. + // (TestCase 24). + ListObjectsResult{ + IsTruncated: true, + Objects: []ObjectInfo{ + ObjectInfo{Name: "Asia-maps"}, + ObjectInfo{Name: "Asia/India/India-summer-photos-1"}, + ObjectInfo{Name: "Asia/India/Karnataka/Bangalore/Koramangala/pics"}, + ObjectInfo{Name: "newPrefix0"}, + }, + }, + // ListObjectsResult-3. + // (TestCase 25). + ListObjectsResult{ + IsTruncated: true, + Objects: []ObjectInfo{ + ObjectInfo{Name: "Asia-maps"}, + ObjectInfo{Name: "Asia/India/India-summer-photos-1"}, + ObjectInfo{Name: "Asia/India/Karnataka/Bangalore/Koramangala/pics"}, + }, + }, + // ListObjectsResult-4. + // Again used for truncated case. + // (TestCase 26). + ListObjectsResult{ + IsTruncated: true, + Objects: []ObjectInfo{ + ObjectInfo{Name: "Asia-maps"}, + }, + }, + // ListObjectsResult-5. + // Used for Asserting prefixes. + // Used for test case with prefix "new", (testCase 27-29). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newPrefix0"}, + ObjectInfo{Name: "newPrefix1"}, + ObjectInfo{Name: "newzen/zen/recurse/again/again/again/pics"}, + }, + }, + // ListObjectsResult-6. + // Used for Asserting prefixes. + // Used for test case with prefix = "obj", (testCase 30). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "obj0"}, + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-7. + // Used for Asserting prefixes and truncation. + // Used for test case with prefix = "new" and maxKeys = 1, (testCase 31). + ListObjectsResult{ + IsTruncated: true, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newPrefix0"}, + }, + }, + // ListObjectsResult-8. + // Used for Asserting prefixes. + // Used for test case with prefix = "obj" and maxKeys = 2, (testCase 32). + ListObjectsResult{ + IsTruncated: true, + Objects: []ObjectInfo{ + ObjectInfo{Name: "obj0"}, + ObjectInfo{Name: "obj1"}, + }, + }, + // ListObjectsResult-9. + // Used for asserting the case with marker, but without prefix. + //marker is set to "newPrefix0" in the testCase, (testCase 33). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newPrefix1"}, + ObjectInfo{Name: "newzen/zen/recurse/again/again/again/pics"}, + ObjectInfo{Name: "obj0"}, + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-10. + //marker is set to "newPrefix1" in the testCase, (testCase 34). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newzen/zen/recurse/again/again/again/pics"}, + ObjectInfo{Name: "obj0"}, + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-11. + //marker is set to "obj0" in the testCase, (testCase 35). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-12. + // Marker is set to "obj1" in the testCase, (testCase 36). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-13. + // Marker is set to "man" in the testCase, (testCase37). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newPrefix0"}, + ObjectInfo{Name: "newPrefix1"}, + ObjectInfo{Name: "newzen/zen/recurse/again/again/again/pics"}, + ObjectInfo{Name: "obj0"}, + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-14. + // Marker is set to "Abc" in the testCase, (testCase 39). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "Asia-maps"}, + ObjectInfo{Name: "Asia/India/India-summer-photos-1"}, + ObjectInfo{Name: "Asia/India/Karnataka/Bangalore/Koramangala/pics"}, + ObjectInfo{Name: "newPrefix0"}, + ObjectInfo{Name: "newPrefix1"}, + ObjectInfo{Name: "newzen/zen/recurse/again/again/again/pics"}, + ObjectInfo{Name: "obj0"}, + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-15. + // Marker is set to "Asia/India/India-summer-photos-1" in the testCase, (testCase 40). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "Asia/India/Karnataka/Bangalore/Koramangala/pics"}, + ObjectInfo{Name: "newPrefix0"}, + ObjectInfo{Name: "newPrefix1"}, + ObjectInfo{Name: "newzen/zen/recurse/again/again/again/pics"}, + ObjectInfo{Name: "obj0"}, + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-16. + // Marker is set to "Asia/India/Karnataka/Bangalore/Koramangala/pics" in the testCase, (testCase 41). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newPrefix0"}, + ObjectInfo{Name: "newPrefix1"}, + ObjectInfo{Name: "newzen/zen/recurse/again/again/again/pics"}, + ObjectInfo{Name: "obj0"}, + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-17. + // Used for asserting the case with marker, without prefix but with truncation. + // Marker = "newPrefix0" & maxKeys = 3 in the testCase, (testCase42). + // Output truncated to 3 values. + ListObjectsResult{ + IsTruncated: true, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newPrefix1"}, + ObjectInfo{Name: "newzen/zen/recurse/again/again/again/pics"}, + ObjectInfo{Name: "obj0"}, + }, + }, + // ListObjectsResult-18. + // Marker = "newPrefix1" & maxkeys = 1 in the testCase, (testCase43). + // Output truncated to 1 value. + ListObjectsResult{ + IsTruncated: true, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newzen/zen/recurse/again/again/again/pics"}, + }, + }, + // ListObjectsResult-19. + // Marker = "obj0" & maxKeys = 1 in the testCase, (testCase44). + // Output truncated to 1 value. + ListObjectsResult{ + IsTruncated: true, + Objects: []ObjectInfo{ + ObjectInfo{Name: "obj1"}, + }, + }, + // ListObjectsResult-20. + // Marker = "obj0" & prefix = "obj" in the testCase, (testCase 45). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-21. + // Marker = "obj1" & prefix = "obj" in the testCase, (testCase 46). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-22. + // Marker = "newPrefix0" & prefix = "new" in the testCase,, (testCase 47). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newPrefix1"}, + ObjectInfo{Name: "newzen/zen/recurse/again/again/again/pics"}, + }, + }, + // ListObjectsResult-23. + // Prefix is set to "Asia/India/" in the testCase, and delimiter is not set (testCase 55). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "Asia/India/India-summer-photos-1"}, + ObjectInfo{Name: "Asia/India/Karnataka/Bangalore/Koramangala/pics"}, + }, + }, + + // ListObjectsResult-24. + // Prefix is set to "Asia" in the testCase, and delimiter is not set (testCase 56). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "Asia-maps"}, + ObjectInfo{Name: "Asia/India/India-summer-photos-1"}, + ObjectInfo{Name: "Asia/India/Karnataka/Bangalore/Koramangala/pics"}, + }, + }, + + // ListObjectsResult-25. + // Prefix is set to "Asia" in the testCase, and delimiter is set (testCase 57). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "Asia-maps"}, + }, + }, + // ListObjectsResult-26. + // prefix = "new" and delimiter is set in the testCase.(testCase 58). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newPrefix0"}, + ObjectInfo{Name: "newPrefix1"}, + }, + }, + // ListObjectsResult-27. + // Prefix is set to "Asia/India/" in the testCase, and delimiter is set to forward slash '/' (testCase 59). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "Asia/India/India-summer-photos-1"}, + }, + }, + // ListObjectsResult-28. + // Marker is set to "Asia/India/India-summer-photos-1" and delimiter set in the testCase, (testCase 60). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newPrefix0"}, + ObjectInfo{Name: "newPrefix1"}, + ObjectInfo{Name: "obj0"}, + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + // ListObjectsResult-29. + // Marker is set to "Asia/India/Karnataka/Bangalore/Koramangala/pics" in the testCase and delimeter set, (testCase 61). + ListObjectsResult{ + IsTruncated: false, + Objects: []ObjectInfo{ + ObjectInfo{Name: "newPrefix0"}, + ObjectInfo{Name: "newPrefix1"}, + ObjectInfo{Name: "obj0"}, + ObjectInfo{Name: "obj1"}, + ObjectInfo{Name: "obj2"}, + }, + }, + } + + testCases := []struct { + // Inputs to ListObjects. + bucketName string + prefix string + marker string + delimeter string + maxKeys int + // Expected output of ListObjects. + result ListObjectsResult + err error + // Flag indicating whether the test is expected to pass or not. + shouldPass bool + }{ + // Test cases with invalid bucket names ( Test number 1-4 ). + {".test", "", "", "", 0, ListObjectsResult{}, BucketNameInvalid{Bucket: ".test"}, false}, + {"Test", "", "", "", 0, ListObjectsResult{}, BucketNameInvalid{Bucket: "Test"}, false}, + {"---", "", "", "", 0, ListObjectsResult{}, BucketNameInvalid{Bucket: "---"}, false}, + {"ad", "", "", "", 0, ListObjectsResult{}, BucketNameInvalid{Bucket: "ad"}, false}, + // Using an existing file for bucket name, but its not a directory (5). + {"simple-file.txt", "", "", "", 0, ListObjectsResult{}, BucketNotFound{Bucket: "simple-file.txt"}, false}, + // Valid bucket names, but they donot exist (6-8). + {"volatile-bucket-1", "", "", "", 0, ListObjectsResult{}, BucketNotFound{Bucket: "volatile-bucket-1"}, false}, + {"volatile-bucket-2", "", "", "", 0, ListObjectsResult{}, BucketNotFound{Bucket: "volatile-bucket-2"}, false}, + {"volatile-bucket-3", "", "", "", 0, ListObjectsResult{}, BucketNotFound{Bucket: "volatile-bucket-3"}, false}, + // Valid, existing bucket, but sending invalid delimeter values (9-10). + // Empty string < "" > and forward slash < / > are the ony two valid arguments for delimeter. + {"test-bucket-list-object", "", "", "*", 0, ListObjectsResult{}, fmt.Errorf("delimiter '%s' is not supported", "*"), false}, + {"test-bucket-list-object", "", "", "-", 0, ListObjectsResult{}, fmt.Errorf("delimiter '%s' is not supported", "-"), false}, + // Marker goes through url QueryUnescape, sending inputs for which QueryUnescape would fail (11-12). + // Here is how QueryUnescape behaves https://golang.org/pkg/net/url/#QueryUnescape. + // QueryUnescape is necessasry since marker is provided as URL query parameter. + {"test-bucket-list-object", "", "test%", "", 0, ListObjectsResult{}, fmt.Errorf("invalid URL escape"), false}, + {"test-bucket-list-object", "", "test%A", "", 0, ListObjectsResult{}, fmt.Errorf("invalid URL escape"), false}, + // Testing for failure cases with both perfix and marker (13). + // The prefix and marker combination to be valid it should satisy strings.HasPrefix(marker, prefix). + {"test-bucket-list-object", "asia", "europe-object", "", 0, ListObjectsResult{}, fmt.Errorf("Invalid combination of marker '%s' and prefix '%s'", "europe-object", "asia"), false}, + // Setting a non-existing directory to be prefix (14-15). + {"empty-bucket", "europe/france/", "", "", 1, ListObjectsResult{}, fmt.Errorf("%s:", filepath.FromSlash("/empty-bucket/europe/france")), false}, + {"empty-bucket", "europe/tunisia/", "", "", 1, ListObjectsResult{}, fmt.Errorf("%s:", filepath.FromSlash("/empty-bucket/europe/tunisia")), false}, + // Testing on empty bucket, that is, bucket without any objects in it (16). + {"empty-bucket", "", "", "", 0, ListObjectsResult{}, nil, true}, + // Setting maxKeys to negative value (17-18). + {"empty-bucket", "", "", "", -1, ListObjectsResult{}, nil, true}, + {"empty-bucket", "", "", "", 1, ListObjectsResult{}, nil, true}, + // Setting maxKeys to a very large value (19). + {"empty-bucket", "", "", "", 1111000000000000, ListObjectsResult{}, nil, true}, + // Testing for all 7 objects in the bucket (20). + {"test-bucket-list-object", "", "", "", 9, resultCases[0], nil, true}, + //Testing for negative value of maxKey, this should set maxKeys to listObjectsLimit (21). + {"test-bucket-list-object", "", "", "", -1, resultCases[0], nil, true}, + // Testing for very large value of maxKey, this should set maxKeys to listObjectsLimit (22). + {"test-bucket-list-object", "", "", "", 1234567891011, resultCases[0], nil, true}, + // Testing for trancated value (23-26). + {"test-bucket-list-object", "", "", "", 5, resultCases[1], nil, true}, + {"test-bucket-list-object", "", "", "", 4, resultCases[2], nil, true}, + {"test-bucket-list-object", "", "", "", 3, resultCases[3], nil, true}, + {"test-bucket-list-object", "", "", "", 1, resultCases[4], nil, true}, + // Testing with prefix (27-30). + {"test-bucket-list-object", "new", "", "", 3, resultCases[5], nil, true}, + {"test-bucket-list-object", "new", "", "", 4, resultCases[5], nil, true}, + {"test-bucket-list-object", "new", "", "", 5, resultCases[5], nil, true}, + {"test-bucket-list-object", "obj", "", "", 3, resultCases[6], nil, true}, + // Testing with prefix and truncation (31-32). + {"test-bucket-list-object", "new", "", "", 1, resultCases[7], nil, true}, + {"test-bucket-list-object", "obj", "", "", 2, resultCases[8], nil, true}, + // Testing with marker, but without prefix and truncation (33-37). + {"test-bucket-list-object", "", "newPrefix0", "", 5, resultCases[9], nil, true}, + {"test-bucket-list-object", "", "newPrefix1", "", 4, resultCases[10], nil, true}, + {"test-bucket-list-object", "", "obj0", "", 2, resultCases[11], nil, true}, + {"test-bucket-list-object", "", "obj1", "", 1, resultCases[12], nil, true}, + {"test-bucket-list-object", "", "man", "", 10, resultCases[13], nil, true}, + // Marker being set to a value which is greater than and all object names when sorted (38). + // Expected to send an empty response in this case. + {"test-bucket-list-object", "", "zen", "", 10, ListObjectsResult{}, nil, true}, + // Marker being set to a value which is lesser than and all object names when sorted (39). + // Expected to send all the objects in the bucket in this case. + {"test-bucket-list-object", "", "Abc", "", 10, resultCases[14], nil, true}, + // Marker is to a hierarhical value (40-41). + {"test-bucket-list-object", "", "Asia/India/India-summer-photos-1", "", 10, resultCases[15], nil, true}, + {"test-bucket-list-object", "", "Asia/India/Karnataka/Bangalore/Koramangala/pics", "", 10, resultCases[16], nil, true}, + // Testing with marker and truncation, but no prefix (42-44). + {"test-bucket-list-object", "", "newPrefix0", "", 3, resultCases[17], nil, true}, + {"test-bucket-list-object", "", "newPrefix1", "", 1, resultCases[18], nil, true}, + {"test-bucket-list-object", "", "obj0", "", 1, resultCases[19], nil, true}, + // Testing with both marker and prefix, but without truncation (45-47). + // The valid combination of marker and prefix should satisfy strings.HasPrefix(marker, prefix). + {"test-bucket-list-object", "obj", "obj0", "", 2, resultCases[20], nil, true}, + {"test-bucket-list-object", "obj", "obj1", "", 1, resultCases[21], nil, true}, + {"test-bucket-list-object", "new", "newPrefix0", "", 2, resultCases[22], nil, true}, + // Testing with maxKeys set to 0 (48-54). + // The parameters have to valid. + {"test-bucket-list-object", "", "obj1", "", 0, ListObjectsResult{}, nil, true}, + {"test-bucket-list-object", "", "obj0", "", 0, ListObjectsResult{}, nil, true}, + {"test-bucket-list-object", "new", "", "", 0, ListObjectsResult{}, nil, true}, + {"test-bucket-list-object", "obj", "", "", 0, ListObjectsResult{}, nil, true}, + {"test-bucket-list-object", "obj", "obj0", "", 0, ListObjectsResult{}, nil, true}, + {"test-bucket-list-object", "obj", "obj1", "", 0, ListObjectsResult{}, nil, true}, + {"test-bucket-list-object", "new", "newPrefix0", "", 0, ListObjectsResult{}, nil, true}, + // Tests on hierarchical key names as prefix. + // Without delimteter the code should recurse into the prefix Dir. + // Tests with prefix, but without delimiter (55-56). + {"test-bucket-list-object", "Asia/India/", "", "", 10, resultCases[23], nil, true}, + {"test-bucket-list-object", "Asia", "", "", 10, resultCases[24], nil, true}, + // Tests with prefix and delimiter (57-59). + // With delimeter the code shouldnot recurse into the sub-directories of prefix Dir. + {"test-bucket-list-object", "Asia", "", "/", 10, resultCases[25], nil, true}, + {"test-bucket-list-object", "new", "", "/", 10, resultCases[26], nil, true}, + {"test-bucket-list-object", "Asia/India/", "", "/", 10, resultCases[27], nil, true}, + // Test with marker set as hierarhical value and with delimiter. (60-61) + {"test-bucket-list-object", "", "Asia/India/India-summer-photos-1", "/", 10, resultCases[28], nil, true}, + {"test-bucket-list-object", "", "Asia/India/Karnataka/Bangalore/Koramangala/pics", "/", 10, resultCases[29], nil, true}, + } + + for i, testCase := range testCases { + result, err := fs.ListObjects(testCase.bucketName, testCase.prefix, testCase.marker, testCase.delimeter, testCase.maxKeys) + if err != nil && testCase.shouldPass { + t.Errorf("Test %d: Expected to pass, but failed with: %s", i+1, err.Cause.Error()) + } + if err == nil && !testCase.shouldPass { + t.Errorf("Test %d: Expected to fail with \"%s\", but passed instead", i+1, testCase.err.Error()) + } + // Failed as expected, but does it fail for the expected reason. + if err != nil && !testCase.shouldPass { + if !strings.Contains(err.Cause.Error(), testCase.err.Error()) { + t.Errorf("Test %d: Expected to fail with error \"%s\", but instead failed with error \"%s\" instead", i+1, testCase.err.Error(), err.Cause.Error()) + } + } + // Since there are cases for which ListObjects fails, this is necessary. + // Test passes as expected, but the output values are verified for correctness here. + if err == nil && testCase.shouldPass { + // The length of the expected ListObjectsResult.Objects should match in both expected result from test cases and in the output. + // On failure calling t.Fatalf, otherwise it may lead to index out of range error in assertion following this. + if len(testCase.result.Objects) != len(result.Objects) { + t.Fatalf("Test %d: Expected number of object in the result to be '%d', but found '%d' objects instead", i+1, len(testCase.result.Objects), len(result.Objects)) + } + for j := 0; j < len(testCase.result.Objects); j++ { + if testCase.result.Objects[j].Name != result.Objects[j].Name { + t.Errorf("Test %d: Expected object name to be \"%s\", but found \"%s\" instead", i+1, testCase.result.Objects[j].Name, result.Objects[j].Name) + } + } + if testCase.result.IsTruncated != result.IsTruncated { + t.Errorf("Test %d: Expected IsTruncated flag to be %v, but instead found it to be %v", i+1, testCase.result.IsTruncated, result.IsTruncated) + } + + } + } + +} + +func BenchmarkListObjects(b *testing.B) { + // Make a temporary directory to use as the filesystem. + directory, e := ioutil.TempDir("", "minio-list-benchmark") + if e != nil { + b.Fatal(e) + } + defer os.RemoveAll(directory) + + // Create the filesystem. + filesystem, err := New(directory, 0) + if err != nil { + b.Fatal(err) + } + + // Create a bucket. + err = filesystem.MakeBucket("ls-benchmark-bucket") + if err != nil { + b.Fatal(err) + } + + for i := 0; i < 20000; i++ { + key := "obj" + strconv.Itoa(i) + _, err = filesystem.CreateObject("ls-benchmark-bucket", key, "", int64(len(key)), bytes.NewBufferString(key), nil) + if err != nil { + b.Fatal(err) + } + } + + b.ResetTimer() + + // List the buckets over and over and over. + for i := 0; i < b.N; i++ { + _, err = filesystem.ListObjects("ls-benchmark-bucket", "", "obj9000", "", -1) + if err != nil { + b.Fatal(err) + } + } +} diff --git a/pkg/fs/fs-bucket_test.go b/pkg/fs/fs-bucket_test.go index 5e0ed3d22..e0f5ac287 100644 --- a/pkg/fs/fs-bucket_test.go +++ b/pkg/fs/fs-bucket_test.go @@ -69,7 +69,7 @@ func TestGetBucketInfo(t *testing.T) { {"meta-test-bucket.3", BucketInfo{Name: "meta-test-bucket.3"}, nil, true}, } for i, testCase := range testCases { - //the err returned is of type *probe.Error + // The err returned is of type *probe.Error. bucketInfo, err := filesystem.GetBucketInfo(testCase.bucketName) if err != nil && testCase.shouldPass {