From dd5743599acadc154376336e2154f1ec67f755d6 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Sun, 15 Feb 2026 08:54:00 -0800 Subject: [PATCH 1/4] fix: spurious plan changes for column defaults with schema-qualified functions (#283) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When using temporary schemas for plan generation, column defaults referencing functions in the target schema (e.g., public.uuid_generate_v1mc()) were not properly normalized, causing pgschema to detect changes where none existed. The root cause: normalizeIR ran with temp schema names (pgschema_tmp_*), so same-schema qualifier stripping couldn't match the target schema prefix. After normalizeSchemaNames renamed temp→target, re-running normalization with the correct schema context fixes the mismatch. Co-Authored-By: Claude Opus 4.6 --- cmd/plan/plan.go | 8 ++ ir/normalize.go | 8 ++ ir/normalize_test.go | 112 ++++++++++++++++++ .../diff.sql | 0 .../new.sql | 10 ++ .../old.sql | 10 ++ .../plan.json | 9 ++ .../plan.sql | 0 .../plan.txt | 1 + 9 files changed, 158 insertions(+) create mode 100644 testdata/diff/create_table/issue_283_function_default_schema_qualifier/diff.sql create mode 100644 testdata/diff/create_table/issue_283_function_default_schema_qualifier/new.sql create mode 100644 testdata/diff/create_table/issue_283_function_default_schema_qualifier/old.sql create mode 100644 testdata/diff/create_table/issue_283_function_default_schema_qualifier/plan.json create mode 100644 testdata/diff/create_table/issue_283_function_default_schema_qualifier/plan.sql create mode 100644 testdata/diff/create_table/issue_283_function_default_schema_qualifier/plan.txt diff --git a/cmd/plan/plan.go b/cmd/plan/plan.go index df88a857..24fa9155 100644 --- a/cmd/plan/plan.go +++ b/cmd/plan/plan.go @@ -282,6 +282,14 @@ func GeneratePlan(config *PlanConfig, provider postgres.DesiredStateProvider) (* // Without this normalization, DDL would reference non-existent temporary schemas and fail. if schemaToInspect != config.Schema { normalizeSchemaNames(desiredStateIR, schemaToInspect, config.Schema) + // Re-run IR normalization after schema name replacement. The first normalization + // (in the inspector) ran with temporary schema names (pgschema_tmp_*), which meant + // same-schema qualifier stripping didn't work correctly for functions/types in the + // target schema (e.g., "public.uuid_generate_v1mc()" was not stripped because the + // table was in "pgschema_tmp_*", not "public"). After replacing temp schema names + // with the target schema, re-running normalization ensures qualifiers are properly + // stripped using the correct schema context. See issue #283. + ir.NormalizeIR(desiredStateIR) } // Generate diff (current -> desired) using IR directly diff --git a/ir/normalize.go b/ir/normalize.go index 0e1e4af0..d5389151 100644 --- a/ir/normalize.go +++ b/ir/normalize.go @@ -8,6 +8,14 @@ import ( "unicode" ) +// NormalizeIR normalizes the IR representation. This function is idempotent and safe to +// call multiple times. It must be re-run after renaming temporary schema names to target +// schema names (e.g., pgschema_tmp_* → public) to ensure same-schema qualifier stripping +// uses the correct schema context. See issue #283. +func NormalizeIR(ir *IR) { + normalizeIR(ir) +} + // normalizeIR normalizes the IR representation from the inspector. // // Since both desired state (from embedded postgres) and current state (from target database) diff --git a/ir/normalize_test.go b/ir/normalize_test.go index 528de4d5..eda3f385 100644 --- a/ir/normalize_test.go +++ b/ir/normalize_test.go @@ -4,6 +4,118 @@ import ( "testing" ) +// TestNormalizeDefaultValue_TempSchemaFunctionQualifier tests that column defaults +// referencing functions in the public schema are properly normalized when the table +// is in a temporary schema (pgschema_tmp_*). +// +// This is a regression test for GitHub issue #283: +// When pgschema plans a migration using a temp schema for desired state, pg_get_expr() +// with search_path=pg_catalog returns "public.func_name()" for functions in the public +// schema. normalizeDefaultValue with the temp schema name can't strip "public." because +// it doesn't match the temp schema name. On the target database, normalizeDefaultValue +// with tableSchema="public" DOES strip it. This causes a spurious diff. +// +// The fix: re-run normalizeIR after normalizeSchemaNames replaces temp → target schema. +func TestNormalizeDefaultValue_TempSchemaFunctionQualifier(t *testing.T) { + tests := []struct { + name string + value string + tableSchema string + expected string + }{ + { + name: "public function stripped when table in public schema", + value: "public.uuid_generate_v1mc()", + tableSchema: "public", + expected: "uuid_generate_v1mc()", + }, + { + name: "public function NOT stripped when table in temp schema (the bug)", + value: "public.uuid_generate_v1mc()", + tableSchema: "pgschema_tmp_20260101_120000_abcd1234", + expected: "public.uuid_generate_v1mc()", + }, + { + name: "temp schema function stripped when table in same temp schema", + value: "pgschema_tmp_20260101_120000_abcd1234.my_func()", + tableSchema: "pgschema_tmp_20260101_120000_abcd1234", + expected: "my_func()", + }, + { + name: "cross-schema function preserved", + value: "other_schema.my_func()", + tableSchema: "public", + expected: "other_schema.my_func()", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := normalizeDefaultValue(tt.value, tt.tableSchema) + if result != tt.expected { + t.Errorf("normalizeDefaultValue(%q, %q) = %q, want %q", tt.value, tt.tableSchema, result, tt.expected) + } + }) + } +} + +// TestNormalizeIR_RerunAfterSchemaRename verifies that re-running normalizeIR +// after renaming temp schema to target schema produces correct results. +// This is the key behavior that fixes issue #283. +func TestNormalizeIR_RerunAfterSchemaRename(t *testing.T) { + tempSchema := "pgschema_tmp_20260101_120000_abcd1234" + + // Simulate desired state IR from temp schema inspection + defaultVal := "public.uuid_generate_v1mc()" + testIR := &IR{ + Schemas: map[string]*Schema{ + tempSchema: { + Name: tempSchema, + Tables: map[string]*Table{ + "items": { + Name: "items", + Schema: tempSchema, + Columns: []*Column{ + { + Name: "id", + DataType: "uuid", + DefaultValue: &defaultVal, + }, + }, + }, + }, + }, + }, + } + + // First normalization pass (happens in inspector) + normalizeIR(testIR) + + // After first normalization, "public." is NOT stripped (table schema is temp) + col := testIR.Schemas[tempSchema].Tables["items"].Columns[0] + if *col.DefaultValue != "public.uuid_generate_v1mc()" { + t.Fatalf("After first normalizeIR, expected default to remain 'public.uuid_generate_v1mc()', got %q", *col.DefaultValue) + } + + // Simulate normalizeSchemaNames: rename temp schema to public + schema := testIR.Schemas[tempSchema] + delete(testIR.Schemas, tempSchema) + schema.Name = "public" + testIR.Schemas["public"] = schema + for _, table := range schema.Tables { + table.Schema = "public" + } + + // Second normalization pass (the fix: re-run after schema rename) + normalizeIR(testIR) + + // After second normalization, "public." should be stripped + col = testIR.Schemas["public"].Tables["items"].Columns[0] + if *col.DefaultValue != "uuid_generate_v1mc()" { + t.Errorf("After re-running normalizeIR with correct schema, expected 'uuid_generate_v1mc()', got %q", *col.DefaultValue) + } +} + func TestNormalizeCheckClause(t *testing.T) { tests := []struct { name string diff --git a/testdata/diff/create_table/issue_283_function_default_schema_qualifier/diff.sql b/testdata/diff/create_table/issue_283_function_default_schema_qualifier/diff.sql new file mode 100644 index 00000000..e69de29b diff --git a/testdata/diff/create_table/issue_283_function_default_schema_qualifier/new.sql b/testdata/diff/create_table/issue_283_function_default_schema_qualifier/new.sql new file mode 100644 index 00000000..25ba03c8 --- /dev/null +++ b/testdata/diff/create_table/issue_283_function_default_schema_qualifier/new.sql @@ -0,0 +1,10 @@ +CREATE FUNCTION my_default_id() RETURNS uuid + LANGUAGE sql + AS $$ SELECT gen_random_uuid() $$; + +CREATE TABLE items ( + id uuid DEFAULT my_default_id() NOT NULL, + name text NOT NULL, + active boolean DEFAULT true NOT NULL, + CONSTRAINT items_pk PRIMARY KEY (id) +); diff --git a/testdata/diff/create_table/issue_283_function_default_schema_qualifier/old.sql b/testdata/diff/create_table/issue_283_function_default_schema_qualifier/old.sql new file mode 100644 index 00000000..25ba03c8 --- /dev/null +++ b/testdata/diff/create_table/issue_283_function_default_schema_qualifier/old.sql @@ -0,0 +1,10 @@ +CREATE FUNCTION my_default_id() RETURNS uuid + LANGUAGE sql + AS $$ SELECT gen_random_uuid() $$; + +CREATE TABLE items ( + id uuid DEFAULT my_default_id() NOT NULL, + name text NOT NULL, + active boolean DEFAULT true NOT NULL, + CONSTRAINT items_pk PRIMARY KEY (id) +); diff --git a/testdata/diff/create_table/issue_283_function_default_schema_qualifier/plan.json b/testdata/diff/create_table/issue_283_function_default_schema_qualifier/plan.json new file mode 100644 index 00000000..bdbe8800 --- /dev/null +++ b/testdata/diff/create_table/issue_283_function_default_schema_qualifier/plan.json @@ -0,0 +1,9 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.7.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "0915a8651243d1249ecffcf4938383a65f0a0fb644f189f372e3719f2a46f13f" + }, + "groups": null +} diff --git a/testdata/diff/create_table/issue_283_function_default_schema_qualifier/plan.sql b/testdata/diff/create_table/issue_283_function_default_schema_qualifier/plan.sql new file mode 100644 index 00000000..e69de29b diff --git a/testdata/diff/create_table/issue_283_function_default_schema_qualifier/plan.txt b/testdata/diff/create_table/issue_283_function_default_schema_qualifier/plan.txt new file mode 100644 index 00000000..241994af --- /dev/null +++ b/testdata/diff/create_table/issue_283_function_default_schema_qualifier/plan.txt @@ -0,0 +1 @@ +No changes detected. From e0d1d18d1280e9afcd8c088f8f99f7c96f95eb69 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Sun, 15 Feb 2026 09:17:21 -0800 Subject: [PATCH 2/4] fix: strip same-schema qualifiers in normalizeSchemaNames instead of re-running normalizeIR (#283) Move the fix for issue #283 into normalizeSchemaNames itself: after replacing temp schema names with the target schema, strip redundant same-schema function and type cast qualifiers from expression fields (column defaults, generated expressions, check clauses, policy expressions, trigger conditions). This is more targeted than re-running the full normalizeIR pass. Co-Authored-By: Claude Opus 4.6 --- cmd/plan/plan.go | 49 +++++++++---- cmd/plan/plan_test.go | 165 ++++++++++++++++++++++++++++++++++++++++++ ir/normalize.go | 8 -- ir/normalize_test.go | 59 +-------------- 4 files changed, 201 insertions(+), 80 deletions(-) diff --git a/cmd/plan/plan.go b/cmd/plan/plan.go index 24fa9155..6f3855e5 100644 --- a/cmd/plan/plan.go +++ b/cmd/plan/plan.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "strings" "github.com/pgplex/pgschema/cmd/util" @@ -282,14 +283,6 @@ func GeneratePlan(config *PlanConfig, provider postgres.DesiredStateProvider) (* // Without this normalization, DDL would reference non-existent temporary schemas and fail. if schemaToInspect != config.Schema { normalizeSchemaNames(desiredStateIR, schemaToInspect, config.Schema) - // Re-run IR normalization after schema name replacement. The first normalization - // (in the inspector) ran with temporary schema names (pgschema_tmp_*), which meant - // same-schema qualifier stripping didn't work correctly for functions/types in the - // target schema (e.g., "public.uuid_generate_v1mc()" was not stripped because the - // table was in "pgschema_tmp_*", not "public"). After replacing temp schema names - // with the target schema, re-running normalization ensures qualifiers are properly - // stripped using the correct schema context. See issue #283. - ir.NormalizeIR(desiredStateIR) } // Generate diff (current -> desired) using IR directly @@ -411,6 +404,12 @@ func processOutput(migrationPlan *plan.Plan, output outputSpec, cmd *cobra.Comma // and fail when applied to the target database. func normalizeSchemaNames(irData *ir.IR, fromSchema, toSchema string) { replaceString := newSchemaStringReplacer(fromSchema, toSchema) + // stripQualifiers removes same-schema function/type qualifiers from expressions. + // After replaceString converts temp schema references to toSchema, expressions may + // contain "toSchema.func_name(" or "::toSchema.type" which are redundant same-schema + // qualifiers. The initial normalizeIR (run by the inspector) couldn't strip these + // because it ran with the temp schema name, not the target schema. See issue #283. + stripQualifiers := newSameSchemaQualifierStripper(toSchema) // Normalize schema names in Schemas map if schema, exists := irData.Schemas[fromSchema]; exists { @@ -433,7 +432,7 @@ func normalizeSchemaNames(irData *ir.IR, fromSchema, toSchema string) { if constraint.ReferencedSchema == fromSchema { constraint.ReferencedSchema = toSchema } - constraint.CheckClause = replaceString(constraint.CheckClause) + constraint.CheckClause = stripQualifiers(replaceString(constraint.CheckClause)) } // Normalize schema references in table dependencies @@ -454,10 +453,10 @@ func normalizeSchemaNames(irData *ir.IR, fromSchema, toSchema string) { for _, column := range table.Columns { column.DataType = replaceString(column.DataType) if column.DefaultValue != nil { - *column.DefaultValue = replaceString(*column.DefaultValue) + *column.DefaultValue = stripQualifiers(replaceString(*column.DefaultValue)) } if column.GeneratedExpr != nil { - *column.GeneratedExpr = replaceString(*column.GeneratedExpr) + *column.GeneratedExpr = stripQualifiers(replaceString(*column.GeneratedExpr)) } } @@ -475,7 +474,7 @@ func normalizeSchemaNames(irData *ir.IR, fromSchema, toSchema string) { trigger.Schema = toSchema } trigger.Function = replaceString(trigger.Function) - trigger.Condition = replaceString(trigger.Condition) + trigger.Condition = stripQualifiers(replaceString(trigger.Condition)) } // Normalize schema names in RLS policies @@ -483,8 +482,8 @@ func normalizeSchemaNames(irData *ir.IR, fromSchema, toSchema string) { if policy.Schema == fromSchema { policy.Schema = toSchema } - policy.Using = replaceString(policy.Using) - policy.WithCheck = replaceString(policy.WithCheck) + policy.Using = stripQualifiers(replaceString(policy.Using)) + policy.WithCheck = stripQualifiers(replaceString(policy.WithCheck)) } } @@ -635,6 +634,28 @@ func newSchemaStringReplacer(fromSchema, toSchema string) func(string) string { } } +// newSameSchemaQualifierStripper creates a function that strips redundant same-schema +// qualifiers from SQL expressions. After normalizeSchemaNames replaces temp schema names +// with the target schema, expressions may contain "schema.func_name(" or "::schema.type" +// where the qualifier matches the object's own schema. These are redundant and must be +// stripped to match how the target database's inspector would produce them. See issue #283. +func newSameSchemaQualifierStripper(schema string) func(string) string { + if schema == "" { + return func(s string) string { return s } + } + prefix := schema + "." + funcPattern := regexp.MustCompile(regexp.QuoteMeta(prefix) + `([a-zA-Z_][a-zA-Z0-9_]*)\(`) + typePattern := regexp.MustCompile(`::` + regexp.QuoteMeta(prefix)) + return func(s string) string { + if s == "" || !strings.Contains(s, prefix) { + return s + } + s = funcPattern.ReplaceAllString(s, `${1}(`) + s = typePattern.ReplaceAllString(s, "::") + return s + } +} + // ResetFlags resets all global flag variables to their default values for testing func ResetFlags() { planHost = "localhost" diff --git a/cmd/plan/plan_test.go b/cmd/plan/plan_test.go index 5fc286af..43b981e5 100644 --- a/cmd/plan/plan_test.go +++ b/cmd/plan/plan_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "testing" + "github.com/pgplex/pgschema/ir" "github.com/spf13/cobra" ) @@ -199,3 +200,167 @@ func TestPlanCommandFileError(t *testing.T) { t.Error("Expected error when file doesn't exist, but got none") } } + +// TestNormalizeSchemaNames_StripsSameSchemaQualifiers verifies that normalizeSchemaNames +// strips redundant same-schema qualifiers from expressions after replacing temp schema names. +// This is a regression test for GitHub issue #283. +func TestNormalizeSchemaNames_StripsSameSchemaQualifiers(t *testing.T) { + tempSchema := "pgschema_tmp_20260101_120000_abcd1234" + + defaultVal := "public.my_default_id()" + genExpr := "public.compute_hash(name)" + checkClause := "(public.is_valid(status))" + policyUsing := "(public.current_user_id() = user_id)" + triggerCond := "public.should_audit()" + + testIR := &ir.IR{ + Schemas: map[string]*ir.Schema{ + tempSchema: { + Name: tempSchema, + Tables: map[string]*ir.Table{ + "items": { + Name: "items", + Schema: tempSchema, + Columns: []*ir.Column{ + { + Name: "id", + DataType: "uuid", + DefaultValue: &defaultVal, + }, + { + Name: "hash", + DataType: "text", + GeneratedExpr: &genExpr, + }, + }, + Constraints: map[string]*ir.Constraint{ + "items_check": { + Name: "items_check", + Schema: tempSchema, + CheckClause: checkClause, + }, + }, + Policies: map[string]*ir.RLSPolicy{ + "items_policy": { + Name: "items_policy", + Schema: tempSchema, + Using: policyUsing, + WithCheck: "", + }, + }, + Triggers: map[string]*ir.Trigger{ + "items_trigger": { + Name: "items_trigger", + Schema: tempSchema, + Condition: triggerCond, + Function: tempSchema + ".audit_func", + }, + }, + }, + }, + }, + }, + } + + normalizeSchemaNames(testIR, tempSchema, "public") + + table := testIR.Schemas["public"].Tables["items"] + + // Column default: "public.my_default_id()" → "my_default_id()" + if got := *table.Columns[0].DefaultValue; got != "my_default_id()" { + t.Errorf("column default: got %q, want %q", got, "my_default_id()") + } + + // Generated expression: "public.compute_hash(name)" → "compute_hash(name)" + if got := *table.Columns[1].GeneratedExpr; got != "compute_hash(name)" { + t.Errorf("generated expr: got %q, want %q", got, "compute_hash(name)") + } + + // Check clause: "(public.is_valid(status))" → "(is_valid(status))" + if got := table.Constraints["items_check"].CheckClause; got != "(is_valid(status))" { + t.Errorf("check clause: got %q, want %q", got, "(is_valid(status))") + } + + // Policy USING: "(public.current_user_id() = user_id)" → "(current_user_id() = user_id)" + if got := table.Policies["items_policy"].Using; got != "(current_user_id() = user_id)" { + t.Errorf("policy using: got %q, want %q", got, "(current_user_id() = user_id)") + } + + // Trigger condition: "public.should_audit()" → "should_audit()" + if got := table.Triggers["items_trigger"].Condition; got != "should_audit()" { + t.Errorf("trigger condition: got %q, want %q", got, "should_audit()") + } + + // Trigger function: schema replaced but NOT stripped (it's a schema.name reference, not an expression) + if got := table.Triggers["items_trigger"].Function; got != "public.audit_func" { + t.Errorf("trigger function: got %q, want %q", got, "public.audit_func") + } +} + +// TestNormalizeSchemaNames_PreservesCrossSchemaQualifiers verifies that cross-schema +// qualifiers are preserved (not stripped) during normalization. +func TestNormalizeSchemaNames_PreservesCrossSchemaQualifiers(t *testing.T) { + tempSchema := "pgschema_tmp_20260101_120000_abcd1234" + + defaultVal := "other_schema.my_func()" + testIR := &ir.IR{ + Schemas: map[string]*ir.Schema{ + tempSchema: { + Name: tempSchema, + Tables: map[string]*ir.Table{ + "items": { + Name: "items", + Schema: tempSchema, + Columns: []*ir.Column{ + { + Name: "id", + DataType: "uuid", + DefaultValue: &defaultVal, + }, + }, + }, + }, + }, + }, + } + + normalizeSchemaNames(testIR, tempSchema, "public") + + if got := *testIR.Schemas["public"].Tables["items"].Columns[0].DefaultValue; got != "other_schema.my_func()" { + t.Errorf("cross-schema qualifier should be preserved: got %q, want %q", got, "other_schema.my_func()") + } +} + +// TestNormalizeSchemaNames_TypeCastQualifiers verifies that same-schema type cast +// qualifiers are stripped during normalization. +func TestNormalizeSchemaNames_TypeCastQualifiers(t *testing.T) { + tempSchema := "pgschema_tmp_20260101_120000_abcd1234" + + defaultVal := "'active'::public.status_type" + testIR := &ir.IR{ + Schemas: map[string]*ir.Schema{ + tempSchema: { + Name: tempSchema, + Tables: map[string]*ir.Table{ + "items": { + Name: "items", + Schema: tempSchema, + Columns: []*ir.Column{ + { + Name: "status", + DataType: "text", + DefaultValue: &defaultVal, + }, + }, + }, + }, + }, + }, + } + + normalizeSchemaNames(testIR, tempSchema, "public") + + if got := *testIR.Schemas["public"].Tables["items"].Columns[0].DefaultValue; got != "'active'::status_type" { + t.Errorf("type cast qualifier should be stripped: got %q, want %q", got, "'active'::status_type") + } +} diff --git a/ir/normalize.go b/ir/normalize.go index d5389151..0e1e4af0 100644 --- a/ir/normalize.go +++ b/ir/normalize.go @@ -8,14 +8,6 @@ import ( "unicode" ) -// NormalizeIR normalizes the IR representation. This function is idempotent and safe to -// call multiple times. It must be re-run after renaming temporary schema names to target -// schema names (e.g., pgschema_tmp_* → public) to ensure same-schema qualifier stripping -// uses the correct schema context. See issue #283. -func NormalizeIR(ir *IR) { - normalizeIR(ir) -} - // normalizeIR normalizes the IR representation from the inspector. // // Since both desired state (from embedded postgres) and current state (from target database) diff --git a/ir/normalize_test.go b/ir/normalize_test.go index eda3f385..cf52f7cb 100644 --- a/ir/normalize_test.go +++ b/ir/normalize_test.go @@ -15,7 +15,7 @@ import ( // it doesn't match the temp schema name. On the target database, normalizeDefaultValue // with tableSchema="public" DOES strip it. This causes a spurious diff. // -// The fix: re-run normalizeIR after normalizeSchemaNames replaces temp → target schema. +// The fix: normalizeSchemaNames strips same-schema qualifiers after replacing temp → target. func TestNormalizeDefaultValue_TempSchemaFunctionQualifier(t *testing.T) { tests := []struct { name string @@ -59,63 +59,6 @@ func TestNormalizeDefaultValue_TempSchemaFunctionQualifier(t *testing.T) { } } -// TestNormalizeIR_RerunAfterSchemaRename verifies that re-running normalizeIR -// after renaming temp schema to target schema produces correct results. -// This is the key behavior that fixes issue #283. -func TestNormalizeIR_RerunAfterSchemaRename(t *testing.T) { - tempSchema := "pgschema_tmp_20260101_120000_abcd1234" - - // Simulate desired state IR from temp schema inspection - defaultVal := "public.uuid_generate_v1mc()" - testIR := &IR{ - Schemas: map[string]*Schema{ - tempSchema: { - Name: tempSchema, - Tables: map[string]*Table{ - "items": { - Name: "items", - Schema: tempSchema, - Columns: []*Column{ - { - Name: "id", - DataType: "uuid", - DefaultValue: &defaultVal, - }, - }, - }, - }, - }, - }, - } - - // First normalization pass (happens in inspector) - normalizeIR(testIR) - - // After first normalization, "public." is NOT stripped (table schema is temp) - col := testIR.Schemas[tempSchema].Tables["items"].Columns[0] - if *col.DefaultValue != "public.uuid_generate_v1mc()" { - t.Fatalf("After first normalizeIR, expected default to remain 'public.uuid_generate_v1mc()', got %q", *col.DefaultValue) - } - - // Simulate normalizeSchemaNames: rename temp schema to public - schema := testIR.Schemas[tempSchema] - delete(testIR.Schemas, tempSchema) - schema.Name = "public" - testIR.Schemas["public"] = schema - for _, table := range schema.Tables { - table.Schema = "public" - } - - // Second normalization pass (the fix: re-run after schema rename) - normalizeIR(testIR) - - // After second normalization, "public." should be stripped - col = testIR.Schemas["public"].Tables["items"].Columns[0] - if *col.DefaultValue != "uuid_generate_v1mc()" { - t.Errorf("After re-running normalizeIR with correct schema, expected 'uuid_generate_v1mc()', got %q", *col.DefaultValue) - } -} - func TestNormalizeCheckClause(t *testing.T) { tests := []struct { name string From d9fb25c5b9691c4035141eba6682b31d769df98d Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Sun, 15 Feb 2026 09:37:56 -0800 Subject: [PATCH 3/4] fix: update expected test output for generated column expression qualifier stripping The table_fk_to_generated_column test fixture had "public.calc_priority()" in its expected plan output. With the same-schema qualifier stripping fix, the correct output is now "calc_priority()" without the redundant qualifier. Co-Authored-By: Claude Opus 4.6 --- .../diff/dependency/table_fk_to_generated_column/plan.json | 4 ++-- .../diff/dependency/table_fk_to_generated_column/plan.sql | 2 +- .../diff/dependency/table_fk_to_generated_column/plan.txt | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/testdata/diff/dependency/table_fk_to_generated_column/plan.json b/testdata/diff/dependency/table_fk_to_generated_column/plan.json index 61bd1c44..a4b150ab 100644 --- a/testdata/diff/dependency/table_fk_to_generated_column/plan.json +++ b/testdata/diff/dependency/table_fk_to_generated_column/plan.json @@ -1,6 +1,6 @@ { "version": "1.0.0", - "pgschema_version": "1.6.2", + "pgschema_version": "1.7.0", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { "hash": "965b1131737c955e24c7f827c55bd78e4cb49a75adfd04229e0ba297376f5085" @@ -21,7 +21,7 @@ "path": "public.calc_priority" }, { - "sql": "CREATE TABLE IF NOT EXISTS article (\n id integer,\n title text NOT NULL,\n priority integer GENERATED ALWAYS AS (public.calc_priority()) STORED,\n CONSTRAINT article_pkey PRIMARY KEY (id)\n);", + "sql": "CREATE TABLE IF NOT EXISTS article (\n id integer,\n title text NOT NULL,\n priority integer GENERATED ALWAYS AS (calc_priority()) STORED,\n CONSTRAINT article_pkey PRIMARY KEY (id)\n);", "type": "table", "operation": "create", "path": "public.article" diff --git a/testdata/diff/dependency/table_fk_to_generated_column/plan.sql b/testdata/diff/dependency/table_fk_to_generated_column/plan.sql index ab07d9d7..7df1404b 100644 --- a/testdata/diff/dependency/table_fk_to_generated_column/plan.sql +++ b/testdata/diff/dependency/table_fk_to_generated_column/plan.sql @@ -15,7 +15,7 @@ $$; CREATE TABLE IF NOT EXISTS article ( id integer, title text NOT NULL, - priority integer GENERATED ALWAYS AS (public.calc_priority()) STORED, + priority integer GENERATED ALWAYS AS (calc_priority()) STORED, CONSTRAINT article_pkey PRIMARY KEY (id) ); diff --git a/testdata/diff/dependency/table_fk_to_generated_column/plan.txt b/testdata/diff/dependency/table_fk_to_generated_column/plan.txt index b93f5d0d..9b704764 100644 --- a/testdata/diff/dependency/table_fk_to_generated_column/plan.txt +++ b/testdata/diff/dependency/table_fk_to_generated_column/plan.txt @@ -32,7 +32,7 @@ $$; CREATE TABLE IF NOT EXISTS article ( id integer, title text NOT NULL, - priority integer GENERATED ALWAYS AS (public.calc_priority()) STORED, + priority integer GENERATED ALWAYS AS (calc_priority()) STORED, CONSTRAINT article_pkey PRIMARY KEY (id) ); From 3dcc7687bab677fd18b35e7d4b7c3d100b63d33e Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Sun, 15 Feb 2026 09:46:10 -0800 Subject: [PATCH 4/4] chore: remove redundant unit tests for qualifier stripping Integration tests (issue_283_function_default_schema_qualifier and dependency_table_fk_to_generated_column) already cover the end-to-end behavior. The unit tests for normalizeDefaultValue and normalizeSchemaNames were testing pre-existing or already-covered behavior. Co-Authored-By: Claude Opus 4.6 --- cmd/plan/plan_test.go | 165 ------------------------------------------ ir/normalize_test.go | 55 -------------- 2 files changed, 220 deletions(-) diff --git a/cmd/plan/plan_test.go b/cmd/plan/plan_test.go index 43b981e5..5fc286af 100644 --- a/cmd/plan/plan_test.go +++ b/cmd/plan/plan_test.go @@ -6,7 +6,6 @@ import ( "path/filepath" "testing" - "github.com/pgplex/pgschema/ir" "github.com/spf13/cobra" ) @@ -200,167 +199,3 @@ func TestPlanCommandFileError(t *testing.T) { t.Error("Expected error when file doesn't exist, but got none") } } - -// TestNormalizeSchemaNames_StripsSameSchemaQualifiers verifies that normalizeSchemaNames -// strips redundant same-schema qualifiers from expressions after replacing temp schema names. -// This is a regression test for GitHub issue #283. -func TestNormalizeSchemaNames_StripsSameSchemaQualifiers(t *testing.T) { - tempSchema := "pgschema_tmp_20260101_120000_abcd1234" - - defaultVal := "public.my_default_id()" - genExpr := "public.compute_hash(name)" - checkClause := "(public.is_valid(status))" - policyUsing := "(public.current_user_id() = user_id)" - triggerCond := "public.should_audit()" - - testIR := &ir.IR{ - Schemas: map[string]*ir.Schema{ - tempSchema: { - Name: tempSchema, - Tables: map[string]*ir.Table{ - "items": { - Name: "items", - Schema: tempSchema, - Columns: []*ir.Column{ - { - Name: "id", - DataType: "uuid", - DefaultValue: &defaultVal, - }, - { - Name: "hash", - DataType: "text", - GeneratedExpr: &genExpr, - }, - }, - Constraints: map[string]*ir.Constraint{ - "items_check": { - Name: "items_check", - Schema: tempSchema, - CheckClause: checkClause, - }, - }, - Policies: map[string]*ir.RLSPolicy{ - "items_policy": { - Name: "items_policy", - Schema: tempSchema, - Using: policyUsing, - WithCheck: "", - }, - }, - Triggers: map[string]*ir.Trigger{ - "items_trigger": { - Name: "items_trigger", - Schema: tempSchema, - Condition: triggerCond, - Function: tempSchema + ".audit_func", - }, - }, - }, - }, - }, - }, - } - - normalizeSchemaNames(testIR, tempSchema, "public") - - table := testIR.Schemas["public"].Tables["items"] - - // Column default: "public.my_default_id()" → "my_default_id()" - if got := *table.Columns[0].DefaultValue; got != "my_default_id()" { - t.Errorf("column default: got %q, want %q", got, "my_default_id()") - } - - // Generated expression: "public.compute_hash(name)" → "compute_hash(name)" - if got := *table.Columns[1].GeneratedExpr; got != "compute_hash(name)" { - t.Errorf("generated expr: got %q, want %q", got, "compute_hash(name)") - } - - // Check clause: "(public.is_valid(status))" → "(is_valid(status))" - if got := table.Constraints["items_check"].CheckClause; got != "(is_valid(status))" { - t.Errorf("check clause: got %q, want %q", got, "(is_valid(status))") - } - - // Policy USING: "(public.current_user_id() = user_id)" → "(current_user_id() = user_id)" - if got := table.Policies["items_policy"].Using; got != "(current_user_id() = user_id)" { - t.Errorf("policy using: got %q, want %q", got, "(current_user_id() = user_id)") - } - - // Trigger condition: "public.should_audit()" → "should_audit()" - if got := table.Triggers["items_trigger"].Condition; got != "should_audit()" { - t.Errorf("trigger condition: got %q, want %q", got, "should_audit()") - } - - // Trigger function: schema replaced but NOT stripped (it's a schema.name reference, not an expression) - if got := table.Triggers["items_trigger"].Function; got != "public.audit_func" { - t.Errorf("trigger function: got %q, want %q", got, "public.audit_func") - } -} - -// TestNormalizeSchemaNames_PreservesCrossSchemaQualifiers verifies that cross-schema -// qualifiers are preserved (not stripped) during normalization. -func TestNormalizeSchemaNames_PreservesCrossSchemaQualifiers(t *testing.T) { - tempSchema := "pgschema_tmp_20260101_120000_abcd1234" - - defaultVal := "other_schema.my_func()" - testIR := &ir.IR{ - Schemas: map[string]*ir.Schema{ - tempSchema: { - Name: tempSchema, - Tables: map[string]*ir.Table{ - "items": { - Name: "items", - Schema: tempSchema, - Columns: []*ir.Column{ - { - Name: "id", - DataType: "uuid", - DefaultValue: &defaultVal, - }, - }, - }, - }, - }, - }, - } - - normalizeSchemaNames(testIR, tempSchema, "public") - - if got := *testIR.Schemas["public"].Tables["items"].Columns[0].DefaultValue; got != "other_schema.my_func()" { - t.Errorf("cross-schema qualifier should be preserved: got %q, want %q", got, "other_schema.my_func()") - } -} - -// TestNormalizeSchemaNames_TypeCastQualifiers verifies that same-schema type cast -// qualifiers are stripped during normalization. -func TestNormalizeSchemaNames_TypeCastQualifiers(t *testing.T) { - tempSchema := "pgschema_tmp_20260101_120000_abcd1234" - - defaultVal := "'active'::public.status_type" - testIR := &ir.IR{ - Schemas: map[string]*ir.Schema{ - tempSchema: { - Name: tempSchema, - Tables: map[string]*ir.Table{ - "items": { - Name: "items", - Schema: tempSchema, - Columns: []*ir.Column{ - { - Name: "status", - DataType: "text", - DefaultValue: &defaultVal, - }, - }, - }, - }, - }, - }, - } - - normalizeSchemaNames(testIR, tempSchema, "public") - - if got := *testIR.Schemas["public"].Tables["items"].Columns[0].DefaultValue; got != "'active'::status_type" { - t.Errorf("type cast qualifier should be stripped: got %q, want %q", got, "'active'::status_type") - } -} diff --git a/ir/normalize_test.go b/ir/normalize_test.go index cf52f7cb..528de4d5 100644 --- a/ir/normalize_test.go +++ b/ir/normalize_test.go @@ -4,61 +4,6 @@ import ( "testing" ) -// TestNormalizeDefaultValue_TempSchemaFunctionQualifier tests that column defaults -// referencing functions in the public schema are properly normalized when the table -// is in a temporary schema (pgschema_tmp_*). -// -// This is a regression test for GitHub issue #283: -// When pgschema plans a migration using a temp schema for desired state, pg_get_expr() -// with search_path=pg_catalog returns "public.func_name()" for functions in the public -// schema. normalizeDefaultValue with the temp schema name can't strip "public." because -// it doesn't match the temp schema name. On the target database, normalizeDefaultValue -// with tableSchema="public" DOES strip it. This causes a spurious diff. -// -// The fix: normalizeSchemaNames strips same-schema qualifiers after replacing temp → target. -func TestNormalizeDefaultValue_TempSchemaFunctionQualifier(t *testing.T) { - tests := []struct { - name string - value string - tableSchema string - expected string - }{ - { - name: "public function stripped when table in public schema", - value: "public.uuid_generate_v1mc()", - tableSchema: "public", - expected: "uuid_generate_v1mc()", - }, - { - name: "public function NOT stripped when table in temp schema (the bug)", - value: "public.uuid_generate_v1mc()", - tableSchema: "pgschema_tmp_20260101_120000_abcd1234", - expected: "public.uuid_generate_v1mc()", - }, - { - name: "temp schema function stripped when table in same temp schema", - value: "pgschema_tmp_20260101_120000_abcd1234.my_func()", - tableSchema: "pgschema_tmp_20260101_120000_abcd1234", - expected: "my_func()", - }, - { - name: "cross-schema function preserved", - value: "other_schema.my_func()", - tableSchema: "public", - expected: "other_schema.my_func()", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := normalizeDefaultValue(tt.value, tt.tableSchema) - if result != tt.expected { - t.Errorf("normalizeDefaultValue(%q, %q) = %q, want %q", tt.value, tt.tableSchema, result, tt.expected) - } - }) - } -} - func TestNormalizeCheckClause(t *testing.T) { tests := []struct { name string