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

feat: postgres setup #508

Merged
merged 12 commits into from
Oct 14, 2021
Merged

feat: postgres setup #508

merged 12 commits into from
Oct 14, 2021

Conversation

vasco-santos
Copy link
Contributor

@vasco-santos vasco-santos commented Sep 30, 2021

This PR adds:

  1. local setup for postgres database using docker compose
  • The database setup runs automatically for tests, but can also be used independently.
    • hooks were added to Playwright test to setup docker compose
    • there are scripts that can be used to start the DB and initialize it in several useful ways
    • Github actions now run DB tests
  • The content under packages/db/postgres/* and /packages/db/scripts/* is heavily inspired from nft.storage
    • The most important aspects to review under this scope are: tables.sql, functions.sql
    • packages/db/postgres/db-types.d.ts is auto generated by openapi-typescript
  1. DB Client revamped to be used for the API
  • No breaking change at this point. Client will still use Fauna by default, but can receive options to use Postgres instead
  • All the query logic for Postgres is within the DB Client for separation of concerns and enabling us to simplify the API codebase
    • Next PR with API rewire will move Fauna queries to DB Client (likely separate file to ease future removal), so that it can be transparent of what backend database will be used.
  • The most important files to review are:
    • packages/db/index.js and packages/db/db-client-types.ts
    • It is worth pointing out that the return values used had into consideration not breaking the API current HTTP responses format + unify with Fauna + normalize snake case to camel case
      • with the above in mind, I decided to not change return types now to avoid API HTTP breaking changes
  1. DB now has tests 🎉
  • all the flows I could think about are now tested. I likely missed a few, but we will get to there as we do further integration

@vasco-santos vasco-santos force-pushed the feat/postgres-setup branch 14 times, most recently from d5a6cf7 to 3a253d4 Compare October 6, 2021 15:40
@@ -7,8 +7,6 @@ on:
- 'packages/api/**'
- '.github/workflows/api.yml'
pull_request:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be able to run CI tests for other branch targets, like here where we target the DB migration branch

@vasco-santos vasco-santos marked this pull request as ready for review October 6, 2021 15:43
@vasco-santos vasco-santos force-pushed the feat/postgres-setup branch 8 times, most recently from d92e7d5 to fe538cd Compare October 8, 2021 09:07
This was referenced Oct 8, 2021
@olizilla
Copy link
Contributor

e.g

DATABASE_URL        -> PG_REST_URL
DATABASE_TOKEN      -> PG_REST_JWT
DATABASE_CONNECTION -> PG_CONNECTION

@olizilla
Copy link
Contributor

I'm ok with the "dbClient" being a wrapper wround both fauna and postgres currently. I agree that fauna queries should live in their own file. Ideally the postgres implmentation would also live in it's own file and then the wrapper pulls in both implementations for now, so we could more easily compare the two implementations side by side.

packages/db/index.js Outdated Show resolved Hide resolved
@vasco-santos
Copy link
Contributor Author

Ideally the postgres implmentation would also live in it's own file and then the wrapper pulls in both implementations for now

I avoided that to make it easier when we remove Fauna. But if you prefer for readability, I can make it its own file too. Probably better in #524 though? which is where I move fauna code around.

@olizilla
Copy link
Contributor

olizilla commented Oct 13, 2021

Probably better in #524 though? which is where I move fauna code around.

yep, sounds good.

@@ -50,6 +50,7 @@ One time set up of your cloudflare worker subdomain for dev:
wrangler secret put S3_ACCESS_KEY_ID --env $(whoami) # Get from Amazon S3 (not required for dev)
wrangler secret put S3_SECRET_ACCESS_KEY_ID --env $(whoami) # Get from Amazon S3 (not required for dev)
wrangler secret put S3_BUCKET_NAME --env $(whoami) # e.g web3.storage-staging-us-east-2 (not required for dev)
wrangler secret put DATABASE_TOKEN --env USER # Get from database postgrest
Copy link
Contributor

Choose a reason for hiding this comment

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

Change docs?

DROP FUNCTION IF EXISTS json_arr_to_upload__pin_type_arr;
DROP FUNCTION IF EXISTS create_upload;
DROP FUNCTION IF EXISTS find_deals_by_content_cids;

Copy link
Contributor

Choose a reason for hiding this comment

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

please to add a comment of what the input and output of these fns looks like!

RETURNS text[] LANGUAGE sql IMMUTABLE PARALLEL SAFE AS
'SELECT ARRAY(SELECT json_array_elements_text(_json))';

CREATE OR REPLACE FUNCTION json_arr_to_upload__pin_type_arr(_json json)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we rename to be clearer about what it does rather than where it is called from

foreach backup_url in array json_arr_to_text_arr(data -> 'backup_urls')
loop
-- insert into backup with update
insert into backup (upload_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs an on conflict here... if the same user uploads the same car, the insert should fail... we dont want multiple backup records with the same s3 url

Comment on lines 89 to 92
return query select *
from upload u
where u.user_id = (data ->> 'user_id')::BIGINT
AND u.content_cid = data ->> 'content_cid';
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the inserted_upload_id!

select * from upload u where u.id =  inserted_upload_id 

also AND is shouty case

Comment on lines +64 to +77
cargoSql = cargoSql.replace(
`
-- Create materialized view from cargo "aggregate_entries" table
CREATE MATERIALIZED VIEW public.aggregate_entry
AS
SELECT *
FROM cargo.aggregate_entries;`,
`
CREATE MATERIALIZED VIEW public.aggregate_entry
AS
SELECT *
FROM cargo.aggregate_entries
WHERE cid_v1 in ('bafybeiaj5yqocsg5cxsuhtvclnh4ulmrgsmnfbhbrfxrc3u2kkh35mts4e');
`
Copy link
Contributor

Choose a reason for hiding this comment

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

@hugomrdias can you say why we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

merging this now! We can revisit in followup PR according to hugo's thoughts

Copy link
Contributor

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

Huge lift! thanks!

* @param {import('./db-client-types').ContentItem} content
* @return {import('./db-client-types').ContentItemOutput}
*/
export function normalizeContent (content) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do as much of the normalising as we can in the queries, and only do the steps we can't do via postgrest here

@vasco-santos vasco-santos merged commit a193f80 into chore/db-migration Oct 14, 2021
@vasco-santos vasco-santos deleted the feat/postgres-setup branch October 14, 2021 11:14
vasco-santos added a commit that referenced this pull request Oct 22, 2021
* local setup for postgres database using docker compose
* DB Client revamped to be used for the API
* DB tests

Co-authored-by: Oli Evans <oli@tableflip.io>
@vasco-santos vasco-santos mentioned this pull request Oct 22, 2021
vasco-santos added a commit that referenced this pull request Oct 22, 2021
* local setup for postgres database using docker compose
* DB Client revamped to be used for the API
* DB tests

Co-authored-by: Oli Evans <oli@tableflip.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants