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

Wire up a database #8

Merged
merged 7 commits into from
Aug 7, 2024
Merged

Wire up a database #8

merged 7 commits into from
Aug 7, 2024

Conversation

david-mears-2
Copy link
Contributor

@david-mears-2 david-mears-2 commented Jul 25, 2024

Adds scripts for running a postgres db in a docker container, and Prisma for managing migrations and ORM.

Go to localhost:3000/scenarios to check if the app can talk to the database. As you can see below, the nuxt devtools can also show you what data was sent along with the rendered page - proving that the call to the api endpoint '/api/scenarios' happened server-side.

image

I'll do tests when we have something real to test - the api endpoint and page in this PR are only dummy ones to prove that the wiring up worked.

TODO:

  • Figure out how migrations get run on CI envs and applying new migrations to existing envs (prod, dev, etc)

Copy link

codecov bot commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 15.00000% with 17 lines in your changes missing coverage. Please review.

Project coverage is 50.62%. Comparing base (35eac62) to head (ec4dfd5).

Files Patch % Lines
lib/prisma.ts 0.00% 9 Missing and 1 partial ⚠️
server/api/scenarios.ts 0.00% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main       #8      +/-   ##
==========================================
- Coverage   58.93%   50.62%   -8.31%     
==========================================
  Files          13       16       +3     
  Lines         599      401     -198     
  Branches       23       25       +2     
==========================================
- Hits          353      203     -150     
+ Misses        242      192      -50     
- Partials        4        6       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

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

Works for me, but I think we really need a single-liner script to run and migrate the db and spin up other dependencies - the README is a little unclear about what you need to do just to run the current branch (and in what order) vs upating and generating new migrations.

Other than that, mostly questions!

I'm not really sure where Anmol got all his pg config from for packit - maybe chat to him about it when he gets back!

I was expecting to see a model Typescript type scenario generated by the ORM as part of this branch, in addition to the prisma schema and the migrations. How are the queries to prisma made type safe? Is it all wrapped up in the prisma client that ends up in node_modules somehow?

Comment on lines +36 to +42
Copy `.env.example` to `.env`.
Build and run the database container:

```bash
./db/scripts/build
./db/scripts/run
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're going to have to do this a lot during development, I think it would be better to make this a single script call - you might as well put that in a run-dependencies now since you know you're going to have to include the api soon too. It should apply the db migrations you need to.

Do we need to distinguish .env from .env.example here? Can't we just make .env the settings that will work for local development and then in the deployment tool we'll make sure we overwrite with whatever settings we need for the environments we're deploying?

Copy link
Contributor Author

@david-mears-2 david-mears-2 Jul 30, 2024

Choose a reason for hiding this comment

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

To streamline setting up .env, in the next PR I'm updating the README to make copying and npm installing a single action:

Copy .env.example to .env and install the JS dependencies:

cp .env.example .env
npm install


```bash
npm install
```

## Local development
Prisma ORM can only query the database once you 'generate' the Prisma Client, which generates into `node_modules/.prisma/client`. This should happen when you install the JS dependencies and whenever you run a migration, but if the Prisma client gets out of sync or doesn't generate, you can manually generate it:
Copy link
Contributor

Choose a reason for hiding this comment

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

..and this generates the client based on schema.prisma?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating the README to make this clear

README.md Outdated
Comment on lines 70 to 78
### DB

To create migrations to the database, first update the Prisma schema at ./prisma/schema.prisma as required, then run the below to generate the corresponding SQL migration and to apply it to the database:

```bash
npx prisma migrate dev
```

The same command is also used to apply migrations that already exist in ./prisma/migrations but which have not been applied to the database.
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so we're going to keep this as a manual development process? When you need to update the schema, you'll update both the .schema file, and generate the SQL migrations, and commit both to git?

..and as well as generating the migrations this command also applies them to the running database (defined in .env) - running in the container. So we'll run ths same thing as part of deployment (?) to update the db?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Updating README to add "You should commit both the Prisma schema and the migration file to Git."
  • Will look into the precise process for how migrations are done in deployment, but probably it's a GitHub Action that runs the command prisma migrate deploy (as opposed to the dev-specific command above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we just need to run prisma migrate deploy in a github action, and remember to set the DATABASE_URL env var (which we can do in github per-environment).

https://www.prisma.io/docs/orm/prisma-client/deployment/deploy-database-changes-with-prisma-migrate

Copy link
Contributor Author

@david-mears-2 david-mears-2 Aug 1, 2024

Choose a reason for hiding this comment

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

I think it makes most sense to try that out when we have some environments up, with databases running, which we can see whether or not they correctly get migrated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self for when I'm working on this:

It might be the case that we need to explicitly re-generate the prisma client (using prisma generate) after running prisma migrate deploy, since that command "does not reset the database or generate artifacts (such as Prisma Client)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I now have a 'Add prisma migrations to deploy workflow' ticket.

Run an SQL query:

```
./db/scripts/runQuery "SELECT id FROM scenario"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!


docker run --rm -d \
--network=bridge \
--name daedalus-web-app-db \
Copy link
Contributor

Choose a reason for hiding this comment

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

daedalus-web-db might save a few characters of pain every time you need to tye this! Or even daedalus-db if there isn't any danger of an R api db container (not sure what the plan is there though)...

Copy link
Contributor Author

@david-mears-2 david-mears-2 Jul 30, 2024

Choose a reason for hiding this comment

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

Personally I'll find remembering to omit 'app' more confusing, since I'm generically calling this app 'daedalus-web-app' everywhere, e.g. the github repo, package name. (Didn't want to fully commit to the provisional name 'Daedalus Explore'!)

There will be at least this many Daedalusy 'packages' (broadly defined) to keep track of, so explicit naming should help us steer clear of conflation:

  • web app
  • nginx for web app
  • r api
  • model

Comment on lines +21 to +22
if (process.env.NODE_ENV !== "production")
globalThis.prismaGlobal = prisma;
Copy link
Contributor

Choose a reason for hiding this comment

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

So the Prisma Client is the thing that will be used to query the db? I'm not clear on why a distinction is made for production mode? It looks like in this case the client is expected to already exist?

Copy link
Contributor Author

@david-mears-2 david-mears-2 Jul 30, 2024

Choose a reason for hiding this comment

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

I can't explain what the production condition is for. The origin of the code is (as commented at top of file) (a linted version of) https://www.prisma.io/docs/orm/more/help-and-troubleshooting/help-articles/prisma-nuxt-module#option-b-libprismats

@david-mears-2
Copy link
Contributor Author

david-mears-2 commented Jul 30, 2024

Works for me, but I think we really need a single-liner script to run and migrate the db and spin up other dependencies - the README is a little unclear about what you need to do just to run the current branch (and in what order) vs upating and generating new migrations.

Yeah, let's address this in a follow-up PR.

Edit: #10 adds this

@david-mears-2
Copy link
Contributor Author

david-mears-2 commented Jul 30, 2024

I was expecting to see a model Typescript type scenario generated by the ORM as part of this branch, in addition to the prisma schema and the migrations. How are the queries to prisma made type safe? Is it all wrapped up in the prisma client that ends up in node_modules somehow?

Yes, prisma client generates the typescript stuff! Here is an example that appeared as a tooltip in my IDE:

const scenario: {
    id: string;
    model_version: string;
    parameters: Prisma.JsonValue;
    parameters_hash: string;
    result_data: Prisma.JsonValue;
    result_data_completed_at: Date | null;
    created_at: Date;
    updated_at: Date;
} | null

I got a TS warning when trying to use a wrong column: "Object literal may only specify known properties, and 'email' does not exist in type"

Relevant docs: https://www.prisma.io/docs/orm/prisma-client/type-safety

@david-mears-2
Copy link
Contributor Author

I'm not really sure where Anmol got all his pg config from for packit - maybe chat to him about it when he gets back!

The git blame for the files I copied into /db/conf shows that it's exclusively Alex Hill's work I've copied, as far as pg config!

@david-mears-2
Copy link
Contributor Author

Ready for review again @EmmaLRussell - as noted in the PR description I won't add a test for the api endpoint since it's a dummy endpoint that we'll replace in the future.

Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

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

👍

@david-mears-2 david-mears-2 merged commit 7159823 into main Aug 7, 2024
3 of 5 checks passed
@david-mears-2 david-mears-2 deleted the jidea-38-containerisation branch August 7, 2024 09:36
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