2769: code review comments

* DeleteHandler returns http.StatusNotFound when no blocks could be found at all,
  and http.StatusMethodNotAllowed when all delete attempts
  fail (e.g. read-only volumes)

* Unit test cleanup: "http://localhost:25107" is not actually necessary
  to build a http.Request, and it is confusing, so leave it out.

Refs #2769.

diff --git a/services/keep/src/keep/handler_test.go b/services/keep/src/keep/handler_test.go
index 2998f60427298f019f21871352f7db26a380fedf..030e3338f6a7ade8ee4f23c2bc5221ca333588a7 100644 (file)
--- a/services/keep/src/keep/handler_test.go
+++ b/services/keep/src/keep/handler_test.go
@@ -44,7 +44,7 @@ func TestGetHandler(t *testing.T) {
 
        // Prepare two test Keep volumes. Our block is stored on the second volume.
        KeepVM = MakeTestVolumeManager(2)
-       defer func() { KeepVM.Quit() }()
+       defer KeepVM.Quit()
 
        vols := KeepVM.Volumes()
        if err := vols[0].Put(TEST_HASH, TEST_BLOCK); err != nil {
@@ -61,11 +61,11 @@ func TestGetHandler(t *testing.T) {
        permission_ttl = time.Duration(300) * time.Second
 
        var (
-               unsigned_locator  = "http://localhost:25107/" + TEST_HASH
+               unsigned_locator  = "/" + TEST_HASH
                valid_timestamp   = time.Now().Add(permission_ttl)
                expired_timestamp = time.Now().Add(-time.Hour)
-               signed_locator    = "http://localhost:25107/" + SignLocator(TEST_HASH, known_token, valid_timestamp)
-               expired_locator   = "http://localhost:25107/" + SignLocator(TEST_HASH, known_token, expired_timestamp)
+               signed_locator    = "/" + SignLocator(TEST_HASH, known_token, valid_timestamp)
+               expired_locator   = "/" + SignLocator(TEST_HASH, known_token, expired_timestamp)
        )
 
        // -----------------
@@ -153,7 +153,7 @@ func TestPutHandler(t *testing.T) {
 
        // Prepare two test Keep volumes.
        KeepVM = MakeTestVolumeManager(2)
-       defer func() { KeepVM.Quit() }()
+       defer KeepVM.Quit()
 
        // Set up a REST router for testing the handlers.
        rest := MakeRESTRouter()
@@ -163,7 +163,7 @@ func TestPutHandler(t *testing.T) {
 
        // Unauthenticated request, no server key
        // => OK (unsigned response)
-       unsigned_locator := "http://localhost:25107/" + TEST_HASH
+       unsigned_locator := "/" + TEST_HASH
        response := IssueRequest(rest,
                &RequestTester{
                        method:       "PUT",
@@ -247,7 +247,7 @@ func TestIndexHandler(t *testing.T) {
        // Include multiple blocks on different volumes, and
        // some metadata files (which should be omitted from index listings)
        KeepVM = MakeTestVolumeManager(2)
-       defer func() { KeepVM.Quit() }()
+       defer KeepVM.Quit()
 
        vols := KeepVM.Volumes()
        vols[0].Put(TEST_HASH, TEST_BLOCK)
@@ -262,30 +262,30 @@ func TestIndexHandler(t *testing.T) {
 
        unauthenticated_req := &RequestTester{
                method: "GET",
-               uri:    "http://localhost:25107/index",
+               uri:    "/index",
        }
        authenticated_req := &RequestTester{
                method:    "GET",
-               uri:       "http://localhost:25107/index",
+               uri:       "/index",
                api_token: known_token,
        }
        superuser_req := &RequestTester{
                method:    "GET",
-               uri:       "http://localhost:25107/index",
+               uri:       "/index",
                api_token: data_manager_token,
        }
        unauth_prefix_req := &RequestTester{
                method: "GET",
-               uri:    "http://localhost:25107/index/" + TEST_HASH[0:3],
+               uri:    "/index/" + TEST_HASH[0:3],
        }
        auth_prefix_req := &RequestTester{
                method:    "GET",
-               uri:       "http://localhost:25107/index/" + TEST_HASH[0:3],
+               uri:       "/index/" + TEST_HASH[0:3],
                api_token: known_token,
        }
        superuser_prefix_req := &RequestTester{
                method:    "GET",
-               uri:       "http://localhost:25107/index/" + TEST_HASH[0:3],
+               uri:       "/index/" + TEST_HASH[0:3],
                api_token: data_manager_token,
        }
 
@@ -415,7 +415,7 @@ func TestIndexHandler(t *testing.T) {
 
 // TestDeleteHandler
 //
-// Cases to test:
+// Cases tested:
 //
 //   With no token and with a non-data-manager token:
 //   * Delete existing block
@@ -446,7 +446,7 @@ func TestDeleteHandler(t *testing.T) {
        // Include multiple blocks on different volumes, and
        // some metadata files (which should be omitted from index listings)
        KeepVM = MakeTestVolumeManager(2)
-       defer func() { KeepVM.Quit() }()
+       defer KeepVM.Quit()
 
        vols := KeepVM.Volumes()
        vols[0].Put(TEST_HASH, TEST_BLOCK)
@@ -459,24 +459,24 @@ func TestDeleteHandler(t *testing.T) {
 
        unauth_req := &RequestTester{
                method: "DELETE",
-               uri:    "http://localhost:25107/" + TEST_HASH,
+               uri:    "/" + TEST_HASH,
        }
 
        user_req := &RequestTester{
                method:    "DELETE",
-               uri:       "http://localhost:25107/" + TEST_HASH,
+               uri:       "/" + TEST_HASH,
                api_token: user_token,
        }
 
        superuser_existing_block_req := &RequestTester{
                method:    "DELETE",
-               uri:       "http://localhost:25107/" + TEST_HASH,
+               uri:       "/" + TEST_HASH,
                api_token: data_manager_token,
        }
 
        superuser_nonexistent_block_req := &RequestTester{
                method:    "DELETE",
-               uri:       "http://localhost:25107/" + TEST_HASH_2,
+               uri:       "/" + TEST_HASH_2,
                api_token: data_manager_token,
        }
 
@@ -500,17 +500,19 @@ func TestDeleteHandler(t *testing.T) {
                Deleted int `json:"copies_deleted"`
                Failed  int `json:"copies_failed"`
        }
-       var dc deletecounter
+       var dc, expected_dc deletecounter
 
        response = IssueRequest(rest, superuser_nonexistent_block_req)
        ExpectStatusCode(t,
                "data manager request, nonexistent block",
-               http.StatusOK,
+               http.StatusNotFound,
                response)
        // Expect response {"copies_deleted":0,"copies_failed":0}
+       expected_dc = deletecounter{0, 0}
        json.NewDecoder(response.Body).Decode(&dc)
-       if dc.Deleted != 0 || dc.Failed != 0 {
-               t.Errorf("superuser_nonexistent_block_req: response = %+v", dc)
+       if dc != expected_dc {
+               t.Errorf("superuser_nonexistent_block_req\nexpected: %+v\nreceived: %+v",
+                       expected_dc, dc)
        }
 
        // Authenticated admin request for existing block.
@@ -520,9 +522,11 @@ func TestDeleteHandler(t *testing.T) {
                http.StatusOK,
                response)
        // Expect response {"copies_deleted":1,"copies_failed":0}
+       expected_dc = deletecounter{1, 0}
        json.NewDecoder(response.Body).Decode(&dc)
-       if dc.Deleted != 1 || dc.Failed != 0 {
-               t.Errorf("superuser_existing_block_req: response = %+v", dc)
+       if dc != expected_dc {
+               t.Errorf("superuser_existing_block_req\nexpected: %+v\nreceived: %+v",
+                       expected_dc, dc)
        }
        // Confirm the block has been deleted
        _, err := vols[0].Get(TEST_HASH)

diff --git a/services/keep/src/keep/handlers.go b/services/keep/src/keep/handlers.go
index b9b8caebfb81d3a7bd13b349f2883f065b52b9c8..d5a2a3591712d7ab4df3e084c93764665be7f0ee 100644 (file)
--- a/services/keep/src/keep/handlers.go
+++ b/services/keep/src/keep/handlers.go
@@ -326,14 +326,29 @@ func GetVolumeStatus(volume string) *VolumeStatus {
 // authenticated or is issued by a non-admin user, the server returns
 // a PermissionError.
 //
-// Upon completion, DELETE returns HTTP 200 OK with a JSON message body
-// in the format
+// Upon receiving a valid request from an authorized user,
+// DeleteHandler deletes all copies of the specified block on local
+// volumes.
+//
+// Response format:
+//
+// The response body consists of the JSON message
 //
 //    {"copies_deleted":d,"copies_failed":f}
 //
 // where d and f are integers representing the number of blocks that
 // were successfully and unsuccessfully deleted.
 //
+//   * If any blocks were successfully deleted (copies_deleted > 0), the
+//     HTTP response code is 200 OK.
+//
+//   * If no blocks were found at all (copies_deleted == copies_failed
+//     == 0), the response code is 404 Not Found.
+//
+//   * If blocks were found but none could be deleted (copies_deleted
+//     == 0 and copies_failed > 0), the response code is 405 Method Not
+//     Allowed.
+//
 func DeleteHandler(resp http.ResponseWriter, req *http.Request) {
        hash := mux.Vars(req)["hash"]
        log.Printf("%s %s", req.Method, hash)
@@ -364,6 +379,15 @@ func DeleteHandler(resp http.ResponseWriter, req *http.Request) {
        }
 
        if j, err := json.Marshal(result); err == nil {
+               if result.Deleted == 0 && result.Failed == 0 {
+                       // If no blocks were found, HTTP 404
+                       resp.WriteHeader(http.StatusNotFound)
+               } else if result.Deleted == 0 && result.Failed > 0 {
+                       // If all delete attempts failed, HTTP 405
+                       resp.WriteHeader(http.StatusMethodNotAllowed)
+               } else {
+                       resp.WriteHeader(http.StatusOK)
+               }
                resp.Write(j)
        } else {
                log.Printf("json.Marshal: %s\n", err)