Move temp table creation into the prepare job

* Hopefully fixes spec failures in which the table doesn’t exist
* Decouples the background migration from the post-deploy migration, e.g. we could easily run it again even though the table is dropped when finished.

6 files changed, 76 insertions, 77 deletions

diff --git a/db/post_migrate/20171103140253_track_untracked_uploads.rb b/db/post_migrate/20171103140253_track_untracked_uploads.rb
index 7a34abc85ee..548a94d2d38 100644
--- a/db/post_migrate/20171103140253_track_untracked_uploads.rb
+++ b/db/post_migrate/20171103140253_track_untracked_uploads.rb
@@ -10,8 +10,6 @@ class TrackUntrackedUploads < ActiveRecord::Migration
   MIGRATION = 'PrepareUntrackedUploads'
 
   def up
-    ensure_temporary_tracking_table_exists
-
     BackgroundMigrationWorker.perform_async(MIGRATION)
   end
 
@@ -20,22 +18,4 @@ class TrackUntrackedUploads < ActiveRecord::Migration
       drop_table :untracked_files_for_uploads
     end
   end
-
-  def ensure_temporary_tracking_table_exists
-    unless table_exists?(:untracked_files_for_uploads)
-      create_table :untracked_files_for_uploads do |t|
-        t.string :path, limit: 600, null: false
-        t.boolean :tracked, default: false, null: false
-        t.timestamps_with_timezone null: false
-      end
-    end
-
-    unless index_exists?(:untracked_files_for_uploads, :path)
-      add_index :untracked_files_for_uploads, :path, unique: true
-    end
-
-    unless index_exists?(:untracked_files_for_uploads, :tracked)
-      add_index :untracked_files_for_uploads, :tracked
-    end
-  end
 end
diff --git a/lib/gitlab/background_migration/prepare_untracked_uploads.rb b/lib/gitlab/background_migration/prepare_untracked_uploads.rb
index 8772092da64..e0c1daaccf7 100644
--- a/lib/gitlab/background_migration/prepare_untracked_uploads.rb
+++ b/lib/gitlab/background_migration/prepare_untracked_uploads.rb
@@ -19,16 +19,29 @@ module Gitlab
       end
 
       def perform
-        return unless migrate?
-
+        ensure_temporary_tracking_table_exists
         store_untracked_file_paths
         schedule_populate_untracked_uploads_jobs
       end
 
       private
 
-      def migrate?
-        UntrackedFile.table_exists?
+      def ensure_temporary_tracking_table_exists
+        unless UntrackedFile.connection.table_exists?(:untracked_files_for_uploads)
+          UntrackedFile.connection.create_table :untracked_files_for_uploads do |t|
+            t.string :path, limit: 600, null: false
+            t.boolean :tracked, default: false, null: false
+            t.timestamps_with_timezone null: false
+          end
+        end
+
+        unless UntrackedFile.connection.index_exists?(:untracked_files_for_uploads, :path)
+          UntrackedFile.connection.add_index :untracked_files_for_uploads, :path, unique: true
+        end
+
+        unless UntrackedFile.connection.index_exists?(:untracked_files_for_uploads, :tracked)
+          UntrackedFile.connection.add_index :untracked_files_for_uploads, :tracked
+        end
       end
 
       def store_untracked_file_paths
diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
index 7f5c7b99742..317890bd854 100644
--- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
+++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_uploads')
 
-describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sidekiq, :temp_table_may_drop, schema: 20171103140253 do
+describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sidekiq, schema: 20171103140253 do
   include TrackUntrackedUploadsHelpers
 
   subject { described_class.new }
@@ -10,8 +10,11 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid
   let!(:uploads) { table(:uploads) }
 
   before do
-    # Prevent the TrackUntrackedUploads migration from running PrepareUntrackedUploads job
-    allow(BackgroundMigrationWorker).to receive(:perform_async).and_return(true)
+    ensure_temporary_tracking_table_exists
+  end
+
+  after(:all) do
+    drop_temp_table_if_exists
   end
 
   context 'with untracked files and tracked files in untracked_files_for_uploads' do
@@ -130,6 +133,14 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do
 
   let(:upload_class) { Gitlab::BackgroundMigration::PopulateUntrackedUploads::Upload }
 
+  before(:all) do
+    ensure_temporary_tracking_table_exists
+  end
+
+  after(:all) do
+    drop_temp_table_if_exists
+  end
+
   describe '#ensure_tracked!' do
     let!(:user1) { create(:user, :with_avatar) }
     let!(:untracked_file) { described_class.create!(path: user1.uploads.first.path) }
diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
index 81af3f78307..042fdead281 100644
--- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
+++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
@@ -17,6 +17,14 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side
     end
   end
 
+  before do
+    drop_temp_table_if_exists
+  end
+
+  after(:all) do
+    drop_temp_table_if_exists
+  end
+
   around do |example|
     # Especially important so the follow-up migration does not get run
     Sidekiq::Testing.fake! do
@@ -24,6 +32,27 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side
     end
   end
 
+  it 'ensures the untracked_files_for_uploads table exists' do
+    expect do
+      described_class.new.perform
+    end.to change { table_exists?(:untracked_files_for_uploads) }.from(false).to(true)
+  end
+
+  it 'has a path field long enough for really long paths' do
+    described_class.new.perform
+
+    component = 'a' * 255
+
+    long_path = [
+      'uploads',
+      component, # project.full_path
+      component  # filename
+    ].flatten.join('/')
+
+    record = untracked_files_for_uploads.create!(path: long_path)
+    expect(record.reload.path.size).to eq(519)
+  end
+
   context 'when files were uploaded before and after hashed storage was enabled' do
     let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) }
     let!(:user) { create(:user, :with_avatar) }
@@ -41,9 +70,9 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side
     end
 
     it 'adds unhashed files to the untracked_files_for_uploads table' do
-      expect do
-        described_class.new.perform
-      end.to change { untracked_files_for_uploads.count }.from(0).to(5)
+      described_class.new.perform
+
+      expect(untracked_files_for_uploads.count).to eq(5)
     end
 
     it 'adds files with paths relative to CarrierWave.root' do
@@ -94,9 +123,9 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side
       end
 
       it 'does not add files from /uploads/tmp' do
-        expect do
-          described_class.new.perform
-        end.to change { untracked_files_for_uploads.count }.from(0).to(5)
+        described_class.new.perform
+
+        expect(untracked_files_for_uploads.count).to eq(5)
       end
     end
   end
@@ -105,9 +134,9 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side
   # may not have an upload directory because they have no uploads.
   context 'when no files were ever uploaded' do
     it 'does not add to the untracked_files_for_uploads table (and does not raise error)' do
-      expect do
-        described_class.new.perform
-      end.not_to change { untracked_files_for_uploads.count }.from(0)
+      described_class.new.perform
+
+      expect(untracked_files_for_uploads.count).to eq(0)
     end
   end
 end
diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb
index 5632c81f231..11824bebb91 100644
--- a/spec/migrations/track_untracked_uploads_spec.rb
+++ b/spec/migrations/track_untracked_uploads_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_uploads')
 
-describe TrackUntrackedUploads, :migration, :sidekiq, :temp_table_may_drop do
+describe TrackUntrackedUploads, :migration, :sidekiq do
   include TrackUntrackedUploadsHelpers
 
   let(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) }
@@ -18,41 +18,13 @@ describe TrackUntrackedUploads, :migration, :sidekiq, :temp_table_may_drop do
     end
   end
 
-  context 'before running the background migration' do
-    around do |example|
-      # Especially important so the follow-up migration does not get run
-      Sidekiq::Testing.fake! do
-        example.run
-      end
-    end
-
-    it 'correctly schedules the follow-up background migration' do
+  it 'correctly schedules the follow-up background migration' do
+    Sidekiq::Testing.fake! do
       migrate!
 
       expect(described_class::MIGRATION).to be_scheduled_migration
       expect(BackgroundMigrationWorker.jobs.size).to eq(1)
     end
-
-    it 'ensures the untracked_files_for_uploads table exists' do
-      expect do
-        migrate!
-      end.to change { table_exists?(:untracked_files_for_uploads) }.from(false).to(true)
-    end
-
-    it 'has a path field long enough for really long paths' do
-      migrate!
-
-      component = 'a' * 255
-
-      long_path = [
-        'uploads',
-        component, # project.full_path
-        component  # filename
-      ].flatten.join('/')
-
-      record = untracked_files_for_uploads.create!(path: long_path)
-      expect(record.reload.path.size).to eq(519)
-    end
   end
 
   context 'with tracked and untracked uploads' do
diff --git a/spec/support/track_untracked_uploads_helpers.rb b/spec/support/track_untracked_uploads_helpers.rb
index bb700bc53f1..4d4745fd7f4 100644
--- a/spec/support/track_untracked_uploads_helpers.rb
+++ b/spec/support/track_untracked_uploads_helpers.rb
@@ -4,17 +4,11 @@ module TrackUntrackedUploadsHelpers
     fixture_file_upload(fixture_path)
   end
 
-  def recreate_temp_table_if_dropped
-    TrackUntrackedUploads.new.ensure_temporary_tracking_table_exists
+  def ensure_temporary_tracking_table_exists
+    Gitlab::BackgroundMigration::PrepareUntrackedUploads.new.send(:ensure_temporary_tracking_table_exists)
   end
 
-  RSpec.configure do |config|
-    config.after(:each, :temp_table_may_drop) do
-      recreate_temp_table_if_dropped
-    end
-
-    config.after(:context, :temp_table_may_drop) do
-      recreate_temp_table_if_dropped
-    end
+  def drop_temp_table_if_exists
+    ActiveRecord::Base.connection.drop_table(:untracked_files_for_uploads) if ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads)
   end
 end