fix(api): add missing migration to create shares tables#186
Conversation
The Phase 14 PR added a migration that modifies the shares table (partial unique index) but never added a migration to create the shares and share_keys tables. In dev/test, synchronize:true masks this gap. In staging (synchronize:false), the migration fails with "relation shares does not exist". Add 1740250000000-AddSharesTables migration (runs before the partial index migration) and update FullSchema baseline for fresh databases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b7c0a6854937
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds two new database tables (shares, share_keys) via a new migration and updates the historical full-schema migration and documentation to record and guide proper migration ordering and practices. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8258c5726908
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/api/src/migrations/1740250000000-AddSharesTables.ts (2)
19-38:uuid_generate_v4()with nouuid-osspextension guard
up()usesuuid_generate_v4()in theDEFAULTclauses but never issuesCREATE EXTENSION IF NOT EXISTS "uuid-ossp". The extension is guaranteed to exist becauseFullSchema(timestamp1700000000000) always runs before this migration. However, if this migration is ever executed in isolation (e.g., a CI environment that bootstraps only incremental migrations), theCREATE TABLEcalls will fail.✨ Proposed self-containment fix
public async up(queryRunner: QueryRunner): Promise<void> { + await queryRunner.query(`CREATE EXTENSION IF NOT EXISTS "uuid-ossp"`); + // ────────────────────────────────────────────── // 1. shares🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/migrations/1740250000000-AddSharesTables.ts` around lines 19 - 38, The migration uses uuid_generate_v4() in the CREATE TABLE SQL inside the migration's up() (the queryRunner.query(...) call in AddSharesTables migration) but doesn't ensure the uuid-ossp extension exists; prepend a statement that creates the extension if missing by executing CREATE EXTENSION IF NOT EXISTS "uuid-ossp" (via queryRunner.query) before running the CREATE TABLE SQL so the DEFAULT uuid_generate_v4() will always be available when this migration runs in isolation.
24-24:item_typeandkey_typelack database-levelCHECKconstraintsBoth columns are declared as
varchar(10)with noCHECKconstraint, leaving invalid values (e.g., empty string, unknown type names) to be caught only at the application layer. ACHECKconstraint documents the allowed domain directly in the schema and prevents invalid rows from external DB access.✨ Proposed CHECK constraints
- "item_type" varchar(10) NOT NULL, + "item_type" varchar(10) NOT NULL CHECK ("item_type" IN ('file', 'folder')),- "key_type" varchar(10) NOT NULL, + "key_type" varchar(10) NOT NULL CHECK ("key_type" IN ('file', 'folder')),Adjust the allowed values to match the TypeORM entity enum.
Also applies to: 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/migrations/1740250000000-AddSharesTables.ts` at line 24, Add DB-level CHECK constraints for the item_type and key_type columns in the AddSharesTables migration so the schema enforces the same allowed enum values as the TypeORM entity; locate the CREATE TABLE statement that defines "item_type" varchar(10) and the similar "key_type" definition and append appropriate CHECK(...) clauses listing the exact allowed string literals used by the entity enum (and disallow empty string), updating both the up and down migration logic if necessary so the constraint is created and removed alongside the table.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/migrations/1700000000000-FullSchema.ts`:
- Around line 15-18: Update the file-level doc comment that currently says
"complete schema state as of Phase 9.1" to reflect the new baseline that
includes Phase 14 tables; change the phase label text (the string "complete
schema state as of Phase 9.1") to "Phase 14" (or an appropriate Phase 14
descriptor) and optionally mention that the schema now includes the added tables
`shares` and `share_keys` so readers understand why the phase increased.
In `@apps/api/src/migrations/1740250000000-AddSharesTables.ts`:
- Around line 83-86: The down() method in the AddSharesTables migration
currently drops "share_keys" and "shares" unconditionally (using DROP TABLE IF
EXISTS), which can remove tables created by FullSchema on a fresh DB; add a
concise comment inside the AddSharesTables class above the down() method (or
inline before the DROP TABLE statements) explaining the dual behavior: on
production up() creates the tables so down() should drop them, but on fresh/dev
databases FullSchema may have already created those tables and up() is a no-op,
so down() uses IF EXISTS and may remove FullSchema-owned tables—document this
tradeoff so maintainers understand why the IF EXISTS drops are preserved.
---
Nitpick comments:
In `@apps/api/src/migrations/1740250000000-AddSharesTables.ts`:
- Around line 19-38: The migration uses uuid_generate_v4() in the CREATE TABLE
SQL inside the migration's up() (the queryRunner.query(...) call in
AddSharesTables migration) but doesn't ensure the uuid-ossp extension exists;
prepend a statement that creates the extension if missing by executing CREATE
EXTENSION IF NOT EXISTS "uuid-ossp" (via queryRunner.query) before running the
CREATE TABLE SQL so the DEFAULT uuid_generate_v4() will always be available when
this migration runs in isolation.
- Line 24: Add DB-level CHECK constraints for the item_type and key_type columns
in the AddSharesTables migration so the schema enforces the same allowed enum
values as the TypeORM entity; locate the CREATE TABLE statement that defines
"item_type" varchar(10) and the similar "key_type" definition and append
appropriate CHECK(...) clauses listing the exact allowed string literals used by
the entity enum (and disallow empty string), updating both the up and down
migration logic if necessary so the constraint is created and removed alongside
the table.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9dddc33c46d0
Formalizes migration discipline rules to prevent the class of error from Phase 14 where new entities lacked CREATE TABLE migrations. Covers change classification, evolution checklist, naming conventions, environment behavior matrix, and historical incidents. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 564c4834ac78
Area: api Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: fd35e4894cee
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/migrations/1700000000000-FullSchema.ts`:
- Around line 234-282: The FullSchema baseline omits the unique index
UQ_shares_sharer_recipient_ipns on the "shares" table, making AddSharesTables
not fully no-op on fresh DBs; add a UNIQUE constraint/index creation for
UQ_shares_sharer_recipient_ipns covering
("sharer_id","recipient_id","ipns_name") in the same block that creates the
"shares" table (and/or immediately after) so the baseline matches
AddSharesTables exactly and the later partial-unique-index migration becomes a
true no-op on fresh databases.
In `@docs/DATABASE_EVOLUTION_PROTOCOL.md`:
- Around line 152-157: Update the "Timestamp" guidance in
DATABASE_EVOLUTION_PROTOCOL.md so bullet 1 includes a qualifying exception for
backfilled prerequisite migrations: state that timestamps should normally be
later than all existing migration timestamps unless you are inserting a backfill
prerequisite (e.g., adding 1740250000000-AddSharesTables between 1740200000000
and 1740300000000), in which case inserting a timestamp that falls between
existing ones is allowed; keep bullet 3's rule about CREATE TABLE migrations
remaining earlier than any modification of the same table. Identify and edit the
"Timestamp:" bullets and mention the example migration names/timestamps to
clarify the exception.
- Around line 64-67: Replace the stale bullet in Section 3.1 that reads "Update
FullSchema baseline to include the new structure" with wording that aligns with
Guiding Principle 4 and Section 9.2: state that the FullSchema baseline does not
need to be updated for new tables and that the incremental migration must handle
table creation on both fresh and existing databases (use the device_approvals
example in Section 9.2 as canonical guidance); ensure the revised line
explicitly references idempotent incremental migrations rather than instructing
FullSchema edits to avoid confusion.
Addressing remaining CodeRabbit nitpick comments1.
|
FullSchema is a point-in-time snapshot (Phase 9.1) and should not be updated for new tables. Reverted the shares/share_keys additions — the incremental AddSharesTables migration handles creation on both fresh and existing databases via IF NOT EXISTS. Fixed three doc inconsistencies: - Section 3.1: "Update FullSchema baseline" → "Do not update FullSchema" - Section 9.1: Removed incorrect claim that FullSchema was updated - Timestamp guidance: Added backfill prerequisite exception - Learnings: Corrected takeaway about FullSchema Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 13ca730c9231
Adds a `migration-check` CI job that runs existing migrations against a fresh Postgres, then uses `typeorm migration:generate` to detect if any entity changes lack a corresponding migration. Prevents the exact class of bug that hit in Phase 14 (PR #186) where a missing CREATE TABLE migration was invisible locally due to synchronize:true. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: d092a7bc6505
* ci: add migration drift check to detect entity/migration divergence Adds a `migration-check` CI job that runs existing migrations against a fresh Postgres, then uses `typeorm migration:generate` to detect if any entity changes lack a corresponding migration. Prevents the exact class of bug that hit in Phase 14 (PR #186) where a missing CREATE TABLE migration was invisible locally due to synchronize:true. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: d092a7bc6505 * fix(ci): resolve ts-node decorator metadata error and filter cosmetic drift Two issues found during CI validation: 1. TypeORM CLI fails with TS1240 (decorator metadata) when ts-node starts with a .js entry file and lazily initializes the TypeScript service without the correct tsconfig. Fix: add --project tsconfig.json to the typeorm script in package.json. 2. migration:generate always reports "drift" because our hand-written migrations use explicit constraint/index names (e.g., FK_refresh_tokens_user) while TypeORM expects auto-generated hash names. Fix: only flag structural drift (CREATE/DROP TABLE, ADD/DROP COLUMN, type changes) and ignore cosmetic constraint/index renames. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 140578ed345c * fix(ci): distinguish datasource errors from no-drift in migration check The `|| true` suppressed all failures from `migration:generate`, masking real datasource/config errors (TypeORM exits 1 for both "no changes" and actual errors). Capture exit code and output separately, only treating the "No changes" message as success. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f0b63e72d3e4 * fix(ci): check migration:generate exit status before inspecting output Move STATUS check before the drift file check. Previously, if migration:generate failed without producing a file, the else branch exited 0 and the STATUS check was unreachable — masking real errors like DB connection failures or TypeORM bugs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: a3a82c38a9d8 --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
sharestable (partial unique index) but never included a migration to create thesharesandshare_keystablessynchronize: trueauto-creates tables from entity definitions, masking the gapsynchronize: false), the migration fails:relation "shares" does not exist1740250000000-AddSharesTablesmigration (runs before the partial index migration) withIF NOT EXISTSfor idempotencyRoot cause
synchronize: truein dev/test creates tables automatically from TypeORM entity decorators. The shares feature was developed and tested without ever needing an explicitCREATE TABLEmigration. Staging (and production) havesynchronize: falseand rely entirely on migrations.Test plan
pnpm --filter api buildSELECT * FROM shares LIMIT 1🤖 Generated with Claude Code
Summary by CodeRabbit