[ARVADOS] updated: fe5556e0f3105f2d8b752d796fea21363a1b0eb6

git at public.curoverse.com git at public.curoverse.com
Fri Aug 1 17:05:49 EDT 2014


Summary of changes:
 services/keep/src/keep/handler_test.go | 54 ++++++++++++++++++----------------
 services/keep/src/keep/handlers.go     | 28 ++++++++++++++++--
 2 files changed, 55 insertions(+), 27 deletions(-)

       via  fe5556e0f3105f2d8b752d796fea21363a1b0eb6 (commit)
      from  fb460bdf4b45dd9d8b46951a1457a301bd565430 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit fe5556e0f3105f2d8b752d796fea21363a1b0eb6
Author: Tim Pierce <twp at curoverse.com>
Date:   Fri Aug 1 17:03:28 2014 -0400

    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 2998f60..030e333 100644
--- 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 b9b8cae..d5a2a35 100644
--- 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)

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list