DEV: column dropper class for cleaner removal of superflous columns

Also fixes issues during deploy cause target column was renamed in
theme_fields

db/fixtures/001_categories.rb

@@ -1,3 +1,5 @@
+require 'column_dropper'
+
 # fix any bust caches post initial migration
 ActiveRecord::Base.send(:subclasses).each { |m| m.reset_column_information }
 
@@ -24,26 +26,11 @@ if uncat_id == -1 || !Category.exists?(uncat_id)
            VALUES ('uncategorized_category_id', 3, #{category_id}, now(), now())"
 end
 
-# 60 minutes after our migration runs we need to exectue this code...
+ColumnDropper.drop(
-duration = Rails.env.production? ? 60 : 0
+  table: 'categories',
-if Category.exec_sql("
+  after_migration: 'AddUploadsToCategories',
-    SELECT 1 FROM schema_migration_details
+  columns: ['logo_url', 'background_url'],
-    WHERE EXISTS(
+  on_remove: ->(){
-      SELECT 1 FROM INFORMATION_SCHEMA.COLUMNS
+    STDERR.puts 'Removing superflous categories columns!'
-        WHERE table_schema = 'public' AND table_name = 'categories' AND column_name = 'logo_url'
+  }
-      ) AND
+)
-    name = 'AddUploadsToCategories' AND
-    created_at < (current_timestamp at time zone 'UTC' - interval '#{duration} minutes')
-  ").to_a.length > 0
-
-
-  Category.transaction do
-    STDERR.puts "Removing superflous category columns!"
-    %w[
-      logo_url
-      background_url
-    ].each do |column|
-      Category.exec_sql("ALTER TABLE categories DROP column IF EXISTS #{column}")
-    end
-  end
-end

db/fixtures/009_users.rb

@@ -27,22 +27,11 @@ UserOption.where(user_id: -1).update_all(
 
 Group.user_trust_level_change!(-1, TrustLevel[4])
 
-# 60 minutes after our migration runs we need to exectue this code...
-duration = Rails.env.production? ? 60 : 0
-if User.exec_sql("SELECT 1 FROM schema_migration_details
-                  WHERE EXISTS(
-                      SELECT 1 FROM INFORMATION_SCHEMA.COLUMNS
-                      WHERE table_schema = 'public' AND table_name = 'users'
-                      AND column_name = 'auth_token'
-                    ) AND
-                    name = 'AddUserAuthTokens' AND
-                    created_at < (current_timestamp at time zone 'UTC' - interval '#{duration} minutes')
-                 ").to_a.length > 0
 
-
+ColumnDropper.drop(
-  User.transaction do
+  table: 'users',
-    STDERR.puts "Removing superflous user columns!"
+  after_migration: 'AddUserAuthTokens',
-    %w[
+  columns:  %w[
       email_always
       mailing_list_mode
       email_digests
@@ -59,13 +48,11 @@ if User.exec_sql("SELECT 1 FROM schema_migration_details
       new_topic_duration_minutes
       last_redirected_to_top_at
       auth_token
-      auth_token_updated_at
+      auth_token_updated_at ],
-].each do |column|
+  on_remove: ->(){
-      User.exec_sql("ALTER TABLE users DROP column IF EXISTS #{column}")
+    STDERR.puts 'Removing superflous users columns!'
-    end
+  }
-
+)
-  end
-end
 
 # User for the smoke tests
 if ENV["SMOKE"] == "1"

db/fixtures/600_themes.rb

@@ -17,3 +17,12 @@ if !Theme.exists?
 
   default_theme.set_default!
 end
+
+ColumnDropper.drop(
+  table: 'theme_fields',
+  after_migration: 'AddUploadIdToThemeFields',
+  columns: ['target'],
+  on_remove: ->(){
+    STDERR.puts 'Removing superflous theme_fields target column!'
+  }
+)

db/fixtures/999_topics.rb

@@ -19,7 +19,7 @@ unless Rails.env.test?
 
       SiteSetting.send("#{site_setting_key}=", post.topic_id)
 
-      reply = PostCreator.create( Discourse.system_user,
+      _reply = PostCreator.create( Discourse.system_user,
                                   raw: I18n.t('static_topic_first_reply', page_name: I18n.t(title_key, default: I18n.t(title_key, locale: :en))),
                                   skip_validations: true,
                                   topic_id: post.topic_id )
@@ -67,28 +67,18 @@ end
 
 
 # run this later, cause we need to make sure new application controller resilience is in place first
-duration = Rails.env.production? ? 60 : 0
-if Topic.exec_sql("SELECT 1 FROM schema_migration_details
-                  WHERE EXISTS(
-                      SELECT 1 FROM INFORMATION_SCHEMA.COLUMNS
-                      WHERE table_schema = 'public' AND table_name = 'topics' AND column_name = 'inappropriate_count'
-                    ) AND
-                    name = 'AddTopicColumnsBack' AND
-                    created_at < (current_timestamp at time zone 'UTC' - interval '#{duration} minutes')
-                 ").to_a.length > 0
 
-
+ColumnDropper.drop(
-  Topic.transaction do
+  table: 'topics',
-    STDERR.puts "Removing superflous topic columns!"
+  after_migration: 'AddTopicColumnsBack',
-    %w[
+  columns: %w{
     inappropriate_count
     bookmark_count
     off_topic_count
     illegal_count
     notify_user_count
-].each do |column|
+  },
-      User.exec_sql("ALTER TABLE topics DROP COLUMN IF EXISTS #{column}")
+  on_remove: ->(){
-    end
+    STDERR.puts "Removing superflous topic columns!"
-
+  }
-  end
+)
-end

db/migrate/20170501191912_add_upload_id_to_theme_fields.rb

@@ -2,6 +2,11 @@ class AddUploadIdToThemeFields < ActiveRecord::Migration
   def up
     remove_index :theme_fields, [:theme_id, :target, :name]
     rename_column :theme_fields, :target, :target_id
+
+    # we need a throwaway column here to keep running
+    add_column :theme_fields, :target, :integer
+    execute "UPDATE theme_fields SET target = target_id"
+
     change_column :theme_fields, :name, :string, null: false, limit: 30
 
     add_column :theme_fields, :upload_id, :integer
@@ -12,6 +17,7 @@ class AddUploadIdToThemeFields < ActiveRecord::Migration
   end
 
   def down
+    remove_column :theme_fields, :target
     execute 'drop index theme_field_unique_index'
     rename_column :theme_fields, :target_id, :target
     remove_column :theme_fields, :upload_id

lib/column_dropper.rb

@@ -0,0 +1,38 @@
+class ColumnDropper
+  def self.drop(table:, after_migration:, columns:, delay: nil, on_remove: nil)
+    raise ArgumentError.new("Invalid table name passed to drop #{table}") if table =~ /[^a-z0-9_]/i
+
+    columns.each do |column|
+      raise ArgumentError.new("Invalid column name passed to drop #{column}") if column =~ /[^a-z0-9_]/i
+    end
+
+    delay ||= Rails.env.production? ? 60 : 0
+
+    sql = <<SQL
+    SELECT 1
+    FROM INFORMATION_SCHEMA.COLUMNS
+    WHERE table_schema = 'public' AND
+          table_name = :table AND
+          column_name IN (:columns) AND
+          EXISTS (
+            SELECT 1
+            FROM schema_migration_details
+            WHERE name = :after_migration AND
+                  created_at <= (current_timestamp at time zone 'UTC' - interval :delay)
+          )
+    LIMIT 1
+SQL
+
+    if ActiveRecord::Base.exec_sql(sql, table: table,
+                                        columns: columns,
+                                        delay: "#{delay.to_i || 0} seconds",
+                                        after_migration: after_migration).to_a.length > 0
+        on_remove&.call
+
+        columns.each do |column|
+          # safe cause it is protected on method entry, can not be passed in params
+          ActiveRecord::Base.exec_sql("ALTER TABLE #{table} DROP COLUMN IF EXISTS #{column}")
+        end
+    end
+  end
+end

spec/components/column_dropper_spec.rb

@@ -0,0 +1,52 @@
+require 'rails_helper'
+require 'column_dropper'
+
+describe ColumnDropper do
+
+  def has_column?(table, column)
+    Topic.exec_sql("SELECT 1 FROM INFORMATION_SCHEMA.COLUMNS
+                    WHERE
+                      table_schema = 'public' AND
+                      table_name = :table AND
+                      column_name = :column
+                   ",
+                      table: table, column: column
+                  ).to_a.length == 1
+  end
+
+  it "can correctly drop columns after correct delay" do
+    Topic.exec_sql "ALTER TABLE topics ADD COLUMN junk int"
+    name = Topic
+            .exec_sql("SELECT name FROM schema_migration_details LIMIT 1")
+            .getvalue(0,0)
+
+    Topic.exec_sql("UPDATE schema_migration_details SET created_at = :created_at WHERE name = :name",
+                  name: name, created_at: 15.minutes.ago)
+
+    dropped_proc_called = false
+
+    ColumnDropper.drop(
+      table: 'topics',
+      after_migration: name,
+      columns: ['junk'],
+      delay: 20.minutes,
+      on_remove: ->(){dropped_proc_called = true}
+    )
+
+    expect(has_column?('topics', 'junk')).to eq(true)
+    expect(dropped_proc_called).to eq(false)
+
+    ColumnDropper.drop(
+      table: 'topics',
+      after_migration: name,
+      columns: ['junk'],
+      delay: 10.minutes,
+      on_remove: ->(){dropped_proc_called = true}
+    )
+
+    expect(has_column?('topics', 'junk')).to eq(false)
+    expect(dropped_proc_called).to eq(true)
+
+  end
+end
+