From c880d0c088ac124a0c2c5a861f8fd9cf09b74db2 Mon Sep 17 00:00:00 2001 From: Fela Maslen Date: Wed, 1 Apr 2026 16:33:38 +0100 Subject: [PATCH] fix: use key-order-independent comparison in `equals` methods `Inspected.equals()`, `ColumnInfo.equals()`, and `PostgreSQL.equals()` all used `JSON.stringify` for comparison, which is sensitive to object key order. When two databases have the same table columns in different physical order, the `columns` object serialises differently, making the table appear modified. This cascades through `add_dependents_for_modified` to spuriously drop and recreate dependent views, even when the views themselves are identical. Replace `JSON.stringify` comparisons with the existing `isEqual` utility, which sorts keys before comparing. --- packages/migra/test/column-order.test.ts | 133 +++++++++++++++++++++++ packages/schemainspect/src/inspected.ts | 5 +- packages/schemainspect/src/pg/obj.ts | 2 +- 3 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 packages/migra/test/column-order.test.ts diff --git a/packages/migra/test/column-order.test.ts b/packages/migra/test/column-order.test.ts new file mode 100644 index 00000000..bf90d087 --- /dev/null +++ b/packages/migra/test/column-order.test.ts @@ -0,0 +1,133 @@ +import {createPool, sql} from '@pgkit/client' +import {beforeAll, afterAll, beforeEach, describe, expect, test} from 'vitest' +import {run as runMigra} from '../src/command' + +const connectionString = process.env.MIGRA_TEST_URL ?? 'postgresql://postgres:postgres@localhost:5432/postgres' + +let admin: Awaited> + +beforeAll(async () => { + admin = createPool(connectionString) +}) + +afterAll(async () => { + await admin.pgp.$pool.end() +}) + +async function setupDb(name: string, setupSql: string) { + await admin.query(sql`drop database if exists ${sql.identifier([name])}`) + await admin.query(sql`create database ${sql.identifier([name])}`) + const url = connectionString.replace(/\/[^/]+$/, `/${name}`) + const pool = createPool(url) + await pool.query(sql.raw(setupSql)) + await pool.pgp.$pool.end() + return url +} + +async function cleanupDb(name: string) { + await admin.query(sql` + select pg_terminate_backend(pid) + from pg_stat_activity + where datname = ${name} and pid <> pg_backend_pid() + `) + await admin.query(sql`drop database if exists ${sql.identifier([name])}`) +} + +describe('column order should not cause spurious view diffs', () => { + const dbA = 'migra_colorder_test_a' + const dbB = 'migra_colorder_test_b' + + beforeEach(async () => { + await cleanupDb(dbA) + await cleanupDb(dbB) + }) + + test('view with explicit columns is not recreated when table column order differs', async () => { + const a = await setupDb( + dbA, + ` + create table users ( + id serial primary key, + name text not null, + email text not null + ); + create view user_emails as select id, email from users; + `, + ) + const b = await setupDb( + dbB, + ` + create table users ( + id serial primary key, + email text not null, + name text not null + ); + create view user_emails as select id, email from users; + `, + ) + + const result = await runMigra(a, b, {unsafe: true}) + expect(result.sql.trim()).toBe('') + }) + + test('view with SELECT * is recreated when table column order differs', async () => { + const a = await setupDb( + dbA, + ` + create table users ( + id serial primary key, + name text not null, + email text not null + ); + create view all_users as select * from users; + `, + ) + const b = await setupDb( + dbB, + ` + create table users ( + id serial primary key, + email text not null, + name text not null + ); + create view all_users as select * from users; + `, + ) + + const result = await runMigra(a, b, {unsafe: true}) + expect(result.sql).toContain('drop view') + expect(result.sql).toContain('create or replace view') + }) + + test('view with explicit columns is not recreated even with many reordered columns', async () => { + const a = await setupDb( + dbA, + ` + create table items ( + id serial primary key, + alpha text, + beta text, + gamma text, + delta text + ); + create view item_summary as select id, delta, alpha from items; + `, + ) + const b = await setupDb( + dbB, + ` + create table items ( + id serial primary key, + delta text, + gamma text, + beta text, + alpha text + ); + create view item_summary as select id, delta, alpha from items; + `, + ) + + const result = await runMigra(a, b, {unsafe: true}) + expect(result.sql.trim()).toBe('') + }) +}) diff --git a/packages/schemainspect/src/inspected.ts b/packages/schemainspect/src/inspected.ts index 38155d70..2c99e8c8 100644 --- a/packages/schemainspect/src/inspected.ts +++ b/packages/schemainspect/src/inspected.ts @@ -4,6 +4,7 @@ import {AutoThisAssigner, SomeOptional, SomeRequired} from './auto-this' import {AutoRepr, quoted_identifier, quotify, unquoted_identifier} from './misc' import {InspectedConstraint, InspectedEnum, InspectedIndex} from './pg' import {Relationtype} from './types' +import {isEqual} from './util' export const pick = (obj: T, keys: readonly K[]): Pick => { return Object.fromEntries(keys.map(k => [k, obj[k]])) as Pick @@ -19,7 +20,7 @@ export abstract class Inspected extends AutoRepr { abstract get create_statement(): string equals(other: Inspected): boolean { - return JSON.stringify(this) === JSON.stringify(other) + return isEqual(this.toJSON(), other.toJSON()) } get quoted_full_name() { @@ -87,7 +88,7 @@ export class ColumnInfo extends AutoThisAssigner