From bb8e8725e8c42ead2bca2e3a662c2d08f552e4ec Mon Sep 17 00:00:00 2001 From: Hector Sanjuan Date: Fri, 17 Jun 2022 16:59:22 +0200 Subject: [PATCH 1/2] PinSVC API: fix bugs and increase compliance These changes fix a few bugs in the PinSVC API and have been discovered by running the compliance tool at https://github.com/ipfs-shipyard/pinning-service-compliance. In particular, the error objects did not respect the spec, filters and counts were not done right. An additional PR will follow with fixes to the pintracker because some pin status information was not correctly set. --- api/pinsvcapi/config.go | 5 +++- api/pinsvcapi/pinsvc/pinsvc.go | 21 ++++++++++----- api/pinsvcapi/pinsvcapi.go | 48 +++++++++++++++++++++++---------- api/pinsvcapi/pinsvcapi_test.go | 12 ++++----- 4 files changed, 58 insertions(+), 28 deletions(-) diff --git a/api/pinsvcapi/config.go b/api/pinsvcapi/config.go index 9464472c..c5a5a182 100644 --- a/api/pinsvcapi/config.go +++ b/api/pinsvcapi/config.go @@ -67,7 +67,10 @@ func NewConfig() *Config { cfg.DefaultFunc = defaultFunc cfg.APIErrorFunc = func(err error, status int) error { return pinsvc.APIError{ - Reason: err.Error(), + Details: pinsvc.APIErrorDetails{ + Reason: err.Error(), + Details: err.Error(), + }, } } return &cfg diff --git a/api/pinsvcapi/pinsvc/pinsvc.go b/api/pinsvcapi/pinsvc/pinsvc.go index bf549851..c71d5036 100644 --- a/api/pinsvcapi/pinsvc/pinsvc.go +++ b/api/pinsvcapi/pinsvc/pinsvc.go @@ -24,12 +24,17 @@ func init() { // APIError is returned by the API as a body when an error // occurs. It implements the error interface. type APIError struct { + Details APIErrorDetails `json:"error"` +} + +// APIErrorDetails contains details about the APIError. +type APIErrorDetails struct { Reason string `json:"reason"` - Details string `json:"details"` + Details string `json:"details,omitempty"` } func (apiErr APIError) Error() string { - return apiErr.Reason + return apiErr.Details.Reason } // PinName is a string limited to 255 chars when serializing JSON. @@ -54,9 +59,9 @@ func (pname *PinName) UnmarshalJSON(data []byte) error { // Pin contains basic information about a Pin and pinning options. type Pin struct { Cid types.Cid `json:"cid"` - Name PinName `json:"name"` - Origins []types.Multiaddr `json:"origins"` - Meta map[string]string `json:"meta"` + Name PinName `json:"name,omitempty"` + Origins []types.Multiaddr `json:"origins,omitempty"` + Meta map[string]string `json:"meta,omitempty"` } // Defined returns if the pinis empty (Cid not set). @@ -221,12 +226,12 @@ type PinStatus struct { Created time.Time `json:"created"` Pin Pin `json:"pin"` Delegates []types.Multiaddr `json:"delegates"` - Info map[string]string `json:"info"` + Info map[string]string `json:"info,omitempty"` } // PinList is the result of a call to List pins type PinList struct { - Count int `json:"count"` + Count uint64 `json:"count"` Results []PinStatus `json:"results"` } @@ -293,6 +298,8 @@ func (lo *ListOptions) FromQuery(q url.Values) error { return fmt.Errorf("error parsing 'limit' query param: %s: %w", v, err) } lo.Limit = lim + } else { + lo.Limit = 10 // implicit default } if meta := q.Get("meta"); meta != "" { diff --git a/api/pinsvcapi/pinsvcapi.go b/api/pinsvcapi/pinsvcapi.go index f3afbce0..c00d2a1e 100644 --- a/api/pinsvcapi/pinsvcapi.go +++ b/api/pinsvcapi/pinsvcapi.go @@ -103,8 +103,10 @@ func globalPinInfoToSvcPinStatus( Origins: gpi.Origins, Meta: gpi.Metadata, } + status.Info = apiInfo + status.Delegates = []types.Multiaddr{} for _, pi := range gpi.PeerMap { status.Delegates = append(status.Delegates, pi.IPFSAddresses...) } @@ -228,6 +230,23 @@ func (api *API) addPin(w http.ResponseWriter, r *http.Request) { return } + // Unpin old item + if clusterPin.PinUpdate.Defined() { + var oldPin types.Pin + err = api.rpcClient.CallContext( + r.Context(), + "", + "Cluster", + "Unpin", + types.PinCid(clusterPin.PinUpdate), + &oldPin, + ) + if err != nil { + api.SendResponse(w, common.SetStatusAutomatically, err, nil) + return + } + } + status := api.pinToSvcPinStatus(r.Context(), pin.Cid.String(), pinObj) api.SendResponse(w, common.SetStatusAutomatically, nil, status) } @@ -297,6 +316,9 @@ func (api *API) listPins(w http.ResponseWriter, r *http.Request) { tst := svcStatusToTrackerStatus(opts.Status) var pinList pinsvc.PinList + pinList.Results = []pinsvc.PinStatus{} + count := uint64(0) + if len(opts.Cids) > 0 { // copy approach from restapi type statusResult struct { @@ -321,19 +343,18 @@ func (api *API) listPins(w http.ResponseWriter, r *http.Request) { } var err error - i := uint64(0) + for stResult := range stCh { if stResult.st.Status == pinsvc.StatusUndefined && stResult.err == nil { // ignore things unpinning continue } - pinList.Results = append(pinList.Results, stResult.st) - err = multierr.Append(err, stResult.err) - if i+1 == opts.Limit { - break + if count < opts.Limit { + pinList.Results = append(pinList.Results, stResult.st) + err = multierr.Append(err, stResult.err) } - i++ + count++ } if err != nil { @@ -360,18 +381,17 @@ func (api *API) listPins(w http.ResponseWriter, r *http.Request) { ) }() - i := uint64(0) for gpi := range out { st := globalPinInfoToSvcPinStatus(gpi.Cid.String(), gpi) if st.Status == pinsvc.StatusUndefined { // i.e things unpinning continue } - if st.Created.Before(opts.After) { + if !opts.After.IsZero() && st.Created.Before(opts.After) { continue } - if st.Created.After(opts.Before) { + if !opts.Before.IsZero() && st.Created.After(opts.Before) { continue } @@ -381,11 +401,10 @@ func (api *API) listPins(w http.ResponseWriter, r *http.Request) { if !st.Pin.MatchesMeta(opts.Meta) { continue } - pinList.Results = append(pinList.Results, st) - i++ - if i == opts.Limit { - break + if count < opts.Limit { + pinList.Results = append(pinList.Results, st) } + count++ } err := <-errCh @@ -395,7 +414,7 @@ func (api *API) listPins(w http.ResponseWriter, r *http.Request) { } } - pinList.Count = len(pinList.Results) + pinList.Count = count api.SendResponse(w, common.SetStatusAutomatically, err, pinList) } @@ -431,6 +450,7 @@ func (api *API) pinToSvcPinStatus(ctx context.Context, rID string, pin types.Pin peers = pin.Allocations } + status.Delegates = []types.Multiaddr{} for _, peer := range peers { var ipfsid types.IPFSID err := api.rpcClient.CallContext( diff --git a/api/pinsvcapi/pinsvcapi_test.go b/api/pinsvcapi/pinsvcapi_test.go index 1ea8d680..545c4e7c 100644 --- a/api/pinsvcapi/pinsvcapi_test.go +++ b/api/pinsvcapi/pinsvcapi_test.go @@ -116,14 +116,14 @@ func TestAPIListEndpoint(t *testing.T) { // Test with cids+limit var resp8 pinsvc.PinList test.MakeGet(t, svcapi, url(svcapi)+"/pins?cid=QmP63DkAFEnDYNjDYBpyNDfttu1fvUw99x1brscPzpqmmq,QmP63DkAFEnDYNjDYBpyNDfttu1fvUw99x1brscPzpqmmb&limit=1", &resp8) - if resp8.Count != 1 { + if resp8.Count != 2 || len(resp8.Results) != 1 { t.Errorf("unexpected statusAll+cids+limit resp:\n %+v", resp8) } // Test with limit var resp9 pinsvc.PinList test.MakeGet(t, svcapi, url(svcapi)+"/pins?limit=1", &resp9) - if resp9.Count != 1 { + if resp9.Count != 3 || len(resp9.Results) != 1 { t.Errorf("unexpected statusAll+limit=1 resp:\n %+v", resp9) } @@ -143,8 +143,8 @@ func TestAPIListEndpoint(t *testing.T) { var errorResp pinsvc.APIError test.MakeGet(t, svcapi, url(svcapi)+"/pins?status=invalid", &errorResp) - if errorResp.Reason == "" { - t.Errorf("expected an error: %s", errorResp.Reason) + if errorResp.Details.Reason == "" { + t.Errorf("expected an error: %s", errorResp.Details.Reason) } } @@ -200,7 +200,7 @@ func TestAPIPinEndpoint(t *testing.T) { t.Fatal(err) } test.MakePost(t, svcapi, url(svcapi)+"/pins", pinJSON, &errName) - if !strings.Contains(errName.Reason, "255") { + if !strings.Contains(errName.Details.Reason, "255") { t.Error("expected name error") } } @@ -231,7 +231,7 @@ func TestAPIGetPinEndpoint(t *testing.T) { var err pinsvc.APIError test.MakeGet(t, svcapi, url(svcapi)+"/pins/"+clustertest.ErrorCid.String(), &err) - if err.Reason == "" { + if err.Details.Reason == "" { t.Error("expected an error") } } From ff488e1fec86d34c86cbb89268e52823dfb574ac Mon Sep 17 00:00:00 2001 From: Hector Sanjuan Date: Mon, 20 Jun 2022 18:49:23 +0200 Subject: [PATCH 2/2] pinsvc: do not set error descriptions It is duplicated with "reason". --- api/pinsvcapi/config.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/api/pinsvcapi/config.go b/api/pinsvcapi/config.go index c5a5a182..077c4bd8 100644 --- a/api/pinsvcapi/config.go +++ b/api/pinsvcapi/config.go @@ -68,8 +68,7 @@ func NewConfig() *Config { cfg.APIErrorFunc = func(err error, status int) error { return pinsvc.APIError{ Details: pinsvc.APIErrorDetails{ - Reason: err.Error(), - Details: err.Error(), + Reason: err.Error(), }, } }