From a5d0bef4e2941fd70f176158dff05799b19fcf04 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Mon, 7 Mar 2016 19:02:36 -0800 Subject: [PATCH 1/2] pkg/fs: test, bench, and drop unnecessary check in ListBuckets There is now a simple test and a benchmark for ListBuckets. I also dropped an unnecessary check that was simply repeated from above, guaranteed to be true. --- pkg/fs/fs-bucket.go | 2 +- pkg/fs/fs-bucket_test.go | 74 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/pkg/fs/fs-bucket.go b/pkg/fs/fs-bucket.go index 1a598e82a..0b8545bb2 100644 --- a/pkg/fs/fs-bucket.go +++ b/pkg/fs/fs-bucket.go @@ -78,7 +78,7 @@ func (fs Filesystem) ListBuckets() ([]BucketMetadata, *probe.Error) { } // If directories are found with odd names, skip them. dirName := strings.ToLower(file.Name()) - if file.IsDir() && !IsValidBucketName(dirName) { + if !IsValidBucketName(dirName) { continue } metadata := BucketMetadata{ diff --git a/pkg/fs/fs-bucket_test.go b/pkg/fs/fs-bucket_test.go index c50ac1eb6..7701f2faf 100644 --- a/pkg/fs/fs-bucket_test.go +++ b/pkg/fs/fs-bucket_test.go @@ -19,10 +19,51 @@ package fs import ( "io/ioutil" "os" + "strconv" "strings" "testing" ) +func TestListBuckets(t *testing.T) { + // Make a temporary directory to use as the filesystem. + directory, fserr := ioutil.TempDir("", "minio-benchmark") + if fserr != nil { + t.Fatal(fserr) + } + defer os.RemoveAll(directory) + + // Create the filesystem. + filesystem, err := New(directory, 0) + if err != nil { + t.Fatal(err) + } + + // Create a few buckets. + for i := 0; i < 10; i++ { + err = filesystem.MakeBucket("testbucket."+strconv.Itoa(i), "public-read") + if err != nil { + t.Fatal(err) + } + } + + // List, and ensure that they are all there. + metadatas, err := filesystem.ListBuckets() + if err != nil { + t.Fatal(err) + } + + if len(metadatas) != 10 { + t.Errorf("incorrect length of metadatas (%i)\n", len(metadatas)) + } + + // Iterate over the buckets, ensuring that the name is correct. + for i := 0; i < len(metadatas); i++ { + if !strings.Contains(metadatas[i].Name, "testbucket") { + t.Fail() + } + } +} + func TestDeleteBucket(t *testing.T) { // Make a temporary directory to use as the filesystem. directory, fserr := ioutil.TempDir("", "minio-benchmark") @@ -44,6 +85,39 @@ func TestDeleteBucket(t *testing.T) { } } +func BenchmarkListBuckets(b *testing.B) { + // Make a temporary directory to use as the filesystem. + directory, fserr := ioutil.TempDir("", "minio-benchmark") + if fserr != nil { + b.Fatal(fserr) + } + defer os.RemoveAll(directory) + + // Create the filesystem. + filesystem, err := New(directory, 0) + if err != nil { + b.Fatal(err) + } + + // Create a few buckets. + for i := 0; i < 20; i++ { + err = filesystem.MakeBucket("bucket."+strconv.Itoa(i), "public-read") + 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.ListBuckets() + if err != nil { + b.Fatal(err) + } + } +} + func BenchmarkDeleteBucket(b *testing.B) { // Make a temporary directory to use as the filesystem. directory, fserr := ioutil.TempDir("", "minio-benchmark") From cd3eb63c4ae13d5a5601cb69cb1cb5cae606a43c Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Mon, 7 Mar 2016 19:30:30 -0800 Subject: [PATCH 2/2] pkg/fs: test, document, and fix IsValid{Bucket,Object}Name This commit improves the docs for both functions (more Go-like) and drops an unnecessary condition in IsValidBucketName. This also drops a condition in IsValidObjectName where "" (empty string) was a valid object name. This has been fixed and will no longer return true. This commit also adds tests for both functions, including a regression test for the bug fix. --- pkg/fs/fs-utils.go | 18 +++++------ pkg/fs/fs-utils_test.go | 72 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 10 deletions(-) create mode 100644 pkg/fs/fs-utils_test.go diff --git a/pkg/fs/fs-utils.go b/pkg/fs/fs-utils.go index 309431592..6490e1551 100644 --- a/pkg/fs/fs-utils.go +++ b/pkg/fs/fs-utils.go @@ -24,12 +24,11 @@ import ( // validBucket regexp. var validBucket = regexp.MustCompile(`^[a-z0-9][a-z0-9\.\-]{1,61}[a-z0-9]$`) -// IsValidBucketName - verify bucket name in accordance with -// - http://docs.aws.amazon.com/AmazonS3/latest/dev/UsingBucket.html +// IsValidBucketName verifies a bucket name in accordance with Amazon's +// requirements. It must be 3-63 characters long, can contain dashes +// and periods, but must begin and end with a lowercase letter or a number. +// See: http://docs.aws.amazon.com/AmazonS3/latest/dev/BucketRestrictions.html func IsValidBucketName(bucket string) bool { - if bucket == "" { - return false - } if len(bucket) < 3 || len(bucket) > 63 { return false } @@ -39,12 +38,11 @@ func IsValidBucketName(bucket string) bool { return validBucket.MatchString(bucket) } -// IsValidObjectName - verify object name in accordance with -// - http://docs.aws.amazon.com/AmazonS3/latest/dev/UsingMetadata.html +// IsValidObjectName verifies an object name in accordance with Amazon's +// requirements. It cannot exceed 1024 characters and must be a valid UTF8 +// string. +// See: http://docs.aws.amazon.com/AmazonS3/latest/dev/UsingMetadata.html func IsValidObjectName(object string) bool { - if object == "" { - return true - } if len(object) > 1024 || len(object) == 0 { return false } diff --git a/pkg/fs/fs-utils_test.go b/pkg/fs/fs-utils_test.go new file mode 100644 index 000000000..c1eb1d61c --- /dev/null +++ b/pkg/fs/fs-utils_test.go @@ -0,0 +1,72 @@ +/* + * Minio Cloud Storage, (C) 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 ( + "testing" +) + +func ensureBucketName(name string, t *testing.T, pass bool) { + if pass && IsValidBucketName(name) { + return + } + if !pass && !IsValidBucketName(name) { + return + } + + t.Errorf("\"%s\" should have passed [%t]\n", name, pass) +} + +func TestIsValidBucketName(t *testing.T) { + ensureBucketName("lol", t, true) + ensureBucketName("s3-eu-west-1.amazonaws.com", t, true) + ensureBucketName("ideas-are-more-powerful-than-guns", t, true) + ensureBucketName("testbucket", t, true) + ensureBucketName("1bucket", t, true) + ensureBucketName("bucket1", t, true) + + ensureBucketName("testing.", t, false) + ensureBucketName("", t, false) + ensureBucketName("......", t, false) + ensureBucketName("THIS-IS-UPPERCASE", t, false) + ensureBucketName("una ñina", t, false) + ensureBucketName("lalalallalallalalalallalallalallalallalallalallalallalallallalala", t, false) +} + +func ensureObjectName(name string, t *testing.T, pass bool) { + if pass && IsValidObjectName(name) { + return + } + if !pass && !IsValidObjectName(name) { + return + } + + t.Errorf("\"%s\" should have passed [%t]\n", name, pass) +} + +func TestIsValidObjectName(t *testing.T) { + ensureObjectName("object", t, true) + ensureObjectName("The Shining Script .pdf", t, true) + ensureObjectName("Cost Benefit Analysis (2009-2010).pptx", t, true) + ensureObjectName("117Gn8rfHL2ACARPAhaFd0AGzic9pUbIA/5OCn5A", t, true) + ensureObjectName("SHØRT", t, true) + ensureObjectName("There are far too many object names, and far too few bucket names!", t, true) + + ensureObjectName("", t, false) + // Bad UTF8 strings should not pass. + ensureObjectName(string([]byte{0xff, 0xfe, 0xfd}), t, false) +}