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

@@ -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: <ERROR> %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)
+			}
+		}
+	}
+}

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 {

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)

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).
+		// Testing for all 10 objects in the bucket (18).
-		{"test-bucket-list-object", "", "", "", 9, resultCases[0], nil, true},
+		{"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", "", "newPrefix0", "", 6, resultCases[9], nil, true},
-		{"test-bucket-list-object", "", "newPrefix1", "", 4, resultCases[10], nil, true},
+		{"test-bucket-list-object", "", "newPrefix1", "", 5, resultCases[10], nil, true},
-		{"test-bucket-list-object", "", "obj0", "", 2, resultCases[11], nil, true},
+		{"test-bucket-list-object", "", "obj0", "", 4, resultCases[11], nil, true},
-		{"test-bucket-list-object", "", "obj1", "", 1, resultCases[12], nil, true},
+		{"test-bucket-list-object", "", "obj1", "", 2, resultCases[12], nil, true},
-		{"test-bucket-list-object", "", "man", "", 10, resultCases[13], 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 == "" {
+				//FIXME: we should always check for ETag
-					t.Errorf("Test %d: %s: Expected ETag to be not empty, but found empty instead", i+1, instanceType)
+				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)
 				}
 
 			}

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}
 	}