Fix #854: 404 on deleting a pin that isn't part of pinset (#854)

With this commit
- If cid in `DELETE /pins/{cid}` isn't part of the pinset, it would
return 404
- If path in `DELETE /pins/{keyType}/{path}` resolves to a cid that
isn't part of the pinset, it would return 404

api/rest/restapi.go

@@ -22,6 +22,7 @@ import (
 
 	"github.com/ipfs/ipfs-cluster/adder/adderutils"
 	types "github.com/ipfs/ipfs-cluster/api"
+	"github.com/ipfs/ipfs-cluster/state"
 
 	cid "github.com/ipfs/go-cid"
 	logging "github.com/ipfs/go-log"
@@ -694,7 +695,7 @@ func (api *API) pinHandler(w http.ResponseWriter, r *http.Request) {
 			pin,
 			&pinObj,
 		)
-		api.sendResponse(w, http.StatusOK, err, pinObj)
+		api.sendResponse(w, autoStatus, err, pinObj)
 		logger.Debug("rest api pinHandler done")
 	}
 }
@@ -712,7 +713,11 @@ func (api *API) unpinHandler(w http.ResponseWriter, r *http.Request) {
 			pin,
 			&pinObj,
 		)
-		api.sendResponse(w, http.StatusOK, err, pinObj)
+		if err != nil && err.Error() == state.ErrNotFound.Error() {
+			api.sendResponse(w, http.StatusNotFound, err, nil)
+			return
+		}
+		api.sendResponse(w, autoStatus, err, pinObj)
 		logger.Debug("rest api unpinHandler done")
 	}
 }
@@ -730,7 +735,7 @@ func (api *API) pinPathHandler(w http.ResponseWriter, r *http.Request) {
 			&pin,
 		)
 
-		api.sendResponse(w, http.StatusOK, err, pin)
+		api.sendResponse(w, autoStatus, err, pin)
 		logger.Debug("rest api pinPathHandler done")
 	}
 }
@@ -747,7 +752,11 @@ func (api *API) unpinPathHandler(w http.ResponseWriter, r *http.Request) {
 			pinpath,
 			&pin,
 		)
-		api.sendResponse(w, http.StatusOK, err, pin)
+		if err != nil && err.Error() == state.ErrNotFound.Error() {
+			api.sendResponse(w, http.StatusNotFound, err, nil)
+			return
+		}
+		api.sendResponse(w, autoStatus, err, pin)
 		logger.Debug("rest api unpinPathHandler done")
 	}
 }

api/rest/restapi_test.go

@@ -631,7 +631,14 @@ var pathTestCases = []pathCase{
 		http.StatusBadRequest,
 		"",
 	},
-	// TODO: Test StatusNotFound and a case with trailing slash with paths
+	{
+		"/ipfs/bafyreiay3jpjk74dkckv2r74eyvf3lfnxujefay2rtuluintasq2zlapv4",
+		testPinOpts,
+		true,
+		http.StatusNotFound,
+		"",
+	},
+	// TODO: A case with trailing slash with paths
 	// test.PathIPNS2, test.PathIPLD2, test.InvalidPath1
 }
 
@@ -641,7 +648,7 @@ func TestAPIPinEndpointWithPath(t *testing.T) {
 	defer rest.Shutdown(ctx)
 
 	tf := func(t *testing.T, url urlF) {
-		for _, testCase := range pathTestCases {
+		for _, testCase := range pathTestCases[:3] {
 			c, _ := cid.Decode(testCase.expectedCid)
 			resultantPin := api.PinWithOpts(
 				c,
@@ -689,9 +696,14 @@ func TestAPIUnpinEndpoint(t *testing.T) {
 			t.Error("expected different error: ", errResp.Message)
 		}
 
+		makeDelete(t, rest, url(rest)+"/pins/"+test.NotFoundCid.String(), &errResp)
+		if errResp.Code != http.StatusNotFound {
+			t.Error("expected different error code: ", errResp.Code)
+		}
+
 		makeDelete(t, rest, url(rest)+"/pins/abcd", &errResp)
 		if errResp.Code != 400 {
-			t.Error("should fail with bad Cid")
+			t.Error("expected different error code: ", errResp.Code)
 		}
 	}
 

test/cids.go

@@ -17,12 +17,15 @@ var (
 	// ErrorCid is meant to be used as a Cid which causes errors. i.e. the
 	// ipfs mock fails when pinning this CID.
 	ErrorCid, _ = cid.Decode("QmP63DkAFEnDYNjDYBpyNDfttu1fvUw99x1brscPzpqmmc")
-	PeerID1, _  = peer.IDB58Decode("QmXZrtE5jQwXNqCJMfHUTQkvhQ4ZAnqMnmzFMJfLewuabc")
-	PeerID2, _  = peer.IDB58Decode("QmUZ13osndQ5uL4tPWHXe3iBgBgq9gfewcBMSCAuMBsDJ6")
-	PeerID3, _  = peer.IDB58Decode("QmPGDFvBkgWhvzEK9qaTWrWurSwqXNmhnK3hgELPdZZNPa")
-	PeerID4, _  = peer.IDB58Decode("QmZ8naDy5mEz4GLuQwjWt9MPYqHTBbsm8tQBrNSjiq6zBc")
-	PeerID5, _  = peer.IDB58Decode("QmZVAo3wd8s5eTTy2kPYs34J9PvfxpKPuYsePPYGjgRRjg")
-	PeerID6, _  = peer.IDB58Decode("QmR8Vu6kZk7JvAN2rWVWgiduHatgBq2bb15Yyq8RRhYSbx")
+	// NotFoundCid is meant to be used as a CID that doesn't exist in the
+	// pinset.
+	NotFoundCid, _ = cid.Decode("bafyreiay3jpjk74dkckv2r74eyvf3lfnxujefay2rtuluintasq2zlapv4")
+	PeerID1, _     = peer.IDB58Decode("QmXZrtE5jQwXNqCJMfHUTQkvhQ4ZAnqMnmzFMJfLewuabc")
+	PeerID2, _     = peer.IDB58Decode("QmUZ13osndQ5uL4tPWHXe3iBgBgq9gfewcBMSCAuMBsDJ6")
+	PeerID3, _     = peer.IDB58Decode("QmPGDFvBkgWhvzEK9qaTWrWurSwqXNmhnK3hgELPdZZNPa")
+	PeerID4, _     = peer.IDB58Decode("QmZ8naDy5mEz4GLuQwjWt9MPYqHTBbsm8tQBrNSjiq6zBc")
+	PeerID5, _     = peer.IDB58Decode("QmZVAo3wd8s5eTTy2kPYs34J9PvfxpKPuYsePPYGjgRRjg")
+	PeerID6, _     = peer.IDB58Decode("QmR8Vu6kZk7JvAN2rWVWgiduHatgBq2bb15Yyq8RRhYSbx")
 
 	PeerName1 = "TestPeer1"
 	PeerName2 = "TestPeer2"
@@ -39,6 +42,9 @@ var (
 	PathIPLD1 = "/ipld/QmaNJ5acV31sx8jq626qTpAWW4DXKw34aGhx53dECLvXbY"
 	PathIPLD2 = "/ipld/QmaNJ5acV31sx8jq626qTpAWW4DXKw34aGhx53dECLvXbY/"
 
+	// NotFoundPath is meant to be used as a path that resolves into a CID that doesn't exist in the
+	// pinset.
+	NotFoundPath = "/ipfs/bafyreiay3jpjk74dkckv2r74eyvf3lfnxujefay2rtuluintasq2zlapv4"
 	InvalidPath1 = "/invalidkeytype/QmaNJ5acV31sx8jq626qTpAWW4DXKw34aGhx53dECLvXbY/"
 	InvalidPath2 = "/ipfs/invalidhash"
 	InvalidPath3 = "/ipfs/"

test/rpc_api_mock.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/ipfs/ipfs-cluster/api"
+	"github.com/ipfs/ipfs-cluster/state"
 
 	cid "github.com/ipfs/go-cid"
 	gopath "github.com/ipfs/go-path"
@@ -73,6 +74,9 @@ func (mock *mockCluster) Unpin(ctx context.Context, in *api.Pin, out *api.Pin) e
 	if in.Cid.Equals(ErrorCid) {
 		return ErrBadCid
 	}
+	if in.Cid.Equals(NotFoundCid) {
+		return state.ErrNotFound
+	}
 	*out = *in
 	return nil
 }
@@ -102,7 +106,11 @@ func (mock *mockCluster) PinPath(ctx context.Context, in *api.PinPath, out *api.
 }
 
 func (mock *mockCluster) UnpinPath(ctx context.Context, in *api.PinPath, out *api.Pin) error {
-	// Mock-Unpin behaves exactly pin (doing nothing).
+	if in.Path == NotFoundPath {
+		return state.ErrNotFound
+	}
+
+	// Mock-Unpin behaves like pin (doing nothing).
 	return mock.PinPath(ctx, in, out)
 }