Tweak how options[:temporal] is handled

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
-

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

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

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
 

spec/chrono_model/adapter/base_spec.rb

@@ -66,61 +66,118 @@
   describe '.on_schema' do
     before(:all) do
       adapter.execute 'BEGIN'
+
       5.times { |i| adapter.execute "CREATE SCHEMA test_#{i}" }
     end
 
     after(:all) do
       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

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