fix(core): Prevent duplicate translations from concurrent saves - #5007
fix(core): Prevent duplicate translations from concurrent saves#5007brmk wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTranslation entities now enforce uniqueness for each base entity and language. A migration utility removes pre-existing duplicate rows while retaining the newest record. Translation save logic detects unique-constraint conflicts caused by concurrent inserts, reloads the existing translation, and continues saving. A product end-to-end test covers simultaneous updates adding the same translation language and verifies that only one row remains. Suggested reviewers: 🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/service/helpers/translatable-saver/translation-differ.ts (1)
71-78: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPrevent PostgreSQL from aborting the parent transaction on constraint violation.
In PostgreSQL, when a query throws an error (such as a unique constraint violation), the entire transaction enters an aborted state. Any subsequent queries within that same transaction block (like the
findOneandsavecalls in thiscatchblock) will automatically fail with the error:current transaction is aborted, commands ignored until end of transaction block.Because mutations in Vendure are typically wrapped in an active transaction (and the query runner is attached to
ctx), this concurrency fallback logic will always fail on PostgreSQL.To handle this safely without aborting the parent transaction, you should wrap the initial insert attempt in a savepoint (nested transaction). If the constraint violation occurs, only the savepoint is rolled back, leaving the parent transaction healthy to execute the fallback queries in the
catchblock. Based on learnings, Vendure natively supports nested savepoint transactions viaconnection.withTransaction().🔒️ Proposed fix to use a savepoint for the insert
- try { - newTranslation = await this.connection - .getRepository(ctx, this.translationCtor) - .save(translation as any); - } catch (err: any) { + try { + // Wrap in a savepoint to prevent Postgres from aborting the parent transaction + // on a unique constraint violation. + newTranslation = await this.connection.withTransaction(ctx, async ctxWithSavepoint => { + return await this.connection + .getRepository(ctxWithSavepoint, this.translationCtor) + .save(translation as any); + }); + } catch (err: any) {🤖 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 `@packages/core/src/service/helpers/translatable-saver/translation-differ.ts` around lines 71 - 78, Wrap the initial translation save attempt in the nested savepoint transaction provided by connection.withTransaction(), using the existing ctx so only the savepoint rolls back on a unique constraint violation. Keep the fallback logic in the surrounding catch block, allowing its findOne and save operations to run against the still-healthy parent transaction; preserve rethrowing non-unique errors as InternalServerError.Source: Learnings
🤖 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.
Outside diff comments:
In `@packages/core/src/service/helpers/translatable-saver/translation-differ.ts`:
- Around line 71-78: Wrap the initial translation save attempt in the nested
savepoint transaction provided by connection.withTransaction(), using the
existing ctx so only the savepoint rolls back on a unique constraint violation.
Keep the fallback logic in the surrounding catch block, allowing its findOne and
save operations to run against the still-healthy parent transaction; preserve
rethrowing non-unique errors as InternalServerError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d07c1e79-7c3b-4bac-9f8f-2fea83e0cd4e
📒 Files selected for processing (18)
packages/core/e2e/product.e2e-spec.tspackages/core/src/entity/api-key/api-key-translation.entity.tspackages/core/src/entity/asset/asset-translation.entity.tspackages/core/src/entity/collection/collection-translation.entity.tspackages/core/src/entity/facet-value/facet-value-translation.entity.tspackages/core/src/entity/facet/facet-translation.entity.tspackages/core/src/entity/payment-method/payment-method-translation.entity.tspackages/core/src/entity/product-option-group/product-option-group-translation.entity.tspackages/core/src/entity/product-option/product-option-translation.entity.tspackages/core/src/entity/product-variant/product-variant-translation.entity.tspackages/core/src/entity/product/product-translation.entity.tspackages/core/src/entity/promotion/promotion-translation.entity.tspackages/core/src/entity/region/region-translation.entity.tspackages/core/src/entity/shipping-method/shipping-method-translation.entity.tspackages/core/src/migration-utils/index.tspackages/core/src/migration-utils/translation-deduplication.tspackages/core/src/service/helpers/translatable-saver/translation-differ.tspackages/core/src/service/helpers/utils/db-errors.ts
…re enforcing uniqueness
d5a4568 to
9510965
Compare
michaelbromley
added
the
T3: Systemic
|
T3: Systemic — resolves an open question by picking one interpretation (pattern 7).
|
2 participants
Description
Translatable entities (
Product,Collection,ProductVariant, etc.) could end up with two translation rows for the same(baseId, languageCode)pair.TranslatableSaver.update()loads existing translations, thenTranslationDifferinserts any language not already present — with no locking and no unique constraint. Two concurrent updates that both add the same new language (e.g. a double-click on Update in the Admin UI) can both read "noenrow yet" before either commits, and both insert one. Once that happens, the entity becomes uneditable through the Admin UI.This PR:
@Unique(['languageCode', 'base'])to all 13 translation entities, so the database itself now rejects a second row for the same(baseId, languageCode)pair.isUniqueConstraintViolationError()todb-errors.ts(Postgres/MySQL/SQLite), mirroring the existingisForeignKeyViolationError().TranslationDiffer.applyDiff()so that when an insert hits this constraint (a concurrent request already inserted the row), it re-fetches that row and updates it instead of throwing a 500 — so a losing concurrent save converges instead of leaving the entity stuck.deduplicateTranslations()tomigration-utils, a helper that accepts a single table name or an array of table names, which users can call from their own migration'sup()(before the new unique constraint is created) to remove any duplicate rows that already exist in their database. It also works for any custom translatable entity a plugin defines, not just the 13 core ones.updateProductcalls adding the same new language) and asserting only one translation row results.Fixes #4884
Breaking changes
Adds a unique index to the 13
*_translationtables. This only affects databases where duplicate rows already exist for the same(baseId, languageCode)— in that case, runningvendure migrate generatewill produce a migration that fails onup()until the duplicates are removed. No changes to any public API surface otherwise (deduplicateTranslationsis a new, additive export).Migration guide for users
Since existing production databases may already contain the duplicate rows this PR guards against, upgrading needs one extra manual step — this should be called out in the release notes:
@vendure/coreand runvendure migrate generate <name>as usual. If your database has no duplicates, the generated migration only adds the new constraints and needs no further changes.up()), open the generated migration file and add adeduplicateTranslations(queryRunner, [...])call at the very top ofup(), listing every*_translationtable touched by that migration (see the JSDoc example ondeduplicateTranslationsinmigration-utils/translation-deduplication.tsfor a full worked Postgres example with the exact, deterministic constraint names TypeORM will generate). If you have your own plugins with custom translatable entities, include their translation tables in that same array.deduplicateTranslationskeeps the most recently updated row per(baseId, languageCode)and removes the rest, so no translation content is lost — only the redundant duplicate rows are dropped.Worth adding to the upgrade guide / release notes so users hitting the failed-migration case aren't stuck guessing what to do.
Checklist
📌 Always:
👍 Most of the time:
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.