From b48e2de0c63b2de27116ada5bca14c2378f4c7b3 Mon Sep 17 00:00:00 2001 From: mpl Date: Mon, 11 Jun 2018 17:29:23 +0200 Subject: [PATCH] pkg/blobserver: print RemoveBlobs errors Both on server and client sides. So far we did on neither. Fixes #1186 Change-Id: I7ff3484d82329b4f83a5f3fa8f150ac7ad7f732c --- pkg/blobserver/handlers/remove.go | 30 ++++++++++++++------------- pkg/client/remove.go | 34 ++++++++++++++++--------------- 2 files changed, 34 insertions(+), 30 deletions(-) diff --git a/pkg/blobserver/handlers/remove.go b/pkg/blobserver/handlers/remove.go index f8fdefe3d..2923a074d 100644 --- a/pkg/blobserver/handlers/remove.go +++ b/pkg/blobserver/handlers/remove.go @@ -17,7 +17,6 @@ limitations under the License. package handlers import ( - "context" "fmt" "log" "net/http" @@ -38,23 +37,26 @@ func CreateRemoveHandler(storage blobserver.Storage) http.Handler { // RemoveResponse is the JSON response to a remove request. type RemoveResponse struct { Removed []blob.Ref `json:"removed"` // Refs of the removed blobs. + Error string `json:"error"` } -func handleRemove(rw http.ResponseWriter, req *http.Request, storage blobserver.Storage) { - ctx := context.TODO() - if req.Method != "POST" { +func handleRemove(w http.ResponseWriter, r *http.Request, storage blobserver.Storage) { + ctx := r.Context() + if r.Method != "POST" { log.Fatalf("Invalid method; handlers misconfigured") } configer, ok := storage.(blobserver.Configer) if !ok { - rw.WriteHeader(http.StatusForbidden) - fmt.Fprintf(rw, "Remove handler's blobserver.Storage isn't a blobserver.Configer; can't remove") + msg := fmt.Sprintf("remove handler's blobserver.Storage isn't a blobserver.Configer, but a %T; can't remove", storage) + log.Printf("blobserver/handlers: %v", msg) + httputil.ReturnJSONCode(w, http.StatusForbidden, RemoveResponse{Error: msg}) return } if !configer.Config().Deletable { - rw.WriteHeader(http.StatusForbidden) - fmt.Fprintf(rw, "storage does not permit deletes.\n") + msg := fmt.Sprintf("storage %T does not permit deletes", storage) + log.Printf("blobserver/handlers: %v", msg) + httputil.ReturnJSONCode(w, http.StatusForbidden, RemoveResponse{Error: msg}) return } @@ -63,18 +65,18 @@ func handleRemove(rw http.ResponseWriter, req *http.Request, storage blobserver. for { n++ if n > maxRemovesPerRequest { - httputil.BadRequestError(rw, + httputil.BadRequestError(w, fmt.Sprintf("Too many removes in this request; max is %d", maxRemovesPerRequest)) return } key := fmt.Sprintf("blob%v", n) - value := req.FormValue(key) + value := r.FormValue(key) if value == "" { break } ref, ok := blob.Parse(value) if !ok { - httputil.BadRequestError(rw, "Bogus blobref for key "+key) + httputil.BadRequestError(w, "Bogus blobref for key "+key) return } toRemove = append(toRemove, ref) @@ -82,11 +84,11 @@ func handleRemove(rw http.ResponseWriter, req *http.Request, storage blobserver. err := storage.RemoveBlobs(ctx, toRemove) if err != nil { - rw.WriteHeader(http.StatusInternalServerError) + w.WriteHeader(http.StatusInternalServerError) log.Printf("Server error during remove: %v", err) - fmt.Fprintf(rw, "Server error") + fmt.Fprintf(w, "Server error") return } - httputil.ReturnJSON(rw, &RemoveResponse{Removed: toRemove}) + httputil.ReturnJSON(w, &RemoveResponse{Removed: toRemove}) } diff --git a/pkg/client/remove.go b/pkg/client/remove.go index 68fb97d3f..187e38931 100644 --- a/pkg/client/remove.go +++ b/pkg/client/remove.go @@ -26,12 +26,9 @@ import ( "perkeep.org/internal/httputil" "perkeep.org/pkg/blob" + "perkeep.org/pkg/blobserver/handlers" ) -type removeResponse struct { - Removed []string `json:"removed"` -} - // RemoveBlobs removes the list of blobs. An error is returned if the // server failed to remove a blob. Removing a non-existent blob isn't // an error. @@ -44,15 +41,15 @@ func (c *Client) RemoveBlobs(ctx context.Context, blobs []blob.Ref) error { return err } url_ := fmt.Sprintf("%s/camli/remove", pfx) - params := make(url.Values) // "blobN" -> BlobRefStr - needsDelete := make(map[string]bool) // BlobRefStr -> true + params := make(url.Values) // "blobN" -> BlobRefStr + needsDelete := make(map[blob.Ref]bool) // BlobRefStr -> true for n, b := range blobs { if !b.Valid() { return errors.New("Cannot delete invalid blobref") } key := fmt.Sprintf("blob%v", n+1) params.Add(key, b.String()) - needsDelete[b.String()] = true + needsDelete[b] = true } req, err := http.NewRequest("POST", url_, strings.NewReader(params.Encode())) @@ -67,19 +64,24 @@ func (c *Client) RemoveBlobs(ctx context.Context, blobs []blob.Ref) error { resp.Body.Close() return fmt.Errorf("Got status code %d from blobserver for remove %s", resp.StatusCode, params.Encode()) } + var remResp handlers.RemoveResponse + decodeErr := httputil.DecodeJSON(resp, &remResp) if resp.StatusCode != 200 { resp.Body.Close() - return fmt.Errorf("Invalid http response %d in remove response", resp.StatusCode) + if decodeErr == nil { + return fmt.Errorf("invalid http response %d in remove response: %v", resp.StatusCode, remResp.Error) + } else { + return fmt.Errorf("invalid http response %d in remove response", resp.StatusCode) + } } - var remResp removeResponse - if err := httputil.DecodeJSON(resp, &remResp); err != nil { - return fmt.Errorf("Failed to parse remove response: %v", err) + if decodeErr != nil { + return fmt.Errorf("failed to parse remove response: %v", err) } - for _, value := range remResp.Removed { - delete(needsDelete, value) + for _, br := range remResp.Removed { + delete(needsDelete, br) } if len(needsDelete) > 0 { - return fmt.Errorf("Failed to remove blobs %s", strings.Join(stringKeys(needsDelete), ", ")) + return fmt.Errorf("failed to remove blobs %s", strings.Join(stringKeys(needsDelete), ", ")) } return nil } @@ -90,10 +92,10 @@ func (c *Client) RemoveBlob(ctx context.Context, b blob.Ref) error { return c.RemoveBlobs(ctx, []blob.Ref{b}) } -func stringKeys(m map[string]bool) (s []string) { +func stringKeys(m map[blob.Ref]bool) (s []string) { s = make([]string, 0, len(m)) for key := range m { - s = append(s, key) + s = append(s, key.String()) } return }