feat(dump): qualify same-schema type references under --qualify-schema (#493)

cmd/dump/dump.go

@@ -59,7 +59,7 @@ func init() {
 	DumpCmd.Flags().BoolVar(&multiFile, "multi-file", false, "Output schema to multiple files organized by object type")
 	DumpCmd.Flags().StringVar(&file, "file", "", "Output file path (required when --multi-file is used)")
 	DumpCmd.Flags().BoolVar(&noComments, "no-comments", false, "Do not output object comment headers")
-	DumpCmd.Flags().BoolVar(&qualifySchema, "qualify-schema", false, "Always schema-qualify object identifiers in the dump, even for the target schema (does not affect same-schema type references, which the IR stores without a schema)")
+	DumpCmd.Flags().BoolVar(&qualifySchema, "qualify-schema", false, "Always schema-qualify object identifiers and type references in the dump, even for the target schema (function/procedure parameter and return types are not yet qualified)")
 	DumpCmd.Flags().StringVar(&sslmode, "sslmode", "prefer", "SSL mode for database connection (disable, allow, prefer, require, verify-ca, verify-full) (env: PGSSLMODE)")
 }
 

cmd/dump/qualify_schema_integration_test.go

@@ -0,0 +1,121 @@
+package dump
+
+// Positive integration coverage for `dump --qualify-schema` type references (#493).
+//
+// Unlike the builder-level tests in internal/diff (which hand the builder a
+// pre-qualified IR), this test drives the whole pipeline: pgdump SQL → embedded
+// database → inspector → IR → dump. It proves the inspector now *preserves* the
+// schema of same-schema user-defined type references for the four "Mechanism A"
+// slices (column type, domain base type, composite attribute, aggregate state
+// type), so --qualify-schema can emit them fully qualified — while the default
+// (smart-qualification) dump keeps them bare.
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/pgplex/pgschema/testutil"
+)
+
+const qualifySchemaSetupSQL = `
+CREATE TYPE color AS ENUM ('r', 'g', 'b');
+-- column type: same-schema enum (scalar and array)
+CREATE TABLE swatch (
+    id     integer PRIMARY KEY,
+    shade  color,
+    shades color[]
+);
+-- domain base type: same-schema enum (scalar and array)
+CREATE DOMAIN color_domain AS color;
+CREATE DOMAIN color_list AS color[];
+-- composite attribute: same-schema enum (scalar and array, plus a built-in that must stay bare)
+CREATE TYPE money_amount AS (
+    amount   numeric,
+    currency color,
+    palette  color[]
+);
+-- aggregate state type: same-schema composite
+CREATE TYPE acc AS (n integer);
+CREATE FUNCTION acc_add(acc, integer) RETURNS acc
+    LANGUAGE sql IMMUTABLE AS $$ SELECT ROW(($1).n + $2)::acc $$;
+CREATE AGGREGATE mysum(integer) (SFUNC = acc_add, STYPE = acc);
+`
+
+func TestDumpCommand_QualifySchemaTypeReferences(t *testing.T) {
+	if testing.Short() {
+		t.Skip("Skipping integration test in short mode")
+	}
+
+	embeddedPG := testutil.SetupPostgres(t)
+	defer embeddedPG.Stop()
+
+	conn, host, port, dbname, user, password := testutil.ConnectToPostgres(t, embeddedPG)
+	defer conn.Close()
+
+	if _, err := conn.ExecContext(context.Background(), qualifySchemaSetupSQL); err != nil {
+		t.Fatalf("Failed to set up schema: %v", err)
+	}
+
+	baseConfig := func(qualify bool) *DumpConfig {
+		return &DumpConfig{
+			Host:          host,
+			Port:          port,
+			DB:            dbname,
+			User:          user,
+			Password:      password,
+			Schema:        "public",
+			MultiFile:     false,
+			File:          "",
+			QualifySchema: qualify,
+		}
+	}
+
+	// --qualify-schema: same-schema type references are fully qualified.
+	qualified, err := ExecuteDump(baseConfig(true))
+	if err != nil {
+		t.Fatalf("qualified dump failed: %v", err)
+	}
+	for _, want := range []string{
+		"shade public.color",       // column type
+		"shades public.color[]",    // column type: same-schema array
+		"AS public.color;",         // domain base type: scalar (CREATE DOMAIN ... AS public.color)
+		"AS public.color[]",        // domain base type: array (CREATE DOMAIN ... AS public.color[])
+		"currency public.color",    // composite attribute: scalar
+		"palette public.color[]",   // composite attribute: array
+		"STYPE = public.acc",       // aggregate state type
+	} {
+		if !strings.Contains(qualified, want) {
+			t.Errorf("qualified dump missing %q\n---\n%s", want, qualified)
+		}
+	}
+	// Built-in types must never be schema-qualified.
+	if strings.Contains(qualified, "pg_catalog.") {
+		t.Errorf("qualified dump must not qualify built-in types with pg_catalog:\n%s", qualified)
+	}
+	if !strings.Contains(qualified, "amount numeric") {
+		t.Errorf("qualified dump should keep built-in composite attr type bare (amount numeric):\n%s", qualified)
+	}
+
+	// Default (smart qualification): the same references stay bare.
+	def, err := ExecuteDump(baseConfig(false))
+	if err != nil {
+		t.Fatalf("default dump failed: %v", err)
+	}
+	for _, want := range []string{
+		"shade color",
+		"shades color[]",
+		"AS color;",    // domain base type: scalar
+		"AS color[]",   // domain base type: array
+		"currency color",
+		"palette color[]",
+		"STYPE = acc",
+	} {
+		if !strings.Contains(def, want) {
+			t.Errorf("default dump missing bare form %q\n---\n%s", want, def)
+		}
+	}
+	if strings.Contains(def, "public.color") || strings.Contains(def, "public.acc") {
+		t.Errorf("default dump must not qualify same-schema type references:\n%s", def)
+	}
+}

internal/diff/qualify_schema_test.go

@@ -10,10 +10,13 @@ import (
 // These tests cover the `dump --qualify-schema` behavior at the SQL-builder level:
 // when qualifySchema is true, structural entity names are schema-qualified, even for
 // the target schema; when false (the default), the standard "smart qualification"
-// omits the target-schema prefix. Type references that arrive from the inspector
-// without schema identity stay bare (the flag cannot invent a schema for them) until
-// the IR preserves their schema separately (#493). The false cases double as the
-// byte-identical default-output guardrail for each builder.
+// omits the target-schema prefix. For type references whose schema the IR preserves
+// (columns, domain base types, composite attributes, aggregate state types — #493),
+// forced qualification keeps the schema and the default strips it. Function/procedure
+// parameter and return types still arrive from the inspector without schema identity
+// (it strips them during signature parsing), so for those the flag cannot invent a
+// schema and the reference stays bare. The false cases double as the byte-identical
+// default-output guardrail for each builder.
 
 func TestQualifySchema_ForeignKeyReference(t *testing.T) {
 	fk := &ir.Constraint{
@@ -48,9 +51,11 @@ func TestQualifySchema_TableAndColumnType(t *testing.T) {
 		Name:   "account",
 		Columns: []*ir.Column{
 			{Name: "id", Position: 1, DataType: "integer", IsNullable: false},
-			// Same-schema user-defined type references arrive from the inspector
-			// without schema identity, so the IR stores them bare (#493).
-			{Name: "kind", Position: 2, DataType: "user_kind", IsNullable: true},
+			// Post-#493 the inspector preserves the schema of same-schema
+			// user-defined type references, so the IR stores them qualified
+			// (e.g. public.user_kind). The render seam strips the target-schema
+			// prefix in default mode and keeps it under forced qualification.
+			{Name: "kind", Position: 2, DataType: "public.user_kind", IsNullable: true},
 		},
 	}
 	empty := map[string]bool{}
@@ -62,18 +67,41 @@ func TestQualifySchema_TableAndColumnType(t *testing.T) {
 	if strings.Contains(def, "public.account") || strings.Contains(def, "public.user_kind") {
 		t.Errorf("default should not qualify the target schema: %q", def)
 	}
+	if !strings.Contains(def, "kind user_kind") {
+		t.Errorf("default should strip the target-schema prefix from the type ref: %q", def)
+	}
 
 	qualified, _ := generateTableSQL(table, "public", true, empty, empty, empty, nil)
 	if !strings.Contains(qualified, "CREATE TABLE IF NOT EXISTS public.account (") {
 		t.Errorf("forced qualification should qualify the table name: %q", qualified)
 	}
-	// #493 limitation: forced qualification cannot *add* a schema to a bare
-	// same-schema type reference, so the column type stays bare.
-	if strings.Contains(qualified, "public.user_kind") {
-		t.Errorf("forced qualification must not invent a schema for a bare type ref: %q", qualified)
+	// #493: forced qualification keeps the schema the IR now carries on the
+	// same-schema type reference.
+	if !strings.Contains(qualified, "kind public.user_kind") {
+		t.Errorf("forced qualification should qualify the same-schema type ref: %q", qualified)
 	}
-	if !strings.Contains(qualified, "kind user_kind") {
-		t.Errorf("forced qualification should preserve the bare same-schema type ref: %q", qualified)
+}
+
+func TestStripSchemaPrefixMode_QuotedTargetSchema(t *testing.T) {
+	// The inspector qualifies same-schema type references via quote_ident, so a
+	// target schema whose name requires quoting arrives as "My Schema".user_kind.
+	// Default (smart-qualification) mode must still strip it; forced
+	// qualification keeps it. (Regression: previously only the raw, unquoted
+	// prefix was stripped, so quoted target schemas leaked into default output.)
+	const typeRef = `"My Schema".user_kind`
+	if got := stripSchemaPrefixMode(typeRef, "My Schema", false); got != "user_kind" {
+		t.Errorf("default should strip quoted target-schema prefix: got %q", got)
+	}
+	if got := stripSchemaPrefixMode(typeRef, "My Schema", true); got != typeRef {
+		t.Errorf("forced qualification should keep the ref: got %q", got)
+	}
+	// Cast form (e.g. a parameter default) with a quoted schema.
+	if got := stripSchemaPrefixMode(`'a'::"My Schema".mood`, "My Schema", false); got != `'a'::mood` {
+		t.Errorf("default should strip quoted schema in cast: got %q", got)
+	}
+	// Lowercase schema (no quoting required) is unaffected.
+	if got := stripSchemaPrefixMode("public.user_kind", "public", false); got != "user_kind" {
+		t.Errorf("default should strip unquoted prefix: got %q", got)
 	}
 }
 

internal/diff/table.go

@@ -25,17 +25,29 @@ func stripSchemaPrefixMode(typeName, targetSchema string, qualifySchema bool) st
 		return typeName
 	}
 
-	// Check if the type has the target schema prefix at the beginning
-	prefix := targetSchema + "."
-	if after, found := strings.CutPrefix(typeName, prefix); found {
-		return after
-	}
-
-	// Also handle type casts within expressions: ::schema.typename -> ::typename
-	// This is needed for function parameter default values like 'value'::schema.enum_type
-	castPrefix := "::" + targetSchema + "."
-	if strings.Contains(typeName, castPrefix) {
-		return strings.ReplaceAll(typeName, castPrefix, "::")
+	// The inspector emits schema prefixes on type references via quote_ident, so
+	// the prefix is quoted when the schema name requires it (e.g. "My Schema").
+	// Strip both the raw and quote_ident forms so default (smart-qualification)
+	// output omits the target-schema prefix regardless of whether it needs quoting.
+	// QuoteIdentifier is a no-op for names that don't require quoting, so the two
+	// forms coincide for the common lowercase case.
+	forms := []string{targetSchema}
+	if quoted := ir.QuoteIdentifier(targetSchema); quoted != targetSchema {
+		forms = append(forms, quoted)
+	}
+	for _, schema := range forms {
+		// Check if the type has the target schema prefix at the beginning
+		prefix := schema + "."
+		if after, found := strings.CutPrefix(typeName, prefix); found {
+			return after
+		}
+
+		// Also handle type casts within expressions: ::schema.typename -> ::typename
+		// This is needed for function parameter default values like 'value'::schema.enum_type
+		castPrefix := "::" + schema + "."
+		if strings.Contains(typeName, castPrefix) {
+			return strings.ReplaceAll(typeName, castPrefix, "::")
+		}
 	}
 
 	return typeName

internal/diff/topological.go

@@ -265,7 +265,7 @@ func topologicallySortTypes(types []*ir.Type) []*ir.Type {
 	typeMap := make(map[string]*ir.Type)
 	var insertionOrder []string
 	for _, t := range types {
-		key := t.Schema + "." + t.Name
+		key := typeGraphKey(t.Schema, t.Name)
 		typeMap[key] = t
 		insertionOrder = append(insertionOrder, key)
 	}
@@ -387,8 +387,18 @@ func topologicallySortTypes(types []*ir.Type) []*ir.Type {
 	return sortedTypes
 }
 
-// extractTypeName extracts a fully qualified type name from a data type string
-// It handles array notation (e.g., "status_type[]") and schema prefixes
+// typeGraphKey builds the lookup key used to identify a type in the dependency
+// graph. It joins schema and name with a NUL byte, which cannot appear in a
+// PostgreSQL identifier, so distinct (schema, name) pairs never collide even when
+// an identifier legally contains a '.' (e.g. schema "a.b" type "c" vs schema "a"
+// type "b.c").
+func typeGraphKey(schema, name string) string {
+	return schema + "\x00" + name
+}
+
+// extractTypeName maps a data type string to its dependency-graph key
+// (see typeGraphKey). It handles array notation (e.g., "status_type[]") and
+// schema prefixes.
 func extractTypeName(dataType, defaultSchema string) string {
 	if dataType == "" {
 		return ""
@@ -400,23 +410,56 @@ func extractTypeName(dataType, defaultSchema string) string {
 		typeName = typeName[:len(typeName)-2]
 	}
 
-	// Check if it's a schema-qualified name
-	if idx := findLastDot(typeName); idx != -1 {
-		return typeName // Already fully qualified
+	// Strip a trailing typmod suffix (e.g. "(10,2)") that the inspector appends
+	// when qualifying user-defined scalar types, so it matches the bare typeMap
+	// key. Only a purely numeric/comma/space typmod is stripped, so a quoted
+	// identifier that legally contains parentheses (which ends in '"', not ')')
+	// is left intact.
+	if i := strings.LastIndexByte(typeName, '('); i > 0 && strings.HasSuffix(typeName, ")") {
+		inner := typeName[i+1 : len(typeName)-1]
+		if inner != "" && strings.IndexFunc(inner, func(r rune) bool {
+			return !(r >= '0' && r <= '9' || r == ',' || r == ' ')
+		}) == -1 {
+			typeName = typeName[:i]
+		}
+	}
+
+	// Check if it's a schema-qualified name. The inspector emits user-defined
+	// type references via quote_ident (e.g. public."user"), so normalize each
+	// component by stripping quote_ident quoting before building the key.
+	if idx := findLastUnquotedDot(typeName); idx != -1 {
+		return typeGraphKey(unquoteIdent(typeName[:idx]), unquoteIdent(typeName[idx+1:]))
 	}
 
 	// Not qualified - use default schema
-	return defaultSchema + "." + typeName
+	return typeGraphKey(unquoteIdent(defaultSchema), unquoteIdent(typeName))
 }
 
-// findLastDot finds the last dot in a string, returning -1 if not found
-func findLastDot(s string) int {
-	for i := len(s) - 1; i >= 0; i-- {
-		if s[i] == '.' {
-			return i
-		}
+// findLastUnquotedDot finds the last '.' that is not inside a double-quoted
+// identifier, returning -1 if none. This avoids splitting on a dot that is part
+// of a quoted identifier (e.g. the name in public."odd.name").
+func findLastUnquotedDot(s string) int {
+	inQuote := false
+	last := -1
+	for i := 0; i < len(s); i++ {
+		switch {
+		case s[i] == '"':
+			inQuote = !inQuote
+		case s[i] == '.' && !inQuote:
+			last = i
+		}
+	}
+	return last
+}
+
+// unquoteIdent removes quote_ident double-quoting from a single identifier
+// (e.g. "user" -> user, "Weird""Name" -> Weird"Name). Bare identifiers are
+// returned unchanged so this is safe on already-unquoted names.
+func unquoteIdent(s string) string {
+	if len(s) >= 2 && s[0] == '"' && s[len(s)-1] == '"' {
+		return strings.ReplaceAll(s[1:len(s)-1], `""`, `"`)
 	}
-	return -1
+	return s
 }
 
 // topologicallySortFunctions sorts functions across all schemas in dependency order