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