Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pinning APIs #726

Merged
merged 28 commits into from
Jan 24, 2022
Merged

Pinning APIs #726

merged 28 commits into from
Jan 24, 2022

Conversation

flea89
Copy link
Contributor

@flea89 flea89 commented Dec 1, 2021

No description provided.

alexandrastoica and others added 5 commits November 23, 2021 16:50
* feat: add pinning endpoints

* feat: add tests

* feat: add validation for endpoints and refactor error messages

* fix: validation bug

* feat: add tests for get and delete endpoints

* fix: test assertions

* feat: add replace pin api endpoint

* chore: rollback type changes for now

Co-authored-by: Paolo <paolo@potatolondon.com>
* chore: rename PinsUpsertInput to PinUpsertInput

* chore: fix PinUpsertInput type

* chore: use PinUpsertInput where required

* chore(refactor types): refactor pinItem

* chore(refactor types): Use location with other types

* chore(refactor types): fix some type errors in db client

* chore(refactor types): rename PinItemOutput to PinItemNormalized

* chore(refactor types): Pin item should have an _id field

* Rename PinItemOutput back

* feat(pinning-api-db): create pinning table and update reset.sql

* feat(pinning-api-db): create initial types

* feat(pinning-api-db): create db client signatures

* feat(pinning-api-db): updat types

* feat(pinning-api-db): write first intial add pin spec

* feat(pinning-api-db): first implementation of create and get pinRequest

* feat(pinning-api-db): get pin data for request

* feat(pinning-api-db): housekeeping

* feat(pinning-api-db): get pin request tests

* feat(pinning-api-db): housekeeping

* feat: add types to db data types

* feat(pinning-api-db): create content function

* feat(pinning-api-db): remove duplicated type

* feat(pinning-api-db): housekeeping

* feat(pinning-api-db): update types

* feat(pinning-api-db): update documentation
* feat(pinning-apis): get request endpoint work

* chore: remove logging

* chore: improve validation get /pin/requestId

* chore: better integer validation

* fix: requestId conditional

* fix: remove extra bracket

Co-authored-by: Alexandra Stoica <alexandra.stoica@potatolondon.com>
* wip: create pin request

* feat: add async task

* chore: merge feature branch

* feat: code improvements

* chore: add todo

* feat: add comments and some small fixes

* fix: pass token id

* fix: normalized cid vs source cid

* fix: minor fix and tests updates

* fix: update return of get user mock

Co-authored-by: Paolo <paolo@potatolondon.com>
@flea89 flea89 changed the title Feat/pinning apis WIP - Pinning APIs Dec 1, 2021
@flea89 flea89 marked this pull request as draft December 1, 2021 11:36
@@ -199,6 +199,22 @@ CREATE TABLE IF NOT EXISTS pin_sync_request

CREATE INDEX IF NOT EXISTS pin_sync_request_pin_id_idx ON pin_sync_request (pin_id);

-- Tracks pinning requests from Pinning Service API
-- TODO(paolo) this table should be considered for track all content stored on web3.storage
CREATE TABLE IF NOT EXISTS pa_pin_request
Copy link
Contributor Author

@flea89 flea89 Dec 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used a not ideal naming for now.
The aim is to rename this table to pin_request and the existing one to be pin_replication_request

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be good to rename this. But we need a strategy to deal with this, given we would need to put DB in maintenance mode to stop writes and do the swap

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defo, that's why we started with the not ideal name here.

Really keen to hear how you'd approach this, and we can create a follow up ticket to implement it?
How does that sound?

Copy link
Contributor Author

@flea89 flea89 Dec 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think while this would be nice, we agreed to name the table psa_pin_request, so this is not strictly required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename done in in #810

}

// Error messages
export const ERROR_CODE = 400
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a package-wide constant, along with other HTTP status codes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we have a /api/errors.js file where this should live. We will also be adding better debuggability with logtail and it would be good to centralize everything there.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* @param {import('./index').Ctx} ctx
*/
export async function pinGet (request, env, ctx) {
// Check if requestId contains other charachers than digits
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor type. Maybe "Ensure requestId only contains digits" is clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #810

name: pinRequest.name
}

// If content already exists updated foreigh key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor type: "foreign"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #810

@flea89
Copy link
Contributor Author

flea89 commented Dec 1, 2021

@vasco-santos (cc @alanshaw)

It'd be great to get your initial thoughts on the work on the Pinning APIs.
While this is still in progress (you will see quite a few TODOs in the code and code is not ready to be merged), I think we're at a stage where getting your eyes and feedback is key.

We approached the work using the data structures and control flow we discussed, hopefully having actual code will make it easier to share thoughts and collaborate on this.

At this stage, the PR has:

  • DB bulk implementation for creating and retrieving a Pin Request (Tested)
  • Skeletons of all endpoints, with some initial data validation (Tested)
  • POST /pins endpoint logic, plugged with the DB package ( Not tested yet) - This likely needs a bit more work but hopefully should be enough for you to feedback on flow and approach
  • GET /pins/{requestId} enpoint (Tested)

In progress:

  • User allowlist for the APIs
  • GET /pins/
  • DELETE /pins/{requestId}

Thanks for taking the time to look at this, let us know your thoughts

Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to see this getting shape 🚀 Left some comments with a first pass. From flow and approach I think we are in a good direction

I would suggest closing the scope of what we want to achieve with this PR, merging it to a temporary branch when approved and have follow up PRs with the rest of the functionalities, like delete, replace, allowlist, crons, backup, etc. Also, have an issue to track progress with what other follow up PRs will exist, in order to make reviews more efficient with smaller set of changes

packages/api/src/index.js Outdated Show resolved Hide resolved
}

// Error messages
export const ERROR_CODE = 400
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we have a /api/errors.js file where this should live. We will also be adding better debuggability with logtail and it would be good to centralize everything there.

packages/db/utils.js Outdated Show resolved Hide resolved
@@ -199,6 +199,22 @@ CREATE TABLE IF NOT EXISTS pin_sync_request

CREATE INDEX IF NOT EXISTS pin_sync_request_pin_id_idx ON pin_sync_request (pin_id);

-- Tracks pinning requests from Pinning Service API
-- TODO(paolo) this table should be considered for track all content stored on web3.storage
CREATE TABLE IF NOT EXISTS pa_pin_request
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be good to rename this. But we need a strategy to deal with this, given we would need to put DB in maintenance mode to stop writes and do the swap

packages/db/postgres/tables.sql Outdated Show resolved Hide resolved
packages/api/src/pins.js Outdated Show resolved Hide resolved
packages/api/src/pins.js Show resolved Hide resolved
packages/api/src/pins.js Outdated Show resolved Hide resolved
packages/api/src/pins.js Outdated Show resolved Hide resolved
packages/api/src/pins.js Outdated Show resolved Hide resolved
packages/api/src/pins.js Outdated Show resolved Hide resolved
packages/api/src/pins.js Show resolved Hide resolved
packages/api/src/pins.js Outdated Show resolved Hide resolved
packages/api/src/pins.js Outdated Show resolved Hide resolved
packages/api/src/pins.js Outdated Show resolved Hide resolved
packages/db/index.js Outdated Show resolved Hide resolved
packages/db/postgres/tables.sql Outdated Show resolved Hide resolved
packages/db/test/pinning.spec.js Outdated Show resolved Hide resolved
packages/db/test/pinning.spec.js Outdated Show resolved Hide resolved
packages/db/test/pinning.spec.js Outdated Show resolved Hide resolved
flea89 and others added 4 commits December 10, 2021 11:36
* wip: delete pin request

* feat: add db definitions

* feat: wip replace pin endpoint

* chore: update mocks

* fix: do not list deleted requests

* feat: add delete db tests

* fix: tests

* fix: improve validation

* feat: tests

* chore: update comments

* fix: replace pin bugs and tests

* fix: coerce to number using parseInt

* chore: disable tests for now

Co-authored-by: Paolo <paolo@potatolondon.com>
* feat: wip list pin requests

* feat: list pinning requests

* chore: renaming test descriptions

* chore: filter for list pins

* chore: fixing tests

* chore: list pins tests

* feat: db and api tests for list pins

* chore: reorder list test

* fix: correct count in list response

* fix: avoid double query on list

* feat: PR feedback

* feat: add todo for future improv

* fix: typos, use new URL in test, improve Date test

* feat: don't skip test

* fix: update tests and more fixes

* fix: take back the todo

Co-authored-by: Gary Homewood <gary@potatolondon.com>
flea89 and others added 8 commits January 13, 2022 12:04
chore: pinning apis refactoring and feedback

* feat: update sql and db client logic

* feat: update tests and minor fixes

* feat: more updates/fixes to db package

* feat: refactor pin creation flow and refactor car upload as well

* feat: pinning, fix tests and add more

* fix: rename table, functions and types

* chore: PR feedback

* fix: rename types

* chore: rename requestedCid to sourceCid

* fix: delete should not return a body and status should be 202

* chore: add some documentation to waitOkPins

* chore: delete stale mock
* fix: get pin req ids by user token

* fix: update tests

* fix: rest api types

* fix: update function params

* fix: type name

* feat: add test

* fix: tests

* fix: merge conflict

* fix: merge

* fix: rename authToken id
* feat: add metadata to psa pin req

* feat: add metadata to api

* feat: add metadata tests

* chore: feedback
@mbommerez mbommerez added the topic/pot Issues handled by PT. label Jan 20, 2022
@flea89 flea89 marked this pull request as ready for review January 24, 2022 11:13
@flea89 flea89 changed the title WIP - Pinning APIs Pinning APIs Jan 24, 2022
* feat: set search_path to public for uuid function, use uuid as pk on pin_request

* feat: reinstated pin_request and updated psa_pin_request

* bug: resolve merge conflicts

* chore: typos and linting

* bug: pinrequestid type string

* fix: types

* fix: update code to use strings for psa request ids

Co-authored-by: Paolo <paolo@potatolondon.com>
@flea89 flea89 dismissed vasco-santos’s stale review January 24, 2022 11:39

All points have been tackled or tracked by new issues.

@flea89 flea89 merged commit 8187bb5 into main Jan 24, 2022
@flea89 flea89 deleted the feat/pinning-apis branch January 24, 2022 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/pot Issues handled by PT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants