Merge pull request #126 from ifad/bugfix/fix-remove-index-add-index-125

Bugfix/fix remove index add index 125

lib/chrono_model/adapter.rb

@@ -1,6 +1,13 @@
 require 'active_record/connection_adapters/postgresql_adapter'
 
 require 'chrono_model/adapter/migrations'
+
+if ActiveRecord::VERSION::STRING >= '6.1'
+  require 'chrono_model/adapter/migrations_modules/stable'
+else
+  require 'chrono_model/adapter/migrations_modules/legacy'
+end
+
 require 'chrono_model/adapter/ddl'
 require 'chrono_model/adapter/indexes'
 require 'chrono_model/adapter/tsrange'

lib/chrono_model/adapter/migrations.rb

@@ -101,83 +101,6 @@ def drop_table(table_name, *)
         chrono_drop_trigger_functions_for(table_name)
       end
 
-      # If adding an index to a temporal table, add it to the one in the
-      # temporal schema and to the history one. If the `:unique` option is
-      # present, it is removed from the index created in the history table.
-      #
-      def add_index(table_name, column_name, options = {})
-        unless is_chrono?(table_name)
-          return super if RUBY_VERSION < '3.0.0'
-          return super(table_name, column_name, **options)
-        end
-
-        transaction do
-          on_temporal_schema do
-            if RUBY_VERSION < '3.0.0'
-              super
-            else
-              super(table_name, column_name, **options)
-            end
-          end
-
-          # Uniqueness constraints do not make sense in the history table
-          options = options.dup.tap {|o| o.delete(:unique)} if options[:unique].present?
-
-          on_history_schema do
-            if RUBY_VERSION < '3.0.0'
-              super table_name, column_name, options
-            else
-              super table_name, column_name, **options
-            end
-          end
-        end
-      end
-
-      # If removing an index from a temporal table, remove it both from the
-      # temporal and the history schemas.
-      #
-      def remove_index(table_name, options = {})
-        unless is_chrono?(table_name)
-          if RUBY_VERSION < '3.0.0'
-            return super
-          else
-            if ActiveRecord::VERSION::STRING < '6.1'
-              return super(table_name, options)
-            else
-              # nil refers to column name
-              return super(table_name, nil, **options)
-            end
-          end
-        end
-
-        transaction do
-          on_temporal_schema do
-            if RUBY_VERSION < '3.0.0'
-              super
-            else
-              if ActiveRecord::VERSION::STRING < '6.1'
-                super(table_name, options)
-              else
-                # nil refers to column name
-                super(table_name, nil, **options)
-              end
-            end
-          end
-          on_history_schema do
-            if RUBY_VERSION < '3.0.0'
-              super
-            else
-              if ActiveRecord::VERSION::STRING < '6.1'
-                super(table_name, options)
-              else
-                # nil refers to column name
-                super(table_name, nil, **options)
-              end
-            end
-          end
-        end
-      end
-
       # If adding a column to a temporal table, creates it in the table in
       # the temporal schema and updates the triggers.
       #

lib/chrono_model/adapter/migrations_modules/legacy.rb

@@ -0,0 +1,39 @@
+module ChronoModel
+  class Adapter
+    module MigrationsModules
+      module Legacy
+        # If adding an index to a temporal table, add it to the one in the
+        # temporal schema and to the history one. If the `:unique` option is
+        # present, it is removed from the index created in the history table.
+        #
+        def add_index(table_name, column_name, options = {})
+          return super unless is_chrono?(table_name)
+
+          transaction do
+            on_temporal_schema { super }
+
+            # Uniqueness constraints do not make sense in the history table
+            options = options.dup.tap { |o| o.delete(:unique) } if options[:unique].present?
+
+            on_history_schema { super table_name, column_name, options }
+          end
+        end
+
+        # If removing an index from a temporal table, remove it both from the
+        # temporal and the history schemas.
+        #
+        def remove_index(table_name, options = {})
+          return super unless is_chrono?(table_name)
+
+          transaction do
+            on_temporal_schema { super }
+
+            on_history_schema { super }
+          end
+        end
+      end
+    end
+  end
+end
+
+ChronoModel::Adapter::Migrations.include ChronoModel::Adapter::MigrationsModules::Legacy

lib/chrono_model/adapter/migrations_modules/stable.rb

@@ -0,0 +1,39 @@
+module ChronoModel
+  class Adapter
+    module MigrationsModules
+      module Stable
+        # If adding an index to a temporal table, add it to the one in the
+        # temporal schema and to the history one. If the `:unique` option is
+        # present, it is removed from the index created in the history table.
+        #
+        def add_index(table_name, column_name, **options)
+          return super unless is_chrono?(table_name)
+
+          transaction do
+            on_temporal_schema { super }
+
+            # Uniqueness constraints do not make sense in the history table
+            options = options.dup.tap { |o| o.delete(:unique) } if options[:unique].present?
+
+            on_history_schema { super table_name, column_name, **options }
+          end
+        end
+
+        # If removing an index from a temporal table, remove it both from the
+        # temporal and the history schemas.
+        #
+        def remove_index(table_name, column_name = nil, **options)
+          return super unless is_chrono?(table_name)
+
+          transaction do
+            on_temporal_schema { super }
+
+            on_history_schema { super }
+          end
+        end
+      end
+    end
+  end
+end
+
+ChronoModel::Adapter::Migrations.include ChronoModel::Adapter::MigrationsModules::Stable

spec/chrono_model/adapter/migrations_spec.rb

@@ -199,30 +199,38 @@
       before :all do
         adapter.add_index table, [:foo, :bar], :name => 'foobar_index'
         adapter.add_index table, [:test],      :name => 'test_index'
+        adapter.add_index table, :baz
       end
 
       it { is_expected.to have_temporal_index 'foobar_index', %w( foo bar ) }
       it { is_expected.to have_history_index  'foobar_index', %w( foo bar ) }
       it { is_expected.to have_temporal_index 'test_index',   %w( test ) }
       it { is_expected.to have_history_index  'test_index',   %w( test ) }
+      it { is_expected.to have_temporal_index 'index_test_table_on_baz', %w( baz ) }
+      it { is_expected.to have_history_index  'index_test_table_on_baz', %w( baz ) }
 
       it { is_expected.to_not have_index 'foobar_index', %w( foo bar ) }
       it { is_expected.to_not have_index 'test_index',   %w( test ) }
+      it { is_expected.to_not have_index 'index_test_table_on_baz', %w( baz ) }
     end
 
     with_plain_table do
       before :all do
         adapter.add_index table, [:foo, :bar], :name => 'foobar_index'
         adapter.add_index table, [:test],      :name => 'test_index'
+        adapter.add_index table, :baz
       end
 
       it { is_expected.to_not have_temporal_index 'foobar_index', %w( foo bar ) }
       it { is_expected.to_not have_history_index  'foobar_index', %w( foo bar ) }
       it { is_expected.to_not have_temporal_index 'test_index',   %w( test ) }
       it { is_expected.to_not have_history_index  'test_index',   %w( test ) }
+      it { is_expected.to_not have_temporal_index 'index_test_table_on_baz', %w( baz ) }
+      it { is_expected.to_not have_history_index  'index_test_table_on_baz', %w( baz ) }
 
       it { is_expected.to have_index 'foobar_index', %w( foo bar ) }
       it { is_expected.to have_index 'test_index',   %w( test ) }
+      it { is_expected.to have_index 'index_test_table_on_baz', %w( baz ) }
     end
   end
 
@@ -231,26 +239,38 @@
       before :all do
         adapter.add_index table, [:foo, :bar], :name => 'foobar_index'
         adapter.add_index table, [:test],      :name => 'test_index'
+        adapter.add_index table, :baz
 
         adapter.remove_index table, :name => 'test_index'
+        adapter.remove_index table, :baz
       end
 
       it { is_expected.to_not have_temporal_index 'test_index', %w( test ) }
       it { is_expected.to_not have_history_index  'test_index', %w( test ) }
       it { is_expected.to_not have_index          'test_index', %w( test ) }
+
+      it { is_expected.to_not have_temporal_index 'index_test_table_on_baz', %w( baz ) }
+      it { is_expected.to_not have_history_index  'index_test_table_on_baz', %w( baz ) }
+      it { is_expected.to_not have_index          'index_test_table_on_baz', %w( baz ) }
     end
 
     with_plain_table do
       before :all do
         adapter.add_index table, [:foo, :bar], :name => 'foobar_index'
         adapter.add_index table, [:test],      :name => 'test_index'
+        adapter.add_index table, :baz
 
         adapter.remove_index table, :name => 'test_index'
+        adapter.remove_index table, :baz
       end
 
       it { is_expected.to_not have_temporal_index 'test_index', %w( test ) }
       it { is_expected.to_not have_history_index  'test_index', %w( test ) }
       it { is_expected.to_not have_index          'test_index', %w( test ) }
+
+      it { is_expected.to_not have_temporal_index 'index_test_table_on_baz', %w( baz ) }
+      it { is_expected.to_not have_history_index  'index_test_table_on_baz', %w( baz ) }
+      it { is_expected.to_not have_index          'index_test_table_on_baz', %w( baz ) }
     end
   end