0baa333809
## Why Fast instance commands run in the ArgoCD **PreSync** hook, before the new pods roll. A bulk `UPDATE`/`INSERT`/`DELETE` held in the **same transaction** as an `ADD COLUMN`/`ALTER` keeps an `ACCESS EXCLUSIVE` lock on the table for the whole write, blocking every read of it. That is what froze prod during the 2.13 `isUIReadOnly → isUIEditable` rename — a bulk `UPDATE "fieldMetadata"` inside the same `up()` transaction as the `ADD COLUMN`s → read timeouts → failed PreSync → aborted sync. @charlesBochet already caught this exact pattern by hand on #21527 ("data migration => make a slow instance command :)"). This turns that manual review into something CI enforces. ## What New oxlint rule **`twenty/no-data-mutation-in-fast-instance-command`**: - Flags statement-leading `UPDATE`/`INSERT`/`DELETE`/`MERGE` passed to `.query(...)` **inside `up()`** of a `*-instance-command-fast-*` file. - Allows: schema DDL (`ALTER`/`CREATE`/`DROP`); `ON DELETE CASCADE` / a column named `updatedAt` (not statement-leading, so never matched); rollback DML in `down()`; and data migrations in **slow** commands' `runDataMigration()`. - The error message points the author straight at the slow-command pattern. Enabled as `error` in `twenty-server`. ## Grandfathering Scoping to `up()` means **only one** existing file violates the rule: the already-shipped 2.13 rename command. It's recorded complete in cloud and must not be rewritten, so it's grandfathered with a documented file-level `oxlint-disable` (the comment makes clear it's an exception, not a precedent). The four other fast commands that contain DML keep theirs in `down()` and are correctly unaffected. ## Tests - 9 RuleTester cases — valid: DDL, FK cascade, `updatedAt`, `down()` DML, slow-command DML, non-upgrade files; invalid: `UPDATE`/`INSERT`/`DELETE` in `up()`. - Verified end-to-end with oxlint: a throwaway violating file → 1 error; all 141 upgrade-command files → 0 errors; full oxlint-rules suite 225/225; typecheck clean. Part of the v2.13 deploy post-mortem follow-ups. https://claude.ai/code/session_013Az1etaGyxWRRVhgjhPWeB --- _Generated by [Claude Code](https://claude.ai/code/session_013Az1etaGyxWRRVhgjhPWeB)_ <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/twentyhq/twenty/pull/21547?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> --------- Co-authored-by: Claude <noreply@anthropic.com>
103 lines
4.3 KiB
TypeScript
103 lines
4.3 KiB
TypeScript
import { RuleTester } from 'oxlint/plugins-dev';
|
|
|
|
import { rule, RULE_NAME } from './no-data-mutation-in-fast-instance-command';
|
|
|
|
const ruleTester = new RuleTester();
|
|
|
|
const BASE =
|
|
'/project/packages/twenty-server/src/database/commands/upgrade-version-command';
|
|
|
|
const FAST_FILE = `${BASE}/2-13/2-13-instance-command-fast-1781277453604-rename-flag.ts`;
|
|
const SLOW_FILE = `${BASE}/2-13/2-13-instance-command-slow-1781277480000-backfill-flag.ts`;
|
|
|
|
ruleTester.run(RULE_NAME, rule, {
|
|
valid: [
|
|
// Schema changes are exactly what a fast command is for.
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async up(q) { await q.query('ALTER TABLE "core"."x" ADD COLUMN IF NOT EXISTS "y" boolean NOT NULL DEFAULT true'); } }`,
|
|
},
|
|
// A foreign key clause contains DELETE/UPDATE but is not statement-leading DML.
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async up(q) { await q.query('ALTER TABLE "core"."a" ADD CONSTRAINT "fk" FOREIGN KEY ("bId") REFERENCES "core"."b"("id") ON DELETE CASCADE ON UPDATE NO ACTION'); } }`,
|
|
},
|
|
// A column named "updatedAt" must not trip the UPDATE keyword.
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async up(q) { await q.query('ALTER TABLE "core"."x" ADD COLUMN "updatedAt" timestamptz'); } }`,
|
|
},
|
|
// A read-only CTE has no data mutation.
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async up(q) { await q.query(\`WITH recent AS (SELECT "id" FROM "core"."x") SELECT count(*) FROM recent\`); } }`,
|
|
},
|
|
// A read-only CTE that only mentions a keyword in a string is not flagged.
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async up(q) { await q.query(\`WITH note AS (SELECT 'needs update later' AS msg) SELECT msg FROM note\`); } }`,
|
|
},
|
|
// A keyword after a parenthesis inside a string literal is not flagged.
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async up(q) { await q.query(\`WITH note AS (SELECT 'reminder: (UPDATE later)' AS msg) SELECT msg FROM note\`); } }`,
|
|
},
|
|
// Rollback DML lives in down() and is allowed (incl. via a CTE).
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async down(q) { await q.query(\`UPDATE "core"."x" SET "y" = false WHERE "z" = true\`); } }`,
|
|
},
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async down(q) { await q.query(\`WITH ids AS (SELECT "id" FROM "core"."x") UPDATE "core"."x" SET "y" = true\`); } }`,
|
|
},
|
|
// down() written as an arrow-function property is also recognized.
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { down = async (q) => { await q.query('DELETE FROM "core"."x" WHERE 1 = 1'); }; }`,
|
|
},
|
|
// A slow command is the correct home for a data migration.
|
|
{
|
|
filename: SLOW_FILE,
|
|
code: `class C { async runDataMigration(ds) { await ds.query(\`UPDATE "core"."x" SET "y" = false\`); } }`,
|
|
},
|
|
// Files outside the upgrade-command tree are ignored.
|
|
{
|
|
filename: '/project/packages/twenty-front/src/foo.ts',
|
|
code: `class C { async up(q) { await q.query(\`UPDATE "x" SET "y" = 1\`); } }`,
|
|
},
|
|
],
|
|
invalid: [
|
|
// The incident pattern: bulk UPDATE in up().
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async up(q) { await q.query(\`UPDATE "core"."fieldMetadata" SET "isUIEditable" = false WHERE "isUIReadOnly" = true\`); } }`,
|
|
errors: [{ messageId: 'dataMutationInFastInstanceCommand' }],
|
|
},
|
|
// INSERT in up().
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async up(q) { await q.query('INSERT INTO "core"."x" ("id") VALUES (1)'); } }`,
|
|
errors: [{ messageId: 'dataMutationInFastInstanceCommand' }],
|
|
},
|
|
// DELETE in up().
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async up(q) { await q.query('DELETE FROM "core"."x" WHERE "t" = 1'); } }`,
|
|
errors: [{ messageId: 'dataMutationInFastInstanceCommand' }],
|
|
},
|
|
// Data mutation moved into a helper called by up() is still forbidden.
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async backfill(q) { await q.query('DELETE FROM "core"."x" WHERE 1 = 1'); } }`,
|
|
errors: [{ messageId: 'dataMutationInFastInstanceCommand' }],
|
|
},
|
|
// CTE-wrapped data mutation in up().
|
|
{
|
|
filename: FAST_FILE,
|
|
code: `class C { async up(q) { await q.query(\`WITH ids AS (SELECT "id" FROM "core"."x") UPDATE "core"."x" SET "y" = false\`); } }`,
|
|
errors: [{ messageId: 'dataMutationInFastInstanceCommand' }],
|
|
},
|
|
],
|
|
});
|