2769: more code review comments

* Abandon HTTP 405, return HTTP 200 whenever any blocks were found at
  all

* Return a JSON message only with HTTP 200 (404 implies
  copies_deleted=0, copies_failed=0)

* More clarity in unit tests

Refs #2769

diff --git a/services/keep/src/keep/handler_test.go b/services/keep/src/keep/handler_test.go
index 030e3338f6a7ade8ee4f23c2bc5221ca333588a7..df8a0ba056c83576705595fa0df16c7b720ed729 100644 (file)
--- a/services/keep/src/keep/handler_test.go
+++ b/services/keep/src/keep/handler_test.go
@@ -500,20 +500,13 @@ func TestDeleteHandler(t *testing.T) {
                Deleted int `json:"copies_deleted"`
                Failed  int `json:"copies_failed"`
        }
-       var dc, expected_dc deletecounter
+       var response_dc, expected_dc deletecounter
 
        response = IssueRequest(rest, superuser_nonexistent_block_req)
        ExpectStatusCode(t,
                "data manager request, nonexistent block",
                http.StatusNotFound,
                response)
-       // Expect response {"copies_deleted":0,"copies_failed":0}
-       expected_dc = deletecounter{0, 0}
-       json.NewDecoder(response.Body).Decode(&dc)
-       if dc != expected_dc {
-               t.Errorf("superuser_nonexistent_block_req\nexpected: %+v\nreceived: %+v",
-                       expected_dc, dc)
-       }
 
        // Authenticated admin request for existing block.
        response = IssueRequest(rest, superuser_existing_block_req)
@@ -523,14 +516,15 @@ func TestDeleteHandler(t *testing.T) {
                response)
        // Expect response {"copies_deleted":1,"copies_failed":0}
        expected_dc = deletecounter{1, 0}
-       json.NewDecoder(response.Body).Decode(&dc)
-       if dc != expected_dc {
+       json.NewDecoder(response.Body).Decode(&response_dc)
+       if response_dc != expected_dc {
                t.Errorf("superuser_existing_block_req\nexpected: %+v\nreceived: %+v",
-                       expected_dc, dc)
+                       expected_dc, response_dc)
        }
        // Confirm the block has been deleted
        _, err := vols[0].Get(TEST_HASH)
-       if !os.IsNotExist(err) {
+       var block_deleted = os.IsNotExist(err)
+       if !block_deleted {
                t.Error("superuser_existing_block_req: block not deleted")
        }
 }

diff --git a/services/keep/src/keep/handlers.go b/services/keep/src/keep/handlers.go
index d5a2a3591712d7ab4df3e084c93764665be7f0ee..dc0f51771ac2b6464fd0c4b76ff8adc81ea351e0 100644 (file)
--- a/services/keep/src/keep/handlers.go
+++ b/services/keep/src/keep/handlers.go
@@ -328,27 +328,21 @@ func GetVolumeStatus(volume string) *VolumeStatus {
 //
 // Upon receiving a valid request from an authorized user,
 // DeleteHandler deletes all copies of the specified block on local
-// volumes.
+// writable volumes.
 //
 // Response format:
 //
-// The response body consists of the JSON message
+// If the requested blocks was not found on any volume, the response
+// code is HTTP 404 Not Found.
+//
+// Otherwise, the response code is 200 OK, with a response body
+// consisting 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)
@@ -378,21 +372,23 @@ 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)
+       var st int
+
+       if result.Deleted == 0 && result.Failed == 0 {
+               st = http.StatusNotFound
+       } else {
+               st = http.StatusOK
+       }
+
+       resp.WriteHeader(st)
+
+       if st == http.StatusOK {
+               if body, err := json.Marshal(result); err == nil {
+                       resp.Write(body)
                } else {
-                       resp.WriteHeader(http.StatusOK)
+                       log.Printf("json.Marshal: %s (result = %v)\n", err, result)
+                       http.Error(resp, err.Error(), 500)
                }
-               resp.Write(j)
-       } else {
-               log.Printf("json.Marshal: %s\n", err)
-               log.Printf("result = %v\n", result)
-               http.Error(resp, err.Error(), 500)
        }
 }