Add formName and migrate season-scoped tables to composite keys #2
Conversation
Lindsay-X
requested review from
BlazingAsher,
NotMyLyfe,
danielp1218,
ferdinandzh4ng,
fpunny and
shoexue
src/db/schema/form.ts
Outdated
| import { season } from "./season"; | ||
|
|
||
| export const form = pgTable("form", { | ||
| formId: uuid("formId") |
There was a problem hiding this comment.
formId should also be composite keyed with seasonCode
migrations/meta/_journal.json
Outdated
| "when": 1764220440632, | ||
| "tag": "0001_cascade_behaviour", | ||
| "when": 1765780179980, | ||
| "tag": "0000_cynical_supreme_intelligence", |
There was a problem hiding this comment.
nit: can we name the migration file like "0000_init" instead of the auto generated names
BlazingAsher
left a comment
BlazingAsher
left a comment
There was a problem hiding this comment.
form table is heavily referenced by other entities --> composite keys would require propagating seasonCode into every dependent table which would cause join complexities
I would push back on this. The point of stamping seasonCode everywhere is to avoid cross-tenant leakage. It really should be everywhere. What join complexities are you foreseeing? I could possibly see an issue if you're trying to join two things that are from different seasons, but that should be intentionally hard because that's a cross-tenant access.
src/db/schema/formQuestion.ts
Outdated
| .array() | ||
| .default(sql`ARRAY[]::text[]`), | ||
| }, | ||
| (t) => [primaryKey({ columns: [t.formQuestionId, t.seasonCode] })], |
There was a problem hiding this comment.
Should be formQuestionid, formId, seasonCode
src/db/schema/formResponse.ts
Outdated
| }, | ||
| (t) => [unique().on(t.seasonCode, t.userId, t.formId)], | ||
| (t) => [ | ||
| primaryKey({ columns: [t.formResponseId, t.seasonCode] }), |
There was a problem hiding this comment.
formResponseId, formId, seasonCode
There was a problem hiding this comment.
I think you forgot to actually add formId into the PK
| }, | ||
| ), | ||
| winnerId: uuid("winnerId"), | ||
| loserId: uuid("loserId"), |
There was a problem hiding this comment.
Should FK to the hacker table userId (assuming you drop hackerId)
src/db/schema/volunteer.ts
Outdated
| volunteerId: uuid("volunteerId") | ||
| .primaryKey() | ||
| .default(sql`uuidv7()`), | ||
| volunteerId: uuid("volunteerId").default(sql`uuidv7()`), |
There was a problem hiding this comment.
For hacker, mentor, sponsor, and this one, I think you should get rid of the "role"Id and make the PK solely the userId, seasonCode.
There should never be more than one "hacker" per userId and seasonCode, so there shouldn't be a need to introduce another Id for it.
BlazingAsher
left a comment
BlazingAsher
left a comment
There was a problem hiding this comment.
Please address comments before merging
|
|
||
| eventId: uuid("eventId").notNull(), | ||
|
|
||
| userId: uuid("userId").notNull(), |
There was a problem hiding this comment.
should probably FK this to users as well
src/db/schema/formResponse.ts
Outdated
| }, | ||
| (t) => [unique().on(t.seasonCode, t.userId, t.formId)], | ||
| (t) => [ | ||
| primaryKey({ columns: [t.formResponseId, t.seasonCode] }), |
There was a problem hiding this comment.
I think you forgot to actually add formId into the PK
* fix: change to composite keys and add form names to form * fix: fix format * fix: added form composite keys, remove specific user id * fix: format * fix: added missing keys and constraints * fix: format fix
* feat: create route-specific auth handlers * fix: better dev support and error handling * feat: update usertype logic * apply suggestions from code review * Add formName and migrate season-scoped tables to composite keys (#2) * fix: change to composite keys and add form names to form * fix: fix format * fix: added form composite keys, remove specific user id * fix: format * fix: added missing keys and constraints * fix: format fix * fix: better user type unit tests ---------
Uh oh!
There was an error while loading. Please reload this page.