From 57c5c756116bc6423c73f7b24808a3c9272d6c3a Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Wed, 26 Apr 2017 23:27:48 -0700 Subject: [PATCH] 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 | 34 +++++++++++++++++ cmd/object-handlers.go | 35 +++--------------- cmd/web-handlers.go | 70 +++++++++++++++++------------------ cmd/web-handlers_test.go | 36 +++++++++++++----- 4 files changed, 100 insertions(+), 75 deletions(-) diff --git a/cmd/object-handlers-common.go b/cmd/object-handlers-common.go index 1ed15b80d..01f2ba2ae 100644 --- a/cmd/object-handlers-common.go +++ b/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 +} diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 850556ffb..108a6139e 100644 --- a/cmd/object-handlers.go +++ b/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, - }) } diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index 7f6e1d870..eff9f3b27 100644 --- a/cmd/web-handlers.go +++ b/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) { - // If not a directory, remove the object. - err = remove(object) - if err != nil { - break objectLoop +next: + for _, objectName := range args.Objects { + // If not a directory, remove the object. + 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 diff --git a/cmd/web-handlers_test.go b/cmd/web-handlers_test.go index 72e172dc2..2520d20c5 100644 --- a/cmd/web-handlers_test.go +++ b/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: %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: %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: %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