-
Notifications
You must be signed in to change notification settings - Fork 0
fix(api): add missing migration to create shares tables #186
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
277706a
fix(api): add missing migration to create shares tables
FSM1 e1d5409
docs: add learnings entry for staging migration gap
FSM1 7a3078f
fix(api): address CodeRabbit review — stale phase label, down() comment
FSM1 900916e
docs: add database schema evolution protocol
FSM1 7992b41
docs: capture todo - disable synchronize:true in dev and CI
FSM1 c14c42f
Merge branch 'main' into fix/staging-shares-migration
FSM1 82353e3
fix(docs): revert FullSchema changes and fix doc inconsistencies
FSM1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
30 changes: 30 additions & 0 deletions
30
.learnings/2026-02-22-staging-migration-missing-create-table.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| # Staging Migration Missing CREATE TABLE | ||
|
|
||
| **Date:** 2026-02-22 | ||
|
|
||
| ## Original Prompt | ||
|
|
||
| > it seems like there was a problem running the migration during the staging deployment. you can check the logs on Github or ssh in to the server to figure that one out. Please dont just push through the migrations by executing them directly on the server. Fix the actual problem of migrations not running correctly from the CD pipeline. | ||
|
|
||
| ## What I Learned | ||
|
|
||
| - **`synchronize: true` in dev/test hides missing migrations.** TypeORM auto-creates tables from entity decorators, so you never notice that no `CREATE TABLE` migration exists. The gap only surfaces in staging/production where `synchronize: false`. | ||
| - **A migration that modifies a table is not sufficient** — you also need a migration that creates the table. Phase 14 added `SharesPartialUniqueIndex` (modifies `shares` unique constraint) but never added a migration to create `shares` and `share_keys`. | ||
| - **The pattern already existed in the codebase.** `1740000000000-AddDeviceApprovals.ts` correctly handled this exact scenario — it was added retroactively for a table that had been auto-created by synchronize. Phase 14 should have followed the same pattern. | ||
| - **Migration timestamp ordering matters.** The create-table migration must have a timestamp earlier than any migration that modifies the table. Used `1740250000000` (before `1740300000000`). | ||
| - **FullSchema baseline does NOT need updating.** It is a point-in-time snapshot. Fresh databases run FullSchema first, then all incremental migrations in timestamp order. The incremental migration's `CREATE TABLE IF NOT EXISTS` handles creation on fresh databases too. | ||
|
|
||
| ## What Would Have Helped | ||
|
|
||
| - A CI check or pre-merge validation that compares entity definitions against migration coverage — ensuring every `@Entity()` has a corresponding `CREATE TABLE` in either FullSchema or an incremental migration | ||
| - A checklist item in the PR template: "If you added new entities, did you add a CREATE TABLE migration?" | ||
| - Running the migration runner against a fresh database in CI (not just `synchronize: true`) to catch this class of error | ||
|
|
||
| ## Key Files | ||
|
|
||
| - `apps/api/src/app.module.ts` — lines 83-85: `synchronize` conditional on NODE_ENV | ||
| - `apps/api/src/migrations/` — all migration files, ordered by timestamp | ||
| - `apps/api/src/migrations/1700000000000-FullSchema.ts` — baseline for fresh databases | ||
| - `apps/api/src/run-migrations.ts` — migration runner used in staging deploy | ||
| - `.github/workflows/deploy-staging.yml` — lines 286-287: migration step in deploy pipeline | ||
| - `apps/api/src/shares/entities/` — the entities that were missing CREATE TABLE migrations |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
23 changes: 23 additions & 0 deletions
23
.planning/todos/pending/2026-02-22-disable-synchronize-true-all-envs.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| --- | ||
| created: 2026-02-22T14:30 | ||
| title: Disable synchronize:true in dev and CI to surface missing migrations | ||
| area: api | ||
| files: | ||
| - apps/api/src/app.module.ts:83-85 | ||
| - apps/api/jest.config.js | ||
| - apps/api/src/run-migrations.ts | ||
| --- | ||
|
|
||
| ## Problem | ||
|
|
||
| TypeORM `synchronize: true` in dev and test environments auto-creates tables from entity decorators, hiding missing `CREATE TABLE` migrations until staging/production deploys fail. Phase 14 hit this exact issue — `shares` and `share_keys` entities had no migration, and the staging deploy broke with `relation "shares" does not exist`. | ||
|
|
||
| Setting `synchronize: false` in all environments forces developers to write migrations before they can interact with new tables, catching gaps at development time rather than deploy time. | ||
|
|
||
| ## Solution | ||
|
|
||
| 1. Change `apps/api/src/app.module.ts` lines 83-85 to set `synchronize: false` for all environments | ||
| 2. Update test setup (jest config / test harness) to run migrations against ephemeral test databases instead of relying on auto-sync | ||
| 3. Add a dev convenience script (`pnpm --filter api migrate:dev`) that runs pending migrations, so the DX stays smooth | ||
| 4. Document that existing dev databases need a one-time reset (`DROP DATABASE cipherbox; CREATE DATABASE cipherbox;` then run migrations) | ||
| 5. Update `docs/DATABASE_EVOLUTION_PROTOCOL.md` environment behavior matrix to reflect the change |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| import { MigrationInterface, QueryRunner } from 'typeorm'; | ||
|
|
||
| /** | ||
| * Create the shares and share_keys tables for user-to-user sharing (Phase 14). | ||
| * | ||
| * These tables were previously only created via synchronize:true in dev/test. | ||
| * This migration ensures they exist in staging/production where synchronize is off. | ||
| * | ||
| * Timestamp 1740250000000 must run BEFORE 1740300000000-SharesPartialUniqueIndex | ||
| * which modifies the shares unique constraint. | ||
| */ | ||
| export class AddSharesTables1740250000000 implements MigrationInterface { | ||
| name = 'AddSharesTables1740250000000'; | ||
|
|
||
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| // ────────────────────────────────────────────── | ||
| // 1. shares | ||
| // ────────────────────────────────────────────── | ||
| await queryRunner.query(` | ||
| CREATE TABLE IF NOT EXISTS "shares" ( | ||
| "id" uuid NOT NULL DEFAULT uuid_generate_v4(), | ||
| "sharer_id" uuid NOT NULL, | ||
| "recipient_id" uuid NOT NULL, | ||
| "item_type" varchar(10) NOT NULL, | ||
| "ipns_name" varchar(255) NOT NULL, | ||
| "item_name" varchar(255) NOT NULL, | ||
| "encrypted_key" bytea NOT NULL, | ||
| "hidden_by_recipient" boolean NOT NULL DEFAULT false, | ||
| "revoked_at" TIMESTAMP, | ||
| "created_at" TIMESTAMP NOT NULL DEFAULT now(), | ||
| "updated_at" TIMESTAMP NOT NULL DEFAULT now(), | ||
| CONSTRAINT "PK_shares" PRIMARY KEY ("id"), | ||
| CONSTRAINT "FK_shares_sharer" FOREIGN KEY ("sharer_id") | ||
| REFERENCES "users" ("id") ON DELETE CASCADE ON UPDATE NO ACTION, | ||
| CONSTRAINT "FK_shares_recipient" FOREIGN KEY ("recipient_id") | ||
| REFERENCES "users" ("id") ON DELETE CASCADE ON UPDATE NO ACTION | ||
| ) | ||
| `); | ||
|
|
||
| await queryRunner.query( | ||
| `CREATE INDEX IF NOT EXISTS "IDX_shares_sharer_id" ON "shares" ("sharer_id")` | ||
| ); | ||
| await queryRunner.query( | ||
| `CREATE INDEX IF NOT EXISTS "IDX_shares_recipient_id" ON "shares" ("recipient_id")` | ||
| ); | ||
| await queryRunner.query( | ||
| `CREATE INDEX IF NOT EXISTS "IDX_shares_ipns_name" ON "shares" ("ipns_name")` | ||
| ); | ||
|
|
||
| // Initial absolute unique constraint — the next migration (1740300000000) | ||
| // replaces this with a partial index (WHERE revoked_at IS NULL). | ||
| await queryRunner.query(` | ||
| CREATE UNIQUE INDEX IF NOT EXISTS "UQ_shares_sharer_recipient_ipns" | ||
| ON "shares" ("sharer_id", "recipient_id", "ipns_name") | ||
| `); | ||
|
|
||
| // ────────────────────────────────────────────── | ||
| // 2. share_keys | ||
| // ────────────────────────────────────────────── | ||
| await queryRunner.query(` | ||
| CREATE TABLE IF NOT EXISTS "share_keys" ( | ||
| "id" uuid NOT NULL DEFAULT uuid_generate_v4(), | ||
| "share_id" uuid NOT NULL, | ||
| "key_type" varchar(10) NOT NULL, | ||
| "item_id" varchar(255) NOT NULL, | ||
| "encrypted_key" bytea NOT NULL, | ||
| "created_at" TIMESTAMP NOT NULL DEFAULT now(), | ||
| CONSTRAINT "PK_share_keys" PRIMARY KEY ("id"), | ||
| CONSTRAINT "UQ_share_keys_share_type_item" UNIQUE ("share_id", "key_type", "item_id"), | ||
| CONSTRAINT "FK_share_keys_share" FOREIGN KEY ("share_id") | ||
| REFERENCES "shares" ("id") ON DELETE CASCADE ON UPDATE NO ACTION | ||
| ) | ||
| `); | ||
|
|
||
| await queryRunner.query( | ||
| `CREATE INDEX IF NOT EXISTS "IDX_share_keys_share_id" ON "share_keys" ("share_id")` | ||
| ); | ||
| await queryRunner.query( | ||
| `CREATE INDEX IF NOT EXISTS "IDX_share_keys_item_id" ON "share_keys" ("item_id")` | ||
| ); | ||
| } | ||
|
|
||
| public async down(queryRunner: QueryRunner): Promise<void> { | ||
| // On staging/production this migration created both tables, so dropping them here is correct. | ||
| // On fresh databases, FullSchema (1700000000000) created these tables and this migration | ||
| // only added indexes. Reverting only this migration on a fresh DB will drop FullSchema-owned | ||
| // tables; ensure FullSchema is also reverted to maintain consistency. | ||
| await queryRunner.query(`DROP TABLE IF EXISTS "share_keys" CASCADE`); | ||
| await queryRunner.query(`DROP TABLE IF EXISTS "shares" CASCADE`); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.