From 29c311ab318f7e443d696baf871b8a0558fa8524 Mon Sep 17 00:00:00 2001 From: Guilhem Date: Mon, 24 Aug 2026 22:35:21 +0200 Subject: [PATCH] fix: qualify foreign key targets in generated datatable migrations (#10821) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: qualify foreign key targets in generated datatable migrations The schema API reports a foreign key's target as a bare table name when it lives in the same schema as the table declaring it. Emitted verbatim that becomes `REFERENCES tickets (id)`, which Postgres resolves against search_path — and a migration's own schema is never on it, so applying it fails with `relation "tickets" does not exist` and the whole transaction rolls back. Nothing is created; the project imports with no tables. qualifyFkTarget resolves the target the way the FK closure does: the declaring table's schema first, then any schema holding that table. The REFERENCES clause is now quoted per part, so a qualified target survives identifiers that need quoting; the constraint name is still built from the unquoted value, so the pg_constraint guard still matches what it creates. `quoteTarget` is opt-in, so alterTable.ts — the only other caller of renderForeignKey — is byte-identical. Reproduced and verified against a real data table: hub.windmill.dev's published helpdesk migration fails as above, and the same SQL with the target qualified creates both tables and the constraint. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Xg8vXUuHCH3aRkf91sfxjx * fix: resolve bare foreign key targets in the declaring schema Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Wq66AbLqo4c5ukWJSc4x4t --------- Co-authored-by: Claude Opus 5 (1M context) --- .../display/dbtable/queries/dbQueriesUtils.ts | 19 +++- .../src/lib/components/datatableSchemaSql.ts | 24 ++++- .../projectMigrations.test.ts | 91 ++++++++++++++++++- .../workspaceSettings/projectMigrations.ts | 16 +++- 4 files changed, 142 insertions(+), 8 deletions(-) diff --git a/frontend/src/lib/components/apps/components/display/dbtable/queries/dbQueriesUtils.ts b/frontend/src/lib/components/apps/components/display/dbtable/queries/dbQueriesUtils.ts index 711fb7ed38..7ad28248d2 100644 --- a/frontend/src/lib/components/apps/components/display/dbtable/queries/dbQueriesUtils.ts +++ b/frontend/src/lib/components/apps/components/display/dbtable/queries/dbQueriesUtils.ts @@ -1,5 +1,6 @@ import type { DbType } from '$lib/components/dbTypes' import type { TableEditorForeignKey, TableEditorValuesColumn } from '../tableEditor' +import { renderDbQuotedIdentifier } from '../utils' export function formatDefaultValue(str: string, datatype: string, resourceType: DbType): string { if (!str) return '' @@ -33,6 +34,13 @@ export function renderForeignKey( useSchema: boolean dbType: DbType tableName: string + /** + * Table to name in the REFERENCES clause, quoted per dot-separated part so a + * schema-qualified target survives identifiers that need quoting. The constraint + * name stays derived from `fk.targetTable`, so qualifying a target here never + * renames a constraint an earlier migration created under the bare name. + */ + qualifiedTarget?: string } ): string { const sourceColumns = fk.columns.map((c) => c.sourceColumn).filter(Boolean) @@ -53,9 +61,16 @@ export function renderForeignKey( .join('_') .replaceAll('.', '_')} `.substring(0, 60) - sql += ` FOREIGN KEY (${sourceColumns.join( + const targetRef = options.qualifiedTarget + ? options.qualifiedTarget + .split('.') + .map((part) => renderDbQuotedIdentifier(part, options.dbType)) + .join('.') + : targetTable + + sql += ` FOREIGN KEY (${sourceColumns.join(', ')}) REFERENCES ${targetRef} (${targetColumns.join( ', ' - )}) REFERENCES ${targetTable} (${targetColumns.join(', ')})` + )})` if (fk.onDelete !== 'NO ACTION') sql += ` ON DELETE ${fk.onDelete}` if (fk.onUpdate !== 'NO ACTION') sql += ` ON UPDATE ${fk.onUpdate}` return sql diff --git a/frontend/src/lib/components/datatableSchemaSql.ts b/frontend/src/lib/components/datatableSchemaSql.ts index cd3d0f9154..3ba818fbd5 100644 --- a/frontend/src/lib/components/datatableSchemaSql.ts +++ b/frontend/src/lib/components/datatableSchemaSql.ts @@ -146,6 +146,27 @@ function resolveColumnType(c: TableEditorValuesColumn): { return { datatype: c.datatype, defaultValue: c.defaultValue } } +/** + * The schema API reports a foreign key's target as a bare table name whenever it + * lives in the same schema as the table declaring it. Emitting that verbatim + * produces `REFERENCES links (id)`, which Postgres resolves against `search_path` + * — so a migration that just created `"bitly"."links"` fails with `relation + * "links" does not exist`. Qualify it: the declaring table's own schema first, + * then any schema that has the table, matching how the FK closure resolves it. + */ +function qualifyFkTarget( + sourceSchema: DatabaseSchema, + targetTable: string | undefined, + declaringSchema: string +): string | undefined { + if (!targetTable || targetTable.includes('.')) return targetTable + if (sourceSchema[declaringSchema]?.[targetTable]) return `${declaringSchema}.${targetTable}` + for (const schemaName of Object.keys(sourceSchema)) { + if (sourceSchema[schemaName]?.[targetTable]) return `${schemaName}.${targetTable}` + } + return targetTable +} + /** * SQL for an added table, with the CREATE TABLE and the FK constraints split so * callers creating several tables can emit every CREATE before any constraint — @@ -180,7 +201,8 @@ export function generateAddedTableSql( const fkSql = renderForeignKey(fk, { useSchema: true, dbType: 'postgresql', - tableName: change.tableName + tableName: change.tableName, + qualifiedTarget: qualifyFkTarget(sourceSchema, fk.targetTable, change.schemaName) }) // With IF NOT EXISTS the table may pre-exist with this FK already in // place; an unconditional ADD would then abort the whole transaction. diff --git a/frontend/src/lib/components/workspaceSettings/projectMigrations.test.ts b/frontend/src/lib/components/workspaceSettings/projectMigrations.test.ts index 7d35c51650..a134ad3448 100644 --- a/frontend/src/lib/components/workspaceSettings/projectMigrations.test.ts +++ b/frontend/src/lib/components/workspaceSettings/projectMigrations.test.ts @@ -184,7 +184,7 @@ describe('generateDatatableMigrations', () => { expect(migrations[0].sql).toContain('"public"."customers"') }) - it('leaves a qualified ref unresolved when its schema misses, never another schema\'s table', async () => { + it("leaves a qualified ref unresolved when its schema misses, never another schema's table", async () => { getDatatableFullSchemaMock.mockResolvedValue(schema) const usage = new Map([['main', new Set(['sales.orders'])]]) const migrations = await generateDatatableMigrations('ws', usage) @@ -275,6 +275,95 @@ describe('generateDatatableMigrations', () => { expect(sql).not.toContain('CREATE SCHEMA IF NOT EXISTS "public"') }) + it('qualifies an FK target the schema reports bare, so it resolves outside search_path', async () => { + // The schema API omits the schema on an FK target that lives in the same schema + // as the table declaring it. Emitted verbatim it becomes `REFERENCES links (id)`, + // which Postgres looks up on `search_path` — and the migration's own schema is + // never on it, so applying it fails with `relation "links" does not exist`. + const bitlySchema = { + bitly: { + links: { + name: 'links', + columns: [{ name: 'id', datatype: 'uuid', primary_key: true, nullable: false }], + foreign_keys: [] + }, + clicks: { + name: 'clicks', + columns: [ + { name: 'id', datatype: 'uuid', primary_key: true, nullable: false }, + { name: 'link_id', datatype: 'uuid', nullable: false } + ], + foreign_keys: [ + { + target_table: 'links', + columns: [{ source_column: 'link_id', target_column: 'id' }], + on_delete: 'CASCADE', + on_update: 'NO ACTION' + } + ] + } + } + } + getDatatableFullSchemaMock.mockResolvedValue(bitlySchema) + const usage = new Map([['main', new Set(['bitly.clicks'])]]) + const migrations = await generateDatatableMigrations('ws', usage) + const sql = migrations[0].sql + expect(sql).toContain('REFERENCES "bitly"."links" (id)') + expect(sql).not.toMatch(/REFERENCES links\b/) + // The guard skips the ALTER when the constraint already exists, so the name it + // looks up has to be the name the ALTER creates. + const created = sql.match(/ADD CONSTRAINT (\S+)/)?.[1] + expect(sql).toContain(`conname = '${created}'`) + // The name comes from the bare target, not the qualified one: a table created by + // a migration generated before this fix carries the bare-target name, and the + // guard has to keep matching it instead of adding a second, identical FK. + expect(created).toBe('fk_clicks_link_id_links_id') + }) + + it('resolves a bare FK target in the declaring schema, not a same-named table elsewhere', async () => { + // `public` comes first, so a first-match-across-schemas resolution creates + // `public.links` and points the FK at it — a silently wrong reference, and + // `bitly.links` never gets created. + const sameNameTwoSchemas = { + public: { + links: { + name: 'links', + columns: [{ name: 'id', datatype: 'integer', primary_key: true, nullable: false }], + foreign_keys: [] + } + }, + bitly: { + links: { + name: 'links', + columns: [{ name: 'id', datatype: 'uuid', primary_key: true, nullable: false }], + foreign_keys: [] + }, + clicks: { + name: 'clicks', + columns: [ + { name: 'id', datatype: 'uuid', primary_key: true, nullable: false }, + { name: 'link_id', datatype: 'uuid', nullable: false } + ], + foreign_keys: [ + { + target_table: 'links', + columns: [{ source_column: 'link_id', target_column: 'id' }], + on_delete: 'CASCADE', + on_update: 'NO ACTION' + } + ] + } + } + } + getDatatableFullSchemaMock.mockResolvedValue(sameNameTwoSchemas) + const usage = new Map([['main', new Set(['bitly.clicks'])]]) + const migrations = await generateDatatableMigrations('ws', usage) + const sql = migrations[0].sql + expect(sql).toContain('REFERENCES "bitly"."links" (id)') + expect(sql).toContain('"bitly"."links"') + expect(sql).not.toContain('"public"."links"') + }) + it('keeps same-named tables from different schemas both created', async () => { const twoSchemas = { public: { diff --git a/frontend/src/lib/components/workspaceSettings/projectMigrations.ts b/frontend/src/lib/components/workspaceSettings/projectMigrations.ts index 8ff4da597d..ecdacba989 100644 --- a/frontend/src/lib/components/workspaceSettings/projectMigrations.ts +++ b/frontend/src/lib/components/workspaceSettings/projectMigrations.ts @@ -130,7 +130,8 @@ export async function detectDatatableTables( // still references the missing one. function resolveTable( schema: DatabaseSchema, - tableRef: string + tableRef: string, + declaringSchema?: string ): { schemaName: string; tableName: string } | undefined { const dot = tableRef.indexOf('.') if (dot !== -1) { @@ -138,6 +139,13 @@ function resolveTable( const tableName = tableRef.slice(dot + 1) return schema[schemaName]?.[tableName] ? { schemaName, tableName } : undefined } + // A bare foreign-key target means a table in the declaring table's own schema — + // that is the only case the schema API drops the schema for — so resolve there + // first. Without it a same-named table in another schema wins, and the closure + // creates that one while the FK points at it. + if (declaringSchema && schema[declaringSchema]?.[tableRef]) { + return { schemaName: declaringSchema, tableName: tableRef } + } // Bare name: find it across every schema, first match wins. for (const schemaName of Object.keys(schema)) { if (schema[schemaName][tableRef]) return { schemaName, tableName: tableRef } @@ -161,7 +169,7 @@ function expandFkClosure(schema: DatabaseSchema, seed: ResolvedTable[]): Resolve const t = queue.shift()! const fks = schema[t.schemaName]?.[t.tableName]?.foreignKeys ?? [] for (const fk of fks) { - const target = resolveTable(schema, fk.targetTable ?? '') + const target = resolveTable(schema, fk.targetTable ?? '', t.schemaName) if (target && !inSet.has(tableKey(target))) { inSet.set(tableKey(target), target) queue.push(target) @@ -184,7 +192,7 @@ function pruneSchemaForTables(schema: DatabaseSchema, tables: ResolvedTable[]): ;(pruned[t.schemaName] ??= {})[t.tableName] = { ...orig, foreignKeys: (orig.foreignKeys ?? []).filter((fk) => { - const target = resolveTable(schema, fk.targetTable ?? '') + const target = resolveTable(schema, fk.targetTable ?? '', t.schemaName) return target != null && inSet.has(tableKey(target)) }) } @@ -203,7 +211,7 @@ function orderByFkDependency(schema: DatabaseSchema, tables: ResolvedTable[]): R const fks = schema[t.schemaName]?.[t.tableName]?.foreignKeys ?? [] const targets = new Set() for (const fk of fks) { - const target = resolveTable(schema, fk.targetTable ?? '') + const target = resolveTable(schema, fk.targetTable ?? '', t.schemaName) if (target && tableKey(target) !== tableKey(t) && inSet.has(tableKey(target))) { targets.add(tableKey(target)) }