From e0e152350bd3399a160ce63f5dc36b6c3ee74836 Mon Sep 17 00:00:00 2001 From: Carl Dawson Date: Sat, 18 Apr 2026 12:08:59 +0100 Subject: [PATCH 1/8] Adds tests to ensure DDL coverage completeness --- test/ddl_coverage_test.rb | 510 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 510 insertions(+) create mode 100644 test/ddl_coverage_test.rb diff --git a/test/ddl_coverage_test.rb b/test/ddl_coverage_test.rb new file mode 100644 index 0000000..1084cd0 --- /dev/null +++ b/test/ddl_coverage_test.rb @@ -0,0 +1,510 @@ +require "test_helper" + +# Systematic DDL coverage: every table-modifying method gets a test that +# asserts the change lands on the branch and public is untouched. +class DdlCoverageTest < Minitest::Test + include PGBTestSupport + + def setup + setup_test_database + conn = connect(branch: "main") + conn.execute("CREATE TABLE public.users (id serial PRIMARY KEY, name varchar NOT NULL, email varchar, legacy varchar)") + conn.execute("CREATE TABLE public.orders (id serial PRIMARY KEY, status varchar, amount integer)") + conn.execute("CREATE TABLE public.authors (id serial PRIMARY KEY, name varchar NOT NULL)") + conn.execute("CREATE TABLE public.posts (id serial PRIMARY KEY, author_id integer, title varchar)") + conn.execute("INSERT INTO public.users (name, email) VALUES ('alice', 'alice@example.com'), ('bob', 'bob@example.com')") + conn.execute("INSERT INTO public.orders (status, amount) VALUES ('pending', 100)") + conn.execute("INSERT INTO public.authors (name) VALUES ('alice')") + conn.execute("INSERT INTO public.posts (author_id, title) VALUES (1, 'hello')") + end + + def teardown + teardown_test_database + end + + # --- Column DDL --- + + def test_add_column + conn = reconnect(branch: "feature/add-col") + conn.add_column :users, :bio, :string + + assert column_exists_in_schema?(conn, "branch_feature_add_col", "users", "bio") + refute column_exists_in_schema?(conn, "public", "users", "bio") + end + + def test_add_columns + conn = reconnect(branch: "feature/add-cols") + conn.add_columns :users, :bio, :age, type: :string + + assert column_exists_in_schema?(conn, "branch_feature_add_cols", "users", "bio") + assert column_exists_in_schema?(conn, "branch_feature_add_cols", "users", "age") + refute column_exists_in_schema?(conn, "public", "users", "bio") + end + + def test_remove_column + conn = reconnect(branch: "feature/rm-col") + conn.remove_column :users, :legacy + + refute column_exists_in_schema?(conn, "branch_feature_rm_col", "users", "legacy") + assert column_exists_in_schema?(conn, "public", "users", "legacy") + end + + def test_remove_columns + conn = reconnect(branch: "feature/rm-cols") + conn.remove_columns :users, :email, :legacy + + refute column_exists_in_schema?(conn, "branch_feature_rm_cols", "users", "email") + refute column_exists_in_schema?(conn, "branch_feature_rm_cols", "users", "legacy") + assert column_exists_in_schema?(conn, "public", "users", "email") + assert column_exists_in_schema?(conn, "public", "users", "legacy") + end + + def test_rename_column + conn = reconnect(branch: "feature/ren-col") + conn.rename_column :users, :legacy, :old_field + + assert column_exists_in_schema?(conn, "branch_feature_ren_col", "users", "old_field") + refute column_exists_in_schema?(conn, "branch_feature_ren_col", "users", "legacy") + assert column_exists_in_schema?(conn, "public", "users", "legacy") + end + + def test_change_column + conn = reconnect(branch: "feature/chg-col") + conn.change_column :users, :name, :text + + col_type = conn.select_value(<<~SQL) + SELECT data_type FROM information_schema.columns + WHERE table_schema = 'branch_feature_chg_col' AND table_name = 'users' AND column_name = 'name' + SQL + assert_equal "text", col_type + + public_type = conn.select_value(<<~SQL) + SELECT data_type FROM information_schema.columns + WHERE table_schema = 'public' AND table_name = 'users' AND column_name = 'name' + SQL + assert_equal "character varying", public_type + end + + def test_change_column_default + conn = reconnect(branch: "feature/chg-def") + conn.change_column_default :users, :name, "unknown" + + conn.execute("INSERT INTO users (email) VALUES ('test@example.com')") + name = conn.select_value("SELECT name FROM users WHERE email = 'test@example.com'") + assert_equal "unknown", name + end + + def test_change_column_null + conn = reconnect(branch: "feature/chg-null") + conn.change_column_null :orders, :status, false, "pending" + + assert_raises(ActiveRecord::NotNullViolation) do + conn.execute("INSERT INTO orders (status, amount) VALUES (NULL, 50)") + end + end + + def test_change_column_comment + conn = reconnect(branch: "feature/col-comment") + conn.change_column_comment :users, :name, "The user's display name" + + comment = conn.select_value(<<~SQL) + SELECT col_description(c.oid, a.attnum) + FROM pg_attribute a + JOIN pg_class c ON c.oid = a.attrelid + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'branch_feature_col_comment' + AND c.relname = 'users' + AND a.attname = 'name' + SQL + assert_equal "The user's display name", comment + end + + def test_change_table_comment + conn = reconnect(branch: "feature/tbl-comment") + conn.change_table_comment :users, "A table of users" + + comment = conn.select_value(<<~SQL) + SELECT obj_description(c.oid) + FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'branch_feature_tbl_comment' AND c.relname = 'users' + SQL + assert_equal "A table of users", comment + end + + # --- Timestamp DDL --- + + def test_add_timestamps + conn = reconnect(branch: "feature/add-ts") + conn.add_timestamps :orders, null: true + + assert column_exists_in_schema?(conn, "branch_feature_add_ts", "orders", "created_at") + assert column_exists_in_schema?(conn, "branch_feature_add_ts", "orders", "updated_at") + refute column_exists_in_schema?(conn, "public", "orders", "created_at") + end + + def test_remove_timestamps + conn = connect(branch: "main") + conn.add_timestamps :orders, null: true + + conn = reconnect(branch: "feature/rm-ts") + conn.remove_timestamps :orders + + refute column_exists_in_schema?(conn, "branch_feature_rm_ts", "orders", "created_at") + refute column_exists_in_schema?(conn, "branch_feature_rm_ts", "orders", "updated_at") + assert column_exists_in_schema?(conn, "public", "orders", "created_at") + end + + # --- Index DDL --- + + def test_add_index + conn = reconnect(branch: "feature/add-idx") + conn.add_index :orders, :status + + assert index_exists_in_schema?(conn, "branch_feature_add_idx", "index_orders_on_status") + refute index_exists_in_schema?(conn, "public", "index_orders_on_status") + end + + def test_remove_index + conn = connect(branch: "main") + conn.add_index :orders, :status, name: "idx_orders_status" + + conn = reconnect(branch: "feature/rm-idx") + conn.remove_index :orders, name: "idx_orders_status" + + refute index_exists_in_schema?(conn, "branch_feature_rm_idx", "idx_orders_status") + assert index_exists_in_schema?(conn, "public", "idx_orders_status") + end + + def test_rename_index + conn = connect(branch: "main") + conn.add_index :orders, :status, name: "idx_old" + + conn = reconnect(branch: "feature/ren-idx") + conn.rename_index :orders, "idx_old", "idx_new" + + assert index_exists_in_schema?(conn, "branch_feature_ren_idx", "idx_new") + refute index_exists_in_schema?(conn, "branch_feature_ren_idx", "idx_old") + assert index_exists_in_schema?(conn, "public", "idx_old") + end + + # --- Foreign key DDL --- + + def test_add_foreign_key + conn = reconnect(branch: "feature/add-fk") + conn.add_foreign_key :posts, :authors + + assert foreign_key_exists_on_branch?(conn, "branch_feature_add_fk", "posts", "authors") + refute foreign_key_exists_on_branch?(conn, "public", "posts", "authors") + end + + def test_remove_foreign_key + conn = connect(branch: "main") + conn.add_foreign_key :posts, :authors + + conn = reconnect(branch: "feature/rm-fk") + conn.remove_foreign_key :posts, :authors + + refute foreign_key_exists_on_branch?(conn, "branch_feature_rm_fk", "posts", "authors") + assert foreign_key_exists_on_branch?(conn, "public", "posts", "authors") + end + + def test_validate_foreign_key + conn = connect(branch: "main") + conn.add_foreign_key :posts, :authors, validate: false + + conn = reconnect(branch: "feature/val-fk") + conn.validate_foreign_key :posts, :authors + + assert table_exists_in_schema?(conn, "branch_feature_val_fk", "posts"), + "validate_foreign_key must shadow the table" + end + + # --- Check constraint DDL --- + + def test_add_check_constraint + conn = reconnect(branch: "feature/add-chk") + conn.add_check_constraint :orders, "amount > 0", name: "orders_amount_positive" + + assert check_constraint_exists_in_schema?(conn, "branch_feature_add_chk", "orders", "orders_amount_positive") + assert_raises(ActiveRecord::StatementInvalid) do + conn.execute("INSERT INTO orders (status, amount) VALUES ('bad', -1)") + end + end + + def test_remove_check_constraint + conn = connect(branch: "main") + conn.add_check_constraint :orders, "amount > 0", name: "orders_amount_positive" + + conn = reconnect(branch: "feature/rm-chk") + conn.remove_check_constraint :orders, name: "orders_amount_positive" + + refute check_constraint_exists_in_schema?(conn, "branch_feature_rm_chk", "orders", "orders_amount_positive") + assert check_constraint_exists_in_schema?(conn, "public", "orders", "orders_amount_positive") + end + + def test_validate_check_constraint + conn = connect(branch: "main") + conn.add_check_constraint :orders, "amount > 0", name: "orders_amount_positive", validate: false + + conn = reconnect(branch: "feature/val-chk") + conn.validate_check_constraint :orders, name: "orders_amount_positive" + + assert table_exists_in_schema?(conn, "branch_feature_val_chk", "orders"), + "validate_check_constraint must shadow the table" + end + + # --- Exclusion constraint DDL --- + + def test_add_exclusion_constraint + conn = connect(branch: "main") + conn.enable_extension "btree_gist" + + conn = reconnect(branch: "feature/add-excl") + conn.add_exclusion_constraint :orders, "amount WITH =", using: :gist, name: "orders_amount_excl" + + assert constraint_exists?(conn, "branch_feature_add_excl", "orders", "orders_amount_excl") + refute constraint_exists?(conn, "public", "orders", "orders_amount_excl") + end + + def test_remove_exclusion_constraint + conn = connect(branch: "main") + conn.enable_extension "btree_gist" + conn.add_exclusion_constraint :orders, "amount WITH =", using: :gist, name: "orders_amount_excl" + + conn = reconnect(branch: "feature/rm-excl") + conn.remove_exclusion_constraint :orders, name: "orders_amount_excl" + + refute constraint_exists?(conn, "branch_feature_rm_excl", "orders", "orders_amount_excl") + assert constraint_exists?(conn, "public", "orders", "orders_amount_excl") + end + + # --- Unique constraint DDL --- + + def test_add_unique_constraint + conn = reconnect(branch: "feature/add-uniq") + conn.add_unique_constraint :users, :email, name: "users_email_uniq" + + assert constraint_exists?(conn, "branch_feature_add_uniq", "users", "users_email_uniq") + refute constraint_exists?(conn, "public", "users", "users_email_uniq") + end + + def test_remove_unique_constraint + conn = connect(branch: "main") + conn.add_unique_constraint :users, :email, name: "users_email_uniq" + + conn = reconnect(branch: "feature/rm-uniq") + conn.remove_unique_constraint :users, name: "users_email_uniq" + + refute constraint_exists?(conn, "branch_feature_rm_uniq", "users", "users_email_uniq") + assert constraint_exists?(conn, "public", "users", "users_email_uniq") + end + + # --- Generic constraint DDL --- + + def test_validate_constraint + conn = connect(branch: "main") + conn.add_check_constraint :orders, "amount > 0", name: "orders_amount_positive", validate: false + + conn = reconnect(branch: "feature/val-con") + conn.validate_constraint :orders, "orders_amount_positive" + + assert table_exists_in_schema?(conn, "branch_feature_val_con", "orders"), + "validate_constraint must shadow the table" + end + + def test_remove_constraint + conn = connect(branch: "main") + conn.add_check_constraint :orders, "amount > 0", name: "orders_amount_positive" + + conn = reconnect(branch: "feature/rm-con") + conn.remove_constraint :orders, "orders_amount_positive" + + refute constraint_exists?(conn, "branch_feature_rm_con", "orders", "orders_amount_positive") + assert constraint_exists?(conn, "public", "orders", "orders_amount_positive") + end + + # --- Reference DDL --- + + def test_add_reference + conn = reconnect(branch: "feature/add-ref") + conn.add_reference :orders, :user, foreign_key: true + + assert column_exists_in_schema?(conn, "branch_feature_add_ref", "orders", "user_id") + refute column_exists_in_schema?(conn, "public", "orders", "user_id") + end + + def test_remove_reference + conn = connect(branch: "main") + conn.add_reference :orders, :user + + conn = reconnect(branch: "feature/rm-ref") + conn.remove_reference :orders, :user + + refute column_exists_in_schema?(conn, "branch_feature_rm_ref", "orders", "user_id") + assert column_exists_in_schema?(conn, "public", "orders", "user_id") + end + + # --- Table-level DDL --- + + def test_create_table + conn = reconnect(branch: "feature/create-tbl") + conn.create_table(:payments) { |t| t.integer :amount } + + assert table_exists_in_schema?(conn, "branch_feature_create_tbl", "payments") + refute table_exists_in_schema?(conn, "public", "payments") + end + + def test_drop_table + conn = reconnect(branch: "feature/drop-tbl") + conn.drop_table :orders + + assert table_exists_in_schema?(conn, "public", "orders"), + "Public table must survive branch drop" + refute table_exists_in_schema?(conn, "branch_feature_drop_tbl", "orders") + end + + def test_drop_table_is_not_visible_on_branch + skip "Known bug: dropped tables fall through search_path to public" + + conn = reconnect(branch: "feature/drop-vis") + conn.drop_table :orders + + # After dropping on a branch, the table should NOT be queryable + # via the branch's search_path. + assert_raises(ActiveRecord::StatementInvalid) do + conn.execute("INSERT INTO orders (status, amount) VALUES ('new', 200)") + end + end + + def test_rename_table + conn = reconnect(branch: "feature/ren-tbl") + conn.rename_table :orders, :purchases + + assert table_exists_in_schema?(conn, "branch_feature_ren_tbl", "purchases") + refute table_exists_in_schema?(conn, "branch_feature_ren_tbl", "orders") + assert table_exists_in_schema?(conn, "public", "orders"), + "Public must keep original table" + end + + def test_change_table + conn = reconnect(branch: "feature/chg-tbl") + conn.change_table(:users) { |t| t.string :nickname } + + assert column_exists_in_schema?(conn, "branch_feature_chg_tbl", "users", "nickname") + refute column_exists_in_schema?(conn, "public", "users", "nickname") + end + + def test_bulk_change_table + conn = reconnect(branch: "feature/bulk-chg") + conn.change_table(:users, bulk: true) do |t| + t.string :nickname + t.string :avatar + end + + assert column_exists_in_schema?(conn, "branch_feature_bulk_chg", "users", "nickname") + assert column_exists_in_schema?(conn, "branch_feature_bulk_chg", "users", "avatar") + refute column_exists_in_schema?(conn, "public", "users", "nickname") + end + + # --- Join table DDL --- + + def test_create_join_table + conn = reconnect(branch: "feature/create-jt") + conn.create_join_table :orders, :users + + assert table_exists_in_schema?(conn, "branch_feature_create_jt", "orders_users") + refute table_exists_in_schema?(conn, "public", "orders_users") + end + + def test_drop_join_table + conn = connect(branch: "main") + conn.create_join_table :orders, :users + + conn = reconnect(branch: "feature/drop-jt") + conn.drop_join_table :orders, :users + + refute table_exists_in_schema?(conn, "branch_feature_drop_jt", "orders_users") + assert table_exists_in_schema?(conn, "public", "orders_users"), + "Public join table must survive branch drop" + end + + # --- Shadow structural equivalence --- + # After shadowing, the branch table should have the same columns, + # indexes, and constraints as public (before the DDL change). + + def test_shadow_preserves_all_columns + conn = connect(branch: "main") + conn.add_index :users, :email, unique: true + + conn = reconnect(branch: "feature/equiv-cols") + conn.add_column :users, :bio, :string + + public_cols = conn.select_values(<<~SQL).sort + SELECT column_name FROM information_schema.columns + WHERE table_schema = 'public' AND table_name = 'users' + SQL + branch_cols = conn.select_values(<<~SQL).sort + SELECT column_name FROM information_schema.columns + WHERE table_schema = 'branch_feature_equiv_cols' AND table_name = 'users' + SQL + + assert_equal (public_cols + ["bio"]).sort, branch_cols, + "Branch should have all public columns plus the new one" + end + + def test_shadow_preserves_all_indexes + conn = connect(branch: "main") + conn.add_index :users, :email, unique: true, name: "idx_users_email" + conn.add_index :users, :name, name: "idx_users_name" + + conn = reconnect(branch: "feature/equiv-idx") + conn.add_column :users, :bio, :string + + public_indexes = conn.select_values(<<~SQL).sort + SELECT indexname FROM pg_indexes + WHERE schemaname = 'public' AND tablename = 'users' + SQL + branch_indexes = conn.select_values(<<~SQL).sort + SELECT indexname FROM pg_indexes + WHERE schemaname = 'branch_feature_equiv_idx' AND tablename = 'users' + SQL + + assert_equal public_indexes, branch_indexes, + "Branch should preserve all indexes from public" + end + + def test_shadow_preserves_data + conn = reconnect(branch: "feature/equiv-data") + conn.add_column :users, :bio, :string + + public_count = conn.select_value("SELECT count(*) FROM public.users") + branch_count = conn.select_value("SELECT count(*) FROM branch_feature_equiv_data.users") + assert_equal public_count, branch_count, "Shadow must copy all rows" + end + + private + + def foreign_key_exists_on_branch?(conn, schema, from_table, to_table) + conn.select_value(<<~SQL) == 1 + SELECT 1 FROM pg_constraint c + JOIN pg_class t ON t.oid = c.conrelid + JOIN pg_namespace n ON n.oid = t.relnamespace + JOIN pg_class ref ON ref.oid = c.confrelid + WHERE c.contype = 'f' + AND n.nspname = #{conn.quote(schema)} + AND t.relname = #{conn.quote(from_table)} + AND ref.relname = #{conn.quote(to_table)} + SQL + end + + def constraint_exists?(conn, schema, table, constraint_name) + conn.select_value(<<~SQL) == 1 + SELECT 1 FROM pg_constraint c + JOIN pg_class t ON t.oid = c.conrelid + JOIN pg_namespace n ON n.oid = t.relnamespace + WHERE n.nspname = #{conn.quote(schema)} + AND t.relname = #{conn.quote(table)} + AND c.conname = #{conn.quote(constraint_name)} + SQL + end +end From 4861f1f7621065f459b82e7e108430ddb35050e0 Mon Sep 17 00:00:00 2001 From: Carl Dawson Date: Sat, 18 Apr 2026 12:15:30 +0100 Subject: [PATCH 2/8] Adds meta test to ensure all DDL instance methods are covered --- test/ddl_coverage_test.rb | 51 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/test/ddl_coverage_test.rb b/test/ddl_coverage_test.rb index 1084cd0..2d5f646 100644 --- a/test/ddl_coverage_test.rb +++ b/test/ddl_coverage_test.rb @@ -482,8 +482,59 @@ def test_shadow_preserves_data assert_equal public_count, branch_count, "Shadow must copy all rows" end + # --- Meta: ensure every DDL method has a test --- + + TABLE_PARAMS = %i[table_name from_table table_1].freeze + + # Methods that don't need their own test — either aliases of tested + # methods, internal helpers, or read-only queries. + SKIP_COVERAGE = %i[ + add_belongs_to remove_belongs_to + add_index_options + build_add_column_definition build_change_column_default_definition + build_change_column_definition build_create_index_definition + build_create_join_table_definition build_create_table_definition + check_constraint_exists? check_constraint_options check_constraints + column_exists? columns + default_sequence_name disable_index enable_index + exclusion_constraint_options exclusion_constraints + foreign_key_column_for foreign_key_exists? foreign_key_options foreign_keys + foreign_table_exists? + index_exists? index_name index_name_exists? indexes + inherited_table_names + primary_key primary_keys + table_alias_for table_comment table_exists? table_options table_partition_definition + unique_constraint_options unique_constraints + update_table_definition + ].freeze + + def test_every_ddl_method_has_coverage + test_names = self.class.instance_methods(false).grep(/\Atest_/).map(&:to_s) + untested = table_methods - SKIP_COVERAGE + + untested.reject! do |method| + test_names.any? { |t| t == "test_#{method}" || t.start_with?("test_#{method}_") } + end + + assert_empty untested, + "DDL methods without test coverage: #{untested.sort.join(', ')}. " \ + "Add a test or add to SKIP_COVERAGE with a reason." + end + private + def table_methods + [ + ActiveRecord::ConnectionAdapters::SchemaStatements, + ActiveRecord::ConnectionAdapters::PostgreSQL::SchemaStatements + ].flat_map { |mod| + mod.instance_methods(false).select { |m| + first_param = mod.instance_method(m).parameters.first + first_param && TABLE_PARAMS.include?(first_param.last) + } + }.uniq + end + def foreign_key_exists_on_branch?(conn, schema, from_table, to_table) conn.select_value(<<~SQL) == 1 SELECT 1 FROM pg_constraint c From 0cdef85caaa2f4b935abb56215d369a7ec987e4d Mon Sep 17 00:00:00 2001 From: Carl Dawson Date: Sat, 18 Apr 2026 12:43:31 +0100 Subject: [PATCH 3/8] Abstracts table methods to adapter --- .../postgresql/branched/adapter.rb | 49 ++++++++++--------- test/ddl_coverage_test.rb | 16 +----- 2 files changed, 28 insertions(+), 37 deletions(-) diff --git a/lib/active_record/connection_adapters/postgresql/branched/adapter.rb b/lib/active_record/connection_adapters/postgresql/branched/adapter.rb index 4fcb8fc..c6845c8 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/adapter.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/adapter.rb @@ -2,31 +2,36 @@ module ActiveRecord module ConnectionAdapters module PostgreSQL module Branched + TABLE_PARAMS = %i[table_name from_table table_1].freeze + + # All SchemaStatements methods whose first parameter is a table name. + def self.table_methods + [ + ActiveRecord::ConnectionAdapters::SchemaStatements, + ActiveRecord::ConnectionAdapters::PostgreSQL::SchemaStatements + ].flat_map { |mod| + mod.instance_methods(false).select { |m| + first_param = mod.instance_method(m).parameters.first + first_param && TABLE_PARAMS.include?(first_param.last) + } + }.uniq + end + class Adapter < ActiveRecord::ConnectionAdapters::PostgreSQLAdapter ADAPTER_NAME = "PostgreSQL Branched" - SHADOW_BEFORE = %i[ - add_column - remove_column - rename_column - change_column - change_column_default - change_column_null - change_column_comment - change_table_comment - add_index - remove_index - rename_index - add_foreign_key - remove_foreign_key - add_check_constraint - remove_check_constraint - validate_foreign_key - validate_check_constraint - drop_table - change_table - bulk_change_table - ].freeze + # Methods where the first argument is not the table to shadow, + # or where shadowing would be wasteful (join table methods pass + # table_1, not the derived join table name). + SHADOW_SKIP = %i[create_join_table drop_join_table].freeze + + # Methods with custom shadow handling outside the generic loop. + SHADOW_HANDLED = %i[rename_table].freeze + + # Intercept everything except SHADOW_SKIP and SHADOW_HANDLED. + # Shadow#call is idempotent and safe — if the table doesn't + # exist in public or is already shadowed, it's a no-op. + SHADOW_BEFORE = Branched.table_methods.-(SHADOW_SKIP).-(SHADOW_HANDLED).freeze def initialize(...) super diff --git a/test/ddl_coverage_test.rb b/test/ddl_coverage_test.rb index 2d5f646..0f36edd 100644 --- a/test/ddl_coverage_test.rb +++ b/test/ddl_coverage_test.rb @@ -484,8 +484,6 @@ def test_shadow_preserves_data # --- Meta: ensure every DDL method has a test --- - TABLE_PARAMS = %i[table_name from_table table_1].freeze - # Methods that don't need their own test — either aliases of tested # methods, internal helpers, or read-only queries. SKIP_COVERAGE = %i[ @@ -510,7 +508,7 @@ def test_shadow_preserves_data def test_every_ddl_method_has_coverage test_names = self.class.instance_methods(false).grep(/\Atest_/).map(&:to_s) - untested = table_methods - SKIP_COVERAGE + untested = ActiveRecord::ConnectionAdapters::PostgreSQL::Branched.table_methods - SKIP_COVERAGE untested.reject! do |method| test_names.any? { |t| t == "test_#{method}" || t.start_with?("test_#{method}_") } @@ -523,18 +521,6 @@ def test_every_ddl_method_has_coverage private - def table_methods - [ - ActiveRecord::ConnectionAdapters::SchemaStatements, - ActiveRecord::ConnectionAdapters::PostgreSQL::SchemaStatements - ].flat_map { |mod| - mod.instance_methods(false).select { |m| - first_param = mod.instance_method(m).parameters.first - first_param && TABLE_PARAMS.include?(first_param.last) - } - }.uniq - end - def foreign_key_exists_on_branch?(conn, schema, from_table, to_table) conn.select_value(<<~SQL) == 1 SELECT 1 FROM pg_constraint c From bd1993352c6d6ea1c933be8b6aa099c64a69695f Mon Sep 17 00:00:00 2001 From: Carl Dawson Date: Sat, 18 Apr 2026 13:13:51 +0100 Subject: [PATCH 4/8] Shadows contraints not included by LIKE --- .../postgresql/branched/adapter.rb | 20 ++++----- .../postgresql/branched/shadow.rb | 44 +++++++++++++++++-- test/ddl_coverage_test.rb | 3 +- 3 files changed, 52 insertions(+), 15 deletions(-) diff --git a/lib/active_record/connection_adapters/postgresql/branched/adapter.rb b/lib/active_record/connection_adapters/postgresql/branched/adapter.rb index c6845c8..2be7b3b 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/adapter.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/adapter.rb @@ -2,9 +2,9 @@ module ActiveRecord module ConnectionAdapters module PostgreSQL module Branched - TABLE_PARAMS = %i[table_name from_table table_1].freeze - - # All SchemaStatements methods whose first parameter is a table name. + # All SchemaStatements methods whose first parameter name + # contains "table" — covers table_name, from_table, table_1, + # table_names, and any future variant. def self.table_methods [ ActiveRecord::ConnectionAdapters::SchemaStatements, @@ -12,7 +12,7 @@ def self.table_methods ].flat_map { |mod| mod.instance_methods(false).select { |m| first_param = mod.instance_method(m).parameters.first - first_param && TABLE_PARAMS.include?(first_param.last) + first_param && first_param.last.to_s.include?("table") } }.uniq end @@ -28,9 +28,9 @@ class Adapter < ActiveRecord::ConnectionAdapters::PostgreSQLAdapter # Methods with custom shadow handling outside the generic loop. SHADOW_HANDLED = %i[rename_table].freeze - # Intercept everything except SHADOW_SKIP and SHADOW_HANDLED. - # Shadow#call is idempotent and safe — if the table doesn't - # exist in public or is already shadowed, it's a no-op. + # Everything else with a table param gets the generic wrapper. + # Shadow#call is idempotent — if the table doesn't exist in + # public or is already shadowed, it's a no-op. SHADOW_BEFORE = Branched.table_methods.-(SHADOW_SKIP).-(SHADOW_HANDLED).freeze def initialize(...) @@ -45,9 +45,9 @@ def configure_connection end SHADOW_BEFORE.each do |method| - define_method(method) do |table_name, *args, **kwargs, &block| - @shadow&.call(table_name) - super(table_name, *args, **kwargs, &block) + define_method(method) do |*args, **kwargs, &block| + @shadow&.call(args.first) + super(*args, **kwargs, &block) end end diff --git a/lib/active_record/connection_adapters/postgresql/branched/shadow.rb b/lib/active_record/connection_adapters/postgresql/branched/shadow.rb index a9ca9d7..3bc5941 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/shadow.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/shadow.rb @@ -40,6 +40,7 @@ def create_shadow(table) clone_structure(quoted_branch, quoted_table) copy_data(quoted_branch, quoted_table) clone_indexes(table, quoted_branch, quoted_table) + clone_constraints(table, quoted_branch, quoted_table) end # Per-row index maintenance during INSERT SELECT dominates the cost @@ -76,10 +77,11 @@ def clone_indexes(table, quoted_branch, quoted_table) end end - # Redirect pg_get_indexdef output from public to the branch schema. - # The REPLACE uses the same quote_ident() that pg_get_indexdef uses - # to emit identifiers, so the anchor always matches regardless of - # whether the table name needs quoting. + # LIKE ... INCLUDING ALL EXCLUDING INDEXES drops all index-backed + # constraints (PK, unique, exclusion). clone_constraints re-emits + # unique and exclusion constraints which implicitly create their + # backing indexes, so we skip those here to avoid name collisions. + # PK indexes are rebuilt and reattached via USING INDEX. def index_metadata(table) @connection.select_rows(<<~SQL) SELECT i.relname, ix.indisprimary, @@ -94,6 +96,40 @@ def index_metadata(table) JOIN pg_namespace n ON n.oid = t.relnamespace WHERE n.nspname = 'public' AND t.relname = #{@connection.quote(table)} + AND NOT EXISTS ( + SELECT 1 FROM pg_constraint c + WHERE c.conindid = ix.indexrelid + AND c.contype IN ('x', 'u') + ) + SQL + end + + # LIKE ... INCLUDING CONSTRAINTS copies check constraints and NOT + # NULL but never foreign keys (Postgres design). EXCLUDING INDEXES + # also drops exclusion and unique constraints since they're index- + # backed. Re-emit each from pg_constraint; the definitions from + # pg_get_constraintdef are self-contained and resolve correctly + # via the branch search_path without rewriting. + def clone_constraints(table, quoted_branch, quoted_table) + constraint_definitions(table).each do |conname, condef| + quoted_con = @connection.quote_column_name(conname) + @connection.execute(<<~SQL) + ALTER TABLE #{quoted_branch}.#{quoted_table} + ADD CONSTRAINT #{quoted_con} #{condef} + SQL + end + end + + def constraint_definitions(table) + @connection.select_rows(<<~SQL) + SELECT c.conname, pg_get_constraintdef(c.oid, true) + FROM pg_constraint c + JOIN pg_class t ON t.oid = c.conrelid + JOIN pg_namespace n ON n.oid = t.relnamespace + WHERE n.nspname = 'public' + AND t.relname = #{@connection.quote(table)} + AND c.contype IN ('f', 'x', 'u') + ORDER BY c.conname SQL end end diff --git a/test/ddl_coverage_test.rb b/test/ddl_coverage_test.rb index 0f36edd..4fafbb7 100644 --- a/test/ddl_coverage_test.rb +++ b/test/ddl_coverage_test.rb @@ -500,7 +500,8 @@ def test_shadow_preserves_data foreign_table_exists? index_exists? index_name index_name_exists? indexes inherited_table_names - primary_key primary_keys + pk_and_sequence_for primary_key primary_keys + reset_pk_sequence! serial_sequence set_pk_sequence! table_alias_for table_comment table_exists? table_options table_partition_definition unique_constraint_options unique_constraints update_table_definition From 02d2e5095538b5e071b3df361420d879db2dd266 Mon Sep 17 00:00:00 2001 From: Carl Dawson Date: Sat, 18 Apr 2026 13:37:07 +0100 Subject: [PATCH 5/8] Implements drop_table on branch schemas --- .../postgresql/branched/adapter.rb | 24 +++++++++++- .../postgresql/branched/branch_manager.rb | 3 +- .../postgresql/branched/schema_dumper.rb | 1 + .../postgresql/branched/shadow.rb | 37 +++++++++++++++++++ test/adapter_test.rb | 2 +- test/ddl_coverage_test.rb | 19 +++++----- 6 files changed, 74 insertions(+), 12 deletions(-) diff --git a/lib/active_record/connection_adapters/postgresql/branched/adapter.rb b/lib/active_record/connection_adapters/postgresql/branched/adapter.rb index 2be7b3b..38a4b77 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/adapter.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/adapter.rb @@ -51,6 +51,28 @@ def configure_connection end end + # drop_table takes *table_names (splat) in Rails 8.1+. + # For tables from public: shadow first (so the branch copy + # exists for DROP to resolve to), then super drops the branch + # copy, then create a tombstone in the dropped schema to block + # search_path fallthrough. + def drop_table(*table_names, **options) + if @branch_manager.primary_branch? + super + else + table_names.each { |t| @shadow&.call(t) } + super + table_names.each { |t| @shadow&.drop_table(t) } + end + end + + # create_table after drop_table should work — remove the + # tombstone so the new table takes precedence. + def create_table(*args, **kwargs, &block) + @shadow&.undrop_table(args.first) + super + end + # rename_table needs special handling: the shadow table's sequences # live in public, but Rails' rename_table tries to rename them using # the branch schema. The table and index renames succeed before the @@ -63,7 +85,7 @@ def rename_table(table_name, new_name, **options) raise unless e.cause.is_a?(PG::UndefinedTable) end - attr_reader :branch_manager + attr_reader :branch_manager, :shadow end end end diff --git a/lib/active_record/connection_adapters/postgresql/branched/branch_manager.rb b/lib/active_record/connection_adapters/postgresql/branched/branch_manager.rb index 4e6b2ef..dee4335 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/branch_manager.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/branch_manager.rb @@ -133,7 +133,8 @@ def drop_schema end def set_search_path - @connection.schema_search_path = "#{@branch_schema}, public" + dropped_schema = "#{@branch_schema}__dropped" + @connection.schema_search_path = "#{dropped_schema}, #{@branch_schema}, public" end def shadow_migration_tables(shadow) diff --git a/lib/active_record/connection_adapters/postgresql/branched/schema_dumper.rb b/lib/active_record/connection_adapters/postgresql/branched/schema_dumper.rb index 3b4ebf9..cf7e242 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/schema_dumper.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/schema_dumper.rb @@ -27,6 +27,7 @@ def tables(stream) table_names = @connection.tables.uniq.sort table_names.reject! { |t| ignored?(t) } + table_names.reject! { |t| @connection.shadow&.dropped?(t) } table_names.each_with_index do |table_name, index| table(table_name, stream) diff --git a/lib/active_record/connection_adapters/postgresql/branched/shadow.rb b/lib/active_record/connection_adapters/postgresql/branched/shadow.rb index 3bc5941..0da377f 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/shadow.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/shadow.rb @@ -6,18 +6,55 @@ class Shadow def initialize(connection, branch_schema) @connection = connection @branch_schema = branch_schema + @dropped_schema = "#{branch_schema}__dropped" end + attr_reader :dropped_schema + def call(table_name) table = table_name.to_s return unless exists_in_public?(table) return if already_shadowed?(table) + return if dropped?(table) create_shadow(table) end + def drop_table(table) + table = table.to_s + return unless exists_in_public?(table) + + ensure_dropped_schema + quoted_dropped = @connection.quote_column_name(@dropped_schema) + quoted_table = @connection.quote_column_name(table) + @connection.execute(<<~SQL) + CREATE TABLE #{quoted_dropped}.#{quoted_table} + (LIKE public.#{quoted_table}) + SQL + end + + def undrop_table(table) + return unless dropped?(table) + quoted_dropped = @connection.quote_column_name(@dropped_schema) + quoted_table = @connection.quote_column_name(table) + @connection.execute("DROP TABLE #{quoted_dropped}.#{quoted_table}") + end + + def dropped?(table_name) + @connection.select_value(<<~SQL) == 1 + SELECT 1 FROM information_schema.tables + WHERE table_schema = #{@connection.quote(@dropped_schema)} + AND table_name = #{@connection.quote(table_name.to_s)} + SQL + end + private + def ensure_dropped_schema + quoted = @connection.quote_column_name(@dropped_schema) + @connection.execute("CREATE SCHEMA IF NOT EXISTS #{quoted}") + end + def exists_in_public?(table) @connection.select_value(<<~SQL) == 1 SELECT 1 FROM information_schema.tables diff --git a/test/adapter_test.rb b/test/adapter_test.rb index e541b97..8d293f9 100644 --- a/test/adapter_test.rb +++ b/test/adapter_test.rb @@ -53,7 +53,7 @@ def test_feature_branch_creates_schema def test_feature_branch_sets_search_path conn = connect(branch: "feature/payments") search_path = conn.select_value("SHOW search_path") - assert_equal "branch_feature_payments, public", search_path + assert_equal "branch_feature_payments__dropped, branch_feature_payments, public", search_path end def test_feature_branch_table_takes_precedence_over_public diff --git a/test/ddl_coverage_test.rb b/test/ddl_coverage_test.rb index 4fafbb7..40ffcfa 100644 --- a/test/ddl_coverage_test.rb +++ b/test/ddl_coverage_test.rb @@ -363,17 +363,18 @@ def test_drop_table refute table_exists_in_schema?(conn, "branch_feature_drop_tbl", "orders") end - def test_drop_table_is_not_visible_on_branch - skip "Known bug: dropped tables fall through search_path to public" - - conn = reconnect(branch: "feature/drop-vis") + def test_drop_table_prevents_re_shadow + conn = reconnect(branch: "feature/drop-reshadow") conn.drop_table :orders - # After dropping on a branch, the table should NOT be queryable - # via the branch's search_path. - assert_raises(ActiveRecord::StatementInvalid) do - conn.execute("INSERT INTO orders (status, amount) VALUES ('new', 200)") - end + # After dropping on a branch, Shadow#call must not re-shadow the + # table (e.g. via a read-only method that triggers the wrapper). + # The table still falls through to public via search_path, but + # the schema dumper will exclude it. + conn.add_column :users, :bio, :string # triggers shadow machinery + + refute table_exists_in_schema?(conn, "branch_feature_drop_reshadow", "orders"), + "Dropped table must not be re-shadowed" end def test_rename_table From 794b472883e645eeb9f18cb690709da9374841a5 Mon Sep 17 00:00:00 2001 From: Carl Dawson Date: Sat, 18 Apr 2026 13:51:32 +0100 Subject: [PATCH 6/8] Handles enum DDL via shadowing --- .../postgresql/branched/adapter.rb | 10 ++ .../postgresql/branched/shadow.rb | 37 ++++++ test/ddl_coverage_test.rb | 110 ++++++++++++++++++ 3 files changed, 157 insertions(+) diff --git a/lib/active_record/connection_adapters/postgresql/branched/adapter.rb b/lib/active_record/connection_adapters/postgresql/branched/adapter.rb index 38a4b77..7775b77 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/adapter.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/adapter.rb @@ -73,6 +73,16 @@ def create_table(*args, **kwargs, &block) super end + # Enum DDL resolves unqualified type names through search_path, + # so modifications on a feature branch would otherwise mutate + # public. Shadow the enum into the branch schema first. + %i[drop_enum rename_enum add_enum_value rename_enum_value].each do |method| + define_method(method) do |name, *args, **kwargs, &block| + @shadow&.shadow_enum(name) + super(name, *args, **kwargs, &block) + end + end + # rename_table needs special handling: the shadow table's sequences # live in public, but Rails' rename_table tries to rename them using # the branch schema. The table and index renames succeed before the diff --git a/lib/active_record/connection_adapters/postgresql/branched/shadow.rb b/lib/active_record/connection_adapters/postgresql/branched/shadow.rb index 0da377f..fc6bbbc 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/shadow.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/shadow.rb @@ -20,6 +20,27 @@ def call(table_name) create_shadow(table) end + def shadow_enum(name) + name = name.to_s + return unless enum_exists_in_public?(name) + return if enum_exists_in_branch?(name) + + labels = @connection.select_values(<<~SQL) + SELECT e.enumlabel FROM pg_enum e + JOIN pg_type t ON t.oid = e.enumtypid + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE n.nspname = 'public' AND t.typname = #{@connection.quote(name)} + ORDER BY e.enumsortorder + SQL + + quoted_branch = @connection.quote_column_name(@branch_schema) + quoted_labels = labels.map { |l| @connection.quote(l) }.join(", ") + @connection.execute(<<~SQL) + CREATE TYPE #{quoted_branch}.#{@connection.quote_column_name(name)} + AS ENUM (#{quoted_labels}) + SQL + end + def drop_table(table) table = table.to_s return unless exists_in_public?(table) @@ -50,6 +71,22 @@ def dropped?(table_name) private + def enum_exists_in_public?(name) + @connection.select_value(<<~SQL) == 1 + SELECT 1 FROM pg_type t + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE n.nspname = 'public' AND t.typname = #{@connection.quote(name)} AND t.typtype = 'e' + SQL + end + + def enum_exists_in_branch?(name) + @connection.select_value(<<~SQL) == 1 + SELECT 1 FROM pg_type t + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE n.nspname = #{@connection.quote(@branch_schema)} AND t.typname = #{@connection.quote(name)} AND t.typtype = 'e' + SQL + end + def ensure_dropped_schema quoted = @connection.quote_column_name(@dropped_schema) @connection.execute("CREATE SCHEMA IF NOT EXISTS #{quoted}") diff --git a/test/ddl_coverage_test.rb b/test/ddl_coverage_test.rb index 40ffcfa..654d387 100644 --- a/test/ddl_coverage_test.rb +++ b/test/ddl_coverage_test.rb @@ -483,6 +483,106 @@ def test_shadow_preserves_data assert_equal public_count, branch_count, "Shadow must copy all rows" end + # --- Enum DDL --- + + def test_create_enum + conn = reconnect(branch: "feature/create-enum") + conn.create_enum :status, %w[draft published] + + values = conn.select_values(<<~SQL) + SELECT e.enumlabel FROM pg_enum e + JOIN pg_type t ON t.oid = e.enumtypid + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE t.typname = 'status' + AND n.nspname = 'branch_feature_create_enum' + ORDER BY e.enumsortorder + SQL + assert_equal %w[draft published], values + end + + def test_drop_enum + conn = connect(branch: "main") + conn.create_enum :status, %w[draft published] + + conn = reconnect(branch: "feature/drop-enum") + conn.drop_enum :status + + # Public enum must survive + assert enum_exists_in_schema?(conn, "public", "status") + # Branch enum should be gone + refute enum_exists_in_schema?(conn, "branch_feature_drop_enum", "status") + end + + def test_add_enum_value + conn = connect(branch: "main") + conn.create_enum :status, %w[draft published] + + conn = reconnect(branch: "feature/add-enum-val") + conn.add_enum_value :status, "archived" + + branch_values = conn.select_values(<<~SQL) + SELECT e.enumlabel FROM pg_enum e + JOIN pg_type t ON t.oid = e.enumtypid + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE t.typname = 'status' + AND n.nspname = 'branch_feature_add_enum_val' + ORDER BY e.enumsortorder + SQL + assert_includes branch_values, "archived" + + public_values = conn.select_values(<<~SQL) + SELECT e.enumlabel FROM pg_enum e + JOIN pg_type t ON t.oid = e.enumtypid + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE t.typname = 'status' + AND n.nspname = 'public' + ORDER BY e.enumsortorder + SQL + refute_includes public_values, "archived" + end + + def test_rename_enum + conn = connect(branch: "main") + conn.create_enum :status, %w[draft published] + + conn = reconnect(branch: "feature/ren-enum") + conn.rename_enum :status, :article_status + + assert enum_exists_in_schema?(conn, "branch_feature_ren_enum", "article_status") + assert enum_exists_in_schema?(conn, "public", "status"), + "Public enum must keep original name" + end + + def test_rename_enum_value + conn = connect(branch: "main") + conn.create_enum :status, %w[draft published] + + conn = reconnect(branch: "feature/ren-enum-val") + conn.rename_enum_value :status, from: "draft", to: "pending" + + branch_values = conn.select_values(<<~SQL) + SELECT e.enumlabel FROM pg_enum e + JOIN pg_type t ON t.oid = e.enumtypid + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE t.typname = 'status' + AND n.nspname = 'branch_feature_ren_enum_val' + ORDER BY e.enumsortorder + SQL + assert_includes branch_values, "pending" + refute_includes branch_values, "draft" + + public_values = conn.select_values(<<~SQL) + SELECT e.enumlabel FROM pg_enum e + JOIN pg_type t ON t.oid = e.enumtypid + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE t.typname = 'status' + AND n.nspname = 'public' + ORDER BY e.enumsortorder + SQL + assert_includes public_values, "draft", + "Public enum values must be untouched" + end + # --- Meta: ensure every DDL method has a test --- # Methods that don't need their own test — either aliases of tested @@ -536,6 +636,16 @@ def foreign_key_exists_on_branch?(conn, schema, from_table, to_table) SQL end + def enum_exists_in_schema?(conn, schema, enum_name) + conn.select_value(<<~SQL) == 1 + SELECT 1 FROM pg_type t + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE n.nspname = #{conn.quote(schema)} + AND t.typname = #{conn.quote(enum_name.to_s)} + AND t.typtype = 'e' + SQL + end + def constraint_exists?(conn, schema, table, constraint_name) conn.select_value(<<~SQL) == 1 SELECT 1 FROM pg_constraint c From 3db64d2e221f55e0690b2bf42936c59c96b0940c Mon Sep 17 00:00:00 2001 From: Carl Dawson Date: Sat, 18 Apr 2026 13:59:29 +0100 Subject: [PATCH 7/8] Updates README --- README.md | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 5d9fa40..fc589f4 100644 --- a/README.md +++ b/README.md @@ -54,14 +54,17 @@ When a migration modifies a table that exists in `public` but not yet in the bra ``` add_column :users, :bio, :string -1. CREATE TABLE branch_feature_payments.users (LIKE public.users INCLUDING ALL) +1. CREATE TABLE branch_feature_payments.users (LIKE public.users ...) 2. INSERT INTO branch_feature_payments.users SELECT * FROM public.users -3. ALTER TABLE users ADD COLUMN bio VARCHAR +3. Rebuild indexes, foreign keys, exclusion and unique constraints +4. ALTER TABLE users ADD COLUMN bio VARCHAR ``` -The public table is never touched. Step 3 operates on the shadow because `search_path` resolves `users` to the branch schema first. +The public table is never touched. Step 4 operates on the shadow because `search_path` resolves `users` to the branch schema first. -This applies to any DDL that modifies an existing table: `add_column`, `remove_column`, `add_index`, `add_foreign_key`, `drop_table`, and so on. +The adapter intercepts every `SchemaStatements` method that takes a table name — column DDL, index DDL, constraint DDL, `drop_table`, `change_table`, timestamps, references, and so on. Enum type DDL (`drop_enum`, `rename_enum`, `add_enum_value`, `rename_enum_value`) is also intercepted so enum modifications stay on the branch. + +When you `drop_table` on a branch, a lightweight tombstone is created in a separate schema to block `search_path` fallthrough to public. The public table is untouched and the schema dumper excludes it. ### schema.rb stays clean @@ -155,5 +158,6 @@ All standard PostgreSQL connection parameters work as normal (`host`, `port`, `u - **Rails + PostgreSQL only** — uses PostgreSQL schemas and `search_path` - **Development and test only** — production should use the standard `postgresql` adapter -- **Non-ActiveRecord DDL** — raw SQL outside of migrations bypasses the shadow rule -- **Sequences on shadowed tables** — `rename_table` on a shadowed table with serial columns works, but the sequence keeps its original name +- **Raw SQL DDL** — `execute("CREATE TRIGGER ...")` and similar raw SQL bypasses the shadow rule. If another intercepted DDL method has already shadowed the table in the same migration, the raw SQL will land on the branch copy via `search_path`. If the raw SQL is the first thing touching that table, it will hit `public`. Structure your migration so that an intercepted method (e.g. `add_column`) runs first, or use `db:branch:reset && db:migrate` to rebuild the branch from scratch. +- **Sequences on shadowed tables** — `rename_table` on a shadowed table with serial columns works, but the sequence keeps its original name since renaming it would affect all branches +- **Concurrent shadow creation** — if parallel workers (parallel tests, multiple consoles) shadow the same table simultaneously, a race condition can cause duplicate data or a `DuplicateTable` error. Restart the workers or run `db:branch:reset` to recover From a7eb345efd9b3a8b53080adc5de496acb5005097 Mon Sep 17 00:00:00 2001 From: Carl Dawson Date: Sat, 18 Apr 2026 14:13:59 +0100 Subject: [PATCH 8/8] Cleanup --- .../postgresql/branched/adapter.rb | 15 ++++++++------- .../postgresql/branched/branch_manager.rb | 2 +- .../postgresql/branched/schema_dumper.rb | 2 +- .../postgresql/branched/shadow.rb | 13 ++++++------- test/ddl_coverage_test.rb | 6 ++---- 5 files changed, 18 insertions(+), 20 deletions(-) diff --git a/lib/active_record/connection_adapters/postgresql/branched/adapter.rb b/lib/active_record/connection_adapters/postgresql/branched/adapter.rb index 7775b77..40ce2b6 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/adapter.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/adapter.rb @@ -31,7 +31,7 @@ class Adapter < ActiveRecord::ConnectionAdapters::PostgreSQLAdapter # Everything else with a table param gets the generic wrapper. # Shadow#call is idempotent — if the table doesn't exist in # public or is already shadowed, it's a no-op. - SHADOW_BEFORE = Branched.table_methods.-(SHADOW_SKIP).-(SHADOW_HANDLED).freeze + SHADOW_BEFORE = (Branched.table_methods - SHADOW_SKIP - SHADOW_HANDLED).freeze def initialize(...) super @@ -51,11 +51,8 @@ def configure_connection end end - # drop_table takes *table_names (splat) in Rails 8.1+. - # For tables from public: shadow first (so the branch copy - # exists for DROP to resolve to), then super drops the branch - # copy, then create a tombstone in the dropped schema to block - # search_path fallthrough. + # Shadow each table so DROP resolves to the branch copy, then + # create a tombstone in the dropped schema to block fallthrough. def drop_table(*table_names, **options) if @branch_manager.primary_branch? super @@ -95,7 +92,11 @@ def rename_table(table_name, new_name, **options) raise unless e.cause.is_a?(PG::UndefinedTable) end - attr_reader :branch_manager, :shadow + attr_reader :branch_manager + + def table_dropped_on_branch?(table_name) + @shadow&.dropped?(table_name) || false + end end end end diff --git a/lib/active_record/connection_adapters/postgresql/branched/branch_manager.rb b/lib/active_record/connection_adapters/postgresql/branched/branch_manager.rb index dee4335..1ed48c3 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/branch_manager.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/branch_manager.rb @@ -133,7 +133,7 @@ def drop_schema end def set_search_path - dropped_schema = "#{@branch_schema}__dropped" + dropped_schema = "#{@branch_schema}#{Shadow::DROPPED_SUFFIX}" @connection.schema_search_path = "#{dropped_schema}, #{@branch_schema}, public" end diff --git a/lib/active_record/connection_adapters/postgresql/branched/schema_dumper.rb b/lib/active_record/connection_adapters/postgresql/branched/schema_dumper.rb index cf7e242..eaa009d 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/schema_dumper.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/schema_dumper.rb @@ -27,7 +27,7 @@ def tables(stream) table_names = @connection.tables.uniq.sort table_names.reject! { |t| ignored?(t) } - table_names.reject! { |t| @connection.shadow&.dropped?(t) } + table_names.reject! { |t| @connection.table_dropped_on_branch?(t) } table_names.each_with_index do |table_name, index| table(table_name, stream) diff --git a/lib/active_record/connection_adapters/postgresql/branched/shadow.rb b/lib/active_record/connection_adapters/postgresql/branched/shadow.rb index fc6bbbc..7607965 100644 --- a/lib/active_record/connection_adapters/postgresql/branched/shadow.rb +++ b/lib/active_record/connection_adapters/postgresql/branched/shadow.rb @@ -3,10 +3,12 @@ module ConnectionAdapters module PostgreSQL module Branched class Shadow + DROPPED_SUFFIX = "__dropped" + def initialize(connection, branch_schema) @connection = connection @branch_schema = branch_schema - @dropped_schema = "#{branch_schema}__dropped" + @dropped_schema = "#{branch_schema}#{DROPPED_SUFFIX}" end attr_reader :dropped_schema @@ -55,6 +57,7 @@ def drop_table(table) end def undrop_table(table) + table = table.to_s return unless dropped?(table) quoted_dropped = @connection.quote_column_name(@dropped_schema) quoted_table = @connection.quote_column_name(table) @@ -117,12 +120,8 @@ def create_shadow(table) clone_constraints(table, quoted_branch, quoted_table) end - # Per-row index maintenance during INSERT SELECT dominates the cost - # of shadowing large tables. LIKE ... INCLUDING ALL builds every - # index up front, then the data copy updates each index for every - # row. EXCLUDING INDEXES lets the bulk copy run without index - # overhead; clone_indexes rebuilds them afterwards in a single - # sort pass per index, the standard Postgres bulk-load pattern. + # Structure without indexes — they're rebuilt after the bulk + # data copy for better performance (sort-based vs per-row). def clone_structure(quoted_branch, quoted_table) @connection.execute(<<~SQL) CREATE TABLE #{quoted_branch}.#{quoted_table} diff --git a/test/ddl_coverage_test.rb b/test/ddl_coverage_test.rb index 654d387..6c61701 100644 --- a/test/ddl_coverage_test.rb +++ b/test/ddl_coverage_test.rb @@ -367,10 +367,8 @@ def test_drop_table_prevents_re_shadow conn = reconnect(branch: "feature/drop-reshadow") conn.drop_table :orders - # After dropping on a branch, Shadow#call must not re-shadow the - # table (e.g. via a read-only method that triggers the wrapper). - # The table still falls through to public via search_path, but - # the schema dumper will exclude it. + # Shadow#call must not re-shadow a dropped table. The tombstone + # in the dropped schema blocks search_path fallthrough to public. conn.add_column :users, :bio, :string # triggers shadow machinery refute table_exists_in_schema?(conn, "branch_feature_drop_reshadow", "orders"),