From a3182838c1bd3bc5f77c0434574281c01887ca49 Mon Sep 17 00:00:00 2001 From: Phil Date: Thu, 8 Sep 2022 16:42:27 -0700 Subject: [PATCH 1/7] Improvements to how `on_schema` works so it can retain the original state in the case of a user manually managing "search_path" --- .../chronomodel_adapter.rb | 3 +- lib/chrono_model/adapter.rb | 56 ++++---- spec/chrono_model/adapter/base_spec.rb | 127 +++++++++++++----- 3 files changed, 120 insertions(+), 66 deletions(-) diff --git a/lib/active_record/connection_adapters/chronomodel_adapter.rb b/lib/active_record/connection_adapters/chronomodel_adapter.rb index 265d88f3..f9c85116 100644 --- a/lib/active_record/connection_adapters/chronomodel_adapter.rb +++ b/lib/active_record/connection_adapters/chronomodel_adapter.rb @@ -34,7 +34,7 @@ def chronomodel_connection(config) # :nodoc: return adapter rescue ::PG::Error => error - if error.message.include?(conn_params[:dbname]) + if error.message.include?(conn_params[:dbname].to_s) raise ActiveRecord::NoDatabaseError else raise @@ -43,4 +43,3 @@ def chronomodel_connection(config) # :nodoc: end end - diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index 9c840539..9691bc20 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -107,33 +107,32 @@ def on_history_schema(&block) # # See specs for examples and behaviour. # - def on_schema(schema, recurse: :follow) - old_path = self.schema_search_path - count_recursions do - if recurse == :follow or Thread.current['recursions'] == 1 - self.schema_search_path = schema - end + # def schema_search_path=(schema) + # puts "Setting schema_search_path to #{schema}" + # super + # end + + def on_schema(schema, recurse: :follow) + schema_stack_ensure_default - yield + # Keep the value outside of the closure so we can track when + # we need to pop the value for recurse: :ignore + pre_push_stack_length = schema_stack.length + if recurse == :follow || pre_push_stack_length == 1 + schema_stack.push(self.schema_search_path = schema) end + yield ensure - # If the transaction is aborted, any execute() call will raise - # "transaction is aborted errors" - thus calling the Adapter's - # setter won't update the memoized variable. - # - # Here we reset it to +nil+ to refresh it on the next call, as - # there is no way to know which path will be restored when the - # transaction ends. - # - transaction_aborted = - chrono_connection.transaction_status == PG::Connection::PQTRANS_INERROR - - if transaction_aborted && Thread.current['recursions'] == 1 + if chrono_connection.transaction_status == PG::Connection::PQTRANS_INERROR && pre_push_stack_length > 2 @schema_search_path = nil - else - self.schema_search_path = old_path + schema_stack.clear + end + + if (schema_stack.length > 1 && recurse == :follow) || pre_push_stack_length == 1 + schema_stack.pop + self.schema_search_path = schema_stack.last end end @@ -170,16 +169,15 @@ def chrono_connection @chrono_connection ||= @raw_connection || @connection end - # Counts the number of recursions in a thread local variable - # - def count_recursions # yield - Thread.current['recursions'] ||= 0 - Thread.current['recursions'] += 1 + def schema_stack + (Thread.current["chronomodel-schema-stack"] ||= []) + end - yield + def schema_stack_ensure_default + # If we have no default or only the default from a previous `on_schema` call we + # need to set the default again - ensure - Thread.current['recursions'] -= 1 + schema_stack[0] = select_value("SHOW search_path") if schema_stack.length <= 1 end # Create the temporal and history schemas, unless they already exist diff --git a/spec/chrono_model/adapter/base_spec.rb b/spec/chrono_model/adapter/base_spec.rb index f0eaaf85..f3693c23 100644 --- a/spec/chrono_model/adapter/base_spec.rb +++ b/spec/chrono_model/adapter/base_spec.rb @@ -66,6 +66,7 @@ describe '.on_schema' do before(:all) do adapter.execute 'BEGIN' + 5.times { |i| adapter.execute "CREATE SCHEMA test_#{i}" } end @@ -73,54 +74,110 @@ adapter.execute 'ROLLBACK' end - context 'by default' do - it 'saves the schema at each recursion' do - is_expected.to be_in_schema(:default) - - adapter.on_schema('test_1') { is_expected.to be_in_schema('test_1') - adapter.on_schema('test_2') { is_expected.to be_in_schema('test_2') - adapter.on_schema('test_3') { is_expected.to be_in_schema('test_3') - } - is_expected.to be_in_schema('test_2') - } - is_expected.to be_in_schema('test_1') + context "with an implicit search_path" do + it "saves the schema at each recursion" do + starting_schema = "implicit_schema_#{rand(1000)}" + adapter.execute "CREATE SCHEMA #{starting_schema}" + old_search_path = adapter.select_value("SHOW search_path") + adapter.execute "SET search_path TO '#{starting_schema}'" + + is_expected.to be_in_schema(starting_schema) + + adapter.on_schema('test_1') { + is_expected.to be_in_schema('test_1') + adapter.on_schema('test_2') { + is_expected.to be_in_schema('test_2') + adapter.on_schema('test_3') { is_expected.to be_in_schema('test_3') } + is_expected.to be_in_schema('test_2') + } + is_expected.to be_in_schema('test_1') + } + + is_expected.to be_in_schema(starting_schema) + + adapter.execute "SET search_path TO #{old_search_path}" + end + end + + context "with an explicit search_path" do + it "saves the schema at each recursion" do + starting_schema = "implicit_schema_#{rand(1000)}" + adapter.execute "CREATE SCHEMA #{starting_schema}" + old_search_path = adapter.select_value("SHOW search_path") + adapter.schema_search_path = starting_schema + + is_expected.to be_in_schema(starting_schema) + + adapter.on_schema('test_1') { + is_expected.to be_in_schema('test_1') + adapter.on_schema('test_2') { + is_expected.to be_in_schema('test_2') + adapter.on_schema('test_3') { is_expected.to be_in_schema('test_3') } + is_expected.to be_in_schema('test_2') + } + is_expected.to be_in_schema('test_1') } - is_expected.to be_in_schema(:default) + is_expected.to be_in_schema(starting_schema) + + adapter.schema_search_path = old_search_path end + end + + context "without an explicit starting schema_search_path" do + context 'by default' do + it 'saves the schema at each recursion' do + is_expected.to be_in_schema(:default) + + adapter.on_schema('test_1') { + is_expected.to be_in_schema('test_1') + adapter.on_schema('test_2') { + is_expected.to be_in_schema('test_2') + adapter.on_schema('test_3') { is_expected.to be_in_schema('test_3') } + is_expected.to be_in_schema('test_2') + } + is_expected.to be_in_schema('test_1') + } + + is_expected.to be_in_schema(:default) + end - context 'when errors occur' do - subject do - adapter.on_schema('test_1') do - adapter.on_schema('test_2') do - adapter.execute 'BEGIN' - adapter.execute 'ERRORING ON PURPOSE' + context 'when errors occur' do + subject do + adapter.on_schema('test_1') do + adapter.on_schema('test_2') do + adapter.execute 'BEGIN' + adapter.execute 'ERRORING ON PURPOSE' + end end end - end - it { - expect { subject }. - to raise_error(/current transaction is aborted/). - and change { adapter.instance_variable_get(:@schema_search_path) } - } + it { + expect { subject }. + to raise_error(/current transaction is aborted/). + and change { adapter.instance_variable_get(:@schema_search_path) } + } - after do - adapter.execute 'ROLLBACK' + after do + adapter.execute 'ROLLBACK' + end end end - end - context 'with recurse: :ignore' do - it 'ignores recursive calls' do - is_expected.to be_in_schema(:default) + context 'with recurse: :ignore' do + it 'ignores recursive calls' do + is_expected.to be_in_schema(:default) - adapter.on_schema('test_1', recurse: :ignore) { is_expected.to be_in_schema('test_1') - adapter.on_schema('test_2', recurse: :ignore) { is_expected.to be_in_schema('test_1') - adapter.on_schema('test_3', recurse: :ignore) { is_expected.to be_in_schema('test_1') - } } } + adapter.on_schema('test_1', recurse: :ignore) { + is_expected.to be_in_schema('test_1') + adapter.on_schema('test_2', recurse: :ignore) { + is_expected.to be_in_schema('test_1') + adapter.on_schema('test_3', recurse: :ignore) { + is_expected.to be_in_schema('test_1') + } } } - is_expected.to be_in_schema(:default) + is_expected.to be_in_schema(:default) + end end end end From e86022b73aa0a158cbd7dc3c588fa0229eb1e42f Mon Sep 17 00:00:00 2001 From: Phil Date: Tue, 3 Mar 2020 22:14:28 -0800 Subject: [PATCH 2/7] Tweak how optionals[:temporal] is handled --- lib/chrono_model/adapter/migrations.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/chrono_model/adapter/migrations.rb b/lib/chrono_model/adapter/migrations.rb index f8e8ec43..0777f3ef 100644 --- a/lib/chrono_model/adapter/migrations.rb +++ b/lib/chrono_model/adapter/migrations.rb @@ -79,11 +79,15 @@ def change_table(table_name, **options, &block) super table_name, **options, &block end - else - if is_chrono?(table_name) - chrono_undo_temporal_table(table_name) - end + elsif options[:temporal] == false && is_chrono?(table_name) + chrono_undo_temporal_table(table_name) + super table_name, **options, &block + elsif is_chrono?(table_name) + drop_and_recreate_public_view(table_name, options) do + super table_name, **options, &block + end + else super table_name, **options, &block end @@ -120,6 +124,7 @@ def add_column(table_name, column_name, type, **options) # the temporal schema and update the triggers. # def rename_column(table_name, *) + puts "Entering 'rename column'" return super unless is_chrono?(table_name) # Rename the column in the temporal table and in the view From 3c85965f1c6ae4e6f407862249e62290e83a65e3 Mon Sep 17 00:00:00 2001 From: Phil Date: Wed, 7 Sep 2022 10:20:38 -0700 Subject: [PATCH 3/7] WIP - Getting specs running for :temporal => true changes --- lib/chrono_model/adapter/ddl.rb | 42 ++++++++++---------- lib/chrono_model/adapter/migrations.rb | 1 - spec/chrono_model/adapter/migrations_spec.rb | 34 ++++++++++++++-- 3 files changed, 52 insertions(+), 25 deletions(-) diff --git a/lib/chrono_model/adapter/ddl.rb b/lib/chrono_model/adapter/ddl.rb index 5bd21f4c..d3adfc93 100644 --- a/lib/chrono_model/adapter/ddl.rb +++ b/lib/chrono_model/adapter/ddl.rb @@ -17,33 +17,35 @@ def chrono_public_view_ddl(table, options = nil) # SELECT - return only current data # - execute "DROP VIEW #{table}" if data_source_exists? table - execute "CREATE VIEW #{table} AS SELECT * FROM ONLY #{current}" + on_schema("DEFAULT") do + execute "DROP VIEW #{table}" if data_source_exists? table + execute "CREATE VIEW #{table} AS SELECT * FROM ONLY #{current}" - chrono_metadata_set(table, options.merge(chronomodel: VERSION)) + chrono_metadata_set(table, options.merge(chronomodel: VERSION)) - # Set default values on the view (closes #12) - # - columns(table).each do |column| - default = if column.default.nil? - column.default_function - else - quote(column.default) - end + # Set default values on the view (closes #12) + # + columns(table).each do |column| + default = if column.default.nil? + column.default_function + else + quote(column.default) + end - next if column.name == pk || default.nil? + next if column.name == pk || default.nil? - execute "ALTER VIEW #{table} ALTER COLUMN #{quote_column_name(column.name)} SET DEFAULT #{default}" - end + execute "ALTER VIEW #{table} ALTER COLUMN #{quote_column_name(column.name)} SET DEFAULT #{default}" + end - columns = self.columns(table).map {|c| quote_column_name(c.name)} - columns.delete(quote_column_name(pk)) + columns = self.columns(table).map {|c| quote_column_name(c.name)} + columns.delete(quote_column_name(pk)) - fields, values = columns.join(', '), columns.map {|c| "NEW.#{c}"}.join(', ') + fields, values = columns.join(', '), columns.map {|c| "NEW.#{c}"}.join(', ') - chrono_create_INSERT_trigger(table, pk, current, history, fields, values) - chrono_create_UPDATE_trigger(table, pk, current, history, fields, values, options, columns) - chrono_create_DELETE_trigger(table, pk, current, history) + chrono_create_INSERT_trigger(table, pk, current, history, fields, values) + chrono_create_UPDATE_trigger(table, pk, current, history, fields, values, options, columns) + chrono_create_DELETE_trigger(table, pk, current, history) + end end # Create the history table in the history schema diff --git a/lib/chrono_model/adapter/migrations.rb b/lib/chrono_model/adapter/migrations.rb index 0777f3ef..165c722f 100644 --- a/lib/chrono_model/adapter/migrations.rb +++ b/lib/chrono_model/adapter/migrations.rb @@ -124,7 +124,6 @@ def add_column(table_name, column_name, type, **options) # the temporal schema and update the triggers. # def rename_column(table_name, *) - puts "Entering 'rename column'" return super unless is_chrono?(table_name) # Rename the column in the temporal table and in the view diff --git a/spec/chrono_model/adapter/migrations_spec.rb b/spec/chrono_model/adapter/migrations_spec.rb index 7dd98a65..1e1b7f19 100644 --- a/spec/chrono_model/adapter/migrations_spec.rb +++ b/spec/chrono_model/adapter/migrations_spec.rb @@ -84,12 +84,38 @@ end describe '.change_table' do - with_temporal_table do - before :all do - adapter.change_table table, temporal: false + context "when explicitly requesting temporal: false" do + with_temporal_table do + before :all do + adapter.change_table table, temporal: false + end + + it_should_behave_like 'plain table' end + end - it_should_behave_like 'plain table' + context "when explicitly requesting temporal: true" do + with_temporal_table do + before :all do + adapter.change_table table, temporal: true do |t| + t.integer Random.uuid.to_sym + end + end + + it_should_behave_like "temporal table" + end + end + + context "when adding a column without specifying temporal: true" do + with_temporal_table do + before :all do + adapter.change_table table do |t| + t.string Random.uuid.to_sym + end + end + + it_should_behave_like "temporal table" + end end with_plain_table do From 326cfc87a5f655bf7789f56d19e47253e1641560 Mon Sep 17 00:00:00 2001 From: Phil Date: Tue, 3 Mar 2020 22:14:28 -0800 Subject: [PATCH 4/7] Tweak how optionals[:temporal] is handled --- lib/chrono_model/adapter/migrations.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/chrono_model/adapter/migrations.rb b/lib/chrono_model/adapter/migrations.rb index 165c722f..0777f3ef 100644 --- a/lib/chrono_model/adapter/migrations.rb +++ b/lib/chrono_model/adapter/migrations.rb @@ -124,6 +124,7 @@ def add_column(table_name, column_name, type, **options) # the temporal schema and update the triggers. # def rename_column(table_name, *) + puts "Entering 'rename column'" return super unless is_chrono?(table_name) # Rename the column in the temporal table and in the view From b52907383e478bd9ba3fb4d6196c8ec6cc1a1fee Mon Sep 17 00:00:00 2001 From: Phil Date: Thu, 8 Sep 2022 14:56:13 -0700 Subject: [PATCH 5/7] Fix merge issue --- lib/chrono_model/adapter/migrations.rb | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/chrono_model/adapter/migrations.rb b/lib/chrono_model/adapter/migrations.rb index 0777f3ef..40ce4ea2 100644 --- a/lib/chrono_model/adapter/migrations.rb +++ b/lib/chrono_model/adapter/migrations.rb @@ -82,12 +82,6 @@ def change_table(table_name, **options, &block) elsif options[:temporal] == false && is_chrono?(table_name) chrono_undo_temporal_table(table_name) - super table_name, **options, &block - elsif is_chrono?(table_name) - drop_and_recreate_public_view(table_name, options) do - super table_name, **options, &block - end - else super table_name, **options, &block end @@ -124,7 +118,6 @@ def add_column(table_name, column_name, type, **options) # the temporal schema and update the triggers. # def rename_column(table_name, *) - puts "Entering 'rename column'" return super unless is_chrono?(table_name) # Rename the column in the temporal table and in the view From 188c0a4f5a9e17f444188b87b271cf19e14cc357 Mon Sep 17 00:00:00 2001 From: Phil Date: Mon, 12 Sep 2022 10:22:39 -0700 Subject: [PATCH 6/7] Fix merge issue --- lib/chrono_model/adapter/migrations.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/chrono_model/adapter/migrations.rb b/lib/chrono_model/adapter/migrations.rb index 40ce4ea2..61bc2ee6 100644 --- a/lib/chrono_model/adapter/migrations.rb +++ b/lib/chrono_model/adapter/migrations.rb @@ -82,6 +82,8 @@ def change_table(table_name, **options, &block) elsif options[:temporal] == false && is_chrono?(table_name) chrono_undo_temporal_table(table_name) + super table_name, **options, &block + else super table_name, **options, &block end From 6a2aa61ad89eb96a3df7387253b4caa0a49b0b81 Mon Sep 17 00:00:00 2001 From: Phil Date: Mon, 12 Sep 2022 10:22:49 -0700 Subject: [PATCH 7/7] Add ability to switch to "default" schema --- lib/chrono_model/adapter.rb | 1 + lib/chrono_model/adapter/ddl.rb | 36 +++++++++++++++++++-------------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index 9691bc20..9f6643e8 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -115,6 +115,7 @@ def on_history_schema(&block) def on_schema(schema, recurse: :follow) schema_stack_ensure_default + schema = schema_stack.first if schema == :default # Keep the value outside of the closure so we can track when # we need to pop the value for recurse: :ignore diff --git a/lib/chrono_model/adapter/ddl.rb b/lib/chrono_model/adapter/ddl.rb index d3adfc93..186265c6 100644 --- a/lib/chrono_model/adapter/ddl.rb +++ b/lib/chrono_model/adapter/ddl.rb @@ -14,36 +14,42 @@ def chrono_public_view_ddl(table, options = nil) history = [HISTORY_SCHEMA, table].join('.') options ||= chrono_metadata_for(table) + columns = columns(table).reject { |c| c.name == pk } # SELECT - return only current data # - on_schema("DEFAULT") do + # We use :default here in the case of being already inside an `on_schema` call + on_schema(:default) do execute "DROP VIEW #{table}" if data_source_exists? table execute "CREATE VIEW #{table} AS SELECT * FROM ONLY #{current}" chrono_metadata_set(table, options.merge(chronomodel: VERSION)) - + end # Set default values on the view (closes #12) # - columns(table).each do |column| - default = if column.default.nil? - column.default_function - else - quote(column.default) - end + statements = columns.map do |column| + default = if column.default.nil? + column.default_function + else + quote(column.default) + end - next if column.name == pk || default.nil? + next if default.nil? - execute "ALTER VIEW #{table} ALTER COLUMN #{quote_column_name(column.name)} SET DEFAULT #{default}" - end + "ALTER VIEW #{table} ALTER COLUMN #{quote_column_name(column.name)} SET DEFAULT #{default}" + end.compact + + on_schema(:default) do + statements.each { |stmt| execute stmt } + end - columns = self.columns(table).map {|c| quote_column_name(c.name)} - columns.delete(quote_column_name(pk)) + quoted_columns = columns.map { |c| quote_column_name(c.name) } - fields, values = columns.join(', '), columns.map {|c| "NEW.#{c}"}.join(', ') + fields, values = quoted_columns.join(', '), quoted_columns.map {|c| "NEW.#{c}"}.join(', ') + on_schema(:default) do chrono_create_INSERT_trigger(table, pk, current, history, fields, values) - chrono_create_UPDATE_trigger(table, pk, current, history, fields, values, options, columns) + chrono_create_UPDATE_trigger(table, pk, current, history, fields, values, options, quoted_columns) chrono_create_DELETE_trigger(table, pk, current, history) end end