web: Simplify and converge common functions in web/obj API. (#4179)

RemoveObject() in webAPI currently re-implements some part
of the code to remove objects combine them for simplicity
and code convergence.

cmd/object-handlers-common.go

@@ -17,6 +17,7 @@
 package cmd
 
 import (
+	"net"
 	"net/http"
 	"strings"
 	"time"
@@ -224,3 +225,36 @@ func canonicalizeETag(etag string) string {
 func isETagEqual(left, right string) bool {
 	return canonicalizeETag(left) == canonicalizeETag(right)
 }
+
+// deleteObject is a convenient wrapper to delete an object, this
+// is a common function to be called from object handlers and
+// web handlers.
+func deleteObject(obj ObjectLayer, bucket, object string, r *http.Request) (err error) {
+	// Acquire a write lock before deleting the object.
+	objectLock := globalNSMutex.NewNSLock(bucket, object)
+	objectLock.Lock()
+	defer objectLock.Unlock()
+
+	// Proceed to delete the object.
+	if err = obj.DeleteObject(bucket, object); err != nil {
+		return err
+	}
+
+	// Get host and port from Request.RemoteAddr.
+	host, port, _ := net.SplitHostPort(r.RemoteAddr)
+
+	// Notify object deleted event.
+	eventNotify(eventData{
+		Type:   ObjectRemovedDelete,
+		Bucket: bucket,
+		ObjInfo: ObjectInfo{
+			Name: object,
+		},
+		ReqParams: extractReqParams(r),
+		UserAgent: r.UserAgent(),
+		Host:      host,
+		Port:      port,
+	})
+
+	return nil
+}

cmd/object-handlers.go

@@ -1016,35 +1016,12 @@ func (api objectAPIHandlers) DeleteObjectHandler(w http.ResponseWriter, r *http.
 		return
 	}
 
-	objectLock := globalNSMutex.NewNSLock(bucket, object)
-	objectLock.Lock()
-	defer objectLock.Unlock()
-
-	/// http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectDELETE.html
-	/// Ignore delete object errors, since we are suppposed to reply
-	/// only 204.
-	if err := objectAPI.DeleteObject(bucket, object); err != nil {
-		writeSuccessNoContent(w)
-		return
+	// http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectDELETE.html
+	// Ignore delete object errors while replying to client, since we are
+	// suppposed to reply only 204. Additionally log the error for
+	// investigation.
+	if err := deleteObject(objectAPI, bucket, object, r); err != nil {
+		errorIf(err, "Unable to delete an object %s", pathJoin(bucket, object))
 	}
 	writeSuccessNoContent(w)
-
-	// Get host and port from Request.RemoteAddr.
-	host, port, err := net.SplitHostPort(r.RemoteAddr)
-	if err != nil {
-		host, port = "", ""
-	}
-
-	// Notify object deleted event.
-	eventNotify(eventData{
-		Type:   ObjectRemovedDelete,
-		Bucket: bucket,
-		ObjInfo: ObjectInfo{
-			Name: object,
-		},
-		ReqParams: extractReqParams(r),
-		UserAgent: r.UserAgent(),
-		Host:      host,
-		Port:      port,
-	})
 }

cmd/web-handlers.go

@@ -255,16 +255,22 @@ func (web *webAPIHandlers) ListObjects(r *http.Request, args *ListObjectsArgs, r
 	return nil
 }
 
-// RemoveObjectArgs - args to remove an object
-// JSON will look like:
-// '{"bucketname":"testbucket","objects":["photos/hawaii/","photos/maldives/","photos/sanjose.jpg"]}'
+// RemoveObjectArgs - args to remove an object, JSON will look like.
+//
+// {
+//     "bucketname": "testbucket",
+//     "objects": [
+//         "photos/hawaii/",
+//         "photos/maldives/",
+//         "photos/sanjose.jpg"
+//     ]
+// }
 type RemoveObjectArgs struct {
-	Objects    []string `json:"objects"`    // can be files or sub-directories
-	Prefix     string   `json:"prefix"`     // current directory in the browser-ui
-	BucketName string   `json:"bucketname"` // bucket name.
+	Objects    []string `json:"objects"`    // Contains objects, prefixes.
+	BucketName string   `json:"bucketname"` // Contains bucket name.
 }
 
-// RemoveObject - removes an object.
+// RemoveObject - removes an object, or all the objects at a given prefix.
 func (web *webAPIHandlers) RemoveObject(r *http.Request, args *RemoveObjectArgs, reply *WebGenericRep) error {
 	objectAPI := web.ObjectAPI()
 	if objectAPI == nil {
@@ -273,51 +279,35 @@ func (web *webAPIHandlers) RemoveObject(r *http.Request, args *RemoveObjectArgs,
 	if !isHTTPRequestValid(r) {
 		return toJSONError(errAuthentication)
 	}
+
 	if args.BucketName == "" || len(args.Objects) == 0 {
-		return toJSONError(errUnexpected)
+		return toJSONError(errInvalidArgument)
 	}
+
 	var err error
-objectLoop:
-	for _, object := range args.Objects {
-		remove := func(objectName string) error {
-			objectLock := globalNSMutex.NewNSLock(args.BucketName, objectName)
-			objectLock.Lock()
-			defer objectLock.Unlock()
-			err = objectAPI.DeleteObject(args.BucketName, objectName)
-			if err == nil {
-				// Notify object deleted event.
-				eventNotify(eventData{
-					Type:   ObjectRemovedDelete,
-					Bucket: args.BucketName,
-					ObjInfo: ObjectInfo{
-						Name: objectName,
-					},
-					ReqParams: extractReqParams(r),
-				})
-			}
-			return err
-		}
-		if !hasSuffix(object, slashSeparator) {
+next:
+	for _, objectName := range args.Objects {
 		// If not a directory, remove the object.
-			err = remove(object)
-			if err != nil {
-				break objectLoop
+		if !hasSuffix(objectName, slashSeparator) && objectName != "" {
+			if err = deleteObject(objectAPI, args.BucketName, objectName, r); err != nil {
+				break next
 			}
 			continue
 		}
+
 		// For directories, list the contents recursively and remove.
 		marker := ""
 		for {
 			var lo ListObjectsInfo
-			lo, err = objectAPI.ListObjects(args.BucketName, object, marker, "", 1000)
+			lo, err = objectAPI.ListObjects(args.BucketName, objectName, marker, "", 1000)
 			if err != nil {
-				break objectLoop
+				break next
 			}
 			marker = lo.NextMarker
 			for _, obj := range lo.Objects {
-				err = remove(obj.Name)
+				err = deleteObject(objectAPI, args.BucketName, obj.Name, r)
 				if err != nil {
-					break objectLoop
+					break next
 				}
 			}
 			if !lo.IsTruncated {
@@ -972,6 +962,12 @@ func toWebAPIError(err error) APIError {
 			HTTPStatusCode: http.StatusForbidden,
 			Description:    err.Error(),
 		}
+	} else if err == errInvalidArgument {
+		return APIError{
+			Code:           "InvalidArgument",
+			HTTPStatusCode: http.StatusBadRequest,
+			Description:    err.Error(),
+		}
 	}
 	// Convert error type to api error code.
 	var apiErrCode APIErrorCode

cmd/web-handlers_test.go

@@ -439,7 +439,7 @@ func TestWebHandlerRemoveObject(t *testing.T) {
 	ExecObjectLayerTest(t, testRemoveObjectWebHandler)
 }
 
-// testRemoveObjectWebHandler - Test RemoveObject web handler
+// testRemoveObjectWebHandler - Test RemoveObjectObject web handler
 func testRemoveObjectWebHandler(obj ObjectLayer, instanceType string, t TestErrHandler) {
 	// Register the API end points with XL/FS object layer.
 	apiRouter := initTestWebRPCEndPoint(obj)
@@ -477,9 +477,9 @@ func testRemoveObjectWebHandler(obj ObjectLayer, instanceType string, t TestErrH
 		t.Fatalf("Was not able to upload an object, %v", err)
 	}
 
-	removeObjectRequest := RemoveObjectArgs{BucketName: bucketName, Objects: []string{"a/", "object"}}
-	removeObjectReply := &WebGenericRep{}
-	req, err := newTestWebRPCRequest("Web.RemoveObject", authorization, removeObjectRequest)
+	removeRequest := RemoveObjectArgs{BucketName: bucketName, Objects: []string{"a/", "object"}}
+	removeReply := &WebGenericRep{}
+	req, err := newTestWebRPCRequest("Web.RemoveObject", authorization, removeRequest)
 	if err != nil {
 		t.Fatalf("Failed to create HTTP request: <ERROR> %v", err)
 	}
@@ -487,14 +487,14 @@ func testRemoveObjectWebHandler(obj ObjectLayer, instanceType string, t TestErrH
 	if rec.Code != http.StatusOK {
 		t.Fatalf("Expected the response status to be 200, but instead found `%d`", rec.Code)
 	}
-	err = getTestWebRPCResponse(rec, &removeObjectReply)
+	err = getTestWebRPCResponse(rec, &removeReply)
 	if err != nil {
 		t.Fatalf("Failed, %v", err)
 	}
 
-	removeObjectRequest = RemoveObjectArgs{BucketName: bucketName, Objects: []string{"a/", "object"}}
-	removeObjectReply = &WebGenericRep{}
-	req, err = newTestWebRPCRequest("Web.RemoveObject", authorization, removeObjectRequest)
+	removeRequest = RemoveObjectArgs{BucketName: bucketName, Objects: []string{"a/", "object"}}
+	removeReply = &WebGenericRep{}
+	req, err = newTestWebRPCRequest("Web.RemoveObject", authorization, removeRequest)
 	if err != nil {
 		t.Fatalf("Failed to create HTTP request: <ERROR> %v", err)
 	}
@@ -502,10 +502,28 @@ func testRemoveObjectWebHandler(obj ObjectLayer, instanceType string, t TestErrH
 	if rec.Code != http.StatusOK {
 		t.Fatalf("Expected the response status to be 200, but instead found `%d`", rec.Code)
 	}
-	err = getTestWebRPCResponse(rec, &removeObjectReply)
+	err = getTestWebRPCResponse(rec, &removeReply)
 	if err != nil {
 		t.Fatalf("Failed, %v", err)
 	}
+
+	removeRequest = RemoveObjectArgs{BucketName: bucketName}
+	removeReply = &WebGenericRep{}
+	req, err = newTestWebRPCRequest("Web.RemoveObject", authorization, removeRequest)
+	if err != nil {
+		t.Fatalf("Failed to create HTTP request: <ERROR> %v", err)
+	}
+	apiRouter.ServeHTTP(rec, req)
+	if rec.Code != http.StatusOK {
+		t.Fatalf("Expected the response status to be 200, but instead found `%d`", rec.Code)
+	}
+	b, err := ioutil.ReadAll(rec.Body)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !bytes.Contains(b, []byte("Invalid arguments specified")) {
+		t.Fatalf("Expected response wrong %s", string(b))
+	}
 }
 
 // Wrapper for calling Generate Auth Handler