fix(api): restore translation foreign keys on SQLite - #551
samuelmbabhazi wants to merge 1 commit into
Conversation
Summary by CodeRabbit
WalkthroughThe migration restores cascading ChangesTranslation foreign-key restoration
Estimated code review effort: 4 (Complex) | ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/migrations/1785664547143-restore-translation-foreign-keys.ts`:
- Around line 17-48: Update the migration execution flow around
dataSourceOptions() and dataSource.runMigrations() so SQLite foreign-key
enforcement is disabled before the migration transaction begins and restored
afterward, or configure this migration as non-transactional. Ensure the
translation rebuild in the migration containing translation_temp runs with
enforcement disabled, while restoration occurs reliably after migration
execution.
In `@api/test/term.e2e-spec.ts`:
- Around line 184-223: Add an analogous end-to-end test alongside the existing
term deletion test that creates a project locale and translation, deletes the
locale, and verifies via the database that translations referencing its
project_locale_id are removed. Reuse the existing translation setup and
authentication patterns, and assert the orphan count is zero to cover the
project_locale_id cascade.
- Around line 198-222: In the translation deletion test, query the translation
count for termId after the PATCH and before the DELETE request, and assert it
equals 1 to confirm the translation exists. Keep the existing post-deletion
query and zero-count assertion unchanged so the test verifies both creation and
cascading deletion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c2d83428-67d4-4eac-b605-8d6df837bd46
📒 Files selected for processing (2)
api/src/migrations/1785664547143-restore-translation-foreign-keys.tsapi/test/term.e2e-spec.ts
| await queryRunner.query(`PRAGMA foreign_keys=off;`); | ||
|
|
||
| await queryRunner.query( | ||
| `CREATE TABLE "translation_temp" ( | ||
| "term_id" TEXT NOT NULL, | ||
| "project_locale_id" TEXT NOT NULL, | ||
| "value" TEXT NOT NULL, | ||
| "date_created" TEXT NOT NULL DEFAULT (datetime('now')), | ||
| "date_modified" TEXT NOT NULL DEFAULT (datetime('now')), | ||
| PRIMARY KEY ("term_id", "project_locale_id"), | ||
| FOREIGN KEY ("term_id") REFERENCES "term"("id") ON DELETE CASCADE, | ||
| FOREIGN KEY ("project_locale_id") REFERENCES "project_locale"("id") ON DELETE CASCADE | ||
| )`, | ||
| ); | ||
|
|
||
| // Copying only the rows whose term and project locale still exist | ||
| // drops the orphans accumulated while the foreign keys were missing. | ||
| await queryRunner.query( | ||
| `INSERT INTO "translation_temp" ("term_id", "project_locale_id", "value", "date_created", "date_modified") | ||
| SELECT "term_id", "project_locale_id", "value", "date_created", "date_modified" | ||
| FROM "translation" | ||
| WHERE "term_id" IN (SELECT "id" FROM "term") | ||
| AND "project_locale_id" IN (SELECT "id" FROM "project_locale")`, | ||
| ); | ||
|
|
||
| await queryRunner.query(`DROP TABLE "translation"`); | ||
| await queryRunner.query(`ALTER TABLE "translation_temp" RENAME TO "translation"`); | ||
|
|
||
| await queryRunner.query(`CREATE INDEX IF NOT EXISTS "IDX_translation_term_id" ON "translation" ("term_id")`); | ||
| await queryRunner.query(`CREATE INDEX IF NOT EXISTS "IDX_translation_project_locale_id" ON "translation" ("project_locale_id")`); | ||
|
|
||
| await queryRunner.query(`PRAGMA foreign_keys=on;`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "migrationsTransactionMode|transaction\s*[:=]\s*['\"]?(all|each|none)|--transaction" -C3
rg -n "migration:run|migrationsRun" package.json api/package.json 2>/dev/null
rg -n "ManyToMany|JoinTable" api/src/entity/label.entity.ts api/src/entity/translation.entity.ts -C5
fd -e ts . api/src/migrations | xargs rg -n "translation" -ilRepository: ever-co/ever-traduora
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -u
printf 'Repository files for migrations and package config:\n'
git ls-files | rg '(^|/)package\.json$|api/src/migrations/|api/src/entity/(label|translation)\.ts$|tsconfig|cli' | head -200
printf '\nSearch for TypeORM transaction/migration config:\n'
rg -n "migrationsTransactionMode|migrationsRun|migration:run|migrations:run|dataSource|typeorm|migrations" -S --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' . | head -200 || true
printf '\nLocate migration and entity files:\n'
fd -e ts . api/src/migrations 2>/dev/null | wc -l
fd -e ts . api/src/entity 2>/dev/null | rg 'label|translation' || trueRepository: ever-co/ever-traduora
Length of output: 15450
🏁 Script executed:
#!/bin/bash
set -u
printf 'Translation entity and relevant entities:\n'
for f in $(fd -e ts 'translation\.ts|label\.ts' api/src/entity 2>/dev/null); do
echo "--- $f"
ast-grep outline "$f" --view expanded 2>/dev/null | head -200 || true
sed -n '1,220p' "$f"
done
printf '\nTarget migration contents:\n'
f=$(git ls-files 'api/src/migrations/1785664547143-restore-translation-foreign-keys.ts')
echo "--- $f"
sed -n '1,140p' "$f"
printf '\nMigration index metadata mentioning translations:\n'
fd -e ts . api/src/migrations 2>/dev/null | xargs rg -l "translation|Translation" 2>/dev/null | sort
while IFS= read -r f; do
echo "--- $f"
rg -n "translation|Translation|primary|foreign|index" "$f" | head -200
doneRepository: ever-co/ever-traduora
Length of output: 4951
🏁 Script executed:
#!/bin/bash
set -u
printf 'api/src/connection/datasource.ts:\n'
sed -n '1,140p' api/src/connection/datasource.ts
printf '\napi/src/main.ts migration path:\n'
sed -n '1,80p' api/src/main.ts
printf '\napi/src/config.ts relevant config:\n'
sed -n '1,120p' api/src/config.ts
printf '\napi/package.json scripts:\n'
node -e "const p=require('./api/package.json'); console.log(JSON.stringify(p.scripts,null,2));"
printf '\napi/test/util.ts migration setup:\n'
sed -n '90,150p' api/test/util.ts
printf '\nLabel-Translation join migration:\n'
sed -n '1,240p' api/src/migrations/1575734358119-add-label-join-tables.ts
printf '\nLabel entity ManyToMany definition:\n'
sed -n '1,180p' api/src/entity/label.entity.ts
printf '\nTranslation entity definitions:\n'
sed -n '1,220p' api/src/entity/translation.entity.ts
printf '\nEarlier translation migration DDL snippets:\n'
for f in api/src/migrations/1537535282567-init.ts \
api/src/migrations/1542044660604-fix-translations-primary-key.ts \
api/src/migrations/1543494409127-change-translation-value-type.ts; do
echo "--- $f"
rg -n "CREATE TABLE|ALTER TABLE|DROP TABLE|translation|term_id|project_locale_id|fk|foreign|INDEX" "$f" -C 3
doneRepository: ever-co/ever-traduora
Length of output: 47023
🌐 Web query:
TypeORM 0.3 migration runTransactionMode migrations transaction all each none PRAGMA foreign_keys inside transaction SQLite
💡 Result:
In TypeORM, the migrationsTransactionMode setting controls how migrations are executed within transactions [1][2]. The available options are: - all (default): Wraps all migrations into a single, collective transaction [1][3][2]. - each: Wraps every individual migration in its own separate transaction [3][2][4]. - none: Disables all automatic transaction wrapping for migrations [3][2]. When using SQLite with TypeORM, attempting to use the PRAGMA foreign_keys statement inside a migration often fails [5][6]. This is because SQLite does not permit changing the foreign_keys setting while inside an active, multi-statement transaction [5][7]. Since TypeORM executes migrations within transactions by default (depending on the mode), any PRAGMA command executed via a query runner inside the up or down methods will be ignored [5][6][7]. To effectively manage SQLite foreign key constraints during migrations, you must set them before the transaction begins [5][8]. If you require foreign keys to be disabled, the common pattern is to disable them globally outside of the migration execution flow [5][6][8]: // Pattern for manually handling SQLite Foreign Keys await connection.query("PRAGMA foreign_keys=OFF;"); await connection.runMigrations; await connection.query("PRAGMA foreign_keys=ON;"); When using this approach, you should typically set migrationsTransactionMode to none or ensure that your migration logic accounts for the lack of transaction-based rollback safety [3][6][8]. If you rely on TypeORM's automatic transaction handling (modes all or each), the PRAGMA statement inside the migration will not have the desired effect because the transaction is already active [5][6][7].
Citations:
- 1: https://typeorm.io/docs/migrations/setup/
- 2: https://github.com/typeorm/typeorm/blob/master/src/migration/MigrationExecutor.ts
- 3: https://github.com/typeorm/typeorm/blob/master/docs/migrations.md
- 4: Add new transaction mode to wrap each migration in transaction typeorm/typeorm#4629
- 5: Cascaded table data is dropped on column add during migration typeorm/typeorm#2584
- 6: Can "PRAGMA foreign_keys=OFF" SQLite statement be used inside a migration? typeorm/typeorm#5712
- 7: https://sqlite.org/foreignkeys.html
- 8: fix: disable SQLite FK checks in synchronize / migrations typeorm/typeorm#7922
Disable SQLite foreign-key enforcement outside the migration transaction.
dataSourceOptions() enables foreignKeys: true, and the migration path uses dataSource.runMigrations() without a migrationsTransactionMode override. Since TypeORM wraps migrations by default, these in-migration PRAGMA foreign_keys=off/on statements do not control enforcement for the translation rebuild. Move the disable/restore pattern outside migration execution or make the migration non-transactional, otherwise DROP TABLE "translation" can still execute FK enforcement and cascade-delete rows in "label_translations_translation".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/migrations/1785664547143-restore-translation-foreign-keys.ts` around
lines 17 - 48, Update the migration execution flow around dataSourceOptions()
and dataSource.runMigrations() so SQLite foreign-key enforcement is disabled
before the migration transaction begins and restored afterward, or configure
this migration as non-transactional. Ensure the translation rebuild in the
migration containing translation_temp runs with enforcement disabled, while
restoration occurs reliably after migration execution.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="api/test/term.e2e-spec.ts">
<violation number="1" location="api/test/term.e2e-spec.ts:221">
P3: This new test only covers the `term_id` cascade. The migration also restores the `project_locale_id` foreign key with ON DELETE CASCADE, so consider adding an analogous test that deletes a project locale and verifies its translations are removed too.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| .expect(204); | ||
|
|
||
| const connection = app.get(Connection); | ||
| const orphans = await connection.query('SELECT COUNT(*) as count FROM translation WHERE term_id = ?', [termId]); |
There was a problem hiding this comment.
P3: This new test only covers the term_id cascade. The migration also restores the project_locale_id foreign key with ON DELETE CASCADE, so consider adding an analogous test that deletes a project locale and verifies its translations are removed too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/test/term.e2e-spec.ts, line 221:
<comment>This new test only covers the `term_id` cascade. The migration also restores the `project_locale_id` foreign key with ON DELETE CASCADE, so consider adding an analogous test that deletes a project locale and verifies its translations are removed too.</comment>
<file context>
@@ -181,6 +181,47 @@ describe('TermController (e2e)', () => {
+ .expect(204);
+
+ const connection = app.get(Connection);
+ const orphans = await connection.query('SELECT COUNT(*) as count FROM translation WHERE term_id = ?', [termId]);
+ expect(Number(orphans[0].count)).toEqual(0);
+ });
</file context>
Greptile SummaryThis change rebuilds the SQLite translation table so term and project-locale deletions cascade to their translations, while discarding pre-existing orphaned rows during the copy. The new regression test verifies deletion through a term, but there is no equivalent assertion that deleting a project locale removes its translations. T-Rex validation blocked
Confidence Score: 4/5
 T-Rex LogsWhat T-Rex did
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }); | ||
| }); | ||
|
|
||
| it('/api/v1/projects/:projectId/terms/:termId (DELETE) should also delete related translations', async () => { |
There was a problem hiding this comment.
The migration restores independent cascades for term_id and project_locale_id, but the added regression test checks only term deletion. Add an equivalent orphan-row assertion for locale deletion so the suite detects regressions that preserve the term constraint while dropping or misconfiguring the project-locale constraint.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
70709d1 to
67d396b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/test/term.e2e-spec.ts (1)
184-229: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the
project_locale_idcascade.This test only exercises the
term_idforeign key. The migration also restores theproject_locale_idforeign key withON DELETE CASCADE. Add an analogous test that deletes a project locale and confirms its translations are removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/test/term.e2e-spec.ts` around lines 184 - 229, Add an analogous end-to-end test alongside the existing term deletion test that creates a project locale with a translation, deletes that locale, and verifies through the database query that no translations remain for its project_locale_id. Reuse the existing translation setup and request patterns, but target the project-locale deletion endpoint and assert the 204 response plus a zero orphan count.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@api/test/term.e2e-spec.ts`:
- Around line 184-229: Add an analogous end-to-end test alongside the existing
term deletion test that creates a project locale with a translation, deletes
that locale, and verifies through the database query that no translations remain
for its project_locale_id. Reuse the existing translation setup and request
patterns, but target the project-locale deletion endpoint and assert the 204
response plus a zero orphan count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 784576c1-81f0-4202-9dc9-89d7930ad27f
📒 Files selected for processing (2)
api/src/migrations/1785664547143-restore-translation-foreign-keys.tsapi/test/term.e2e-spec.ts
67d396b to
be48326
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/migrations/1785664547143-restore-translation-foreign-keys.ts`:
- Line 5: Rename the migration class restoreTranslationForeignKeys1785664547143
to a PascalCase name, such as RestoreTranslationForeignKeys1785664547143, and
update any references to the class consistently while preserving its
MigrationInterface implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 93e3530b-c47a-4aaf-b977-5b8ce1c6bc87
📒 Files selected for processing (2)
api/src/migrations/1785664547143-restore-translation-foreign-keys.tsapi/test/term.e2e-spec.ts
The table rebuilds in migrations 1542044660604 and 1543494409127 recreated the translation table without the foreign keys declared in the initial migration, so term and project locale deletions stopped cascading on SQLite and left orphaned translation rows behind. The new migration rebuilds the table with the original ON DELETE CASCADE constraints, keeps the existing indexes, and copies only the rows whose term and project locale still exist, purging the orphans accumulated while the constraints were missing. Fixes ever-co#492
be48326 to
474812d
Compare
|
1 participant
Fixes #492
Root cause
The initial migration created the SQLite translation table with two inline foreign keys, term_id and project_locale_id, both ON DELETE CASCADE. SQLite cannot alter constraints in place, so two later migrations rebuilt the table: 1542044660604 (composite primary key) and 1543494409127 (value column type). Both rebuilds recreated the table without any foreign key, and the second one is the final state. Since then, on SQLite (the default database type), deleting a term or a project locale does not cascade and leaves orphaned translation rows behind. That is what produced the 404 described here. #493 filters the orphans at read time, but they keep accumulating on every deletion. MySQL and Postgres are not affected, their migration branches never dropped the constraints.
Fix
A new migration rebuilds the translation table on SQLite with the two original ON DELETE CASCADE foreign keys and the existing indexes. The copy keeps only the rows whose term and project locale still exist, which purges the orphans accumulated while the constraints were missing. MySQL and Postgres branches are a no op.
Tests
A new e2e test creates a term, adds a locale, sets a translation value, deletes the term and asserts that no translation row remains. Without the new migration this test fails with one orphaned row, which reproduces the issue on develop. The full e2e suite (116 tests) and the unit suite (41 tests) pass. I also replayed the migration SQL against a database in the legacy state containing both orphan kinds, by term and by locale: the copy keeps only the valid rows, PRAGMA foreign_key_list shows both constraints restored, and a term deletion then cascades as expected.
Summary by cubic
Restores ON DELETE CASCADE foreign keys on SQLite
translationso deleting terms or project locales cascades and no orphans remain. Fixes #492; adds a migration that rebuilds the table and purges invalid rows. MySQL/Postgres are unaffected.Bug Fixes
translationwith foreign keys toterm(id)andproject_locale(id).label_translations_translation, then removes invalid links.Migration
Written for commit 474812d. Summary will update on new commits.