[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