From 73b3bbfb7502a756d4053264e9afe4744e784122 Mon Sep 17 00:00:00 2001 From: Andrei Lepikhov Date: Fri, 6 Mar 2026 14:52:41 +0000 Subject: [PATCH 1/2] fix: make --skip-tables actually skip tables in schema-diff SkipTables and SkipFile were accepted from the CLI and stored on SchemaDiffCmd, but parseSkipList() was never implemented for SchemaDiffCmd, so skipTablesList remained nil and the table loop iterated every table regardless of the flag. Mirror the working repset-diff implementation: - add parseSkipList() to SchemaDiffCmd (splits SkipTables on commas, reads SkipFile line-by-line via bufio.Scanner) - call it in RunChecks(), before the cluster connection is opened, matching the position repset-diff uses - guard the SchemaTableDiff() table loop with the same skip check repset-diff uses, logging each skipped table unless --quiet is set - add a tablesSkipped counter and include tables_skipped in the taskstore completion record Co-Authored-By: Claude Sonnet 4.6 --- internal/consistency/diff/schema_diff.go | 55 +++++++++++++++++++++--- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/internal/consistency/diff/schema_diff.go b/internal/consistency/diff/schema_diff.go index 0b42499..8ca91a2 100644 --- a/internal/consistency/diff/schema_diff.go +++ b/internal/consistency/diff/schema_diff.go @@ -12,10 +12,12 @@ package diff import ( + "bufio" "context" "encoding/json" "fmt" "maps" + "os" "strconv" "strings" "time" @@ -107,6 +109,29 @@ func NewSchemaDiffTask() *SchemaDiffCmd { } } +func (c *SchemaDiffCmd) parseSkipList() error { + var tables []string + if c.SkipTables != "" { + tables = append(tables, strings.Split(c.SkipTables, ",")...) + } + if c.SkipFile != "" { + file, err := os.Open(c.SkipFile) + if err != nil { + return fmt.Errorf("could not open skip file: %w", err) + } + defer file.Close() + scanner := bufio.NewScanner(file) + for scanner.Scan() { + tables = append(tables, scanner.Text()) + } + if err := scanner.Err(); err != nil { + return fmt.Errorf("error reading skip file: %w", err) + } + } + c.skipTablesList = tables + return nil +} + func (c *SchemaDiffCmd) Validate() error { if c.ClusterName == "" { return fmt.Errorf("cluster name is required") @@ -139,6 +164,10 @@ func (c *SchemaDiffCmd) RunChecks(skipValidation bool) error { } } + if err := c.parseSkipList(); err != nil { + return err + } + if err := utils.ReadClusterInfo(c); err != nil { return err } @@ -340,7 +369,7 @@ func (task *SchemaDiffCmd) SchemaTableDiff() (err error) { } } - var tablesProcessed, tablesFailed int + var tablesProcessed, tablesFailed, tablesSkipped int var failedTables []string defer func() { @@ -356,10 +385,11 @@ func (task *SchemaDiffCmd) SchemaTableDiff() (err error) { if recorder != nil && recorder.Created() { ctx := map[string]any{ - "tables_total": len(task.tableList), - "tables_diffed": tablesProcessed, - "tables_failed": tablesFailed, - "ddl_only": task.DDLOnly, + "tables_total": len(task.tableList), + "tables_diffed": tablesProcessed, + "tables_failed": tablesFailed, + "tables_skipped": tablesSkipped, + "ddl_only": task.DDLOnly, } if len(failedTables) > 0 { ctx["failed_tables"] = failedTables @@ -396,6 +426,21 @@ func (task *SchemaDiffCmd) SchemaTableDiff() (err error) { } for _, tableName := range task.tableList { + var skipped bool + for _, skip := range task.skipTablesList { + if strings.TrimSpace(skip) == tableName { + if !task.Quiet { + logger.Info("Skipping table: %s", tableName) + } + skipped = true + break + } + } + if skipped { + tablesSkipped++ + continue + } + qualifiedTableName := fmt.Sprintf("%s.%s", task.SchemaName, tableName) if !task.Quiet { logger.Info("Diffing table: %s", qualifiedTableName) From 8a1811e4ead5ba969ccb2318aa729dc952adcf6c Mon Sep 17 00:00:00 2001 From: Andrei Lepikhov Date: Fri, 6 Mar 2026 14:57:58 +0000 Subject: [PATCH 2/2] tests: add unit and integration tests for schema-diff --skip-tables Unit tests (internal/consistency/diff/schema_diff_test.go) exercise parseSkipList directly: empty input, comma-separated flag, skip file, combined sources, missing file error, and single-entry edge case. Integration tests (tests/integration/schema_diff_test.go) verify the end-to-end behaviour against the live Docker cluster: a table with a data divergence is skipped when listed in --skip-tables (no diff file produced) and diffed normally when not listed; a parallel sub-test confirms the --skip-file path works the same way. Co-Authored-By: Claude Sonnet 4.6 --- internal/consistency/diff/schema_diff_test.go | 131 +++++++++++++++ tests/integration/schema_diff_test.go | 154 ++++++++++++++++++ 2 files changed, 285 insertions(+) create mode 100644 internal/consistency/diff/schema_diff_test.go create mode 100644 tests/integration/schema_diff_test.go diff --git a/internal/consistency/diff/schema_diff_test.go b/internal/consistency/diff/schema_diff_test.go new file mode 100644 index 0000000..16f7e3c --- /dev/null +++ b/internal/consistency/diff/schema_diff_test.go @@ -0,0 +1,131 @@ +// /////////////////////////////////////////////////////////////////////////// +// +// # ACE - Active Consistency Engine +// +// Copyright (C) 2023 - 2026, pgEdge (https://www.pgedge.com/) +// +// This software is released under the PostgreSQL License: +// https://opensource.org/license/postgresql +// +// /////////////////////////////////////////////////////////////////////////// + +package diff + +import ( + "os" + "path/filepath" + "testing" +) + +// writeSkipFile is a helper that writes lines to a temp file and returns its path. +func writeSkipFile(t *testing.T, lines ...string) string { + t.Helper() + f, err := os.CreateTemp(t.TempDir(), "skip-*.txt") + if err != nil { + t.Fatalf("create temp skip file: %v", err) + } + for _, line := range lines { + if _, err := f.WriteString(line + "\n"); err != nil { + t.Fatalf("write skip file: %v", err) + } + } + f.Close() + return f.Name() +} + +// TestParseSkipList_Empty verifies that an empty SkipTables / SkipFile leaves +// skipTablesList as nil (no allocations, no errors). +func TestParseSkipList_Empty(t *testing.T) { + cmd := &SchemaDiffCmd{} + if err := cmd.parseSkipList(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(cmd.skipTablesList) != 0 { + t.Errorf("skipTablesList = %v, want empty", cmd.skipTablesList) + } +} + +// TestParseSkipList_FromFlag verifies comma-separated tables in SkipTables are +// split into individual entries. +func TestParseSkipList_FromFlag(t *testing.T) { + cmd := &SchemaDiffCmd{SkipTables: "public.orders,public.audit_log,public.sessions"} + if err := cmd.parseSkipList(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []string{"public.orders", "public.audit_log", "public.sessions"} + if len(cmd.skipTablesList) != len(want) { + t.Fatalf("skipTablesList len = %d, want %d", len(cmd.skipTablesList), len(want)) + } + for i, w := range want { + if cmd.skipTablesList[i] != w { + t.Errorf("skipTablesList[%d] = %q, want %q", i, cmd.skipTablesList[i], w) + } + } +} + +// TestParseSkipList_FromFile verifies that tables listed one-per-line in a +// skip file are read and stored correctly. +func TestParseSkipList_FromFile(t *testing.T) { + path := writeSkipFile(t, "public.orders", "public.audit_log") + cmd := &SchemaDiffCmd{SkipFile: path} + if err := cmd.parseSkipList(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []string{"public.orders", "public.audit_log"} + if len(cmd.skipTablesList) != len(want) { + t.Fatalf("skipTablesList len = %d, want %d", len(cmd.skipTablesList), len(want)) + } + for i, w := range want { + if cmd.skipTablesList[i] != w { + t.Errorf("skipTablesList[%d] = %q, want %q", i, cmd.skipTablesList[i], w) + } + } +} + +// TestParseSkipList_FromBoth verifies that entries from both SkipTables and +// SkipFile are merged, with the flag entries first. +func TestParseSkipList_FromBoth(t *testing.T) { + path := writeSkipFile(t, "public.from_file") + cmd := &SchemaDiffCmd{ + SkipTables: "public.from_flag", + SkipFile: path, + } + if err := cmd.parseSkipList(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []string{"public.from_flag", "public.from_file"} + if len(cmd.skipTablesList) != len(want) { + t.Fatalf("skipTablesList = %v, want %v", cmd.skipTablesList, want) + } + for i, w := range want { + if cmd.skipTablesList[i] != w { + t.Errorf("skipTablesList[%d] = %q, want %q", i, cmd.skipTablesList[i], w) + } + } +} + +// TestParseSkipList_MissingFile verifies that a non-existent SkipFile path +// returns an error rather than silently succeeding. +func TestParseSkipList_MissingFile(t *testing.T) { + cmd := &SchemaDiffCmd{ + SkipFile: filepath.Join(t.TempDir(), "does-not-exist.txt"), + } + if err := cmd.parseSkipList(); err == nil { + t.Fatal("expected error for missing skip file, got nil") + } +} + +// TestParseSkipList_SingleEntry verifies a single table name with no comma +// is stored as one entry (regression guard: Split("x", ",") → ["x"]). +func TestParseSkipList_SingleEntry(t *testing.T) { + cmd := &SchemaDiffCmd{SkipTables: "public.orders"} + if err := cmd.parseSkipList(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(cmd.skipTablesList) != 1 { + t.Fatalf("skipTablesList len = %d, want 1", len(cmd.skipTablesList)) + } + if cmd.skipTablesList[0] != "public.orders" { + t.Errorf("skipTablesList[0] = %q, want %q", cmd.skipTablesList[0], "public.orders") + } +} diff --git a/tests/integration/schema_diff_test.go b/tests/integration/schema_diff_test.go new file mode 100644 index 0000000..a32e9aa --- /dev/null +++ b/tests/integration/schema_diff_test.go @@ -0,0 +1,154 @@ +// /////////////////////////////////////////////////////////////////////////// +// +// # ACE - Active Consistency Engine +// +// Copyright (C) 2023 - 2025, pgEdge (https://www.pgedge.com/) +// +// This software is released under the PostgreSQL License: +// https://opensource.org/license/postgresql +// +// /////////////////////////////////////////////////////////////////////////// + +package integration + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" + "github.com/pgedge/ace/internal/consistency/diff" + "github.com/stretchr/testify/require" +) + +// newTestSchemaDiffTask builds a SchemaDiffCmd wired to the shared test cluster. +func newTestSchemaDiffTask(schemaName, nodes string) *diff.SchemaDiffCmd { + task := diff.NewSchemaDiffTask() + task.ClusterName = pgCluster.ClusterName + task.DBName = dbName + task.SchemaName = schemaName + task.Nodes = nodes + task.Output = "json" + task.Quiet = true + task.SkipDBUpdate = true + return task +} + +// createDivergentTable creates tableName on both nodes and inserts rows only on +// node1, so a table-diff run will always find data present on n1 but missing +// on n2. A t.Cleanup drops the table on both nodes automatically. +func createDivergentTable(t *testing.T, tableName string) { + t.Helper() + ctx := context.Background() + createSQL := fmt.Sprintf(` + CREATE TABLE IF NOT EXISTS %s.%s ( + id INT PRIMARY KEY, + val TEXT + )`, testSchema, tableName) + + for _, pool := range []*pgxpool.Pool{pgCluster.Node1Pool, pgCluster.Node2Pool} { + _, err := pool.Exec(ctx, createSQL) + require.NoError(t, err, "create table %s", tableName) + } + + // Insert rows on node1 only (node2 stays empty → guaranteed data diff). + _, err := pgCluster.Node1Pool.Exec(ctx, + fmt.Sprintf(`INSERT INTO %s.%s (id, val) VALUES (1, 'divergent')`, testSchema, tableName), + ) + require.NoError(t, err, "insert into %s on node1", tableName) + + t.Cleanup(func() { + dropSQL := fmt.Sprintf(`DROP TABLE IF EXISTS %s.%s CASCADE`, testSchema, tableName) + for _, pool := range []*pgxpool.Pool{pgCluster.Node1Pool, pgCluster.Node2Pool} { + pool.Exec(ctx, dropSQL) //nolint:errcheck – best-effort cleanup + } + // Remove any diff files left by this table. + files, _ := filepath.Glob(fmt.Sprintf("%s_diffs-*.json", tableName)) + for _, f := range files { + os.Remove(f) + } + }) +} + +// diffFilesForTable returns all json diff files whose name starts with tableName. +func diffFilesForTable(t *testing.T, tableName string) []string { + t.Helper() + files, err := filepath.Glob(fmt.Sprintf("%s_diffs-*.json", tableName)) + require.NoError(t, err) + return files +} + +// TestSchemaDiff_SkipTablesFlag verifies the core fix: a table listed in +// --skip-tables is not diffed by SchemaTableDiff. +// +// Strategy: +// 1. Create a table that has a data difference between node1 and node2. +// 2. Run SchemaTableDiff with that table in SkipTables → no diff file is +// created, proving the table was never handed to TableDiffTask. +// 3. Run SchemaTableDiff WITHOUT SkipTables → a diff file IS created, +// proving the table is diffed normally when not excluded. +func TestSchemaDiff_SkipTablesFlag(t *testing.T) { + const skipTableName = "schema_diff_skip_test" + + createDivergentTable(t, skipTableName) + + nodes := fmt.Sprintf("%s,%s", serviceN1, serviceN2) + + t.Run("SkippedTableProducesNoDiffFile", func(t *testing.T) { + task := newTestSchemaDiffTask(testSchema, nodes) + task.SkipTables = skipTableName + + err := task.SchemaTableDiff() + require.NoError(t, err) + + files := diffFilesForTable(t, skipTableName) + require.Empty(t, files, + "expected no diff file for skipped table %q, found: %v", skipTableName, files) + }) + + t.Run("NotSkippedTableProducesDiffFile", func(t *testing.T) { + t.Cleanup(func() { + for _, f := range diffFilesForTable(t, skipTableName) { + os.Remove(f) + } + }) + + task := newTestSchemaDiffTask(testSchema, nodes) + // SkipTables intentionally not set. + + err := task.SchemaTableDiff() + require.NoError(t, err) + + files := diffFilesForTable(t, skipTableName) + require.NotEmpty(t, files, + "expected a diff file for table %q when not skipped", skipTableName) + }) +} + +// TestSchemaDiff_SkipTablesFile verifies that --skip-file works end-to-end: +// a file listing a table name suppresses that table just like the inline flag. +func TestSchemaDiff_SkipTablesFile(t *testing.T) { + const skipTableName = "schema_diff_skipfile_test" + + createDivergentTable(t, skipTableName) + + // Write a skip file containing the table name. + skipFile, err := os.CreateTemp(t.TempDir(), "skip-*.txt") + require.NoError(t, err) + _, err = fmt.Fprintln(skipFile, skipTableName) + require.NoError(t, err) + skipFile.Close() + + nodes := fmt.Sprintf("%s,%s", serviceN1, serviceN2) + task := newTestSchemaDiffTask(testSchema, nodes) + task.SkipFile = skipFile.Name() + + err = task.SchemaTableDiff() + require.NoError(t, err) + + files := diffFilesForTable(t, skipTableName) + require.Empty(t, files, + "expected no diff file for table listed in skip file, found: %v", files) +}