search

ipfs-shipyard / pinning-service-compliance

This repo checks the compliance of IPFS Pinning Services against the pinning spec
https://ipfs-shipyard.github.io/pinning-service-compliance/
Other
13 stars 10 forks source link

List pins and default status value #245

Open flea89 opened 1 year ago

flea89 commented 1 year ago

Hi, first of all, thanks for putting this together, it's extremely useful.

I'm fixing some of the reported issues on web3.storage API, but there's one specifically that might be a bit tricky, specifically, the test Can retrieve pin with name 'aCid' via the 'exact' TextMatchingStrategy

The tool sends a 

GET https://api.web3.storage/pins?name=1b1c9a55-9abf-4514-8bb2-9d2494d19184&match=exact

and expects some results.

The challenge with that, according to the spec

status 
Array of strings (Status) non-empty
Items Enum: "queued" "pinning" "pinned" "failed"
Example: status=queued,pinning
Return pin objects for pins with the specified status (when missing, service should default to pinned only)

is that if status is not provided should default to "pinned".

I wonder if here we should send a 

GET https://api.web3.storage/pins?name=1b1c9a55-9abf-4514-8bb2-9d2494d19184&match=exact&status=queued,pinning,pinned,failed

instead, given we can't assume the service has the given pin request pinned by the time the endpoint is hit by the tool.

What do you think?

Thanks

olizilla commented 1 year ago

My gut feel is it's odd that status has a default if omitted, but it makes the simplest curl interaction more approachable.

We should either drop that default, and require that the user send status=pinned to get that behaviour, or we need to be explicit in the spec about the circumstances under which the default status filter applies.

@lidel what you think?

lidel commented 1 year ago

On default behavior and spec

fwiw the spec is explicit on the behavior here:

(when missing, service should default to pinned only)

iirc this default being pinned instead of "all pins in all states" was suggested by @obo20 to make scaling easier: in their case the "pinned" content being handled by different system than "in progress" pins ("queued,pinning,failed").

I don't mind removing implicit default and requiring explicit status to be always sent to remove any ambiguity, but that should happen in https://github.com/ipfs/pinning-services-api-spec/issues :)

On bug in pinning-service-compliance

For the issue at hand, pinning-service-compliance tests should be fixed to look for any status (send explicit &status=queued,pinning,pinned,failed) when we are testing things that are status-agnostic, such as the match filter.

I was not involved in pinning services for a while, @SgtPooki does this sound good to you?

SgtPooki commented 1 year ago

That does sound good. Thanks for chiming in @lidel . Moving this to our work queue