From 6a6eed4ed273ce316e856fe1ef80e64696864b1a Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 4 May 2017 10:15:32 -0400 Subject: [PATCH] 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 | 33 ++++-------- db/fixtures/009_users.rb | 31 ++++------- db/fixtures/600_themes.rb | 9 ++++ db/fixtures/999_topics.rb | 30 ++++------- ...501191912_add_upload_id_to_theme_fields.rb | 6 +++ lib/column_dropper.rb | 38 ++++++++++++++ spec/components/column_dropper_spec.rb | 52 +++++++++++++++++++ 7 files changed, 134 insertions(+), 65 deletions(-) create mode 100644 lib/column_dropper.rb create mode 100644 spec/components/column_dropper_spec.rb diff --git a/db/fixtures/001_categories.rb b/db/fixtures/001_categories.rb index bef41c56d62..67c1c88a904 100644 --- a/db/fixtures/001_categories.rb +++ b/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... -duration = Rails.env.production? ? 60 : 0 -if Category.exec_sql(" - SELECT 1 FROM schema_migration_details - WHERE EXISTS( - SELECT 1 FROM INFORMATION_SCHEMA.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 +ColumnDropper.drop( + table: 'categories', + after_migration: 'AddUploadsToCategories', + columns: ['logo_url', 'background_url'], + on_remove: ->(){ + STDERR.puts 'Removing superflous categories columns!' + } +) diff --git a/db/fixtures/009_users.rb b/db/fixtures/009_users.rb index f2c538b731d..799a05d23fb 100644 --- a/db/fixtures/009_users.rb +++ b/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 - - User.transaction do - STDERR.puts "Removing superflous user columns!" - %w[ +ColumnDropper.drop( + table: 'users', + after_migration: 'AddUserAuthTokens', + 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 -].each do |column| - User.exec_sql("ALTER TABLE users DROP column IF EXISTS #{column}") - end - - end -end + auth_token_updated_at ], + on_remove: ->(){ + STDERR.puts 'Removing superflous users columns!' + } +) # User for the smoke tests if ENV["SMOKE"] == "1" diff --git a/db/fixtures/600_themes.rb b/db/fixtures/600_themes.rb index 28e7e4d78a2..26ab6216f8c 100644 --- a/db/fixtures/600_themes.rb +++ b/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!' + } +) diff --git a/db/fixtures/999_topics.rb b/db/fixtures/999_topics.rb index 477ea2d40c2..2fed96dae6b 100644 --- a/db/fixtures/999_topics.rb +++ b/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 - - Topic.transaction do - STDERR.puts "Removing superflous topic columns!" - %w[ +ColumnDropper.drop( + table: 'topics', + after_migration: 'AddTopicColumnsBack', + 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}") - end - - end -end + }, + on_remove: ->(){ + STDERR.puts "Removing superflous topic columns!" + } +) diff --git a/db/migrate/20170501191912_add_upload_id_to_theme_fields.rb b/db/migrate/20170501191912_add_upload_id_to_theme_fields.rb index c519c0e7920..d72fee80054 100644 --- a/db/migrate/20170501191912_add_upload_id_to_theme_fields.rb +++ b/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 diff --git a/lib/column_dropper.rb b/lib/column_dropper.rb new file mode 100644 index 00000000000..a30adbb7b83 --- /dev/null +++ b/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 = < 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 diff --git a/spec/components/column_dropper_spec.rb b/spec/components/column_dropper_spec.rb new file mode 100644 index 00000000000..02d6a3d56dd --- /dev/null +++ b/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 +