diff --git a/cmd/dump/dump_integration_test.go b/cmd/dump/dump_integration_test.go index 0e20446e..b36af55c 100644 --- a/cmd/dump/dump_integration_test.go +++ b/cmd/dump/dump_integration_test.go @@ -46,11 +46,11 @@ func TestDumpCommand_TenantSchemas(t *testing.T) { runTenantSchemaTest(t, "tenant") } -func TestDumpCommand_Issue78ConstraintNotValid(t *testing.T) { +func TestDumpCommand_Issue78ConstraintNotValidAndQuoting(t *testing.T) { if testing.Short() { t.Skip("Skipping integration test in short mode") } - runExactMatchTest(t, "issue_78_constraint_not_valid") + runExactMatchTest(t, "issue_78_constraint_not_valid_and_quoting") } func TestDumpCommand_Issue80IndexNameQuote(t *testing.T) { diff --git a/internal/diff/constraint.go b/internal/diff/constraint.go index 8e3e52d5..fbf5a8f2 100644 --- a/internal/diff/constraint.go +++ b/internal/diff/constraint.go @@ -21,16 +21,16 @@ func generateConstraintSQL(constraint *ir.Constraint, targetSchema string) strin switch constraint.Type { case ir.ConstraintTypePrimaryKey: // Always include CONSTRAINT name to be explicit and consistent - return fmt.Sprintf("CONSTRAINT %s PRIMARY KEY (%s)", constraint.Name, strings.Join(getColumnNames(constraint.Columns), ", ")) + return fmt.Sprintf("CONSTRAINT %s PRIMARY KEY (%s)", ir.QuoteIdentifier(constraint.Name), strings.Join(getColumnNames(constraint.Columns), ", ")) case ir.ConstraintTypeUnique: // Always include CONSTRAINT name to be explicit and consistent - return fmt.Sprintf("CONSTRAINT %s UNIQUE (%s)", constraint.Name, strings.Join(getColumnNames(constraint.Columns), ", ")) + return fmt.Sprintf("CONSTRAINT %s UNIQUE (%s)", ir.QuoteIdentifier(constraint.Name), strings.Join(getColumnNames(constraint.Columns), ", ")) case ir.ConstraintTypeForeignKey: // Always include CONSTRAINT name to preserve explicit FK names // Use QualifyEntityNameWithQuotes to add schema qualifier when referencing tables in other schemas qualifiedRefTable := ir.QualifyEntityNameWithQuotes(constraint.ReferencedSchema, constraint.ReferencedTable, targetSchema) stmt := fmt.Sprintf("CONSTRAINT %s FOREIGN KEY (%s) REFERENCES %s (%s)", - constraint.Name, + ir.QuoteIdentifier(constraint.Name), strings.Join(getColumnNames(constraint.Columns), ", "), qualifiedRefTable, strings.Join(getColumnNames(constraint.ReferencedColumns), ", ")) // Only add ON UPDATE/DELETE if they are not the default "NO ACTION" @@ -57,7 +57,7 @@ func generateConstraintSQL(constraint *ir.Constraint, targetSchema string) strin // Generate CHECK constraint with proper NOT VALID placement // The CheckClause is normalized to exclude NOT VALID (stripped in normalize.go) // We append NOT VALID based on IsValid field, mimicking pg_dump behavior - result := fmt.Sprintf("CONSTRAINT %s %s", constraint.Name, constraint.CheckClause) + result := fmt.Sprintf("CONSTRAINT %s %s", ir.QuoteIdentifier(constraint.Name), constraint.CheckClause) if !constraint.IsValid { result += " NOT VALID" } diff --git a/internal/diff/table.go b/internal/diff/table.go index 6a278472..d6ba9032 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -601,7 +601,7 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector // 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) - sql := fmt.Sprintf("ALTER TABLE %s DROP CONSTRAINT %s;", tableName, constraint.Name) + sql := fmt.Sprintf("ALTER TABLE %s DROP CONSTRAINT %s;", tableName, ir.QuoteIdentifier(constraint.Name)) context := &diffContext{ Type: DiffTypeTableConstraint, @@ -663,13 +663,13 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector if len(constraint.Columns) == 1 && constraint.Columns[0].Name == column.Name { switch constraint.Type { case ir.ConstraintTypePrimaryKey: - inlineConstraint = fmt.Sprintf(" CONSTRAINT %s PRIMARY KEY", constraint.Name) + inlineConstraint = fmt.Sprintf(" CONSTRAINT %s PRIMARY KEY", ir.QuoteIdentifier(constraint.Name)) case ir.ConstraintTypeUnique: - inlineConstraint = fmt.Sprintf(" CONSTRAINT %s UNIQUE", constraint.Name) + inlineConstraint = fmt.Sprintf(" CONSTRAINT %s UNIQUE", ir.QuoteIdentifier(constraint.Name)) case ir.ConstraintTypeForeignKey: // For FK, use the generateForeignKeyClause with inline=true fkClause := generateForeignKeyClause(constraint, targetSchema, true) - inlineConstraint = fmt.Sprintf(" CONSTRAINT %s%s", constraint.Name, fkClause) + inlineConstraint = fmt.Sprintf(" CONSTRAINT %s%s", ir.QuoteIdentifier(constraint.Name), fkClause) case ir.ConstraintTypeCheck: // For CHECK, format the clause inline checkExpr := constraint.CheckClause @@ -682,7 +682,7 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector if !strings.HasPrefix(checkExpr, "(") { checkExpr = "(" + checkExpr + ")" } - inlineConstraint = fmt.Sprintf(" CONSTRAINT %s CHECK %s", constraint.Name, checkExpr) + inlineConstraint = fmt.Sprintf(" CONSTRAINT %s CHECK %s", ir.QuoteIdentifier(constraint.Name), checkExpr) } if inlineConstraint != "" { @@ -748,7 +748,7 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector } tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) sql := fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s UNIQUE (%s);", - tableName, constraint.Name, strings.Join(columnNames, ", ")) + tableName, ir.QuoteIdentifier(constraint.Name), strings.Join(columnNames, ", ")) context := &diffContext{ Type: DiffTypeTableConstraint, @@ -764,7 +764,7 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) clause := ensureCheckClauseParens(constraint.CheckClause) canonicalSQL := fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s %s;", - tableName, constraint.Name, clause) + tableName, ir.QuoteIdentifier(constraint.Name), clause) context := &diffContext{ Type: DiffTypeTableConstraint, @@ -785,7 +785,7 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) canonicalSQL := fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s FOREIGN KEY (%s) %s;", - tableName, constraint.Name, + tableName, ir.QuoteIdentifier(constraint.Name), strings.Join(columnNames, ", "), generateForeignKeyClause(constraint, targetSchema, false)) @@ -807,7 +807,7 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector } tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) sql := fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s PRIMARY KEY (%s);", - tableName, constraint.Name, strings.Join(columnNames, ", ")) + tableName, ir.QuoteIdentifier(constraint.Name), strings.Join(columnNames, ", ")) context := &diffContext{ Type: DiffTypeTableConstraint, @@ -826,7 +826,7 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector constraint := ConstraintDiff.New // Step 1: Drop the old constraint - dropSQL := fmt.Sprintf("ALTER TABLE %s DROP CONSTRAINT %s;", tableName, ConstraintDiff.Old.Name) + dropSQL := fmt.Sprintf("ALTER TABLE %s DROP CONSTRAINT %s;", tableName, ir.QuoteIdentifier(ConstraintDiff.Old.Name)) dropContext := &diffContext{ Type: DiffTypeTableConstraint, Operation: DiffOperationDrop, @@ -847,12 +847,12 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector columnNames = append(columnNames, ir.QuoteIdentifier(col.Name)) } addSQL = fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s UNIQUE (%s);", - tableName, constraint.Name, strings.Join(columnNames, ", ")) + tableName, ir.QuoteIdentifier(constraint.Name), strings.Join(columnNames, ", ")) case ir.ConstraintTypeCheck: // Add CHECK constraint with ensured outer parentheses addSQL = fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s %s;", - tableName, constraint.Name, ensureCheckClauseParens(constraint.CheckClause)) + tableName, ir.QuoteIdentifier(constraint.Name), ensureCheckClauseParens(constraint.CheckClause)) case ir.ConstraintTypeForeignKey: // Sort columns by position @@ -863,7 +863,7 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector } addSQL = fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s FOREIGN KEY (%s) %s;", - tableName, constraint.Name, + tableName, ir.QuoteIdentifier(constraint.Name), strings.Join(columnNames, ", "), generateForeignKeyClause(constraint, targetSchema, false)) @@ -875,7 +875,7 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector columnNames = append(columnNames, ir.QuoteIdentifier(col.Name)) } addSQL = fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s PRIMARY KEY (%s);", - tableName, constraint.Name, strings.Join(columnNames, ", ")) + tableName, ir.QuoteIdentifier(constraint.Name), strings.Join(columnNames, ", ")) } addContext := &diffContext{ diff --git a/internal/diff/trigger.go b/internal/diff/trigger.go index 7873a54e..4a1f8219 100644 --- a/internal/diff/trigger.go +++ b/internal/diff/trigger.go @@ -210,7 +210,7 @@ func generateTriggerSQLWithMode(trigger *ir.Trigger, targetSchema string) string var stmt string if trigger.IsConstraint { stmt = fmt.Sprintf("CREATE CONSTRAINT TRIGGER %s\n %s %s ON %s", - trigger.Name, trigger.Timing, eventList, tableName) + ir.QuoteIdentifier(trigger.Name), trigger.Timing, eventList, tableName) // Add deferrable clause for constraint triggers if trigger.Deferrable { diff --git a/internal/diff/type.go b/internal/diff/type.go index 24da95c4..a6fce58c 100644 --- a/internal/diff/type.go +++ b/internal/diff/type.go @@ -213,12 +213,12 @@ func generateAlterDomainStatements(oldDomain, newDomain *ir.Type, targetSchema s if newConstraint, exists := newConstraints[key]; !exists { // Constraint was removed if oldConstraint.Name != "" { - statements = append(statements, fmt.Sprintf("ALTER DOMAIN %s DROP CONSTRAINT %s;", domainName, oldConstraint.Name)) + statements = append(statements, fmt.Sprintf("ALTER DOMAIN %s DROP CONSTRAINT %s;", domainName, ir.QuoteIdentifier(oldConstraint.Name))) } // Note: unnamed constraints cannot be dropped individually } else if oldConstraint.Name != "" && oldConstraint.Definition != newConstraint.Definition { // Constraint exists but definition changed - need to drop and recreate - statements = append(statements, fmt.Sprintf("ALTER DOMAIN %s DROP CONSTRAINT %s;", domainName, oldConstraint.Name)) + statements = append(statements, fmt.Sprintf("ALTER DOMAIN %s DROP CONSTRAINT %s;", domainName, ir.QuoteIdentifier(oldConstraint.Name))) } } @@ -229,7 +229,7 @@ func generateAlterDomainStatements(oldDomain, newDomain *ir.Type, targetSchema s // Either new constraint or definition changed constraintDef := newConstraint.Definition if newConstraint.Name != "" { - statements = append(statements, fmt.Sprintf("ALTER DOMAIN %s ADD CONSTRAINT %s %s;", domainName, newConstraint.Name, constraintDef)) + statements = append(statements, fmt.Sprintf("ALTER DOMAIN %s ADD CONSTRAINT %s %s;", domainName, ir.QuoteIdentifier(newConstraint.Name), constraintDef)) } else { statements = append(statements, fmt.Sprintf("ALTER DOMAIN %s ADD %s;", domainName, constraintDef)) } @@ -295,7 +295,7 @@ func generateTypeSQL(typeObj *ir.Type, targetSchema string) string { for _, constraint := range typeObj.Constraints { constraintDef := constraint.Definition if constraint.Name != "" { - lines = append(lines, fmt.Sprintf(" CONSTRAINT %s %s", constraint.Name, constraintDef)) + lines = append(lines, fmt.Sprintf(" CONSTRAINT %s %s", ir.QuoteIdentifier(constraint.Name), constraintDef)) } else { lines = append(lines, fmt.Sprintf(" %s", constraintDef)) } diff --git a/internal/plan/rewrite.go b/internal/plan/rewrite.go index 95e58839..6549b053 100644 --- a/internal/plan/rewrite.go +++ b/internal/plan/rewrite.go @@ -194,9 +194,9 @@ func generateConstraintRewrite(constraint *ir.Constraint) []RewriteStep { tableName := getTableNameWithSchema(constraint.Schema, constraint.Table) notValidSQL := fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s %s NOT VALID;", - tableName, constraint.Name, constraint.CheckClause) + tableName, ir.QuoteIdentifier(constraint.Name), constraint.CheckClause) validateSQL := fmt.Sprintf("ALTER TABLE %s VALIDATE CONSTRAINT %s;", - tableName, constraint.Name) + tableName, ir.QuoteIdentifier(constraint.Name)) return []RewriteStep{ { @@ -249,9 +249,9 @@ func generateForeignKeyRewrite(constraint *ir.Constraint) []RewriteStep { } notValidSQL := fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s %s NOT VALID;", - tableName, constraint.Name, fkClause) + tableName, ir.QuoteIdentifier(constraint.Name), fkClause) validateSQL := fmt.Sprintf("ALTER TABLE %s VALIDATE CONSTRAINT %s;", - tableName, constraint.Name) + tableName, ir.QuoteIdentifier(constraint.Name)) return []RewriteStep{ { @@ -283,11 +283,11 @@ func generateColumnNotNullRewrite(_ *diff.ColumnDiff, path string) []RewriteStep // Step 1: Add CHECK constraint with NOT VALID addConstraintSQL := fmt.Sprintf("ALTER TABLE %s ADD CONSTRAINT %s CHECK (%s IS NOT NULL) NOT VALID;", - tableName, constraintName, column) + tableName, ir.QuoteIdentifier(constraintName), column) // Step 2: Validate the constraint validateConstraintSQL := fmt.Sprintf("ALTER TABLE %s VALIDATE CONSTRAINT %s;", - tableName, constraintName) + tableName, ir.QuoteIdentifier(constraintName)) // Step 3: Set column to NOT NULL setNotNullSQL := fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s SET NOT NULL;", diff --git a/testdata/dump/issue_78_constraint_not_valid/manifest.json b/testdata/dump/issue_78_constraint_not_valid/manifest.json deleted file mode 100644 index 7d19f966..00000000 --- a/testdata/dump/issue_78_constraint_not_valid/manifest.json +++ /dev/null @@ -1,8 +0,0 @@ -{ - "name": "issue_78_constraint_not_valid", - "description": "Test case for NOT VALID constraint dumping (GitHub issue #78)", - "source": "https://github.com/pgschema/pgschema/issues/78", - "notes": [ - "Tests that NOT VALID constraints are correctly preserved when dumping" - ] -} diff --git a/testdata/dump/issue_78_constraint_not_valid_and_quoting/manifest.json b/testdata/dump/issue_78_constraint_not_valid_and_quoting/manifest.json new file mode 100644 index 00000000..2d5aa84e --- /dev/null +++ b/testdata/dump/issue_78_constraint_not_valid_and_quoting/manifest.json @@ -0,0 +1,10 @@ +{ + "name": "issue_78_constraint_not_valid_and_quoting", + "description": "Test case for NOT VALID constraint preservation and constraint name quoting (GitHub issue #78)", + "source": "https://github.com/pgschema/pgschema/issues/78", + "notes": [ + "Tests that NOT VALID constraints are correctly preserved when dumping", + "Tests that constraint names with whitespace are properly quoted (e.g., 'price not negative')", + "Verifies constraint name quoting handles identifiers that need quotes per PostgreSQL rules" + ] +} diff --git a/testdata/dump/issue_78_constraint_not_valid/pgdump.sql b/testdata/dump/issue_78_constraint_not_valid_and_quoting/pgdump.sql similarity index 90% rename from testdata/dump/issue_78_constraint_not_valid/pgdump.sql rename to testdata/dump/issue_78_constraint_not_valid_and_quoting/pgdump.sql index d9af549d..a9cdd1e7 100644 --- a/testdata/dump/issue_78_constraint_not_valid/pgdump.sql +++ b/testdata/dump/issue_78_constraint_not_valid_and_quoting/pgdump.sql @@ -23,13 +23,14 @@ SET default_tablespace = ''; SET default_table_access_method = heap; -- --- Name: products; Type: TABLE; Schema: public; Owner: postgres +-- Name: products; Type: TABLE; Schema: public; Owner: - -- CREATE TABLE public.products ( id uuid DEFAULT gen_random_uuid() NOT NULL, name text NOT NULL, price numeric(10,2) NOT NULL, + CONSTRAINT "price not negative" CHECK ((price >= (0)::numeric)), CONSTRAINT products_price_positive CHECK ((price > (0)::numeric)) ); diff --git a/testdata/dump/issue_78_constraint_not_valid/pgschema.sql b/testdata/dump/issue_78_constraint_not_valid_and_quoting/pgschema.sql similarity index 90% rename from testdata/dump/issue_78_constraint_not_valid/pgschema.sql rename to testdata/dump/issue_78_constraint_not_valid_and_quoting/pgschema.sql index 45c696d4..27634782 100644 --- a/testdata/dump/issue_78_constraint_not_valid/pgschema.sql +++ b/testdata/dump/issue_78_constraint_not_valid_and_quoting/pgschema.sql @@ -14,6 +14,7 @@ CREATE TABLE IF NOT EXISTS products ( id uuid DEFAULT gen_random_uuid() NOT NULL, name text NOT NULL, price numeric(10,2) NOT NULL, + CONSTRAINT "price not negative" CHECK (price >= 0::numeric), CONSTRAINT products_price_positive CHECK (price > 0::numeric) ); diff --git a/testdata/dump/issue_78_constraint_not_valid/raw.sql b/testdata/dump/issue_78_constraint_not_valid_and_quoting/raw.sql similarity index 84% rename from testdata/dump/issue_78_constraint_not_valid/raw.sql rename to testdata/dump/issue_78_constraint_not_valid_and_quoting/raw.sql index c6090a5e..586d3711 100644 --- a/testdata/dump/issue_78_constraint_not_valid/raw.sql +++ b/testdata/dump/issue_78_constraint_not_valid_and_quoting/raw.sql @@ -20,6 +20,11 @@ CREATE TABLE products ( ALTER TABLE products ADD CONSTRAINT products_price_positive CHECK (price > 0); +-- Add constraint with whitespace in name to test proper quoting +-- Tests GitHub issue #78 comment about constraint names with spaces +ALTER TABLE products + ADD CONSTRAINT "price not negative" CHECK (price >= 0); + -- -- Case 2: Table WITH data - constraint added with NOT VALID