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..9f6643e8 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -107,33 +107,33 @@ 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 - yield + 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 + 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 +170,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/lib/chrono_model/adapter/ddl.rb b/lib/chrono_model/adapter/ddl.rb index 5bd21f4c..186265c6 100644 --- a/lib/chrono_model/adapter/ddl.rb +++ b/lib/chrono_model/adapter/ddl.rb @@ -14,36 +14,44 @@ 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 # - execute "DROP VIEW #{table}" if data_source_exists? table - execute "CREATE VIEW #{table} AS SELECT * FROM ONLY #{current}" + # 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)) - - # Set default values on the view (closes #12) - # - columns(table).each do |column| + chrono_metadata_set(table, options.merge(chronomodel: VERSION)) + end + # Set default values on the view (closes #12) + # + 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}" + "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(', ') - 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) + 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, quoted_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 f8e8ec43..61bc2ee6 100644 --- a/lib/chrono_model/adapter/migrations.rb +++ b/lib/chrono_model/adapter/migrations.rb @@ -79,11 +79,11 @@ 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 + else super table_name, **options, &block end 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 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