From f4f263d316697f39e9e4157ec49f3d1ac1408407 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Tue, 21 Oct 2025 11:50:22 +0800 Subject: [PATCH] fix: ignored objects should be excluded from fingerprint calc --- cmd/apply/apply.go | 18 +++-- cmd/ignore_integration_test.go | 122 ++++++++++++++++++++++++++------- cmd/plan/plan.go | 7 +- cmd/util/connection.go | 54 +++------------ 4 files changed, 122 insertions(+), 79 deletions(-) diff --git a/cmd/apply/apply.go b/cmd/apply/apply.go index 72ec9789..045353e3 100644 --- a/cmd/apply/apply.go +++ b/cmd/apply/apply.go @@ -32,14 +32,13 @@ var ( applyApplicationName string ) - var ApplyCmd = &cobra.Command{ Use: "apply", Short: "Apply migration plan to update a database schema", Long: "Apply a migration plan to update a database schema. Either provide a desired state file (--file) to generate and apply a plan, or provide a pre-generated plan file (--plan) to execute directly.", RunE: RunApply, SilenceUsage: true, - PreRunE: util.PreRunEWithEnvVarsAndConnectionAndApp(&applyDB, &applyUser, &applyHost, &applyPort, &applyApplicationName), + PreRunE: util.PreRunEWithEnvVarsAndConnectionAndApp(&applyDB, &applyUser, &applyHost, &applyPort, &applyApplicationName), } func init() { @@ -128,9 +127,15 @@ func RunApply(cmd *cobra.Command, args []string) error { } } + // Load ignore configuration for fingerprint validation + ignoreConfig, err := util.LoadIgnoreFileWithStructure() + if err != nil { + return fmt.Errorf("failed to load .pgschemaignore: %w", err) + } + // Validate schema fingerprint if plan has one if migrationPlan.SourceFingerprint != nil { - err := validateSchemaFingerprint(migrationPlan, applyHost, applyPort, applyDB, applyUser, finalPassword, applySchema, applyApplicationName) + err := validateSchemaFingerprint(migrationPlan, applyHost, applyPort, applyDB, applyUser, finalPassword, applySchema, applyApplicationName, ignoreConfig) if err != nil { return err } @@ -225,9 +230,10 @@ func RunApply(cmd *cobra.Command, args []string) error { } // validateSchemaFingerprint validates that the current database schema matches the expected fingerprint -func validateSchemaFingerprint(migrationPlan *plan.Plan, host string, port int, db, user, password, schema, applicationName string) error { - // Get current state from target database - currentStateIR, err := util.GetIRFromDatabase(host, port, db, user, password, schema, applicationName) +func validateSchemaFingerprint(migrationPlan *plan.Plan, host string, port int, db, user, password, schema, applicationName string, ignoreConfig *ir.IgnoreConfig) error { + // Get current state from target database with ignore config + // This ensures ignored objects are excluded from fingerprint calculation + currentStateIR, err := util.GetIRFromDatabase(host, port, db, user, password, schema, applicationName, ignoreConfig) if err != nil { return fmt.Errorf("failed to get current database state for fingerprint validation: %w", err) } diff --git a/cmd/ignore_integration_test.go b/cmd/ignore_integration_test.go index 0e585d44..687a9ec3 100644 --- a/cmd/ignore_integration_test.go +++ b/cmd/ignore_integration_test.go @@ -386,44 +386,104 @@ CREATE TABLE test_core_config ( } // testIgnoreApply tests the apply command with ignore functionality +// This test verifies that ignored objects are excluded from fingerprint calculation func testIgnoreApply(t *testing.T, containerInfo *testutil.ContainerInfo) { - // For the apply test, let's focus on testing that the ignore config is loaded - // and doesn't cause errors, rather than testing actual schema changes - // which seem to have fingerprint issues in this test environment - // Create .pgschemaignore file cleanup := createIgnoreFile(t) defer cleanup() - // Verify that ignored tables still exist before and after + // Verify that ignored objects exist before apply verifyIgnoredObjectsExist(t, containerInfo.Conn, "before apply") - // Create a minimal schema that should not conflict with fingerprints - minimalSchema := ` --- Just the essential regular objects + // Create a schema file with ONLY regular (non-ignored) objects + // This schema does NOT include ignored objects like sp_temp_cleanup, temp_*, fn_test_*, etc. + regularObjectsSchema := ` +-- Regular enum type (not ignored) CREATE TYPE user_status AS ENUM ('active', 'inactive', 'suspended'); +-- Regular tables (not ignored) CREATE TABLE users ( id SERIAL PRIMARY KEY, name TEXT NOT NULL, email TEXT UNIQUE NOT NULL, status user_status DEFAULT 'active' ); + +CREATE TABLE orders ( + id SERIAL PRIMARY KEY, + user_id INTEGER REFERENCES users(id), + total_amount DECIMAL(10,2) NOT NULL, + created_at TIMESTAMP DEFAULT NOW() +); + +CREATE TABLE products ( + id SERIAL PRIMARY KEY, + name TEXT NOT NULL, + price DECIMAL(10,2) NOT NULL +); + +-- Keep test_core_config (not ignored due to negation pattern !test_core_*) +CREATE TABLE test_core_config ( + id SERIAL PRIMARY KEY, + config_key TEXT NOT NULL, + config_value TEXT NOT NULL +); + +-- Regular sequence (not ignored) +CREATE SEQUENCE IF NOT EXISTS user_id_seq; + +-- Regular views (not ignored) +CREATE VIEW user_orders_view AS +SELECT u.name, u.email, o.total_amount, o.created_at +FROM users u +JOIN orders o ON u.id = o.user_id; + +CREATE VIEW product_summary AS +SELECT COUNT(*) as total_products, AVG(price) as avg_price +FROM products; + +-- Regular functions (not ignored) +CREATE OR REPLACE FUNCTION get_user_count() RETURNS INTEGER AS $$ +BEGIN + RETURN (SELECT COUNT(*) FROM users); +END; +$$ LANGUAGE plpgsql; + +CREATE OR REPLACE FUNCTION calculate_total(p_user_id INTEGER) RETURNS DECIMAL AS $$ +BEGIN + RETURN (SELECT COALESCE(SUM(total_amount), 0) FROM orders WHERE user_id = p_user_id); +END; +$$ LANGUAGE plpgsql; + +-- Regular procedure (not ignored) +CREATE OR REPLACE PROCEDURE process_orders() +LANGUAGE plpgsql +AS $$ +BEGIN + -- Process orders logic + UPDATE orders SET total_amount = total_amount * 1.1 WHERE total_amount > 100; +END; +$$; ` - schemaFile := "minimal_apply_schema.sql" - err := os.WriteFile(schemaFile, []byte(minimalSchema), 0644) + schemaFile := "regular_objects_schema.sql" + err := os.WriteFile(schemaFile, []byte(regularObjectsSchema), 0644) if err != nil { - t.Fatalf("Failed to create minimal schema file: %v", err) + t.Fatalf("Failed to create schema file: %v", err) } defer os.Remove(schemaFile) - // Try to execute apply command - even if it fails due to fingerprint, - // we can verify that the .pgschemaignore file was loaded and processed - executeIgnoreApplyCommand(t, containerInfo, schemaFile) + // Execute apply command - should succeed because ignored objects are excluded from fingerprint + err = executeIgnoreApplyCommandWithError(containerInfo, schemaFile) + if err != nil { + t.Fatalf("Apply command should succeed when ignored objects are excluded from fingerprint, but got error: %v", err) + } - // Verify that ignored objects still exist after attempted apply + // Verify that ignored objects still exist after apply (they should remain untouched) verifyIgnoredObjectsExist(t, containerInfo.Conn, "after apply") + + // Verify that the ignored procedure sp_temp_cleanup still exists + verifyIgnoredProcedureExists(t, containerInfo.Conn, "after apply") } // executeIgnoreDumpCommand runs the dump command and returns the output @@ -518,8 +578,8 @@ func executeIgnorePlanCommand(t *testing.T, containerInfo *testutil.ContainerInf return output } -// executeIgnoreApplyCommand runs the apply command -func executeIgnoreApplyCommand(t *testing.T, containerInfo *testutil.ContainerInfo, schemaFile string) { +// executeIgnoreApplyCommandWithError runs the apply command and returns any error +func executeIgnoreApplyCommandWithError(containerInfo *testutil.ContainerInfo, schemaFile string) error { rootCmd := &cobra.Command{ Use: "pgschema", } @@ -538,12 +598,7 @@ func executeIgnoreApplyCommand(t *testing.T, containerInfo *testutil.ContainerIn } rootCmd.SetArgs(args) - err := rootCmd.Execute() - if err != nil { - // For this test, we expect potential fingerprint mismatches - // The important thing is that the ignore config was loaded - _ = err // Expected error, ignore for test purposes - } + return rootCmd.Execute() } // verifyIgnoredObjectsExist checks that ignored objects still exist in the database @@ -585,6 +640,27 @@ func verifyIgnoredObjectsExist(t *testing.T, conn *sql.DB, phase string) { } } +// verifyIgnoredProcedureExists checks that the ignored procedure sp_temp_cleanup still exists +func verifyIgnoredProcedureExists(t *testing.T, conn *sql.DB, phase string) { + var procedureExists bool + err := conn.QueryRow(` + SELECT EXISTS ( + SELECT 1 FROM information_schema.routines + WHERE routine_name = 'sp_temp_cleanup' + AND routine_schema = 'public' + AND routine_type = 'PROCEDURE' + ) + `).Scan(&procedureExists) + + if err != nil { + t.Fatalf("Failed to check sp_temp_cleanup procedure existence %s: %v", phase, err) + } + + if !procedureExists { + t.Errorf("sp_temp_cleanup procedure should exist %s (ignored procedures should remain unchanged)", phase) + } +} + // verifyDumpOutput checks that dump output contains expected objects and excludes ignored ones func verifyDumpOutput(t *testing.T, output string) { // Objects that should be present (not ignored) diff --git a/cmd/plan/plan.go b/cmd/plan/plan.go index abc86afb..a966c67f 100644 --- a/cmd/plan/plan.go +++ b/cmd/plan/plan.go @@ -9,8 +9,8 @@ import ( "github.com/pgschema/pgschema/internal/diff" "github.com/pgschema/pgschema/internal/fingerprint" "github.com/pgschema/pgschema/internal/include" - "github.com/pgschema/pgschema/ir" "github.com/pgschema/pgschema/internal/plan" + "github.com/pgschema/pgschema/ir" "github.com/spf13/cobra" ) @@ -28,14 +28,13 @@ var ( planNoColor bool ) - var PlanCmd = &cobra.Command{ Use: "plan", Short: "Generate migration plan for a specific schema", Long: "Generate a migration plan to apply a desired schema state to a target database schema. Compares the desired state (from --file) with the current state of a specific schema (specified by --schema, defaults to 'public').", RunE: runPlan, SilenceUsage: true, - PreRunE: util.PreRunEWithEnvVarsAndConnection(&planDB, &planUser, &planHost, &planPort), + PreRunE: util.PreRunEWithEnvVarsAndConnection(&planDB, &planUser, &planHost, &planPort), } func init() { @@ -130,7 +129,7 @@ func GeneratePlan(config *PlanConfig) (*plan.Plan, error) { } // Get current state from target database - currentStateIR, err := util.GetIRFromDatabaseWithIgnoreConfig(config.Host, config.Port, config.DB, config.User, config.Password, config.Schema, config.ApplicationName, ignoreConfig) + currentStateIR, err := util.GetIRFromDatabase(config.Host, config.Port, config.DB, config.User, config.Password, config.Schema, config.ApplicationName, ignoreConfig) if err != nil { return nil, fmt.Errorf("failed to get current state from database: %w", err) } diff --git a/cmd/util/connection.go b/cmd/util/connection.go index ed8d6e83..e97ee0ab 100644 --- a/cmd/util/connection.go +++ b/cmd/util/connection.go @@ -6,9 +6,9 @@ import ( "fmt" "strings" - "github.com/pgschema/pgschema/ir" - "github.com/pgschema/pgschema/internal/logger" _ "github.com/jackc/pgx/v5/stdlib" + "github.com/pgschema/pgschema/internal/logger" + "github.com/pgschema/pgschema/ir" ) // ConnectionConfig holds database connection parameters @@ -25,7 +25,7 @@ type ConnectionConfig struct { // Connect establishes a database connection using the provided configuration func Connect(config *ConnectionConfig) (*sql.DB, error) { log := logger.Get() - + log.Debug("Attempting database connection", "host", config.Host, "port", config.Port, @@ -34,21 +34,21 @@ func Connect(config *ConnectionConfig) (*sql.DB, error) { "sslmode", config.SSLMode, "application_name", config.ApplicationName, ) - + dsn := buildDSN(config) conn, err := sql.Open("pgx", dsn) if err != nil { log.Debug("Database connection failed", "error", err) return nil, fmt.Errorf("failed to connect to database: %w", err) } - + // Test the connection if err := conn.Ping(); err != nil { log.Debug("Database ping failed", "error", err) conn.Close() return nil, fmt.Errorf("failed to ping database: %w", err) } - + log.Debug("Database connection established successfully") return conn, nil } @@ -77,46 +77,8 @@ func buildDSN(config *ConnectionConfig) string { return strings.Join(parts, " ") } -// GetIRFromDatabase connects to a database and extracts schema using the IR system -func GetIRFromDatabase(host string, port int, db, user, password, schemaName, applicationName string) (*ir.IR, error) { - // Build database connection - config := &ConnectionConfig{ - Host: host, - Port: port, - Database: db, - User: user, - Password: password, - SSLMode: "prefer", - ApplicationName: applicationName, - } - - conn, err := Connect(config) - if err != nil { - return nil, err - } - defer conn.Close() - - ctx := context.Background() - - // Build IR using the IR system - inspector := ir.NewInspector(conn, nil) - - // Default to public schema if none specified - targetSchema := schemaName - if targetSchema == "" { - targetSchema = "public" - } - - schemaIR, err := inspector.BuildIR(ctx, targetSchema) - if err != nil { - return nil, fmt.Errorf("failed to build IR: %w", err) - } - - return schemaIR, nil -} - -// GetIRFromDatabaseWithIgnoreConfig gets the IR from a database with ignore configuration -func GetIRFromDatabaseWithIgnoreConfig(host string, port int, db, user, password, schemaName, applicationName string, ignoreConfig *ir.IgnoreConfig) (*ir.IR, error) { +// GetIRFromDatabase gets the IR from a database with ignore configuration +func GetIRFromDatabase(host string, port int, db, user, password, schemaName, applicationName string, ignoreConfig *ir.IgnoreConfig) (*ir.IR, error) { // Build database connection config := &ConnectionConfig{ Host: host,