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

@@ -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})
 }

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
 }