Make create_table idempotent for indexes as well (#28)

* Make create_table idempotent for indexes as well

* Add test were one index was already existing

* Fix test

Author: Thomas Hareau

lib/safe-pg-migrations/plugins/idem_potent_statements.rb

@@ -43,12 +43,19 @@ def add_foreign_key(from_table, to_table, **options)
     end
 
     def create_table(table_name, comment: nil, **options)
-      return super unless table_exists?(table_name)
+      return super if options[:force] || !table_exists?(table_name)
 
-      SafePgMigrations.say(
-        "/!\\ Table '#{table_name}' already exists. Skipping statement.",
-        true
-      )
+      SafePgMigrations.say "/!\\ Table '#{table_name}' already exists.", true
+
+      td = create_table_definition(table_name, **options)
+
+      yield td if block_given?
+
+      SafePgMigrations.say(td.indexes.empty? ? '-- Skipping statement' : '-- Creating indexes', true)
+
+      td.indexes.each do |column_name, index_options|
+        add_index(table_name, column_name, index_options)
+      end
     end
 
     private

test/create_table_test.rb

@@ -0,0 +1,96 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+class SafePgMigrationsTest < Minitest::Test
+  def setup
+    SafePgMigrations.instance_variable_set(:@config, nil)
+    @connection = ActiveRecord::Base.connection
+    @verbose_was = ActiveRecord::Migration.verbose
+    @connection.create_table(:schema_migrations) { |t| t.string :version }
+    ActiveRecord::SchemaMigration.create_table
+    ActiveRecord::Migration.verbose = false
+    @connection.execute("SET statement_timeout TO '70s'")
+    @connection.execute("SET lock_timeout TO '70s'")
+  end
+
+  def teardown
+    ActiveRecord::SchemaMigration.drop_table
+    @connection.execute('SET statement_timeout TO 0')
+    @connection.execute("SET lock_timeout TO '30s'")
+    @connection.drop_table(:users, if_exists: true)
+    ActiveRecord::Migration.verbose = @verbose_was
+  end
+
+  def test_create_table
+    @migration =
+      Class.new(ActiveRecord::Migration::Current) do
+        def change
+          create_table(:users) do |t|
+            t.string :email
+            t.references :user, foreign_key: true
+          end
+        end
+      end.new
+
+    calls = record_calls(@connection, :execute) { run_migration }
+    assert_calls [
+      "SET statement_timeout TO '5s'",
+
+      # Create the table with constraints.
+      'CREATE TABLE "users" ("id" bigserial primary key, "email" character varying, "user_id" bigint, ' \
+        'CONSTRAINT "fk_rails_6d0b8b3c2f" FOREIGN KEY ("user_id") REFERENCES "users" ("id") )',
+
+      # Create the index.
+      'SET statement_timeout TO 0',
+      "SET lock_timeout TO '30s'",
+      'CREATE INDEX CONCURRENTLY "index_users_on_user_id" ON "users" ("user_id")',
+      "SET lock_timeout TO '5s'",
+      "SET statement_timeout TO '5s'",
+
+      "SET statement_timeout TO '70s'",
+    ], calls
+
+    run_migration(:down)
+    refute @connection.table_exists?(:users)
+  end
+
+  def test_create_table_idempotence
+    # Simulates an interruption between the table creation and the index creation
+    @connection.create_table(:users) do |t|
+      t.string :name, index: true
+      t.string :email
+    end
+
+    @migration =
+      Class.new(ActiveRecord::Migration::Current) do
+        def change
+          create_table(:users) do |t|
+            t.string :name, index: true
+            t.string :email, index: true
+          end
+        end
+      end.new
+
+    calls = record_calls(@connection, :execute) { run_migration }
+    indexes = ActiveRecord::Base.connection.indexes :users
+    refute_empty indexes
+    assert_equal 'index_users_on_email', indexes.first.name
+
+    refute_includes flat_calls(calls), 'CREATE INDEX CONCURRENTLY "index_users_on_name" ON "users" ("name")'
+
+    assert_calls [
+      "SET statement_timeout TO '5s'",
+      'SET statement_timeout TO 0',
+      "SET lock_timeout TO '30s'",
+      "SET lock_timeout TO '5s'",
+      "SET statement_timeout TO '5s'",
+      'SET statement_timeout TO 0',
+      "SET lock_timeout TO '30s'",
+      'CREATE INDEX CONCURRENTLY "index_users_on_email" ON "users" ("email")',
+      "SET lock_timeout TO '5s'",
+      "SET statement_timeout TO '5s'",
+      "SET statement_timeout TO '70s'",
+    ], calls
+  end
+end

test/safe_pg_migrations_test.rb

@@ -209,8 +209,9 @@ def change
     assert_equal [
       '== 8128 : migrating ===========================================================',
       '-- create_table(:users)',
-      "   -> /!\\ Table 'users' already exists. Skipping statement.",
-    ], write_calls[0...3]
+      "   -> /!\\ Table 'users' already exists.",
+      '   -> -- Skipping statement',
+    ], write_calls[0...4]
   end
 
   def test_add_column_idem_potent
@@ -319,39 +320,6 @@ def change
     refute @connection.column_exists?(:users, :user)
   end
 
-  def test_create_table
-    @migration =
-      Class.new(ActiveRecord::Migration::Current) do
-        def change
-          create_table(:users) do |t|
-            t.string :email
-            t.references :user, foreign_key: true
-          end
-        end
-      end.new
-
-    calls = record_calls(@connection, :execute) { run_migration }
-    assert_calls [
-      "SET statement_timeout TO '5s'",
-
-      # Create the table with constraints.
-      'CREATE TABLE "users" ("id" bigserial primary key, "email" character varying, "user_id" bigint, ' \
-        'CONSTRAINT "fk_rails_6d0b8b3c2f" FOREIGN KEY ("user_id") REFERENCES "users" ("id") )',
-
-      # Create the index.
-      'SET statement_timeout TO 0',
-      "SET lock_timeout TO '30s'",
-      'CREATE INDEX CONCURRENTLY "index_users_on_user_id" ON "users" ("user_id")',
-      "SET lock_timeout TO '5s'",
-      "SET statement_timeout TO '5s'",
-
-      "SET statement_timeout TO '70s'",
-    ], calls
-
-    run_migration(:down)
-    refute @connection.table_exists?(:users)
-  end
-
   def test_add_index
     @connection.create_table(:users) { |t| t.string :email }
     @migration =

test/test_helper.rb

@@ -35,7 +35,11 @@ def assert_calls(expected, actual)
       "SET lock_timeout TO '5s'",
       *expected,
       "SET lock_timeout TO '70s'",
-    ], actual.map(&:first).map(&:squish).reverse.drop_while { |call| %w[BEGIN COMMIT].include? call }.reverse
+    ], flat_calls(actual)
+  end
+
+  def flat_calls(calls)
+    calls.map(&:first).map(&:squish).reverse.drop_while { |call| %w[BEGIN COMMIT].include? call }.reverse
   end
 
   # Records method calls on an object. Behaves like a test spy.