From fff855d82c0c5b73b5699abcbec47bf7e734c791 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Thu, 6 Nov 2025 23:42:07 -0800 Subject: [PATCH 1/3] Revert "fix: extension qualifier (#146)" This reverts commit e99d0e59c84ab320ec54f3f07b43b3246bf415a0. --- internal/diff/diff.go | 14 +------ internal/diff/table.go | 33 ++------------- ir/queries/queries.sql | 42 ++++++++++--------- ir/queries/queries.sql.go | 40 +++++++----------- .../add_column_custom_type/diff.sql | 6 +-- .../add_column_custom_type/new.sql | 19 ++++----- .../add_column_custom_type/plan.json | 12 ++---- .../add_column_custom_type/plan.sql | 6 +-- .../add_column_custom_type/plan.txt | 7 +--- .../add_column_custom_type/setup.sql | 20 +++++---- 10 files changed, 71 insertions(+), 128 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index b81f9fe0..c17d7e75 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -946,20 +946,8 @@ func (d *ddlDiff) generateModifySQL(targetSchema string, collector *diffCollecto // Modify sequences generateModifySequencesSQL(d.modifiedSequences, targetSchema, collector) - // Create a map of added type names for reference during table modifications. - // This is used to determine whether type references in ALTER TABLE ADD COLUMN - // should be schema-qualified or not: - // - Types being created in this migration (in addedTypes) → unqualified - // Example: CREATE TYPE employee_status; ALTER TABLE ADD COLUMN status employee_status - // - Pre-existing types (not in addedTypes) → qualified with schema - // Example: Types from extensions or setup.sql → ALTER TABLE ADD COLUMN email public.custom_text - addedTypeNames := make(map[string]bool) - for _, typeObj := range d.addedTypes { - addedTypeNames[typeObj.Name] = true - } - // Modify tables - generateModifyTablesSQL(d.modifiedTables, targetSchema, addedTypeNames, collector) + generateModifyTablesSQL(d.modifiedTables, targetSchema, collector) // Modify views generateModifyViewsSQL(d.modifiedViews, targetSchema, collector) diff --git a/internal/diff/table.go b/internal/diff/table.go index a34a4641..1be87df7 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -413,11 +413,11 @@ func generateCreateTablesSQL(tables []*ir.Table, targetSchema string, collector } // generateModifyTablesSQL generates ALTER TABLE statements -func generateModifyTablesSQL(diffs []*tableDiff, targetSchema string, addedTypeNames map[string]bool, collector *diffCollector) { +func generateModifyTablesSQL(diffs []*tableDiff, targetSchema string, collector *diffCollector) { // Diffs are already sorted by the Diff operation for _, diff := range diffs { // Pass collector to generateAlterTableStatements to collect with proper context - diff.generateAlterTableStatements(targetSchema, addedTypeNames, collector) + diff.generateAlterTableStatements(targetSchema, collector) } } @@ -491,7 +491,7 @@ func generateTableSQL(table *ir.Table, targetSchema string) string { } // generateAlterTableStatements generates SQL statements for table modifications -func (td *tableDiff) generateAlterTableStatements(targetSchema string, addedTypeNames map[string]bool, collector *diffCollector) { +func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector *diffCollector) { // Drop constraints first (before dropping columns) - already sorted by the Diff operation for _, constraint := range td.DroppedConstraints { tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) @@ -545,33 +545,6 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, addedType // Build column type columnType := formatColumnDataType(column) - - // Conditionally strip schema prefix based on whether the type is being created. - // - // Context: Types come from the inspector with schema qualification due to the - // pattern matching fix for temp schemas (WHEN dn.nspname LIKE 'pgschema_tmp_%'). - // However, we want different qualification behavior depending on the type's origin: - // - // Case 1: Type is being created in this migration (in addedTypeNames) - // - The type and table are defined together in the same schema file - // - References should be unqualified for readability - // - Example: CREATE TYPE employee_status; ALTER TABLE ADD COLUMN status employee_status - // - // Case 2: Type already exists (NOT in addedTypeNames) - // - The type is pre-existing infrastructure (extension, domain from another migration) - // - References should be qualified for explicitness - // - Example: Extension citext or setup.sql types → ALTER TABLE ADD COLUMN email public.custom_text - if addedTypeNames != nil { - // Extract the base type name (without schema prefix) from the column's data type - baseTypeName := column.DataType - if idx := strings.LastIndex(baseTypeName, "."); idx >= 0 { - baseTypeName = baseTypeName[idx+1:] - } - // If this type is being created in the current diff, strip the schema prefix - if addedTypeNames[baseTypeName] { - columnType = stripSchemaPrefix(columnType, targetSchema) - } - } tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) // Build and append all column clauses diff --git a/ir/queries/queries.sql b/ir/queries/queries.sql index 4fd64a51..aec891ff 100644 --- a/ir/queries/queries.sql +++ b/ir/queries/queries.sql @@ -72,17 +72,20 @@ SELECT c.numeric_scale, c.udt_name, COALESCE(d.description, '') AS column_comment, - -- Always qualify non-pg_catalog types regardless of table schema - -- This is necessary because pgschema uses restricted search_path during plan - -- that may not include the type's schema (even if it matches the table's schema) - -- Exception: Extension types are left unqualified to allow search_path resolution - -- Temp schema types (pgschema_tmp_*) are re-qualified to public schema CASE - WHEN dt.typtype IN ('d', 'e', 'c', 'b') THEN + WHEN dt.typtype = 'd' THEN + CASE WHEN dn.nspname = c.table_schema THEN dt.typname + ELSE dn.nspname || '.' || dt.typname + END + WHEN dt.typtype = 'e' OR dt.typtype = 'c' THEN + CASE WHEN dn.nspname = c.table_schema THEN dt.typname + ELSE dn.nspname || '.' || dt.typname + END + WHEN dt.typtype = 'b' THEN + -- Base types: qualify if not in pg_catalog or table's schema CASE WHEN dn.nspname = 'pg_catalog' THEN c.udt_name - WHEN dep.objid IS NOT NULL THEN dt.typname -- Extension type: unqualified - WHEN dn.nspname LIKE 'pgschema_tmp_%' THEN 'public.' || dt.typname -- Temp schema: re-qualify to public + WHEN dn.nspname = c.table_schema THEN dt.typname ELSE dn.nspname || '.' || dt.typname END ELSE c.udt_name @@ -107,7 +110,6 @@ LEFT JOIN pg_attribute a ON a.attrelid = cl.oid AND a.attname = c.column_name LEFT JOIN pg_attrdef ad ON ad.adrelid = a.attrelid AND ad.adnum = a.attnum LEFT JOIN pg_type dt ON dt.oid = a.atttypid LEFT JOIN pg_namespace dn ON dt.typnamespace = dn.oid -LEFT JOIN pg_depend dep ON dep.objid = dt.oid AND dep.deptype = 'e' WHERE c.table_schema NOT IN ('information_schema', 'pg_catalog', 'pg_toast') AND c.table_schema NOT LIKE 'pg_temp_%' @@ -132,17 +134,20 @@ SELECT c.numeric_scale, c.udt_name, COALESCE(d.description, '') AS column_comment, - -- Always qualify non-pg_catalog types regardless of table schema - -- This is necessary because pgschema uses restricted search_path during plan - -- that may not include the type's schema (even if it matches the table's schema) - -- Exception: Extension types are left unqualified to allow search_path resolution - -- Temp schema types (pgschema_tmp_*) are re-qualified to public schema CASE - WHEN dt.typtype IN ('d', 'e', 'c', 'b') THEN + WHEN dt.typtype = 'd' THEN + CASE WHEN dn.nspname = c.table_schema THEN dt.typname + ELSE dn.nspname || '.' || dt.typname + END + WHEN dt.typtype = 'e' OR dt.typtype = 'c' THEN + CASE WHEN dn.nspname = c.table_schema THEN dt.typname + ELSE dn.nspname || '.' || dt.typname + END + WHEN dt.typtype = 'b' THEN + -- Base types: qualify if not in pg_catalog or table's schema CASE WHEN dn.nspname = 'pg_catalog' THEN c.udt_name - WHEN dep.objid IS NOT NULL THEN dt.typname -- Extension type: unqualified - WHEN dn.nspname LIKE 'pgschema_tmp_%' THEN 'public.' || dt.typname -- Temp schema: re-qualify to public + WHEN dn.nspname = c.table_schema THEN dt.typname ELSE dn.nspname || '.' || dt.typname END ELSE c.udt_name @@ -167,8 +172,7 @@ LEFT JOIN pg_attribute a ON a.attrelid = cl.oid AND a.attname = c.column_name LEFT JOIN pg_attrdef ad ON ad.adrelid = a.attrelid AND ad.adnum = a.attnum LEFT JOIN pg_type dt ON dt.oid = a.atttypid LEFT JOIN pg_namespace dn ON dt.typnamespace = dn.oid -LEFT JOIN pg_depend dep ON dep.objid = dt.oid AND dep.deptype = 'e' -WHERE +WHERE c.table_schema = $1 ORDER BY c.table_name, c.ordinal_position; diff --git a/ir/queries/queries.sql.go b/ir/queries/queries.sql.go index a214af37..838890c2 100644 --- a/ir/queries/queries.sql.go +++ b/ir/queries/queries.sql.go @@ -207,18 +207,14 @@ SELECT c.numeric_scale, c.udt_name, COALESCE(d.description, '') AS column_comment, - -- Always qualify non-pg_catalog types regardless of table schema - -- This is necessary because pgschema uses restricted search_path during plan - -- that may not include the type's schema (even if it matches the table's schema) - -- Exception: Extension types are left unqualified to allow search_path resolution - -- Temp schema types (pgschema_tmp_*) are re-qualified to public schema CASE - WHEN dt.typtype IN ('d', 'e', 'c', 'b') THEN - CASE - WHEN dn.nspname = 'pg_catalog' THEN c.udt_name - WHEN dep.objid IS NOT NULL THEN dt.typname -- Extension type: unqualified - WHEN dn.nspname LIKE 'pgschema_tmp_%' THEN 'public.' || dt.typname -- Temp schema: re-qualify to public - ELSE dn.nspname || '.' || dt.typname + WHEN dt.typtype = 'd' THEN + CASE WHEN dn.nspname = c.table_schema THEN dt.typname + ELSE dn.nspname || '.' || dt.typname + END + WHEN dt.typtype = 'e' OR dt.typtype = 'c' THEN + CASE WHEN dn.nspname = c.table_schema THEN dt.typname + ELSE dn.nspname || '.' || dt.typname END ELSE c.udt_name END AS resolved_type, @@ -242,7 +238,6 @@ LEFT JOIN pg_attribute a ON a.attrelid = cl.oid AND a.attname = c.column_name LEFT JOIN pg_attrdef ad ON ad.adrelid = a.attrelid AND ad.adnum = a.attnum LEFT JOIN pg_type dt ON dt.oid = a.atttypid LEFT JOIN pg_namespace dn ON dt.typnamespace = dn.oid -LEFT JOIN pg_depend dep ON dep.objid = dt.oid AND dep.deptype = 'e' WHERE c.table_schema NOT IN ('information_schema', 'pg_catalog', 'pg_toast') AND c.table_schema NOT LIKE 'pg_temp_%' @@ -339,18 +334,14 @@ SELECT c.numeric_scale, c.udt_name, COALESCE(d.description, '') AS column_comment, - -- Always qualify non-pg_catalog types regardless of table schema - -- This is necessary because pgschema uses restricted search_path during plan - -- that may not include the type's schema (even if it matches the table's schema) - -- Exception: Extension types are left unqualified to allow search_path resolution - -- Temp schema types (pgschema_tmp_*) are re-qualified to public schema CASE - WHEN dt.typtype IN ('d', 'e', 'c', 'b') THEN - CASE - WHEN dn.nspname = 'pg_catalog' THEN c.udt_name - WHEN dep.objid IS NOT NULL THEN dt.typname -- Extension type: unqualified - WHEN dn.nspname LIKE 'pgschema_tmp_%' THEN 'public.' || dt.typname -- Temp schema: re-qualify to public - ELSE dn.nspname || '.' || dt.typname + WHEN dt.typtype = 'd' THEN + CASE WHEN dn.nspname = c.table_schema THEN dt.typname + ELSE dn.nspname || '.' || dt.typname + END + WHEN dt.typtype = 'e' OR dt.typtype = 'c' THEN + CASE WHEN dn.nspname = c.table_schema THEN dt.typname + ELSE dn.nspname || '.' || dt.typname END ELSE c.udt_name END AS resolved_type, @@ -374,8 +365,7 @@ LEFT JOIN pg_attribute a ON a.attrelid = cl.oid AND a.attname = c.column_name LEFT JOIN pg_attrdef ad ON ad.adrelid = a.attrelid AND ad.adnum = a.attnum LEFT JOIN pg_type dt ON dt.oid = a.atttypid LEFT JOIN pg_namespace dn ON dt.typnamespace = dn.oid -LEFT JOIN pg_depend dep ON dep.objid = dt.oid AND dep.deptype = 'e' -WHERE +WHERE c.table_schema = $1 ORDER BY c.table_name, c.ordinal_position ` diff --git a/testdata/diff/create_table/add_column_custom_type/diff.sql b/testdata/diff/create_table/add_column_custom_type/diff.sql index cffb3c31..16c3cfff 100644 --- a/testdata/diff/create_table/add_column_custom_type/diff.sql +++ b/testdata/diff/create_table/add_column_custom_type/diff.sql @@ -1,3 +1,3 @@ -ALTER TABLE users ADD COLUMN email citext NOT NULL; -ALTER TABLE users ADD COLUMN description public.custom_text; -ALTER TABLE users ADD COLUMN status public.status_enum DEFAULT 'active'; +ALTER TABLE users ADD COLUMN email email_address NOT NULL; + +ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL; diff --git a/testdata/diff/create_table/add_column_custom_type/new.sql b/testdata/diff/create_table/add_column_custom_type/new.sql index 87127881..22d5c9c1 100644 --- a/testdata/diff/create_table/add_column_custom_type/new.sql +++ b/testdata/diff/create_table/add_column_custom_type/new.sql @@ -1,18 +1,13 @@ --- New state: Add columns using extension type, custom domain, and enum --- Types are created via setup.sql --- This tests GitHub #144 fix and PR #145 functionality: --- - Extension types (citext) should be unqualified --- - Custom domains (custom_text) should be schema-qualified --- - Enums (status_enum) should be schema-qualified +-- New state: Add columns using custom types from setup.sql +-- Types (email_address, user_status) are created in setup.sql +-- Use unqualified names - types will be resolved via setup.sql context +-- Modified table with new columns using custom types CREATE TABLE public.users ( id bigint PRIMARY KEY, username text NOT NULL, created_at timestamp DEFAULT CURRENT_TIMESTAMP, - -- Extension type: should be unqualified - email citext NOT NULL, - -- Custom domain: should be qualified as public.custom_text - description custom_text, - -- Enum type: should be qualified as public.status_enum - status status_enum DEFAULT 'active' + -- New columns using types from setup.sql + email email_address NOT NULL, + status user_status NOT NULL DEFAULT 'active' ); diff --git a/testdata/diff/create_table/add_column_custom_type/plan.json b/testdata/diff/create_table/add_column_custom_type/plan.json index 4041d3d3..c46910be 100644 --- a/testdata/diff/create_table/add_column_custom_type/plan.json +++ b/testdata/diff/create_table/add_column_custom_type/plan.json @@ -3,25 +3,19 @@ "pgschema_version": "1.4.1", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { - "hash": "b7273391888b1ccf9500e9687c8ef201e6e5534c9325db5026a9eab3b01fdc35" + "hash": "4c7808528324af927b7ef815b8b70beac95edd62ed24c6da247a89cdf74ebb0f" }, "groups": [ { "steps": [ { - "sql": "ALTER TABLE users ADD COLUMN email citext NOT NULL;", + "sql": "ALTER TABLE users ADD COLUMN email email_address NOT NULL;", "type": "table.column", "operation": "create", "path": "public.users.email" }, { - "sql": "ALTER TABLE users ADD COLUMN description public.custom_text;", - "type": "table.column", - "operation": "create", - "path": "public.users.description" - }, - { - "sql": "ALTER TABLE users ADD COLUMN status public.status_enum DEFAULT 'active';", + "sql": "ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL;", "type": "table.column", "operation": "create", "path": "public.users.status" diff --git a/testdata/diff/create_table/add_column_custom_type/plan.sql b/testdata/diff/create_table/add_column_custom_type/plan.sql index fd804350..16c3cfff 100644 --- a/testdata/diff/create_table/add_column_custom_type/plan.sql +++ b/testdata/diff/create_table/add_column_custom_type/plan.sql @@ -1,5 +1,3 @@ -ALTER TABLE users ADD COLUMN email citext NOT NULL; +ALTER TABLE users ADD COLUMN email email_address NOT NULL; -ALTER TABLE users ADD COLUMN description public.custom_text; - -ALTER TABLE users ADD COLUMN status public.status_enum DEFAULT 'active'; +ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL; diff --git a/testdata/diff/create_table/add_column_custom_type/plan.txt b/testdata/diff/create_table/add_column_custom_type/plan.txt index f22481e7..0569c66a 100644 --- a/testdata/diff/create_table/add_column_custom_type/plan.txt +++ b/testdata/diff/create_table/add_column_custom_type/plan.txt @@ -5,15 +5,12 @@ Summary by type: Tables: ~ users - + description (column) + email (column) + status (column) DDL to be executed: -------------------------------------------------- -ALTER TABLE users ADD COLUMN email citext NOT NULL; +ALTER TABLE users ADD COLUMN email email_address NOT NULL; -ALTER TABLE users ADD COLUMN description public.custom_text; - -ALTER TABLE users ADD COLUMN status public.status_enum DEFAULT 'active'; +ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL; diff --git a/testdata/diff/create_table/add_column_custom_type/setup.sql b/testdata/diff/create_table/add_column_custom_type/setup.sql index 33a28cf1..6fc1a2d6 100644 --- a/testdata/diff/create_table/add_column_custom_type/setup.sql +++ b/testdata/diff/create_table/add_column_custom_type/setup.sql @@ -1,10 +1,14 @@ --- Setup: Create extension type, custom domain, and enum to test type qualification --- This reproduces GitHub #144 and validates PR #145 fixes --- Extension types (citext) should be unqualified (search_path resolution) --- Custom domains and enums should be schema-qualified (public.*) +-- Setup: Create types in public schema +-- This simulates extension types like citext installed in public schema -CREATE EXTENSION IF NOT EXISTS citext; +CREATE TYPE public.email_address AS ( + local_part text, + domain text +); -CREATE DOMAIN public.custom_text AS text; - -CREATE TYPE public.status_enum AS ENUM ('active', 'inactive', 'pending'); +CREATE TYPE public.user_status AS ENUM ( + 'active', + 'inactive', + 'suspended', + 'pending' +); From c2dc76bae9d22cddb88a293cc086c1afdae81a86 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Thu, 6 Nov 2025 23:58:19 -0800 Subject: [PATCH 2/3] chore: adjust test case --- .../add_column_custom_type/diff.sql | 6 ++--- .../add_column_custom_type/new.sql | 21 +++++++++------ .../add_column_custom_type/plan.json | 12 ++++++--- .../add_column_custom_type/plan.sql | 6 +++-- .../add_column_custom_type/plan.txt | 7 +++-- .../add_column_custom_type/setup.sql | 26 +++++++++---------- 6 files changed, 46 insertions(+), 32 deletions(-) diff --git a/testdata/diff/create_table/add_column_custom_type/diff.sql b/testdata/diff/create_table/add_column_custom_type/diff.sql index 16c3cfff..4f60fce0 100644 --- a/testdata/diff/create_table/add_column_custom_type/diff.sql +++ b/testdata/diff/create_table/add_column_custom_type/diff.sql @@ -1,3 +1,3 @@ -ALTER TABLE users ADD COLUMN email email_address NOT NULL; - -ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL; +ALTER TABLE users ADD COLUMN email utils.citext NOT NULL; +ALTER TABLE users ADD COLUMN description utils.custom_text; +ALTER TABLE users ADD COLUMN status utils.custom_enum DEFAULT 'active'; \ No newline at end of file diff --git a/testdata/diff/create_table/add_column_custom_type/new.sql b/testdata/diff/create_table/add_column_custom_type/new.sql index 22d5c9c1..fc32898a 100644 --- a/testdata/diff/create_table/add_column_custom_type/new.sql +++ b/testdata/diff/create_table/add_column_custom_type/new.sql @@ -1,13 +1,18 @@ --- New state: Add columns using custom types from setup.sql --- Types (email_address, user_status) are created in setup.sql --- Use unqualified names - types will be resolved via setup.sql context +-- New state: Add columns using extension type, custom domain, and enum +-- Types are created via setup.sql +-- This tests GitHub #144 fix and PR #145 functionality: +-- - Extension types (citext) should be unqualified +-- - Custom domains (custom_text) should be schema-qualified +-- - Enums (status_enum) should be schema-qualified --- Modified table with new columns using custom types CREATE TABLE public.users ( id bigint PRIMARY KEY, username text NOT NULL, created_at timestamp DEFAULT CURRENT_TIMESTAMP, - -- New columns using types from setup.sql - email email_address NOT NULL, - status user_status NOT NULL DEFAULT 'active' -); + -- Extension type: should be unqualified + email utils.citext NOT NULL, + -- Custom domain: should be qualified as public.custom_text + description utils.custom_text, + -- Enum type: should be qualified as public.status_enum + status utils.custom_enum DEFAULT 'active' +); \ No newline at end of file diff --git a/testdata/diff/create_table/add_column_custom_type/plan.json b/testdata/diff/create_table/add_column_custom_type/plan.json index c46910be..6adf6b66 100644 --- a/testdata/diff/create_table/add_column_custom_type/plan.json +++ b/testdata/diff/create_table/add_column_custom_type/plan.json @@ -3,19 +3,25 @@ "pgschema_version": "1.4.1", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { - "hash": "4c7808528324af927b7ef815b8b70beac95edd62ed24c6da247a89cdf74ebb0f" + "hash": "b7273391888b1ccf9500e9687c8ef201e6e5534c9325db5026a9eab3b01fdc35" }, "groups": [ { "steps": [ { - "sql": "ALTER TABLE users ADD COLUMN email email_address NOT NULL;", + "sql": "ALTER TABLE users ADD COLUMN email citext NOT NULL;", "type": "table.column", "operation": "create", "path": "public.users.email" }, { - "sql": "ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL;", + "sql": "ALTER TABLE users ADD COLUMN description custom_text;", + "type": "table.column", + "operation": "create", + "path": "public.users.description" + }, + { + "sql": "ALTER TABLE users ADD COLUMN status status_enum DEFAULT 'active';", "type": "table.column", "operation": "create", "path": "public.users.status" diff --git a/testdata/diff/create_table/add_column_custom_type/plan.sql b/testdata/diff/create_table/add_column_custom_type/plan.sql index 16c3cfff..25a50916 100644 --- a/testdata/diff/create_table/add_column_custom_type/plan.sql +++ b/testdata/diff/create_table/add_column_custom_type/plan.sql @@ -1,3 +1,5 @@ -ALTER TABLE users ADD COLUMN email email_address NOT NULL; +ALTER TABLE users ADD COLUMN email citext NOT NULL; -ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL; +ALTER TABLE users ADD COLUMN description custom_text; + +ALTER TABLE users ADD COLUMN status status_enum DEFAULT 'active'; diff --git a/testdata/diff/create_table/add_column_custom_type/plan.txt b/testdata/diff/create_table/add_column_custom_type/plan.txt index 0569c66a..4e77095b 100644 --- a/testdata/diff/create_table/add_column_custom_type/plan.txt +++ b/testdata/diff/create_table/add_column_custom_type/plan.txt @@ -5,12 +5,15 @@ Summary by type: Tables: ~ users + + description (column) + email (column) + status (column) DDL to be executed: -------------------------------------------------- -ALTER TABLE users ADD COLUMN email email_address NOT NULL; +ALTER TABLE users ADD COLUMN email citext NOT NULL; -ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL; +ALTER TABLE users ADD COLUMN description custom_text; + +ALTER TABLE users ADD COLUMN status status_enum DEFAULT 'active'; diff --git a/testdata/diff/create_table/add_column_custom_type/setup.sql b/testdata/diff/create_table/add_column_custom_type/setup.sql index 6fc1a2d6..f1d23732 100644 --- a/testdata/diff/create_table/add_column_custom_type/setup.sql +++ b/testdata/diff/create_table/add_column_custom_type/setup.sql @@ -1,14 +1,12 @@ --- Setup: Create types in public schema --- This simulates extension types like citext installed in public schema - -CREATE TYPE public.email_address AS ( - local_part text, - domain text -); - -CREATE TYPE public.user_status AS ENUM ( - 'active', - 'inactive', - 'suspended', - 'pending' -); +-- Setup: Create extension type, custom domain, and enum to test type qualification +-- This reproduces GitHub #144 and validates PR #145 fixes +-- Extension types (citext) should be unqualified (search_path resolution) +-- Custom domains and enums should be schema-qualified (public.*) + +CREATE SCHEMA IF NOT EXISTS utils; + +CREATE DOMAIN utils.custom_text AS text; + +CREATE TYPE utils.custom_enum AS ENUM ('active', 'inactive', 'pending'); + +CREATE EXTENSION IF NOT EXISTS citext SCHEMA utils; \ No newline at end of file From 2eaa03ec24cbb62c6bb2dfe3d512c29b8ee16188 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Fri, 7 Nov 2025 00:44:35 -0800 Subject: [PATCH 3/3] chore: only run setup script once --- cmd/migrate_integration_test.go | 46 ++++++------------- internal/diff/diff_test.go | 28 +++++------ ir/queries/queries.sql.go | 14 ++++++ .../diff.sql | 3 +- .../new.sql | 18 ++++++++ .../old.sql | 0 .../plan.json | 38 +++++++++++++++ .../plan.sql | 7 +++ .../plan.txt | 22 +++++++++ .../setup.sql | 8 +++- .../add_column_custom_type/new.sql | 18 -------- .../add_column_custom_type/plan.json | 32 ------------- .../add_column_custom_type/plan.sql | 5 -- .../add_column_custom_type/plan.txt | 19 -------- 14 files changed, 133 insertions(+), 125 deletions(-) rename testdata/diff/create_table/{add_column_custom_type => add_column_cross_schema_custom_type}/diff.sql (54%) create mode 100644 testdata/diff/create_table/add_column_cross_schema_custom_type/new.sql rename testdata/diff/create_table/{add_column_custom_type => add_column_cross_schema_custom_type}/old.sql (100%) create mode 100644 testdata/diff/create_table/add_column_cross_schema_custom_type/plan.json create mode 100644 testdata/diff/create_table/add_column_cross_schema_custom_type/plan.sql create mode 100644 testdata/diff/create_table/add_column_cross_schema_custom_type/plan.txt rename testdata/diff/create_table/{add_column_custom_type => add_column_cross_schema_custom_type}/setup.sql (57%) delete mode 100644 testdata/diff/create_table/add_column_custom_type/new.sql delete mode 100644 testdata/diff/create_table/add_column_custom_type/plan.json delete mode 100644 testdata/diff/create_table/add_column_custom_type/plan.sql delete mode 100644 testdata/diff/create_table/add_column_custom_type/plan.txt diff --git a/cmd/migrate_integration_test.go b/cmd/migrate_integration_test.go index 569199f4..de4d2210 100644 --- a/cmd/migrate_integration_test.go +++ b/cmd/migrate_integration_test.go @@ -239,8 +239,17 @@ func runPlanAndApplyTest(t *testing.T, ctx context.Context, container *struct { t.Fatalf("Failed to read setup.sql: %v", err) } if len(strings.TrimSpace(string(setupContent))) > 0 { + // Execute setup.sql to target database if err := executeSQL(ctx, containerHost, portMapped, dbName, string(setupContent)); err != nil { - t.Fatalf("Failed to execute setup.sql: %v", err) + t.Fatalf("Failed to execute setup.sql to target database: %v", err) + } + + // Also execute setup.sql to shared embedded postgres instance + // This ensures both databases have the setup objects available + embeddedConn, _, _, _, _, _ := testutil.ConnectToPostgres(t, sharedEmbeddedPG) + defer embeddedConn.Close() + if _, err := embeddedConn.ExecContext(ctx, string(setupContent)); err != nil { + t.Fatalf("Failed to execute setup.sql to embedded postgres: %v", err) } } } @@ -259,44 +268,19 @@ func runPlanAndApplyTest(t *testing.T, ctx context.Context, container *struct { } // STEP 2: Test plan command with new.sql as target - // If setup.sql exists, create a temporary combined file (setup + new) - schemaFileForPlan := tc.newFile - if _, err := os.Stat(tc.setupFile); err == nil { - setupContent, err := os.ReadFile(tc.setupFile) - if err != nil { - t.Fatalf("Failed to read setup.sql for plan: %v", err) - } - newContent, err := os.ReadFile(tc.newFile) - if err != nil { - t.Fatalf("Failed to read new.sql for plan: %v", err) - } - - // Create temporary combined file - combinedContent := string(setupContent) + "\n\n" + string(newContent) - tmpFile, err := os.CreateTemp("", "pgschema_test_*.sql") - if err != nil { - t.Fatalf("Failed to create temporary file: %v", err) - } - defer tmpFile.Close() - defer os.Remove(tmpFile.Name()) - - if _, err := tmpFile.WriteString(combinedContent); err != nil { - t.Fatalf("Failed to write to temporary file: %v", err) - } - schemaFileForPlan = tmpFile.Name() - } - - testPlanOutputs(t, container, dbName, schemaFileForPlan, tc.planSQLFile, tc.planJSONFile, tc.planTXTFile) + // Note: setup.sql has already been executed to both databases in STEP 0, + // so we only need to use new.sql for plan and apply operations + testPlanOutputs(t, container, dbName, tc.newFile, tc.planSQLFile, tc.planJSONFile, tc.planTXTFile) if !*generate { // STEP 3: Apply the migration using apply command - err = applySchemaChanges(containerHost, portMapped, dbName, container.User, container.Password, "public", schemaFileForPlan) + err = applySchemaChanges(containerHost, portMapped, dbName, container.User, container.Password, "public", tc.newFile) if err != nil { t.Fatalf("Failed to apply schema changes using pgschema apply: %v", err) } // STEP 4: Test idempotency - plan should produce no changes - secondPlanOutput, err := generatePlanSQLFormatted(containerHost, portMapped, dbName, container.User, container.Password, "public", schemaFileForPlan) + secondPlanOutput, err := generatePlanSQLFormatted(containerHost, portMapped, dbName, container.User, container.Password, "public", tc.newFile) if err != nil { t.Fatalf("Failed to generate plan SQL for idempotency check: %v", err) } diff --git a/internal/diff/diff_test.go b/internal/diff/diff_test.go index f44d5325..bd05cf94 100644 --- a/internal/diff/diff_test.go +++ b/internal/diff/diff_test.go @@ -1,13 +1,13 @@ package diff import ( + "context" "os" "path/filepath" "strings" "testing" "github.com/pgschema/pgschema/internal/postgres" - "github.com/pgschema/pgschema/ir" "github.com/pgschema/pgschema/testutil" ) @@ -53,17 +53,6 @@ func buildSQLFromSteps(diffs []Diff) string { return sqlOutput.String() } -// parseSQLWithSetup applies optional setup SQL before main SQL, then parses to IR -// This allows tests to have shared setup (e.g., types in different schemas) before the main schema -func parseSQLWithSetup(t *testing.T, setup, sql string) *ir.IR { - t.Helper() - - // Combine setup and main SQL with separator if both exist - combinedSQL := setup + "\n\n" + sql - - return testutil.ParseSQLToIR(t, sharedTestPostgres, combinedSQL, "public") -} - // TestDiffFromFiles runs file-based diff tests from testdata directory. // It walks through the testdata/diff directory structure looking for test cases // that contain old.sql, new.sql, and plan.sql files. For each test case, @@ -165,13 +154,18 @@ func runFileBasedDiffTest(t *testing.T, oldFile, newFile, diffFile, testName str // Read optional setup.sql (for cross-schema setup, extension types, etc.) setupFile := filepath.Join(filepath.Dir(oldFile), "setup.sql") - var setupSQL string if _, err := os.Stat(setupFile); err == nil { setupContent, err := os.ReadFile(setupFile) if err != nil { t.Fatalf("Failed to read setup.sql: %v", err) } - setupSQL = string(setupContent) + + // Execute setup.sql once before parsing old/new SQL + // This creates shared infrastructure (e.g., custom types in utils schema) + // that will be available to both old.sql and new.sql + if _, err := conn.ExecContext(context.Background(), string(setupContent)); err != nil { + t.Fatalf("Failed to execute setup.sql: %v", err) + } } // Read old DDL @@ -192,9 +186,9 @@ func runFileBasedDiffTest(t *testing.T, oldFile, newFile, diffFile, testName str t.Fatalf("Failed to read plan.sql: %v", err) } - // Parse DDL to IR (with optional setup SQL) - oldIR := parseSQLWithSetup(t, setupSQL, string(oldDDL)) - newIR := parseSQLWithSetup(t, setupSQL, string(newDDL)) + // Parse DDL to IR (setup.sql already applied, so just parse old/new SQL directly) + oldIR := testutil.ParseSQLToIR(t, sharedTestPostgres, string(oldDDL), "public") + newIR := testutil.ParseSQLToIR(t, sharedTestPostgres, string(newDDL), "public") // Run diff diffs := GenerateMigration(oldIR, newIR, "public") diff --git a/ir/queries/queries.sql.go b/ir/queries/queries.sql.go index 838890c2..0641fc5e 100644 --- a/ir/queries/queries.sql.go +++ b/ir/queries/queries.sql.go @@ -216,6 +216,13 @@ SELECT CASE WHEN dn.nspname = c.table_schema THEN dt.typname ELSE dn.nspname || '.' || dt.typname END + WHEN dt.typtype = 'b' THEN + -- Base types: qualify if not in pg_catalog or table's schema + CASE + WHEN dn.nspname = 'pg_catalog' THEN c.udt_name + WHEN dn.nspname = c.table_schema THEN dt.typname + ELSE dn.nspname || '.' || dt.typname + END ELSE c.udt_name END AS resolved_type, c.is_identity, @@ -343,6 +350,13 @@ SELECT CASE WHEN dn.nspname = c.table_schema THEN dt.typname ELSE dn.nspname || '.' || dt.typname END + WHEN dt.typtype = 'b' THEN + -- Base types: qualify if not in pg_catalog or table's schema + CASE + WHEN dn.nspname = 'pg_catalog' THEN c.udt_name + WHEN dn.nspname = c.table_schema THEN dt.typname + ELSE dn.nspname || '.' || dt.typname + END ELSE c.udt_name END AS resolved_type, c.is_identity, diff --git a/testdata/diff/create_table/add_column_custom_type/diff.sql b/testdata/diff/create_table/add_column_cross_schema_custom_type/diff.sql similarity index 54% rename from testdata/diff/create_table/add_column_custom_type/diff.sql rename to testdata/diff/create_table/add_column_cross_schema_custom_type/diff.sql index 4f60fce0..0a58b5c2 100644 --- a/testdata/diff/create_table/add_column_custom_type/diff.sql +++ b/testdata/diff/create_table/add_column_cross_schema_custom_type/diff.sql @@ -1,3 +1,4 @@ -ALTER TABLE users ADD COLUMN email utils.citext NOT NULL; +ALTER TABLE users ADD COLUMN metadata utils.hstore; +ALTER TABLE users ADD COLUMN fqdn utils.citext NOT NULL; ALTER TABLE users ADD COLUMN description utils.custom_text; ALTER TABLE users ADD COLUMN status utils.custom_enum DEFAULT 'active'; \ No newline at end of file diff --git a/testdata/diff/create_table/add_column_cross_schema_custom_type/new.sql b/testdata/diff/create_table/add_column_cross_schema_custom_type/new.sql new file mode 100644 index 00000000..72460f13 --- /dev/null +++ b/testdata/diff/create_table/add_column_cross_schema_custom_type/new.sql @@ -0,0 +1,18 @@ +-- New state: Add columns using extension types, custom domain, and enum +-- Types are created via setup.sql +-- This tests GitHub #144 fix and PR #145 functionality: +-- - Extension types in external schemas (hstore) should be qualified +-- - Custom domains and enums in external schemas should be qualified + +CREATE TABLE public.users ( + id bigint PRIMARY KEY, + username text NOT NULL, + created_at timestamp DEFAULT CURRENT_TIMESTAMP, + -- Extension type from utils schema: must be qualified + metadata utils.hstore, + fqdn utils.citext NOT NULL, + -- Custom domain from utils schema: must be qualified + description utils.custom_text, + -- Enum type from utils schema: must be qualified + status utils.custom_enum DEFAULT 'active' +); \ No newline at end of file diff --git a/testdata/diff/create_table/add_column_custom_type/old.sql b/testdata/diff/create_table/add_column_cross_schema_custom_type/old.sql similarity index 100% rename from testdata/diff/create_table/add_column_custom_type/old.sql rename to testdata/diff/create_table/add_column_cross_schema_custom_type/old.sql diff --git a/testdata/diff/create_table/add_column_cross_schema_custom_type/plan.json b/testdata/diff/create_table/add_column_cross_schema_custom_type/plan.json new file mode 100644 index 00000000..e63e3aa0 --- /dev/null +++ b/testdata/diff/create_table/add_column_cross_schema_custom_type/plan.json @@ -0,0 +1,38 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.4.1", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "a9e46b459eae7b043790b1744ca6210ad942beafbef337ae3b787e120a2474eb" + }, + "groups": [ + { + "steps": [ + { + "sql": "ALTER TABLE users ADD COLUMN metadata utils.hstore;", + "type": "table.column", + "operation": "create", + "path": "public.users.metadata" + }, + { + "sql": "ALTER TABLE users ADD COLUMN fqdn utils.citext NOT NULL;", + "type": "table.column", + "operation": "create", + "path": "public.users.fqdn" + }, + { + "sql": "ALTER TABLE users ADD COLUMN description utils.custom_text;", + "type": "table.column", + "operation": "create", + "path": "public.users.description" + }, + { + "sql": "ALTER TABLE users ADD COLUMN status utils.custom_enum DEFAULT 'active';", + "type": "table.column", + "operation": "create", + "path": "public.users.status" + } + ] + } + ] +} diff --git a/testdata/diff/create_table/add_column_cross_schema_custom_type/plan.sql b/testdata/diff/create_table/add_column_cross_schema_custom_type/plan.sql new file mode 100644 index 00000000..ca0332ff --- /dev/null +++ b/testdata/diff/create_table/add_column_cross_schema_custom_type/plan.sql @@ -0,0 +1,7 @@ +ALTER TABLE users ADD COLUMN metadata utils.hstore; + +ALTER TABLE users ADD COLUMN fqdn utils.citext NOT NULL; + +ALTER TABLE users ADD COLUMN description utils.custom_text; + +ALTER TABLE users ADD COLUMN status utils.custom_enum DEFAULT 'active'; diff --git a/testdata/diff/create_table/add_column_cross_schema_custom_type/plan.txt b/testdata/diff/create_table/add_column_cross_schema_custom_type/plan.txt new file mode 100644 index 00000000..9093bdd5 --- /dev/null +++ b/testdata/diff/create_table/add_column_cross_schema_custom_type/plan.txt @@ -0,0 +1,22 @@ +Plan: 1 to modify. + +Summary by type: + tables: 1 to modify + +Tables: + ~ users + + description (column) + + fqdn (column) + + metadata (column) + + status (column) + +DDL to be executed: +-------------------------------------------------- + +ALTER TABLE users ADD COLUMN metadata utils.hstore; + +ALTER TABLE users ADD COLUMN fqdn utils.citext NOT NULL; + +ALTER TABLE users ADD COLUMN description utils.custom_text; + +ALTER TABLE users ADD COLUMN status utils.custom_enum DEFAULT 'active'; diff --git a/testdata/diff/create_table/add_column_custom_type/setup.sql b/testdata/diff/create_table/add_column_cross_schema_custom_type/setup.sql similarity index 57% rename from testdata/diff/create_table/add_column_custom_type/setup.sql rename to testdata/diff/create_table/add_column_cross_schema_custom_type/setup.sql index f1d23732..82c5f550 100644 --- a/testdata/diff/create_table/add_column_custom_type/setup.sql +++ b/testdata/diff/create_table/add_column_cross_schema_custom_type/setup.sql @@ -1,7 +1,9 @@ -- Setup: Create extension type, custom domain, and enum to test type qualification -- This reproduces GitHub #144 and validates PR #145 fixes --- Extension types (citext) should be unqualified (search_path resolution) --- Custom domains and enums should be schema-qualified (public.*) +-- All types from external schemas (not the target schema) should be schema-qualified +-- This includes: +-- - Extension types (hstore) +-- - Custom domains and enums CREATE SCHEMA IF NOT EXISTS utils; @@ -9,4 +11,6 @@ CREATE DOMAIN utils.custom_text AS text; CREATE TYPE utils.custom_enum AS ENUM ('active', 'inactive', 'pending'); +-- hstore type stays in utils schema +CREATE EXTENSION IF NOT EXISTS hstore SCHEMA utils; CREATE EXTENSION IF NOT EXISTS citext SCHEMA utils; \ No newline at end of file diff --git a/testdata/diff/create_table/add_column_custom_type/new.sql b/testdata/diff/create_table/add_column_custom_type/new.sql deleted file mode 100644 index fc32898a..00000000 --- a/testdata/diff/create_table/add_column_custom_type/new.sql +++ /dev/null @@ -1,18 +0,0 @@ --- New state: Add columns using extension type, custom domain, and enum --- Types are created via setup.sql --- This tests GitHub #144 fix and PR #145 functionality: --- - Extension types (citext) should be unqualified --- - Custom domains (custom_text) should be schema-qualified --- - Enums (status_enum) should be schema-qualified - -CREATE TABLE public.users ( - id bigint PRIMARY KEY, - username text NOT NULL, - created_at timestamp DEFAULT CURRENT_TIMESTAMP, - -- Extension type: should be unqualified - email utils.citext NOT NULL, - -- Custom domain: should be qualified as public.custom_text - description utils.custom_text, - -- Enum type: should be qualified as public.status_enum - status utils.custom_enum DEFAULT 'active' -); \ No newline at end of file diff --git a/testdata/diff/create_table/add_column_custom_type/plan.json b/testdata/diff/create_table/add_column_custom_type/plan.json deleted file mode 100644 index 6adf6b66..00000000 --- a/testdata/diff/create_table/add_column_custom_type/plan.json +++ /dev/null @@ -1,32 +0,0 @@ -{ - "version": "1.0.0", - "pgschema_version": "1.4.1", - "created_at": "1970-01-01T00:00:00Z", - "source_fingerprint": { - "hash": "b7273391888b1ccf9500e9687c8ef201e6e5534c9325db5026a9eab3b01fdc35" - }, - "groups": [ - { - "steps": [ - { - "sql": "ALTER TABLE users ADD COLUMN email citext NOT NULL;", - "type": "table.column", - "operation": "create", - "path": "public.users.email" - }, - { - "sql": "ALTER TABLE users ADD COLUMN description custom_text;", - "type": "table.column", - "operation": "create", - "path": "public.users.description" - }, - { - "sql": "ALTER TABLE users ADD COLUMN status status_enum DEFAULT 'active';", - "type": "table.column", - "operation": "create", - "path": "public.users.status" - } - ] - } - ] -} diff --git a/testdata/diff/create_table/add_column_custom_type/plan.sql b/testdata/diff/create_table/add_column_custom_type/plan.sql deleted file mode 100644 index 25a50916..00000000 --- a/testdata/diff/create_table/add_column_custom_type/plan.sql +++ /dev/null @@ -1,5 +0,0 @@ -ALTER TABLE users ADD COLUMN email citext NOT NULL; - -ALTER TABLE users ADD COLUMN description custom_text; - -ALTER TABLE users ADD COLUMN status status_enum DEFAULT 'active'; diff --git a/testdata/diff/create_table/add_column_custom_type/plan.txt b/testdata/diff/create_table/add_column_custom_type/plan.txt deleted file mode 100644 index 4e77095b..00000000 --- a/testdata/diff/create_table/add_column_custom_type/plan.txt +++ /dev/null @@ -1,19 +0,0 @@ -Plan: 1 to modify. - -Summary by type: - tables: 1 to modify - -Tables: - ~ users - + description (column) - + email (column) - + status (column) - -DDL to be executed: --------------------------------------------------- - -ALTER TABLE users ADD COLUMN email citext NOT NULL; - -ALTER TABLE users ADD COLUMN description custom_text; - -ALTER TABLE users ADD COLUMN status status_enum DEFAULT 'active';