From 1961f2ef54781f07b56faa31f229c8918c1be29e Mon Sep 17 00:00:00 2001 From: Anis Elleuch Date: Thu, 6 Sep 2018 01:38:03 +0200 Subject: [PATCH] xl: Fix removing an empty directory (#6421) Removing an empty directory is not working because of xl.DeleteObject() was only checking if the passed prefix is an actual object but it should also check if it is an empty directory. --- cmd/object-api-deleteobject_test.go | 120 +++++++++++++++++++++++++++ cmd/object-api-getobject_test.go | 30 ++++--- cmd/object-api-getobjectinfo_test.go | 11 +++ cmd/object-api-listobjects_test.go | 29 +++++-- cmd/xl-v1-object.go | 2 +- 5 files changed, 172 insertions(+), 20 deletions(-) create mode 100644 cmd/object-api-deleteobject_test.go diff --git a/cmd/object-api-deleteobject_test.go b/cmd/object-api-deleteobject_test.go new file mode 100644 index 000000000..7992e2ed3 --- /dev/null +++ b/cmd/object-api-deleteobject_test.go @@ -0,0 +1,120 @@ +/* + * Minio Cloud Storage, (C) 2018 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 cmd + +import ( + "bytes" + "context" + "crypto/md5" + "encoding/hex" + "testing" +) + +// Wrapper for calling DeleteObject tests for both XL multiple disks and single node setup. +func TestDeleteObject(t *testing.T) { + ExecObjectLayerTest(t, testDeleteObject) +} + +// Unit test for DeleteObject in general. +func testDeleteObject(obj ObjectLayer, instanceType string, t TestErrHandler) { + + type objectUpload struct { + name string + content string + } + + testCases := []struct { + bucketName string + objectToUploads []objectUpload + pathToDelete string + objectsAfterDelete []string + }{ + // Test 1: removes an object and checks it is the only object + // that has been deleted. + { + "bucket1", + []objectUpload{{"object0", "content"}, {"object1", "content"}}, + "object0", + []string{"object1"}, + }, + // Test 2: remove an object inside a directory and checks it is deleted + // with its parent since this former becomes empty + { + "bucket2", + []objectUpload{{"object0", "content"}, {"dir/object1", "content"}}, + "dir/object1", + []string{"object0"}, + }, + // Test 3: remove an object inside a directory and checks if it is deleted + // but other sibling object in the same directory still exists + { + "bucket3", + []objectUpload{{"dir/object1", "content"}, {"dir/object2", "content"}}, + "dir/object1", + []string{"dir/object2"}, + }, + // Test 4: remove a non empty directory and checks it has no effect + { + "bucket4", + []objectUpload{{"object0", "content"}, {"dir/object1", "content"}}, + "dir/", + []string{"dir/object1", "object0"}, + }, + // Test 4: Remove an empty directory and checks it is really removed + { + "bucket5", + []objectUpload{{"object0", "content"}, {"dir/", ""}}, + "dir/", + []string{"object0"}, + }, + } + + for i, testCase := range testCases { + + err := obj.MakeBucketWithLocation(context.Background(), testCase.bucketName, "") + if err != nil { + t.Fatalf("%s : %s", instanceType, err.Error()) + } + + for _, object := range testCase.objectToUploads { + md5Bytes := md5.Sum([]byte(object.content)) + _, err = obj.PutObject(context.Background(), testCase.bucketName, object.name, mustGetHashReader(t, bytes.NewBufferString(object.content), + int64(len(object.content)), hex.EncodeToString(md5Bytes[:]), ""), nil) + if err != nil { + t.Fatalf("%s : %s", instanceType, err.Error()) + } + } + + // TODO: check the error in the future + _ = obj.DeleteObject(context.Background(), testCase.bucketName, testCase.pathToDelete) + + result, err := obj.ListObjects(context.Background(), testCase.bucketName, "", "", "", 1000) + if err != nil { + t.Errorf("Test %d: %s: Expected to pass, but failed with: %s", i+1, instanceType, err.Error()) + } + + if len(result.Objects) != len(testCase.objectsAfterDelete) { + t.Errorf("Test %d: %s: mismatch number of objects after delete, expected = %d, found = %d", i+1, instanceType, len(testCase.objectsAfterDelete), len(result.Objects)) + } + + for idx := range result.Objects { + if result.Objects[idx].Name != testCase.objectsAfterDelete[idx] { + t.Errorf("Test %d: %s: Unexpected object found after delete, found = `%v`", i+1, instanceType, result.Objects[idx].Name) + } + } + } +} diff --git a/cmd/object-api-getobject_test.go b/cmd/object-api-getobject_test.go index f3fc819cb..6434f1392 100644 --- a/cmd/object-api-getobject_test.go +++ b/cmd/object-api-getobject_test.go @@ -39,6 +39,8 @@ func testGetObject(obj ObjectLayer, instanceType string, t TestErrHandler) { // Setup for the tests. bucketName := getRandomBucketName() objectName := "test-object" + emptyDirName := "test-empty-dir/" + // create bucket. err := obj.MakeBucketWithLocation(context.Background(), bucketName, "") // Stop the test if creation of the bucket fails. @@ -53,7 +55,10 @@ func testGetObject(obj ObjectLayer, instanceType string, t TestErrHandler) { bytesData := []struct { byteData []byte }{ + // Regular data {generateBytesData(6 * humanize.MiByte)}, + // Empty data for empty directory + {}, } // set of inputs for uploading the objects before tests for downloading is done. putObjectInputs := []struct { @@ -65,6 +70,7 @@ func testGetObject(obj ObjectLayer, instanceType string, t TestErrHandler) { }{ // case - 1. {bucketName, objectName, int64(len(bytesData[0].byteData)), bytesData[0].byteData, make(map[string]string)}, + {bucketName, emptyDirName, int64(len(bytesData[1].byteData)), bytesData[1].byteData, make(map[string]string)}, } // iterate through the above set of inputs and upkoad the object. for i, input := range putObjectInputs { @@ -79,6 +85,7 @@ func testGetObject(obj ObjectLayer, instanceType string, t TestErrHandler) { buffers := []*bytes.Buffer{ new(bytes.Buffer), new(bytes.Buffer), + new(bytes.Buffer), } // test cases with set of inputs @@ -107,41 +114,44 @@ func testGetObject(obj ObjectLayer, instanceType string, t TestErrHandler) { // Test case - 5. // Case with invalid object names. {bucketName, "", 0, 0, nil, nil, false, []byte(""), fmt.Errorf("%s", "Object name invalid: "+bucketName+"#")}, - // Test case - 7. + // Test case - 6. {bucketName, objectName, 0, int64(len(bytesData[0].byteData)), buffers[0], NewEOFWriter(buffers[0], 100), false, []byte{}, io.EOF}, // Test case with start offset set to 0 and length set to size of the object. // Fetching the entire object. - // Test case - 8. + // Test case - 7. {bucketName, objectName, 0, int64(len(bytesData[0].byteData)), buffers[1], buffers[1], true, bytesData[0].byteData, nil}, // Test case with `length` parameter set to a negative value. - // Test case - 9. + // Test case - 8. {bucketName, objectName, 0, int64(-1), buffers[1], buffers[1], true, bytesData[0].byteData, nil}, // Test case with content-range 1 to objectSize . - // Test case - 10. + // Test case - 9. {bucketName, objectName, 1, int64(len(bytesData[0].byteData) - 1), buffers[1], buffers[1], true, bytesData[0].byteData[1:], nil}, // Test case with content-range 100 to objectSize - 100. - // Test case - 11. + // Test case - 10. {bucketName, objectName, 100, int64(len(bytesData[0].byteData) - 200), buffers[1], buffers[1], true, bytesData[0].byteData[100 : len(bytesData[0].byteData)-100], nil}, // Test case with offset greater than the size of the object - // Test case - 12. + // Test case - 11. {bucketName, objectName, int64(len(bytesData[0].byteData) + 1), int64(len(bytesData[0].byteData)), buffers[0], NewEOFWriter(buffers[0], 100), false, []byte{}, InvalidRange{int64(len(bytesData[0].byteData) + 1), int64(len(bytesData[0].byteData)), int64(len(bytesData[0].byteData))}}, // Test case with offset greater than the size of the object. - // Test case - 13. + // Test case - 12. {bucketName, objectName, -1, int64(len(bytesData[0].byteData)), buffers[0], new(bytes.Buffer), false, []byte{}, errUnexpected}, // Test case length parameter is more than the object size. - // Test case - 14. + // Test case - 13. {bucketName, objectName, 0, int64(len(bytesData[0].byteData) + 1), buffers[1], buffers[1], false, bytesData[0].byteData, InvalidRange{0, int64(len(bytesData[0].byteData) + 1), int64(len(bytesData[0].byteData))}}, // Test case with offset + length > objectSize parameter set to a negative value. - // Test case - 15. + // Test case - 14. {bucketName, objectName, 2, int64(len(bytesData[0].byteData)), buffers[1], buffers[1], false, bytesData[0].byteData, InvalidRange{2, int64(len(bytesData[0].byteData)), int64(len(bytesData[0].byteData))}}, // Test case with the writer set to nil. - // Test case - 16. + // Test case - 15. {bucketName, objectName, 0, int64(len(bytesData[0].byteData)), buffers[1], nil, false, bytesData[0].byteData, errUnexpected}, + // Test case - 16. + // Test case when it is an empty directory + {bucketName, emptyDirName, 0, int64(len(bytesData[1].byteData)), buffers[2], buffers[2], true, bytesData[1].byteData, nil}, } for i, testCase := range testCases { diff --git a/cmd/object-api-getobjectinfo_test.go b/cmd/object-api-getobjectinfo_test.go index 01080fb6e..69b0695c8 100644 --- a/cmd/object-api-getobjectinfo_test.go +++ b/cmd/object-api-getobjectinfo_test.go @@ -34,14 +34,24 @@ func testGetObjectInfo(obj ObjectLayer, instanceType string, t TestErrHandler) { if err != nil { t.Fatalf("%s : %s", instanceType, err.Error()) } + + // Put a regular object _, err = obj.PutObject(context.Background(), "test-getobjectinfo", "Asia/asiapics.jpg", mustGetHashReader(t, bytes.NewBufferString("asiapics"), int64(len("asiapics")), "", ""), nil) if err != nil { t.Fatalf("%s : %s", instanceType, err.Error()) } + + // Put an empty directory + _, err = obj.PutObject(context.Background(), "test-getobjectinfo", "Asia/empty-dir/", mustGetHashReader(t, bytes.NewBufferString(""), int64(len("")), "", ""), nil) + if err != nil { + t.Fatalf("%s : %s", instanceType, err.Error()) + } + resultCases := []ObjectInfo{ // ObjectInfo -1. // ObjectName set to a existing object in the test case (Test case 14). {Bucket: "test-getobjectinfo", Name: "Asia/asiapics.jpg", ContentType: "image/jpeg", IsDir: false}, + {Bucket: "test-getobjectinfo", Name: "Asia/empty-dir/", ContentType: "application/octet-stream", IsDir: true}, } testCases := []struct { bucketName string @@ -70,6 +80,7 @@ func testGetObjectInfo(obj ObjectLayer, instanceType string, t TestErrHandler) { {"test-getobjectinfo", "Asia/myfile", ObjectInfo{}, ObjectNotFound{Bucket: "test-getobjectinfo", Object: "Asia/myfile"}, false}, // Valid case with existing object (Test number 12). {"test-getobjectinfo", "Asia/asiapics.jpg", resultCases[0], nil, true}, + {"test-getobjectinfo", "Asia/empty-dir/", resultCases[1], nil, true}, } for i, testCase := range testCases { result, err := obj.GetObjectInfo(context.Background(), testCase.bucketName, testCase.objectName) diff --git a/cmd/object-api-listobjects_test.go b/cmd/object-api-listobjects_test.go index bcd035b66..78c8ffb85 100644 --- a/cmd/object-api-listobjects_test.go +++ b/cmd/object-api-listobjects_test.go @@ -66,6 +66,7 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {"obj0", "obj0", nil}, {"obj1", "obj1", nil}, {"obj2", "obj2", nil}, + {"z-empty-dir/", "", nil}, } for _, object := range testObjects { md5Bytes := md5.Sum([]byte(object.content)) @@ -95,6 +96,7 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, + {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-1. @@ -191,6 +193,7 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, + {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-10. @@ -202,6 +205,7 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, + {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-11. @@ -211,6 +215,7 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { Objects: []ObjectInfo{ {Name: "obj1"}, {Name: "obj2"}, + {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-12. @@ -219,6 +224,7 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { IsTruncated: false, Objects: []ObjectInfo{ {Name: "obj2"}, + {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-13. @@ -232,6 +238,7 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, + {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-14. @@ -248,6 +255,7 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, + {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-15. @@ -262,6 +270,7 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, + {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-16. @@ -275,6 +284,7 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, + {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-17. @@ -452,8 +462,8 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {"empty-bucket", "", "", "", 1, ListObjectsInfo{}, nil, true}, // Setting maxKeys to a very large value (17). {"empty-bucket", "", "", "", 111100000, ListObjectsInfo{}, nil, true}, - // Testing for all 7 objects in the bucket (18). - {"test-bucket-list-object", "", "", "", 9, resultCases[0], nil, true}, + // Testing for all 10 objects in the bucket (18). + {"test-bucket-list-object", "", "", "", 10, resultCases[0], nil, true}, //Testing for negative value of maxKey, this should set maxKeys to listObjectsLimit (19). {"test-bucket-list-object", "", "", "", -1, resultCases[0], nil, true}, // Testing for very large value of maxKey, this should set maxKeys to listObjectsLimit (20). @@ -472,11 +482,11 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {"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 (31-35). - {"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}, + {"test-bucket-list-object", "", "newPrefix0", "", 6, resultCases[9], nil, true}, + {"test-bucket-list-object", "", "newPrefix1", "", 5, resultCases[10], nil, true}, + {"test-bucket-list-object", "", "obj0", "", 4, resultCases[11], nil, true}, + {"test-bucket-list-object", "", "obj1", "", 2, resultCases[12], nil, true}, + {"test-bucket-list-object", "", "man", "", 11, resultCases[13], nil, true}, // Marker being set to a value which is greater than and all object names when sorted (36). // Expected to send an empty response in this case. {"test-bucket-list-object", "", "zen", "", 10, ListObjectsInfo{}, nil, true}, @@ -554,8 +564,9 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { if testCase.result.Objects[j].Name != result.Objects[j].Name { t.Errorf("Test %d: %s: Expected object name to be \"%s\", but found \"%s\" instead", i+1, instanceType, testCase.result.Objects[j].Name, result.Objects[j].Name) } - if result.Objects[j].ETag == "" { - t.Errorf("Test %d: %s: Expected ETag to be not empty, but found empty instead", i+1, instanceType) + //FIXME: we should always check for ETag + if result.Objects[j].ETag == "" && !strings.HasSuffix(result.Objects[j].Name, slashSeparator) { + t.Errorf("Test %d: %s: Expected ETag to be not empty, but found empty instead (%v)", i+1, instanceType, result.Objects[j].Name) } } diff --git a/cmd/xl-v1-object.go b/cmd/xl-v1-object.go index c67ea3760..df0e9fbd2 100644 --- a/cmd/xl-v1-object.go +++ b/cmd/xl-v1-object.go @@ -843,7 +843,7 @@ func (xl xlObjects) DeleteObject(ctx context.Context, bucket, object string) (er return err } - if !xl.isObject(bucket, object) { + if !xl.isObject(bucket, object) && !xl.isObjectDir(bucket, object) { return ObjectNotFound{bucket, object} }