RESTful API Request For Comments

[gmelodie]

I think it's about time we redesign the API endpoints. In terms of actual code that should be easy enough (changing routes, swagger annotations and possibly some arguments). I'm thinking we can change incrementally:

1. update collection endpoints

• /collections
  • GET: list all collections for current user
  • POST: create new collections (body: collectionName, collection description)
• /collections/:id
  • GET: list all files in this collection (query param: dir to filter for files only in this dir)
  • POST: add previously pinned content (that has contentID) to the collection
  • DELETE: remove file from collection
  • UPDATE: change name/description (body: collectionName or collection description)

2. update pinning endpoints

• TODO

3. update content endpoints

• /contents/
  • GET: list all contents for current user
  • POST: upload file/content
    • optional query param: type (ipfs for ipfs-add, or car for add-car)
    • optional query param: coluuid to include this file in a collection (in the / path by default)
    • optional query param: dir to include this file in a specific dir of a collection (coluuid must be present)
• /contents/:id
  • GET: download this content
  • DELETE: remove this content
• TODO the rest

5. update deal endpoints

• TODO

6. update the rest

• TODO

Other ideas:

• change /api-keys/... to /keys/...
• /content/by-cid needs to be replaced by something more RESTful

[anjor]

I quite liked https://github.com/application-research/estuary/pull/426

Should POST to /collections/:coluuid add content to the collection coluuid?

[elijaharita]

this is my favorite type of update, looks great so far!

should /content not be /contents? i guess this is debatable since content can be used as an uncountable noun in some contexts, but to me this usage of the term seems like it should be plural countable noun form since each content is a unique clearly defined object. maybe that's too pedantic, lol.

what is UPDATE btw?

[jimmylee]

Looks like this was resolved in Slack

[anjor]

I am on board with the design. Do we want to make the changes as a big bag, or piecemeal? We should make sure that we are not breaking existing workflows though.

[gmelodie]

@anjor as soon as we got the new API design, I'll create a new branch and a bunch of issues that people can grab. When all issues are done, we merge the api branch to dev and do testing. Then release.

[neelvirdy]

few thoughts:

1. the { by: <>, value: <> } pattern is nice for reducing # of endpoints, but seems less convenient for clients than having separate endpoints for each "by" option. they might feel the need to make a util on their client for constructing these bodies to avoid clunky request construction code. /deals/status-by-proposal/{propcid} is easier to construct, for example
2. have we sanity checked if we have endpoints with 0 usage? might be good to be opinionated about API usage patterns. so if estuary-www doesn't need it and we realize it's not being called elsewhere, we should probably remove the endpoint
3. are we leveraging API versioning to make this change? i.e. make our existing API v1 then make this the v2 API so we have more ways to avoid breaking clients

[gmelodie]

@neelvirdy

1. agree but I don't like the wording in /deals/status-by-proposal/{propid}
2. We could look into that https://github.com/application-research/estuary/issues/521
3. I was thinking that this new API would be called v1 since we're still on alpha and we would completely deprecate the current API. We could make it v2 though

[en0ma]

@gmelodie some suggested changes to scope endpoints down to resources

• POST /admin/autoretrieve/init => POST /auto-retrieves - require admin token

• GET /admin/autoretrieve/list => GET /auto-retrieves - require admin token

• POST /autoretrieve/heartbeat => POST /auto-retrieves/heartbeat

• GET /admin/peering/peers => GET /peering-peers - require admin token

• POST /admin/peering/peers => POST /peering-peers - require admin token

• DELETE /admin/peering/peers => DELETE /peering-peers

• GET /contents/deals => GET /deals

• GET deals/{dealid}/info => GET deals/{deal_id}

• GET /deals/{propcid}/proposal => GET /deal-proposals/{propcid} or GET /proposals/{propcid}

• GET /deals/status-by-proposal/{propcid} => GET /deals?prop_cid={prop_cid}, GET /deals?chan_cid={chan_cid} etc

The above suggestion is for the new design to be more resource-based

[jcace]

looks good!

Somewhat related - would this be a good time to standardize on https://github.com/application-research/estuary/issues/438 too?

[anjor]

Directionally on board with this change. I think we are at a stage where we could start a PR at this point, and people can comment about specific implementation on the PR.

[alvin-reyes]

I agree on this but we need to version this properly. We don't want to modify or meddle with the current endpoints (I'm going to say those are v1 endpoints).

Please put this on a /api/v2/ echo Group. We don't want to retire the current API since there's a high chance that they are being used.

// `e` is the main echo group (from handlers.go)
//  then use this `e` for all v2 group or subgroup.
v2Group := e.Group("/api/v2")

[LucRoy]

Overall looks good. For POST /contents, do we want to default the 'type' field to 'file' and leave it as optional?

[elijaharita]

love it!

only gripe is that i think /users should just be /user, since it points to a singular resource (yourself). if there is /users, it should be qualified with a user identifier after. if we want e.g. admins to be able to manage/delete users, the plural version could be preferable. in that case we could also just have /users/me to reference yourself.

i don't see the greatest utility for that though. i think the most reasonable thing is just to use /user

also, why is /keys under its own path now if it's related to the active user (if i understand correctly)? to me it would make sense that all the paths that are sensitive to the user that is currently logged in should be under the /user path.

[snissn]

Looks good to me!

[snissn]

you can build off this branch!

https://github.com/application-research/estuary/pull/485/files#r1013697298

[gmelodie]

@elijaharita we're going with all plural for resources

[jcace]

What do we think about doing a find-replace on miner -> provider for any endpoints that mention it? Since Filecoin at large has adopted the term Storage Provider over Miner. So we would have /providers/... instead of /miners/. Could go with /storage-provider/ but that might be unnecessarily longer. Thoughts?

ref: https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0018.md

[neelvirdy]

What do we think about doing a find-replace on miner -> provider for any endpoints that mention it? Since Filecoin at large has adopted the term Storage Provider over Miner. So we would have /providers/... instead of /miners/. Could go with /storage-provider/ but that might be unnecessarily longer. Thoughts?

ref: https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0018.md

+1 to just being explicit and using /storage-providers/, it's not too long