mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-27 00:02:35 +00:00
fix: qualify foreign key targets in generated datatable migrations (#10821)
* 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) <noreply@anthropic.com> 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wq66AbLqo4c5ukWJSc4x4t --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+17
-2
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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: {
|
||||
|
||||
@@ -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<string>()
|
||||
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))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user