diff --git a/docs/docs/5-migrations.md b/docs/docs/5-migrations.md index 27fe3b8..2d53bc2 100644 --- a/docs/docs/5-migrations.md +++ b/docs/docs/5-migrations.md @@ -37,6 +37,10 @@ If an entity defines [constraints](./2-models.md#constraints) with `kind: 'check Constraint names in the database follow the pattern `{table}_{constraintName}_check_{index}`. +When comparing existing check constraints, `graphql-magic` asks PostgreSQL to canonicalize expressions before comparing them. This avoids churn for semantically equivalent expressions that differ in formatting or identifier quoting. + +If canonicalization fails (for example due invalid syntax or permission/DDL limitations), `graphql-magic` logs a warning and conservatively treats the constraint as changed, so `check-needs-migration` reports that a migration is needed. + ## Running migrations Migrations themselves are managed with `knex` (see the [knex migration docs](https://knexjs.org/guide/migrations.html)). diff --git a/src/migrations/generate.ts b/src/migrations/generate.ts index 6c83288..62ed1a8 100644 --- a/src/migrations/generate.ts +++ b/src/migrations/generate.ts @@ -237,7 +237,6 @@ export class MigrationGenerator { this.updateFields(model, existingFields, up, down); if (model.constraints?.length) { - const existingMap = this.existingCheckConstraints[model.name]; for (let i = 0; i < model.constraints.length; i++) { const entry = model.constraints[i]; if (entry.kind !== 'check') { @@ -246,25 +245,29 @@ export class MigrationGenerator { validateCheckConstraint(model, entry); const table = model.name; const constraintName = this.getCheckConstraintName(model, entry, i); - const newExpression = entry.expression; - const existingExpression = existingMap?.get(constraintName); - if (existingExpression === undefined) { + const existingConstraint = this.findExistingConstraint(table, entry, constraintName); + if (!existingConstraint) { up.push(() => { - this.addCheckConstraint(table, constraintName, newExpression); + this.addCheckConstraint(table, constraintName, entry.expression); }); down.push(() => { this.dropCheckConstraint(table, constraintName); }); } else if ( - this.normalizeCheckExpression(existingExpression) !== this.normalizeCheckExpression(newExpression) + !(await this.equalExpressions( + table, + existingConstraint.constraintName, + existingConstraint.expression, + entry.expression, + )) ) { up.push(() => { - this.dropCheckConstraint(table, constraintName); - this.addCheckConstraint(table, constraintName, newExpression); + this.dropCheckConstraint(table, existingConstraint.constraintName); + this.addCheckConstraint(table, constraintName, entry.expression); }); down.push(() => { this.dropCheckConstraint(table, constraintName); - this.addCheckConstraint(table, constraintName, existingExpression); + this.addCheckConstraint(table, existingConstraint.constraintName, existingConstraint.expression); }); } } @@ -763,7 +766,182 @@ export class MigrationGenerator { } private normalizeCheckExpression(expr: string): string { - return expr.replace(/\s+/g, ' ').trim(); + let normalized = expr.replace(/\s+/g, ' ').trim(); + while (this.isWrappedByOuterParentheses(normalized)) { + normalized = normalized.slice(1, -1).trim(); + } + + return normalized; + } + + private isWrappedByOuterParentheses(expr: string): boolean { + if (!expr.startsWith('(') || !expr.endsWith(')')) { + return false; + } + + let depth = 0; + let inSingleQuote = false; + for (let i = 0; i < expr.length; i++) { + const char = expr[i]; + const next = expr[i + 1]; + + if (char === "'") { + if (inSingleQuote && next === "'") { + i++; + continue; + } + inSingleQuote = !inSingleQuote; + continue; + } + + if (inSingleQuote) { + continue; + } + + if (char === '(') { + depth++; + } else if (char === ')') { + depth--; + if (depth === 0 && i !== expr.length - 1) { + return false; + } + if (depth < 0) { + return false; + } + } + } + + return depth === 0; + } + + private findExistingConstraint( + table: string, + entry: { kind: 'check'; name: string; expression: string }, + preferredConstraintName: string, + ): { constraintName: string; expression: string } | null { + const existingMap = this.existingCheckConstraints[table]; + if (!existingMap) { + return null; + } + + const preferredExpression = existingMap.get(preferredConstraintName); + if (preferredExpression !== undefined) { + return { + constraintName: preferredConstraintName, + expression: preferredExpression, + }; + } + + const normalizedNewExpression = this.normalizeCheckExpression(entry.expression); + const constraintPrefix = `${table}_${entry.name}_${entry.kind}_`; + + for (const [constraintName, expression] of existingMap.entries()) { + if (!constraintName.startsWith(constraintPrefix)) { + continue; + } + if (this.normalizeCheckExpression(expression) !== normalizedNewExpression) { + continue; + } + + return { constraintName, expression }; + } + + return null; + } + + private async equalExpressions( + table: string, + constraintName: string, + existingExpression: string, + newExpression: string, + ): Promise { + try { + const [canonicalExisting, canonicalNew] = await Promise.all([ + this.canonicalizeCheckExpressionWithPostgres(table, existingExpression), + this.canonicalizeCheckExpressionWithPostgres(table, newExpression), + ]); + + return canonicalExisting === canonicalNew; + } catch (error) { + console.warn( + `Failed to canonicalize check constraint "${constraintName}" on table "${table}". Treating it as changed.`, + error, + ); + + return false; + } + } + + private async canonicalizeCheckExpressionWithPostgres(table: string, expression: string): Promise { + const sourceTableIdentifier = table + .split('.') + .map((part) => this.quoteIdentifier(part)) + .join('.'); + + const uniqueSuffix = `${Date.now()}_${Math.random().toString(36).slice(2, 10)}`; + const tableSlug = table.toLowerCase().replace(/[^a-z0-9_]/g, '_'); + + const tempTableName = `gqm_tmp_check_${tableSlug}_${uniqueSuffix}`; + const tempTableIdentifier = this.quoteIdentifier(tempTableName); + + const constraintName = `gqm_tmp_check_${uniqueSuffix}`; + const constraintIdentifier = this.quoteIdentifier(constraintName); + + const trx = await this.knex.transaction(); + + try { + await trx.raw(`CREATE TEMP TABLE ${tempTableIdentifier} (LIKE ${sourceTableIdentifier}) ON COMMIT DROP`); + await trx.raw(`ALTER TABLE ${tempTableIdentifier} ADD CONSTRAINT ${constraintIdentifier} CHECK (${expression})`); + const result = await trx.raw( + `SELECT pg_get_constraintdef(c.oid, true) AS constraint_definition + FROM pg_constraint c + JOIN pg_class t + ON t.oid = c.conrelid + WHERE t.relname = ? + AND c.conname = ? + ORDER BY c.oid DESC + LIMIT 1`, + [tempTableName, constraintName], + ); + + const rows: { constraint_definition: string }[] = + 'rows' in result && Array.isArray((result as { rows: unknown }).rows) + ? (result as { rows: { constraint_definition: string }[] }).rows + : []; + const definition = rows[0]?.constraint_definition; + if (!definition) { + throw new Error(`Could not read canonical check definition for expression: ${expression}`); + } + + return this.normalizeCheckExpression(this.extractCheckExpressionFromDefinition(definition)); + } finally { + try { + await trx.rollback(); + } catch { + // no-op: transaction may already be closed by driver after failure + } + } + } + + private extractCheckExpressionFromDefinition(definition: string): string { + const trimmed = definition.trim(); + const match = trimmed.match(/^CHECK\s*\(([\s\S]*)\)$/i); + if (!match) { + return trimmed; + } + + return match[1]; + } + + private quoteIdentifier(identifier: string): string { + return `"${identifier.replace(/"/g, '""')}"`; + } + + private quoteQualifiedIdentifier(identifier: string): string { + return identifier + .split('.') + .map((part) => this.quoteIdentifier(part)) + .join('.'); } /** Escape expression for embedding inside a template literal in generated code */ diff --git a/tests/unit/migration-constraints.spec.ts b/tests/unit/migration-constraints.spec.ts new file mode 100644 index 0000000..5e10d44 --- /dev/null +++ b/tests/unit/migration-constraints.spec.ts @@ -0,0 +1,300 @@ +import { MigrationGenerator } from '../../src/migrations/generate'; +import { ModelDefinitions, Models } from '../../src/models'; + +jest.mock('knex-schema-inspector', () => ({ + SchemaInspector: jest.fn(() => ({})), +})); + +jest.mock('code-block-writer', () => { + const Writer = class { + private output = ''; + + write(value: string) { + this.output += value; + return this; + } + + writeLine(value: string) { + this.output += `${value}\n`; + return this; + } + + blankLine() { + this.output += '\n'; + return this; + } + + newLine() { + this.output += '\n'; + return this; + } + + inlineBlock(fn: () => void) { + this.output += ' {\n'; + fn(); + this.output += '}'; + return this; + } + + block(fn: () => void) { + this.output += ' {\n'; + fn(); + this.output += '}\n'; + return this; + } + + toString() { + return this.output; + } + }; + + return { __esModule: true, default: { default: Writer } }; +}); + +type MockColumn = { + name: string; + data_type: string; + is_nullable: boolean; + generation_expression?: string | null; + numeric_precision?: number | null; + numeric_scale?: number | null; + max_length?: number | null; +}; + +type CheckRow = { + table_name: string; + constraint_name: string; + check_clause: string; +}; + +const normalizeForCanonicalizationMock = (expr: string) => { + let normalized = expr.replace(/\s+/g, ' ').trim(); + while (normalized.startsWith('(') && normalized.endsWith(')')) { + normalized = normalized.slice(1, -1).trim(); + } + + return normalized.replace(/"([a-z_][a-z0-9_]*)"/g, '$1'); +}; + +const baseColumnsByTable: Record = { + Product: [ + { name: 'id', data_type: 'uuid', is_nullable: false }, + { name: 'deleted', data_type: 'boolean', is_nullable: true }, + { name: 'startDate', data_type: 'timestamp without time zone', is_nullable: true }, + { name: 'endDate', data_type: 'timestamp without time zone', is_nullable: true }, + { name: 'score', data_type: 'integer', is_nullable: true }, + ], +}; + +const createProductModels = (constraints: { kind: 'check'; name: string; expression: string }[]) => { + const definitions: ModelDefinitions = [ + { + kind: 'entity', + name: 'Product', + fields: [{ name: 'score', type: 'Int' }], + constraints, + }, + ]; + + return new Models(definitions); +}; + +const createGenerator = (rows: CheckRow[], models: Models) => { + const raw = jest.fn().mockResolvedValue({ rows }); + const knexLike = Object.assign( + jest.fn().mockReturnValue({ + where: jest.fn().mockReturnValue({ + select: jest.fn().mockResolvedValue([]), + }), + }), + { raw }, + ); + + const schema = { + knex: knexLike, + tables: jest.fn().mockResolvedValue(['Product']), + columnInfo: jest.fn(async (table: string) => baseColumnsByTable[table] ?? []), + }; + + const generator = new MigrationGenerator({} as never, models); + (generator as unknown as { schema: unknown }).schema = schema; + + return generator; +}; + +describe('MigrationGenerator check constraints', () => { + beforeEach(() => { + jest + .spyOn(MigrationGenerator.prototype as any, 'canonicalizeCheckExpressionWithPostgres') + .mockImplementation(async (...args: unknown[]) => normalizeForCanonicalizationMock(args[1] as string)); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('does not detect changes for equivalent expressions with extra parentheses', async () => { + const models = createProductModels([{ kind: 'check', name: 'score_non_negative', expression: '"score" >= 0' }]); + const generator = createGenerator( + [ + { + table_name: 'Product', + constraint_name: 'Product_score_non_negative_check_0', + check_clause: ' (( "score" >= 0 )) ', + }, + ], + models, + ); + + await generator.generate(); + + expect(generator.needsMigration).toBe(false); + }); + + it('does not detect changes for equivalent expressions with quoted vs unquoted lowercase identifiers', async () => { + jest + .spyOn(MigrationGenerator.prototype as any, 'canonicalizeCheckExpressionWithPostgres') + .mockImplementation( + async () => '(deleted = true) OR ("endDate" IS NULL) OR ("startDate" <= "endDate")', + ); + const models = new Models([ + { + kind: 'entity', + name: 'Product', + fields: [ + { name: 'deleted', type: 'Boolean' }, + { name: 'startDate', type: 'DateTime' }, + { name: 'endDate', type: 'DateTime' }, + ], + constraints: [ + { + kind: 'check', + name: 'period_start_before_end', + expression: '"deleted" = true OR "endDate" IS NULL OR "startDate" <= "endDate"', + }, + ], + }, + ]); + const generator = createGenerator( + [ + { + table_name: 'Product', + constraint_name: 'Product_period_start_before_end_check_0', + check_clause: '((deleted = true) OR ("endDate" IS NULL) OR ("startDate" <= "endDate"))', + }, + ], + models, + ); + + await generator.generate(); + + expect(generator.needsMigration).toBe(false); + }); + + it('does not detect changes if only generated constraint index changed but expression is identical', async () => { + const models = createProductModels([ + { kind: 'check', name: 'first', expression: '"score" >= 0' }, + { kind: 'check', name: 'second', expression: '"score" <= 100' }, + ]); + const generator = createGenerator( + [ + { + table_name: 'Product', + constraint_name: 'Product_first_check_1', + check_clause: '(("score" >= 0))', + }, + { + table_name: 'Product', + constraint_name: 'Product_second_check_0', + check_clause: '(("score" <= 100))', + }, + ], + models, + ); + + await generator.generate(); + + expect(generator.needsMigration).toBe(false); + }); + + it('still detects actual expression changes', async () => { + const models = createProductModels([{ kind: 'check', name: 'score_non_negative', expression: '"score" >= 0' }]); + const generator = createGenerator( + [ + { + table_name: 'Product', + constraint_name: 'Product_score_non_negative_check_0', + check_clause: '"score" > 0', + }, + ], + models, + ); + + const migration = await generator.generate(); + + expect(generator.needsMigration).toBe(true); + expect(migration).toContain('DROP CONSTRAINT "Product_score_non_negative_check_0"'); + expect(migration).toContain('ADD CONSTRAINT "Product_score_non_negative_check_0" CHECK ("score" >= 0)'); + }); + + it('warns and treats constraint as changed when canonicalization fails', async () => { + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); + jest + .spyOn(MigrationGenerator.prototype as any, 'canonicalizeCheckExpressionWithPostgres') + .mockRejectedValue(new Error('canonicalization failed')); + const models = createProductModels([{ kind: 'check', name: 'score_non_negative', expression: '"score" >= 0' }]); + const generator = createGenerator( + [ + { + table_name: 'Product', + constraint_name: 'Product_score_non_negative_check_0', + check_clause: '"score" >= 0', + }, + ], + models, + ); + + await generator.generate(); + + expect(generator.needsMigration).toBe(true); + expect(warnSpy).toHaveBeenCalled(); + }); +}); + +describe('MigrationGenerator canonicalizeCheckExpressionWithPostgres', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('creates temp table using LIKE source table shape', async () => { + const raw = jest + .fn() + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce({ + rows: [ + { + constraint_definition: 'CHECK (((deleted = true) OR ("endDate" IS NULL) OR ("startDate" <= "endDate")))', + }, + ], + }); + const rollback = jest.fn().mockResolvedValue(undefined); + const trx = { raw, rollback }; + const knexLike = { + transaction: jest.fn().mockResolvedValue(trx), + }; + const generator = new MigrationGenerator(knexLike as never, createProductModels([])); + + const canonical = await (generator as any).canonicalizeCheckExpressionWithPostgres( + 'RevenueSplit', + '((deleted = true) OR ("endDate" IS NULL) OR ("startDate" <= "endDate"))', + ); + + const createTempSql = raw.mock.calls[0]?.[0] as string; + expect(createTempSql).toContain('CREATE TEMP TABLE'); + expect(createTempSql).toContain('(LIKE "RevenueSplit")'); + expect(createTempSql).not.toContain('gqm_check_value'); + expect(canonical).toBe('(deleted = true) OR ("endDate" IS NULL) OR ("startDate" <= "endDate")'); + expect(rollback).toHaveBeenCalled(); + }); +});