From ea54330fb0afd575a499a7d4cc6ebe0ae4d071ff Mon Sep 17 00:00:00 2001 From: Martin-Alexander Date: Wed, 16 Jul 2025 17:44:37 -0400 Subject: [PATCH 1/6] add support for add/remove_foreign_key and add/remove_reference --- .../adapter/migrations_modules/stable.rb | 30 +++ spec/chrono_model/adapter/migrations_spec.rb | 187 ++++++++++++++++++ spec/spec_helper.rb | 2 + .../matchers/foreign_key_constraint.rb | 63 ++++++ 4 files changed, 282 insertions(+) create mode 100644 spec/support/matchers/foreign_key_constraint.rb diff --git a/lib/chrono_model/adapter/migrations_modules/stable.rb b/lib/chrono_model/adapter/migrations_modules/stable.rb index 7bb6a80f..cb90492c 100644 --- a/lib/chrono_model/adapter/migrations_modules/stable.rb +++ b/lib/chrono_model/adapter/migrations_modules/stable.rb @@ -33,6 +33,36 @@ def remove_index(table_name, column_name = nil, **options) on_history_schema { super } end end + + def add_foreign_key(from_table, to_table, **options) + if is_chrono?(from_table) != is_chrono?(to_table) + return on_schema("#{TEMPORAL_SCHEMA},#{schema_search_path}") { super } + end + + if is_chrono?(from_table) + return on_temporal_schema { super } + end + + super + end + + def remove_foreign_key(from_table, to_table = nil, **options) + if to_table.nil? + return super unless is_chrono?(from_table) + + on_temporal_schema { super } + end + + if is_chrono?(from_table) != is_chrono?(to_table) + return on_schema("#{TEMPORAL_SCHEMA},#{schema_search_path}") { super } + end + + if is_chrono?(from_table) + return on_temporal_schema { super } + end + + super + end end end end diff --git a/spec/chrono_model/adapter/migrations_spec.rb b/spec/chrono_model/adapter/migrations_spec.rb index 42d10b8d..d968a795 100644 --- a/spec/chrono_model/adapter/migrations_spec.rb +++ b/spec/chrono_model/adapter/migrations_spec.rb @@ -2,6 +2,7 @@ require 'spec_helper' require 'support/adapter/structure' +require 'support/time_machine/structure' # For the structure of these tables, please see spec/support/adabters/structure.rb. @@ -40,6 +41,7 @@ describe '.create_table' do context 'with temporal tables' do include_context 'with temporal tables' + it_behaves_like 'temporal table' end @@ -491,4 +493,189 @@ it { is_expected.to have_columns([['foo', 'double precision']]) } end end + + describe '.add_foreign_key' do + let(:columns) do + native = [%w[baz_id bigint], %w[bar_id bigint]] + + def native.to_proc + proc { |t| + t.references :baz + t.references :bar + } + end + + native + end + + context 'with temporal tables' do + include_context 'with temporal tables' + + it 'adds constraints on foreign keys for plain tables' do + adapter.add_foreign_key table, :bazs + + expect(table).to have_temporal_foreign_key_constraint('bazs', 'public') + end + + it 'adds constraints on foreign keys for other temporal tables' do + adapter.add_foreign_key table, :bars + + expect(table).to have_temporal_foreign_key_constraint('bars', 'temporal') + end + end + + context 'with plain tables' do + include_context 'with plain tables' + + it 'adds constraints on foreign keys for other plain tables' do + adapter.add_foreign_key table, :bazs + + expect(table).to have_foreign_key_constraint('bazs', 'public') + end + + it 'adds constraints on foreign keys for temporal tables' do + adapter.add_foreign_key table, :bars + + expect(table).to have_foreign_key_constraint('bars', 'temporal') + end + end + end + + describe '.remove_foreign_key' do + context 'with temporal tables' do + include_context 'with temporal tables' + + before do + adapter.add_reference table, :baz, foreign_key: true + adapter.add_reference table, :bar, foreign_key: true + end + + it 'removes constraints on plain tables' do + adapter.remove_foreign_key table, :bazs + + expect(table).not_to have_temporal_foreign_key_constraint('bazs', 'public') + end + + it 'removes constraints on temporal tables' do + adapter.remove_foreign_key table, :bars + + expect(table).not_to have_temporal_foreign_key_constraint('bars', 'temporal') + end + end + + context 'with plain tables' do + include_context 'with plain tables' + + before do + adapter.add_reference table, :baz, foreign_key: true + adapter.add_reference table, :bar, foreign_key: true + end + + it 'removes constraints on plain tables' do + adapter.remove_foreign_key table, :bazs + + expect(table).not_to have_foreign_key_constraint('bazs', 'public') + end + + it 'removes constraints on temporal tables' do + adapter.remove_foreign_key table, :bars + + expect(table).not_to have_foreign_key_constraint('bars', 'temporal') + end + end + end + + describe '.add_reference' do + context 'with temporal tables' do + include_context 'with temporal tables' + + context 'to plain tables' do + it 'adds a foreign key and a constraint' do + adapter.add_reference table, :baz, foreign_key: true + + expect(table).to have_columns([%w[baz_id bigint]]) + expect(table).to have_temporal_foreign_key_constraint('bazs', 'public') + end + end + + context 'to other temporal tables' do + it 'adds a foreign key and a constraint' do + adapter.add_reference table, :bar, foreign_key: true + + expect(table).to have_columns([%w[bar_id bigint]]) + expect(table).to have_temporal_foreign_key_constraint('bars', 'temporal') + end + end + end + + context 'with plain tables' do + include_context 'with plain tables' + + context 'to other plain tables' do + it 'adds a foreign key and a constraint' do + adapter.add_reference table, :baz, foreign_key: true + + expect(table).to have_columns([%w[baz_id bigint]]) + expect(table).to have_foreign_key_constraint('bazs', 'public') + end + end + + context 'to temporal tables' do + it 'adds a foreign key and a constraint' do + adapter.add_reference table, :bar, foreign_key: true + + expect(table).to have_columns([%w[bar_id bigint]]) + expect(table).to have_foreign_key_constraint('bars', 'temporal') + end + end + end + end + + describe '.remove_reference' do + context 'with temporal tables' do + include_context 'with temporal tables' + + before do + adapter.add_reference table, :baz, foreign_key: true + adapter.add_reference table, :bar, foreign_key: true + end + + it 'removes the foreign key and constraint from plain tables' do + adapter.remove_reference table, :baz + + expect(table).not_to have_columns([%w[baz_id bigint]]) + expect(table).not_to have_temporal_foreign_key_constraint('bazs', 'public') + end + + it 'removes the foreign key and constraint from other temporal tables' do + adapter.remove_reference table, :bar + + expect(table).not_to have_columns([%w[bar_id bigint]]) + expect(table).not_to have_temporal_foreign_key_constraint('bars', 'temporal') + end + end + + context 'with plain tables' do + include_context 'with plain tables' + + before do + adapter.add_reference table, :baz, foreign_key: true + adapter.add_reference table, :bar, foreign_key: true + end + + it 'removes the foreign key and constraint from other plain tables' do + adapter.remove_reference table, :baz + + expect(table).not_to have_columns([%w[baz_id bigint]]) + expect(table).not_to have_foreign_key_constraint('bazs', 'public') + end + + it 'removes the foreign key and constraint from temporal tables' do + adapter.remove_reference table, :bar + + expect(table).not_to have_columns([%w[bar_id bigint]]) + expect(table).not_to have_foreign_key_constraint('bars', 'temporal') + end + end + end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 5dd06ada..624b7e70 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -14,6 +14,7 @@ require_relative 'support/matchers/column' require_relative 'support/matchers/function' require_relative 'support/matchers/index' +require_relative 'support/matchers/foreign_key_constraint' require_relative 'support/matchers/schema' require_relative 'support/matchers/source' require_relative 'support/matchers/table' @@ -40,6 +41,7 @@ config.include(ChronoTest::Matchers::Column) config.include(ChronoTest::Matchers::Function) config.include(ChronoTest::Matchers::Index) + config.include(ChronoTest::Matchers::ForeignKeyConstraint) config.include(ChronoTest::Matchers::Schema) config.include(ChronoTest::Matchers::Source) config.include(ChronoTest::Matchers::Table) diff --git a/spec/support/matchers/foreign_key_constraint.rb b/spec/support/matchers/foreign_key_constraint.rb new file mode 100644 index 00000000..44539679 --- /dev/null +++ b/spec/support/matchers/foreign_key_constraint.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require_relative 'base' + +module ChronoTest + module Matchers + module ForeignKeyConstraint + class HaveForeignKeyConstraint < ChronoTest::Matchers::Base + attr_reader :ref_table, :ref_schema, :schema + + def initialize(ref_table, ref_schema, schema = 'public') + @ref_table = ref_table + @ref_schema = ref_schema + @schema = schema + end + + def description + 'have foreign key constraint' + end + + def matches?(table) + super + + select_values(<<~SQL.squish, [table, schema, ref_table, ref_schema], 'Check foreign key constraint').any? + SELECT 1 + FROM pg_constraint c + JOIN pg_class t ON c.conrelid = t.oid + JOIN pg_class r ON c.confrelid = r.oid + JOIN pg_namespace tn ON t.relnamespace = tn.oid + JOIN pg_namespace rn ON r.relnamespace = rn.oid + WHERE t.relname = ? + AND tn.nspname = ? + AND r.relname = ? + AND rn.nspname = ? + AND c.contype = 'f'; + SQL + end + + def failure_message + "expected #{schema}.#{table} to have a foreign key constraint on #{ref_schema}#{ref_table}" + end + + def failure_message_when_negated + "expected #{schema}.#{table} to not have a foreign key constraint on #{ref_schema}.#{ref_table}" + end + end + + def have_foreign_key_constraint(*args) + HaveForeignKeyConstraint.new(*args) + end + + class HaveTemporalForeignKeyConstraint < HaveForeignKeyConstraint + def initialize(ref_table, ref_schema) + super(ref_table, ref_schema, temporal_schema) + end + end + + def have_temporal_foreign_key_constraint(*args) + HaveTemporalForeignKeyConstraint.new(*args) + end + end + end +end From a004ec3557c88a2befe425b04af1a9b6addb0e35 Mon Sep 17 00:00:00 2001 From: Martin-Alexander Date: Fri, 18 Jul 2025 01:07:03 -0400 Subject: [PATCH 2/6] add support for foreign keys in create_table and implement better method of tracking original schema --- lib/chrono_model/adapter.rb | 26 +++- lib/chrono_model/adapter/migrations.rb | 17 +++ .../adapter/migrations_modules/stable.rb | 30 ++-- spec/chrono_model/adapter/base_spec.rb | 22 +++ spec/chrono_model/adapter/migrations_spec.rb | 128 ++++++++++++++++++ .../matchers/foreign_key_constraint.rb | 2 +- 6 files changed, 206 insertions(+), 19 deletions(-) diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index afdfee69..9e670c9a 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'active_record/connection_adapters/postgresql_adapter' +require 'active_record/connection_adapters/postgresql/utils.rb' require_relative 'adapter/migrations' require_relative 'adapter/migrations_modules/stable' @@ -112,13 +113,18 @@ def on_history_schema(&block) # Evaluates the given block in the given +schema+ search path. # - # Recursion works by saving the old_path the function closure + # Recursion works by saving the old_path in the method closure # at each recursive call. # + # @original_schema is set to the current schema on the first call, + # and restored to +nil+ when the recursion ends. Use outer_schema() + # to get the original schema. + # # See specs for examples and behaviour. # def on_schema(schema, recurse: :follow) old_path = schema_search_path + @original_schema ||= current_schema count_recursions do if (recurse == :follow) || (Thread.current['recursions'] == 1) @@ -144,6 +150,24 @@ def on_schema(schema, recurse: :follow) else self.schema_search_path = old_path end + + @original_schema = nil if Thread.current['recursions'] == 0 + end + + # The current schema or the schema that was the current schema before + # on_schema() was called. + # + def outer_schema + @original_schema || current_schema + end + + # Implementation copied from a private method in + # ActiveRecord::ConnectionAdapters::PostgreSQL::SchemaStatements of + # the same name + # + def extract_schema_and_table(string) + name = ActiveRecord::ConnectionAdapters::PostgreSQL::Utils.extract_schema_qualified_name(string.to_s) + [name.schema, name.identifier] end # Returns true if the given name references a temporal table. diff --git a/lib/chrono_model/adapter/migrations.rb b/lib/chrono_model/adapter/migrations.rb index 7145aa39..219c331e 100644 --- a/lib/chrono_model/adapter/migrations.rb +++ b/lib/chrono_model/adapter/migrations.rb @@ -171,6 +171,23 @@ def remove_column(table_name, column_name, type = nil, **options) drop_and_recreate_public_view(table_name) { super } end + def build_create_table_definition(table_name, id: :primary_key, primary_key: nil, force: nil, **options) + table_definition = super + + table_definition.foreign_keys.map! do |fk| + to_table_schema, _ = extract_schema_and_table(fk.to_table) + + if to_table_schema.nil? && is_chrono?(fk.to_table) != !!options[:temporal] + schema = is_chrono?(fk.to_table) ? TEMPORAL_SCHEMA : outer_schema + fk.to_table = "#{schema}.#{fk.to_table}" + end + + fk + end + + table_definition + end + private # In destructive changes, such as removing columns or changing column diff --git a/lib/chrono_model/adapter/migrations_modules/stable.rb b/lib/chrono_model/adapter/migrations_modules/stable.rb index cb90492c..d625f6a0 100644 --- a/lib/chrono_model/adapter/migrations_modules/stable.rb +++ b/lib/chrono_model/adapter/migrations_modules/stable.rb @@ -35,33 +35,29 @@ def remove_index(table_name, column_name = nil, **options) end def add_foreign_key(from_table, to_table, **options) - if is_chrono?(from_table) != is_chrono?(to_table) - return on_schema("#{TEMPORAL_SCHEMA},#{schema_search_path}") { super } - end + to_table_schema, _ = extract_schema_and_table(to_table) - if is_chrono?(from_table) - return on_temporal_schema { super } + if to_table_schema.nil? && is_chrono?(to_table) != is_chrono?(from_table) + schema = is_chrono?(to_table) ? TEMPORAL_SCHEMA : outer_schema + to_table = "#{schema}.#{to_table}" end - super + return super unless is_chrono?(from_table) + + on_temporal_schema { super } end def remove_foreign_key(from_table, to_table = nil, **options) - if to_table.nil? - return super unless is_chrono?(from_table) - - on_temporal_schema { super } - end + to_table_schema, _ = extract_schema_and_table(to_table) - if is_chrono?(from_table) != is_chrono?(to_table) - return on_schema("#{TEMPORAL_SCHEMA},#{schema_search_path}") { super } + if to_table_schema.nil? && is_chrono?(to_table) != is_chrono?(from_table) + schema = is_chrono?(to_table) ? TEMPORAL_SCHEMA : outer_schema + to_table = "#{schema}.#{to_table}" end - if is_chrono?(from_table) - return on_temporal_schema { super } - end + return super unless is_chrono?(from_table) - super + on_temporal_schema { super } end end end diff --git a/spec/chrono_model/adapter/base_spec.rb b/spec/chrono_model/adapter/base_spec.rb index 7a9b6409..0b99d582 100644 --- a/spec/chrono_model/adapter/base_spec.rb +++ b/spec/chrono_model/adapter/base_spec.rb @@ -108,6 +108,28 @@ end end + describe '.outer_schema' do + subject(:outer_schema) { adapter.outer_schema } + + context 'when outside of on_schema()' do + it { is_expected.to eq(adapter.current_schema) } + end + + context 'when inside on_schema()' do + around do |example| + @original_schema = adapter.current_schema + + adapter.on_schema('test_schema') do + example.run + end + end + + it 'returns the original schema' do + expect(outer_schema).to eq(@original_schema) + end + end + end + describe '.is_chrono?' do subject(:is_chrono?) { adapter.is_chrono?(table) } diff --git a/spec/chrono_model/adapter/migrations_spec.rb b/spec/chrono_model/adapter/migrations_spec.rb index d968a795..c482f254 100644 --- a/spec/chrono_model/adapter/migrations_spec.rb +++ b/spec/chrono_model/adapter/migrations_spec.rb @@ -43,12 +43,84 @@ include_context 'with temporal tables' it_behaves_like 'temporal table' + + context 'using t.references to temporal tables' do + let(:columns) do + native = [['bar_id', 'bigint']] + + def native.to_proc + proc { |t| t.references :bar, foreign_key: true } + end + + native + end + + it_behaves_like 'temporal table' + + it 'creates a foreign key constraint to the temporal schema' do + expect(table).to have_temporal_foreign_key_constraint('bars', 'temporal') + end + end + + context 'using t.references to plain tables' do + let(:columns) do + native = [['baz_id', 'bigint']] + + def native.to_proc + proc { |t| t.references :baz, foreign_key: true } + end + + native + end + + it_behaves_like 'temporal table' + + it 'creates a foreign key constraint to the public schema' do + expect(table).to have_temporal_foreign_key_constraint('bazs', 'public') + end + end end context 'with plain tables' do include_context 'with plain tables' it_behaves_like 'plain table' + + context 'using t.references to temporal tables' do + let(:columns) do + native = [['bar_id', 'bigint']] + + def native.to_proc + proc { |t| t.references :bar, foreign_key: true } + end + + native + end + + it_behaves_like 'plain table' + + it 'creates a foreign key constraint to the temporal schema' do + expect(table).to have_foreign_key_constraint('bars', 'temporal') + end + end + + context 'using t.references to plain tables' do + let(:columns) do + native = [['baz_id', 'bigint']] + + def native.to_proc + proc { |t| t.references :baz, foreign_key: true } + end + + native + end + + it_behaves_like 'plain table' + + it 'creates a foreign key constraint to the public schema' do + expect(table).to have_foreign_key_constraint('bazs', 'public') + end + end end end @@ -116,6 +188,34 @@ it { is_expected.to have_temporal_columns([%w[new_column integer]]) } it { is_expected.to have_history_columns([%w[new_column integer]]) } end + + context 'using t.references to temporal tables' do + before do + adapter.change_table table do |t| + t.references :bar, foreign_key: true + end + end + + it_behaves_like 'temporal table' + + it 'creates a foreign key constraint to the temporal schema' do + expect(table).to have_temporal_foreign_key_constraint('bars', 'temporal') + end + end + + context 'using t.references to plain tables' do + before do + adapter.change_table table do |t| + t.references :baz, foreign_key: true + end + end + + it_behaves_like 'temporal table' + + it 'creates a foreign key constraint to the public schema' do + expect(table).to have_temporal_foreign_key_constraint('bazs', 'public') + end + end end context 'with plain tables' do @@ -176,6 +276,34 @@ expect(indexes - history_indexes.map(&:name).sort).to be_empty end end + + context 'when using t.references to temporal tables' do + before do + adapter.change_table table do |t| + t.references :bar, foreign_key: true + end + end + + it_behaves_like 'plain table' + + it 'creates a foreign key constraint to the temporal schema' do + expect(table).to have_foreign_key_constraint('bars', 'temporal') + end + end + + context 'when using t.references to plain tables' do + before do + adapter.change_table table do |t| + t.references :baz, foreign_key: true + end + end + + it_behaves_like 'plain table' + + it 'creates a foreign key constraint to the public schema' do + expect(table).to have_foreign_key_constraint('bazs', 'public') + end + end end # https://github.com/ifad/chronomodel/issues/91 diff --git a/spec/support/matchers/foreign_key_constraint.rb b/spec/support/matchers/foreign_key_constraint.rb index 44539679..43f53e3f 100644 --- a/spec/support/matchers/foreign_key_constraint.rb +++ b/spec/support/matchers/foreign_key_constraint.rb @@ -37,7 +37,7 @@ def matches?(table) end def failure_message - "expected #{schema}.#{table} to have a foreign key constraint on #{ref_schema}#{ref_table}" + "expected #{schema}.#{table} to have a foreign key constraint on #{ref_schema}.#{ref_table}" end def failure_message_when_negated From d43f05a0ff15830c356689ed6fbb8619cdef45ea Mon Sep 17 00:00:00 2001 From: Martin-Alexander Date: Fri, 18 Jul 2025 01:48:52 -0400 Subject: [PATCH 3/6] addressed some rubocop offenses --- lib/chrono_model/adapter.rb | 4 +- lib/chrono_model/adapter/migrations.rb | 2 +- .../adapter/migrations_modules/stable.rb | 4 +- spec/chrono_model/adapter/base_spec.rb | 10 +- spec/chrono_model/adapter/migrations_spec.rb | 125 +++++++----------- .../matchers/foreign_key_constraint.rb | 2 +- 6 files changed, 57 insertions(+), 90 deletions(-) diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index 9e670c9a..82dbc44d 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'active_record/connection_adapters/postgresql_adapter' -require 'active_record/connection_adapters/postgresql/utils.rb' +require 'active_record/connection_adapters/postgresql/utils' require_relative 'adapter/migrations' require_relative 'adapter/migrations_modules/stable' @@ -151,7 +151,7 @@ def on_schema(schema, recurse: :follow) self.schema_search_path = old_path end - @original_schema = nil if Thread.current['recursions'] == 0 + @original_schema = nil if Thread.current['recursions'].zero? end # The current schema or the schema that was the current schema before diff --git a/lib/chrono_model/adapter/migrations.rb b/lib/chrono_model/adapter/migrations.rb index 219c331e..3a76242d 100644 --- a/lib/chrono_model/adapter/migrations.rb +++ b/lib/chrono_model/adapter/migrations.rb @@ -175,7 +175,7 @@ def build_create_table_definition(table_name, id: :primary_key, primary_key: nil table_definition = super table_definition.foreign_keys.map! do |fk| - to_table_schema, _ = extract_schema_and_table(fk.to_table) + to_table_schema, = extract_schema_and_table(fk.to_table) if to_table_schema.nil? && is_chrono?(fk.to_table) != !!options[:temporal] schema = is_chrono?(fk.to_table) ? TEMPORAL_SCHEMA : outer_schema diff --git a/lib/chrono_model/adapter/migrations_modules/stable.rb b/lib/chrono_model/adapter/migrations_modules/stable.rb index d625f6a0..7373f019 100644 --- a/lib/chrono_model/adapter/migrations_modules/stable.rb +++ b/lib/chrono_model/adapter/migrations_modules/stable.rb @@ -35,7 +35,7 @@ def remove_index(table_name, column_name = nil, **options) end def add_foreign_key(from_table, to_table, **options) - to_table_schema, _ = extract_schema_and_table(to_table) + to_table_schema, = extract_schema_and_table(to_table) if to_table_schema.nil? && is_chrono?(to_table) != is_chrono?(from_table) schema = is_chrono?(to_table) ? TEMPORAL_SCHEMA : outer_schema @@ -48,7 +48,7 @@ def add_foreign_key(from_table, to_table, **options) end def remove_foreign_key(from_table, to_table = nil, **options) - to_table_schema, _ = extract_schema_and_table(to_table) + to_table_schema, = extract_schema_and_table(to_table) if to_table_schema.nil? && is_chrono?(to_table) != is_chrono?(from_table) schema = is_chrono?(to_table) ? TEMPORAL_SCHEMA : outer_schema diff --git a/spec/chrono_model/adapter/base_spec.rb b/spec/chrono_model/adapter/base_spec.rb index 0b99d582..7296677d 100644 --- a/spec/chrono_model/adapter/base_spec.rb +++ b/spec/chrono_model/adapter/base_spec.rb @@ -116,17 +116,13 @@ end context 'when inside on_schema()' do - around do |example| - @original_schema = adapter.current_schema + it 'returns the original schema' do + original_schema = adapter.current_schema adapter.on_schema('test_schema') do - example.run + expect(outer_schema).to eq(original_schema) end end - - it 'returns the original schema' do - expect(outer_schema).to eq(@original_schema) - end end end diff --git a/spec/chrono_model/adapter/migrations_spec.rb b/spec/chrono_model/adapter/migrations_spec.rb index c482f254..3189be2a 100644 --- a/spec/chrono_model/adapter/migrations_spec.rb +++ b/spec/chrono_model/adapter/migrations_spec.rb @@ -44,9 +44,9 @@ it_behaves_like 'temporal table' - context 'using t.references to temporal tables' do + context 'when adding references to temporal tables' do let(:columns) do - native = [['bar_id', 'bigint']] + native = [%w[bar_id bigint]] def native.to_proc proc { |t| t.references :bar, foreign_key: true } @@ -62,9 +62,9 @@ def native.to_proc end end - context 'using t.references to plain tables' do + context 'when adding references to plain tables' do let(:columns) do - native = [['baz_id', 'bigint']] + native = [%w[baz_id bigint]] def native.to_proc proc { |t| t.references :baz, foreign_key: true } @@ -86,9 +86,9 @@ def native.to_proc it_behaves_like 'plain table' - context 'using t.references to temporal tables' do + context 'when adding references to temporal tables' do let(:columns) do - native = [['bar_id', 'bigint']] + native = [%w[bar_id bigint]] def native.to_proc proc { |t| t.references :bar, foreign_key: true } @@ -104,9 +104,9 @@ def native.to_proc end end - context 'using t.references to plain tables' do + context 'when adding references to plain tables' do let(:columns) do - native = [['baz_id', 'bigint']] + native = [%w[baz_id bigint]] def native.to_proc proc { |t| t.references :baz, foreign_key: true } @@ -189,7 +189,7 @@ def native.to_proc it { is_expected.to have_history_columns([%w[new_column integer]]) } end - context 'using t.references to temporal tables' do + context 'when adding references to temporal tables' do before do adapter.change_table table do |t| t.references :bar, foreign_key: true @@ -203,7 +203,7 @@ def native.to_proc end end - context 'using t.references to plain tables' do + context 'when adding references to plain tables' do before do adapter.change_table table do |t| t.references :baz, foreign_key: true @@ -277,7 +277,7 @@ def native.to_proc end end - context 'when using t.references to temporal tables' do + context 'when when adding references to temporal tables' do before do adapter.change_table table do |t| t.references :bar, foreign_key: true @@ -291,7 +291,7 @@ def native.to_proc end end - context 'when using t.references to plain tables' do + context 'when when adding references to plain tables' do before do adapter.change_table table do |t| t.references :baz, foreign_key: true @@ -623,29 +623,18 @@ def native.to_proc end describe '.add_foreign_key' do - let(:columns) do - native = [%w[baz_id bigint], %w[bar_id bigint]] - - def native.to_proc - proc { |t| - t.references :baz - t.references :bar - } - end - - native - end - context 'with temporal tables' do include_context 'with temporal tables' - it 'adds constraints on foreign keys for plain tables' do + it 'adds foreign key constraints on plain tables' do + adapter.add_reference table, :baz, foreign_key: false adapter.add_foreign_key table, :bazs expect(table).to have_temporal_foreign_key_constraint('bazs', 'public') end - it 'adds constraints on foreign keys for other temporal tables' do + it 'adds foreign key constraints on other temporal tables' do + adapter.add_reference table, :bar, foreign_key: false adapter.add_foreign_key table, :bars expect(table).to have_temporal_foreign_key_constraint('bars', 'temporal') @@ -655,13 +644,15 @@ def native.to_proc context 'with plain tables' do include_context 'with plain tables' - it 'adds constraints on foreign keys for other plain tables' do + it 'adds foreign key constraints on other plain tables' do + adapter.add_reference table, :baz, foreign_key: false adapter.add_foreign_key table, :bazs expect(table).to have_foreign_key_constraint('bazs', 'public') end - it 'adds constraints on foreign keys for temporal tables' do + it 'adds foreign key constraints on temporal tables' do + adapter.add_reference table, :bar, foreign_key: false adapter.add_foreign_key table, :bars expect(table).to have_foreign_key_constraint('bars', 'temporal') @@ -673,18 +664,15 @@ def native.to_proc context 'with temporal tables' do include_context 'with temporal tables' - before do + it 'removes foreign key constraints on plain tables' do adapter.add_reference table, :baz, foreign_key: true - adapter.add_reference table, :bar, foreign_key: true - end - - it 'removes constraints on plain tables' do adapter.remove_foreign_key table, :bazs expect(table).not_to have_temporal_foreign_key_constraint('bazs', 'public') end - it 'removes constraints on temporal tables' do + it 'removes foreign key constraints on other temporal tables' do + adapter.add_reference table, :bar, foreign_key: true adapter.remove_foreign_key table, :bars expect(table).not_to have_temporal_foreign_key_constraint('bars', 'temporal') @@ -694,18 +682,15 @@ def native.to_proc context 'with plain tables' do include_context 'with plain tables' - before do + it 'removes foreign key constraint on other plain tables' do adapter.add_reference table, :baz, foreign_key: true - adapter.add_reference table, :bar, foreign_key: true - end - - it 'removes constraints on plain tables' do adapter.remove_foreign_key table, :bazs expect(table).not_to have_foreign_key_constraint('bazs', 'public') end - it 'removes constraints on temporal tables' do + it 'removes foreign key constraints on temporal tables' do + adapter.add_reference table, :bar, foreign_key: true adapter.remove_foreign_key table, :bars expect(table).not_to have_foreign_key_constraint('bars', 'temporal') @@ -717,44 +702,36 @@ def native.to_proc context 'with temporal tables' do include_context 'with temporal tables' - context 'to plain tables' do - it 'adds a foreign key and a constraint' do - adapter.add_reference table, :baz, foreign_key: true + it 'adds a foreign key column and constraint to plain tables' do + adapter.add_reference table, :baz, foreign_key: true - expect(table).to have_columns([%w[baz_id bigint]]) - expect(table).to have_temporal_foreign_key_constraint('bazs', 'public') - end + expect(table).to have_columns([%w[baz_id bigint]]) + expect(table).to have_temporal_foreign_key_constraint('bazs', 'public') end - context 'to other temporal tables' do - it 'adds a foreign key and a constraint' do - adapter.add_reference table, :bar, foreign_key: true + it 'adds a foreign key column and constraint to other temporal tables' do + adapter.add_reference table, :bar, foreign_key: true - expect(table).to have_columns([%w[bar_id bigint]]) - expect(table).to have_temporal_foreign_key_constraint('bars', 'temporal') - end + expect(table).to have_columns([%w[bar_id bigint]]) + expect(table).to have_temporal_foreign_key_constraint('bars', 'temporal') end end context 'with plain tables' do include_context 'with plain tables' - context 'to other plain tables' do - it 'adds a foreign key and a constraint' do - adapter.add_reference table, :baz, foreign_key: true + it 'adds a foreign key column and constraint to other plain tables' do + adapter.add_reference table, :baz, foreign_key: true - expect(table).to have_columns([%w[baz_id bigint]]) - expect(table).to have_foreign_key_constraint('bazs', 'public') - end + expect(table).to have_columns([%w[baz_id bigint]]) + expect(table).to have_foreign_key_constraint('bazs', 'public') end - context 'to temporal tables' do - it 'adds a foreign key and a constraint' do - adapter.add_reference table, :bar, foreign_key: true + it 'adds a foreign key column and constraint to temporal tables' do + adapter.add_reference table, :bar, foreign_key: true - expect(table).to have_columns([%w[bar_id bigint]]) - expect(table).to have_foreign_key_constraint('bars', 'temporal') - end + expect(table).to have_columns([%w[bar_id bigint]]) + expect(table).to have_foreign_key_constraint('bars', 'temporal') end end end @@ -763,19 +740,16 @@ def native.to_proc context 'with temporal tables' do include_context 'with temporal tables' - before do + it 'removes the foreign key column and constraint from plain tables' do adapter.add_reference table, :baz, foreign_key: true - adapter.add_reference table, :bar, foreign_key: true - end - - it 'removes the foreign key and constraint from plain tables' do adapter.remove_reference table, :baz expect(table).not_to have_columns([%w[baz_id bigint]]) expect(table).not_to have_temporal_foreign_key_constraint('bazs', 'public') end - it 'removes the foreign key and constraint from other temporal tables' do + it 'removes the foreign key column and constraint from other temporal tables' do + adapter.add_reference table, :bar, foreign_key: true adapter.remove_reference table, :bar expect(table).not_to have_columns([%w[bar_id bigint]]) @@ -786,19 +760,16 @@ def native.to_proc context 'with plain tables' do include_context 'with plain tables' - before do + it 'removes the foreign key column and constraint from other plain tables' do adapter.add_reference table, :baz, foreign_key: true - adapter.add_reference table, :bar, foreign_key: true - end - - it 'removes the foreign key and constraint from other plain tables' do adapter.remove_reference table, :baz expect(table).not_to have_columns([%w[baz_id bigint]]) expect(table).not_to have_foreign_key_constraint('bazs', 'public') end - it 'removes the foreign key and constraint from temporal tables' do + it 'removes the foreign key column and constraint from temporal tables' do + adapter.add_reference table, :bar, foreign_key: true adapter.remove_reference table, :bar expect(table).not_to have_columns([%w[bar_id bigint]]) diff --git a/spec/support/matchers/foreign_key_constraint.rb b/spec/support/matchers/foreign_key_constraint.rb index 43f53e3f..93b2a329 100644 --- a/spec/support/matchers/foreign_key_constraint.rb +++ b/spec/support/matchers/foreign_key_constraint.rb @@ -11,7 +11,7 @@ class HaveForeignKeyConstraint < ChronoTest::Matchers::Base def initialize(ref_table, ref_schema, schema = 'public') @ref_table = ref_table @ref_schema = ref_schema - @schema = schema + @schema = schema end def description From c35e9dae0577f4259c56277feec2d4908dcf7298 Mon Sep 17 00:00:00 2001 From: Martin-Alexander Date: Sat, 19 Jul 2025 20:25:38 -0400 Subject: [PATCH 4/6] add support older versions of AR and slight refactor of schema modificationlogic --- lib/chrono_model/adapter.rb | 33 +++++++++++++++---- lib/chrono_model/adapter/migrations.rb | 17 ---------- .../adapter/migrations_modules/stable.rb | 14 +++----- 3 files changed, 30 insertions(+), 34 deletions(-) diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index 82dbc44d..e4bf42f0 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -42,6 +42,20 @@ def initialize(*) end end + class TableDefinition < ActiveRecord::ConnectionAdapters::PostgreSQL::TableDefinition + def foreign_key(to_table, **options) + if !@conn.is_schema_qualified?(to_table) && (@conn.current_schema == TEMPORAL_SCHEMA) != @conn.is_chrono?(to_table) + to_table = @conn.qualified_table_name(to_table) + end + + super + end + end + + def create_table_definition(name, **options) + ChronoModel::Adapter::TableDefinition.new(self, name, **options) + end + # Returns true whether the connection adapter supports our # implementation of temporal tables. Currently, Chronomodel # is supported starting with PostgreSQL 9.3 (90300 in PostgreSQL's @@ -161,13 +175,18 @@ def outer_schema @original_schema || current_schema end - # Implementation copied from a private method in - # ActiveRecord::ConnectionAdapters::PostgreSQL::SchemaStatements of - # the same name - # - def extract_schema_and_table(string) - name = ActiveRecord::ConnectionAdapters::PostgreSQL::Utils.extract_schema_qualified_name(string.to_s) - [name.schema, name.identifier] + def foreign_key_column_for(table_name, *args) # :nodoc: + _schema, table_name = extract_schema_qualified_name(table_name) + super(table_name, *args) + end + + def qualified_table_name(table_name) + schema = is_chrono?(table_name) ? TEMPORAL_SCHEMA : outer_schema + "#{schema}.#{table_name}" + end + + def is_schema_qualified?(name) + name.to_s.include?('.') end # Returns true if the given name references a temporal table. diff --git a/lib/chrono_model/adapter/migrations.rb b/lib/chrono_model/adapter/migrations.rb index 3a76242d..7145aa39 100644 --- a/lib/chrono_model/adapter/migrations.rb +++ b/lib/chrono_model/adapter/migrations.rb @@ -171,23 +171,6 @@ def remove_column(table_name, column_name, type = nil, **options) drop_and_recreate_public_view(table_name) { super } end - def build_create_table_definition(table_name, id: :primary_key, primary_key: nil, force: nil, **options) - table_definition = super - - table_definition.foreign_keys.map! do |fk| - to_table_schema, = extract_schema_and_table(fk.to_table) - - if to_table_schema.nil? && is_chrono?(fk.to_table) != !!options[:temporal] - schema = is_chrono?(fk.to_table) ? TEMPORAL_SCHEMA : outer_schema - fk.to_table = "#{schema}.#{fk.to_table}" - end - - fk - end - - table_definition - end - private # In destructive changes, such as removing columns or changing column diff --git a/lib/chrono_model/adapter/migrations_modules/stable.rb b/lib/chrono_model/adapter/migrations_modules/stable.rb index 7373f019..7d423036 100644 --- a/lib/chrono_model/adapter/migrations_modules/stable.rb +++ b/lib/chrono_model/adapter/migrations_modules/stable.rb @@ -35,11 +35,8 @@ def remove_index(table_name, column_name = nil, **options) end def add_foreign_key(from_table, to_table, **options) - to_table_schema, = extract_schema_and_table(to_table) - - if to_table_schema.nil? && is_chrono?(to_table) != is_chrono?(from_table) - schema = is_chrono?(to_table) ? TEMPORAL_SCHEMA : outer_schema - to_table = "#{schema}.#{to_table}" + if !is_schema_qualified?(to_table) && is_chrono?(to_table) != is_chrono?(from_table) + to_table = qualified_table_name(to_table) end return super unless is_chrono?(from_table) @@ -48,11 +45,8 @@ def add_foreign_key(from_table, to_table, **options) end def remove_foreign_key(from_table, to_table = nil, **options) - to_table_schema, = extract_schema_and_table(to_table) - - if to_table_schema.nil? && is_chrono?(to_table) != is_chrono?(from_table) - schema = is_chrono?(to_table) ? TEMPORAL_SCHEMA : outer_schema - to_table = "#{schema}.#{to_table}" + if !is_schema_qualified?(to_table) && is_chrono?(to_table) != is_chrono?(from_table) + to_table = qualified_table_name(to_table) end return super unless is_chrono?(from_table) From b173143b5d63b0773af036d5d95d7b672a900d28 Mon Sep 17 00:00:00 2001 From: Martin-Alexander Date: Sat, 19 Jul 2025 20:29:56 -0400 Subject: [PATCH 5/6] remove unneccessary require --- lib/chrono_model/adapter.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index e4bf42f0..37289321 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require 'active_record/connection_adapters/postgresql_adapter' -require 'active_record/connection_adapters/postgresql/utils' require_relative 'adapter/migrations' require_relative 'adapter/migrations_modules/stable' From 89cbaeb4f32968a1243fe3a385c207eaaf6d6e23 Mon Sep 17 00:00:00 2001 From: Martin-Alexander Date: Sat, 19 Jul 2025 20:30:16 -0400 Subject: [PATCH 6/6] fix style for super --- lib/chrono_model/adapter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index 37289321..173cd5db 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -176,7 +176,7 @@ def outer_schema def foreign_key_column_for(table_name, *args) # :nodoc: _schema, table_name = extract_schema_qualified_name(table_name) - super(table_name, *args) + super end def qualified_table_name(table_name)