Skip to content

Conversation

martakam
Copy link
Member

@martakam martakam commented Oct 3, 2026 •

The following has changed:

  • Drizzle ORM has been added, along with a basic schema
  • A bunch of commands for local Postgres setup and a guide
  • A basic database seeding script and a guide on adding the book dataset to the database

I will need a lot of help here to test if the setup works for all platforms.
Furthermore, please let me know whether any of the instructions are vague or unclear.

I understand this PR adds a lot of tech at the same time, so please feel free to ask questions about how this will be used in the app! The frontend does not call our API yet, so that will need to be done in another PR.

Note: The base branch is the one used in #23 so it's easier to read the changes and it will be changed to main once that PR is merged.
This PR should be merged after #23.

@martakam martakam self-assigned this Oct 3, 2026
@martakam martakam added enhancement New feature or request documentation Improvements or additions to documentation labels Oct 3, 2026
@martakam martakam added this to the Deliverable 2 milestone Oct 3, 2026
@martakam martakam linked an issue Oct 3, 2026 that may be closed by this pull request
@martakam martakam marked this pull request as ready for review October 7, 2026 18:36
@martakam martakam requested a review from a team as a code owner October 7, 2026 18:36
@johahor
Copy link
Member

johahor commented Oct 7, 2026

Tested the setup on macOS (Apple Silicon, Node 24.20, pnpm 12.8). It works, and seeding completed. Some notes from following the backend README:

  1. The README never says to run the commands from backend/. I ran the curl download and pnpm db:setup from the repo root first, which fails with:
Error: ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL
  × Command "db:setup" not found

Adding cd backend at the top of the guide would help, and make it clear that all the following commands will be done inside /backend

  1. The curl download from Kaggle worked without logging in (28.5 MB zip).

  2. brew install postgresql@18 was enough. initdb and pg_ctl were on PATH without any extra steps.

  3. I missed the .env step at first. drizzle-kit push then fails with an error that doesn't hint at the missing file:

Error  Either connection "url" or "host", "database" are required for PostgreSQL database connection

A short explanation of what .env is, plus the exact command, would make it clearer:

cp .env.example .env

@johahor
Copy link
Member

johahor commented Oct 7, 2026

This was found with the use of AI, and the comment is AI generated:

While testing I checked how empty values are stored, since csv-parse returns empty strings for empty columns.

From backend/, open the database:

pnpm db:psql

Then run:

SELECT count(*) AS totalt,
       count(*) FILTER (WHERE series = '') AS tomme,
       count(*) FILTER (WHERE series IS NULL) AS null_verdier
FROM books;

Result:

 totalt | tomme | null_verdier
--------+-------+--------------
  52478 | 29008 |            0

All 52,478 books were imported, but 29,008 of them (55%) have an empty string instead of NULL in series, and none are NULL. The other nullable columns (language, publisher, edition, etc.) are probably the same.

Search works today because of this: searchBooks always adds ILIKE '%...%' on every column, and '' matches '%%'. If the seed is changed to store NULL, those books would disappear from every search, since NULL ILIKE '%%' is not true in SQL.

Suggestion: store NULL for empty values in the seed, and only add a condition in searchBooks when the argument is actually set. Both should be changed together. Not urgent for delivery 2.

@martakam
Copy link
Member Author

martakam commented Oct 7, 2026

Thanks for pointing out these issues! The documentation has been improved.

The seeding script now actually adds NULL values as expected. Here's the same output after the fix:

readr=# SELECT count(*) AS totalt,
readr-#        count(*) FILTER (WHERE series = '') AS tomme,
readr-#        count(*) FILTER (WHERE series IS NULL) AS null_verdier
readr-# FROM books;
 totalt | tomme | null_verdier                                                                                                                                                                                           
--------+-------+--------------
  52478 |     0 |        29008
(1 row)

@martakam martakam changed the base branch from feat/basic-graphql-setup to main October 7, 2026 19:07
@johahor
Copy link
Member

johahor commented Oct 8, 2026

Backend works well after the changes, but main's frontend is broken after merging #21 and #22. When main was merged into the express branch, the new shadcn files stayed at the root (app/components/ui, app/lib/utils.ts, components.json) instead of moving to frontend/. frontend/app/root.tsx imports ~/components/ui/Toast, which doesn't exist in frontend/, so pnpm dev fails. I can make a quick fix PR that moves them, get them into main, and then you can update this branch with that change before we merge. Holding off the approval until then, since I need to re-approve anyways.

@johahor
Copy link
Member

johahor commented Oct 8, 2026

PR #29 is now ready for approval. As soon as that is approved and merged, this PR can move forward.

@martakam
Copy link
Member Author

martakam commented Oct 8, 2026

#29 has been merged. Merged main into this branch as well. I think everything is okay now

@johahor
Copy link
Member

johahor commented Oct 8, 2026

Yup, tested frontend. Works fine. Good job, approving.

@martakam martakam merged commit 982e5ad into main Oct 8, 2026
Sign in to join this conversation on GitHub.
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: setup database and add book dataset
2 participants