Merge branch 'master' into 'backstage/gb/build-stages-catch-up-migration'

 Conflicts:
   db/schema.rb

9 files changed, 131 insertions, 203 deletions

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 8590522f3ef..c8fa252439a 100644
--- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
+++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
@@ -1,6 +1,11 @@
 require 'spec_helper'
 
-describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq do
+# This migration is using UploadService, which sets uploads.secret that is only
+# added to the DB schema in 20180129193323. Since the test isn't isolated, we
+# just use the latest schema when testing this migration.
+# Ideally, the test should not use factories nor UploadService, and rely on the
+# `table` helper instead.
+describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migration, schema: 20180129193323 do
   include TrackUntrackedUploadsHelpers
 
   subject { described_class.new }
@@ -9,16 +14,10 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq do
   let!(:uploads) { described_class::Upload }
 
   before do
-    DatabaseCleaner.clean
-    drop_temp_table_if_exists
     ensure_temporary_tracking_table_exists
     uploads.delete_all
   end
 
-  after(:all) do
-    drop_temp_table_if_exists
-  end
-
   context 'with untracked files and tracked files in untracked_files_for_uploads' do
     let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) }
     let!(:user1) { create(:user, :with_avatar) }
@@ -117,9 +116,9 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq do
     end
 
     it 'drops the temporary tracking table after processing the batch, if there are no untracked rows left' do
-      subject.perform(1, untracked_files_for_uploads.last.id)
+      expect(subject).to receive(:drop_temp_table_if_finished)
 
-      expect(ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads)).to be_falsey
+      subject.perform(1, untracked_files_for_uploads.last.id)
     end
 
     it 'does not block a whole batch because of one bad path' do
@@ -255,10 +254,6 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do
     ensure_temporary_tracking_table_exists
   end
 
-  after(:all) do
-    drop_temp_table_if_exists
-  end
-
   describe '#upload_path' do
     def assert_upload_path(file_path, expected_upload_path)
       untracked_file = create_untracked_file(file_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 48204114ae8..ca77e64ae40 100644
--- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
+++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
@@ -1,21 +1,11 @@
 require 'spec_helper'
 
-describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
+describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq, :migration, schema: 20180129193323 do
   include TrackUntrackedUploadsHelpers
   include MigrationsHelpers
 
   let!(:untracked_files_for_uploads) { described_class::UntrackedFile }
 
-  before do
-    DatabaseCleaner.clean
-
-    drop_temp_table_if_exists
-  end
-
-  after 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
@@ -23,57 +13,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
     end
   end
 
-  # E.g. The installation is in use at the time of migration, and someone has
-  # just uploaded a file
-  shared_examples 'does not add files in /uploads/tmp' do
-    let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') }
-
-    before do
-      FileUtils.mkdir(File.dirname(tmp_file))
-      FileUtils.touch(tmp_file)
-    end
-
-    after do
-      FileUtils.rm(tmp_file)
-    end
-
-    it 'does not add files from /uploads/tmp' do
-      described_class.new.perform
-
-      expect(untracked_files_for_uploads.count).to eq(5)
-    end
-  end
-
-  it 'ensures the untracked_files_for_uploads table exists' do
-    expect do
-      described_class.new.perform
-    end.to change { ActiveRecord::Base.connection.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 "test bulk insert with ON CONFLICT DO NOTHING or IGNORE" do
-    around do |example|
-      # If this is CI, we use Postgres 9.2 so this whole context should be
-      # skipped since we're unable to use ON CONFLICT DO NOTHING or IGNORE.
-      if described_class.new.send(:can_bulk_insert_and_ignore_duplicates?)
-        example.run
-      end
-    end
-
+  shared_examples 'prepares the untracked_files_for_uploads table' do
     context 'when files were uploaded before and after hashed storage was enabled' do
       let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) }
       let!(:user) { create(:user, :with_avatar) }
@@ -90,6 +30,21 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
         UploadService.new(project2, uploaded_file, FileUploader).execute
       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
+
       it 'adds unhashed files to the untracked_files_for_uploads table' do
         described_class.new.perform
 
@@ -113,7 +68,8 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
       it 'correctly schedules the follow-up background migration jobs' do
         described_class.new.perform
 
-        expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5)
+        ids = described_class::UntrackedFile.all.order(:id).pluck(:id)
+        expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(ids.first, ids.last)
         expect(BackgroundMigrationWorker.jobs.size).to eq(1)
       end
 
@@ -130,91 +86,68 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
         end
       end
 
+      # E.g. The installation is in use at the time of migration, and someone has
+      # just uploaded a file
       context 'when there are files in /uploads/tmp' do
-        it_behaves_like 'does not add files in /uploads/tmp'
-      end
-    end
-  end
-
-  context 'test bulk insert without ON CONFLICT DO NOTHING or IGNORE' do
-    before do
-      # If this is CI, we use Postgres 9.2 so this stub has no effect.
-      #
-      # If this is being run on Postgres 9.5+ or MySQL, then this stub allows us
-      # to test the bulk insert functionality without ON CONFLICT DO NOTHING or
-      # IGNORE.
-      allow_any_instance_of(described_class).to receive(:postgresql_pre_9_5?).and_return(true)
-    end
-
-    context 'when files were uploaded before and after hashed storage was enabled' do
-      let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) }
-      let!(:user) { create(:user, :with_avatar) }
-      let!(:project1) { create(:project, :with_avatar, :legacy_storage) }
-      let(:project2) { create(:project) } # instantiate after enabling hashed_storage
-
-      before do
-        # Markdown upload before enabling hashed_storage
-        UploadService.new(project1, uploaded_file, FileUploader).execute
+        let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') }
 
-        stub_application_setting(hashed_storage_enabled: true)
-
-        # Markdown upload after enabling hashed_storage
-        UploadService.new(project2, uploaded_file, FileUploader).execute
-      end
-
-      it 'adds unhashed files to the untracked_files_for_uploads table' do
-        described_class.new.perform
-
-        expect(untracked_files_for_uploads.count).to eq(5)
-      end
-
-      it 'adds files with paths relative to CarrierWave.root' do
-        described_class.new.perform
-        untracked_files_for_uploads.all.each do |file|
-          expect(file.path.start_with?('uploads/')).to be_truthy
+        before do
+          FileUtils.mkdir(File.dirname(tmp_file))
+          FileUtils.touch(tmp_file)
         end
-      end
-
-      it 'does not add hashed files to the untracked_files_for_uploads table' do
-        described_class.new.perform
 
-        hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path
-        expect(untracked_files_for_uploads.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey
-      end
+        after do
+          FileUtils.rm(tmp_file)
+        end
 
-      it 'correctly schedules the follow-up background migration jobs' do
-        described_class.new.perform
+        it 'does not add files from /uploads/tmp' do
+          described_class.new.perform
 
-        expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5)
-        expect(BackgroundMigrationWorker.jobs.size).to eq(1)
+          expect(untracked_files_for_uploads.count).to eq(5)
+        end
       end
 
-      # E.g. from a previous failed run of this background migration
-      context 'when there is existing data in untracked_files_for_uploads' do
-        before do
-          described_class.new.perform
-        end
+      context 'when the last batch size exactly matches the max batch size' do
+        it 'does not raise error' do
+          stub_const("#{described_class}::FIND_BATCH_SIZE", 5)
 
-        it 'does not error or produce duplicates of existing data' do
           expect do
             described_class.new.perform
-          end.not_to change { untracked_files_for_uploads.count }.from(5)
+          end.not_to raise_error
+
+          expect(untracked_files_for_uploads.count).to eq(5)
         end
       end
+    end
+  end
 
-      context 'when there are files in /uploads/tmp' do
-        it_behaves_like 'does not add files in /uploads/tmp'
-      end
+  # If running on Postgres 9.2 (like on CI), this whole context is skipped
+  # since we're unable to use ON CONFLICT DO NOTHING or IGNORE.
+  context "test bulk insert with ON CONFLICT DO NOTHING or IGNORE", if: described_class.new.send(:can_bulk_insert_and_ignore_duplicates?) do
+    it_behaves_like 'prepares the untracked_files_for_uploads table'
+  end
+
+  # If running on Postgres 9.2 (like on CI), the stubbed method has no effect.
+  #
+  # If running on Postgres 9.5+ or MySQL, then this context effectively tests
+  # the bulk insert functionality without ON CONFLICT DO NOTHING or IGNORE.
+  context 'test bulk insert without ON CONFLICT DO NOTHING or IGNORE' do
+    before do
+      allow_any_instance_of(described_class).to receive(:postgresql_pre_9_5?).and_return(true)
     end
+
+    it_behaves_like 'prepares the untracked_files_for_uploads table'
   end
 
   # Very new or lightly-used installations that are running this migration
   # 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
-      described_class.new.perform
+    it 'deletes the `untracked_files_for_uploads` table (and does not raise error)' do
+      background_migration = described_class.new
+
+      expect(background_migration).to receive(:drop_temp_table)
 
-      expect(untracked_files_for_uploads.count).to eq(0)
+      background_migration.perform
     end
   end
 end
diff --git a/spec/lib/gitlab/import_export/wiki_restorer_spec.rb b/spec/lib/gitlab/import_export/wiki_restorer_spec.rb
new file mode 100644
index 00000000000..81b654e9c5f
--- /dev/null
+++ b/spec/lib/gitlab/import_export/wiki_restorer_spec.rb
@@ -0,0 +1,45 @@
+require 'spec_helper'
+
+describe Gitlab::ImportExport::WikiRestorer do
+  describe 'restore a wiki Git repo' do
+    let!(:project_with_wiki) { create(:project, :wiki_repo) }
+    let!(:project_without_wiki) { create(:project) }
+    let!(:project) { create(:project) }
+    let(:export_path) { "#{Dir.tmpdir}/project_tree_saver_spec" }
+    let(:shared) { Gitlab::ImportExport::Shared.new(relative_path: project.full_path) }
+    let(:bundler) { Gitlab::ImportExport::WikiRepoSaver.new(project: project_with_wiki, shared: shared) }
+    let(:bundle_path) { File.join(shared.export_path, Gitlab::ImportExport.project_bundle_filename) }
+    let(:restorer) do
+      described_class.new(path_to_bundle: bundle_path,
+                          shared: shared,
+                          project: project.wiki,
+                          wiki_enabled: true)
+    end
+
+    before do
+      allow(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
+
+      bundler.save
+    end
+
+    after do
+      FileUtils.rm_rf(export_path)
+      Gitlab::Shell.new.remove_repository(project_with_wiki.wiki.repository_storage_path, project_with_wiki.wiki.disk_path)
+      Gitlab::Shell.new.remove_repository(project.wiki.repository_storage_path, project.wiki.disk_path)
+    end
+
+    it 'restores the wiki repo successfully' do
+      expect(restorer.restore).to be true
+    end
+
+    describe "no wiki in the bundle" do
+      let(:bundler) { Gitlab::ImportExport::WikiRepoSaver.new(project: project_without_wiki, shared: shared) }
+
+      it 'creates an empty wiki' do
+        expect(restorer.restore).to be true
+
+        expect(project.wiki_repository_exists?).to be true
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ldap/person_spec.rb b/spec/lib/gitlab/ldap/person_spec.rb
index b54d4000b53..05e1e394bb1 100644
--- a/spec/lib/gitlab/ldap/person_spec.rb
+++ b/spec/lib/gitlab/ldap/person_spec.rb
@@ -66,15 +66,6 @@ describe Gitlab::LDAP::Person do
     end
   end
 
-  describe '.validate_entry' do
-    it 'raises InvalidEntryError' do
-      entry['foo'] = 'bar'
-
-      expect { described_class.new(entry, 'ldapmain') }
-        .to raise_error(Gitlab::LDAP::Person::InvalidEntryError)
-    end
-  end
-
   describe '#name' do
     it 'uses the configured name attribute and handles values as an array' do
       name = 'John Doe'
diff --git a/spec/lib/gitlab/middleware/multipart_spec.rb b/spec/lib/gitlab/middleware/multipart_spec.rb
index 8d925460f01..a2ba91dae80 100644
--- a/spec/lib/gitlab/middleware/multipart_spec.rb
+++ b/spec/lib/gitlab/middleware/multipart_spec.rb
@@ -5,15 +5,17 @@ require 'tempfile'
 describe Gitlab::Middleware::Multipart do
   let(:app) { double(:app) }
   let(:middleware) { described_class.new(app) }
+  let(:original_filename) { 'filename' }
 
   it 'opens top-level files' do
     Tempfile.open('top-level') do |tempfile|
-      env = post_env({ 'file' => tempfile.path }, { 'file.name' => 'filename' }, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+      env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename }, Gitlab::Workhorse.secret, 'gitlab-workhorse')
 
       expect(app).to receive(:call) do |env|
         file = Rack::Request.new(env).params['file']
         expect(file).to be_a(::UploadedFile)
         expect(file.path).to eq(tempfile.path)
+        expect(file.original_filename).to eq(original_filename)
       end
 
       middleware.call(env)
@@ -34,13 +36,14 @@ describe Gitlab::Middleware::Multipart do
 
   it 'opens files one level deep' do
     Tempfile.open('one-level') do |tempfile|
-      in_params = { 'user' => { 'avatar' => { '.name' => 'filename' } } }
+      in_params = { 'user' => { 'avatar' => { '.name' => original_filename } } }
       env = post_env({ 'user[avatar]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
 
       expect(app).to receive(:call) do |env|
         file = Rack::Request.new(env).params['user']['avatar']
         expect(file).to be_a(::UploadedFile)
         expect(file.path).to eq(tempfile.path)
+        expect(file.original_filename).to eq(original_filename)
       end
 
       middleware.call(env)
@@ -49,13 +52,14 @@ describe Gitlab::Middleware::Multipart do
 
   it 'opens files two levels deep' do
     Tempfile.open('two-levels') do |tempfile|
-      in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => 'filename' } } } }
+      in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => original_filename } } } }
       env = post_env({ 'project[milestone][themesong]' => tempfile.path }, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
 
       expect(app).to receive(:call) do |env|
         file = Rack::Request.new(env).params['project']['milestone']['themesong']
         expect(file).to be_a(::UploadedFile)
         expect(file.path).to eq(tempfile.path)
+        expect(file.original_filename).to eq(original_filename)
       end
 
       middleware.call(env)
diff --git a/spec/lib/gitlab/query_limiting/transaction_spec.rb b/spec/lib/gitlab/query_limiting/transaction_spec.rb
index b4231fcd0fa..b72b8574174 100644
--- a/spec/lib/gitlab/query_limiting/transaction_spec.rb
+++ b/spec/lib/gitlab/query_limiting/transaction_spec.rb
@@ -59,18 +59,6 @@ describe Gitlab::QueryLimiting::Transaction do
         expect { transaction.act_upon_results }
           .to raise_error(described_class::ThresholdExceededError)
       end
-
-      it 'reports the error in Sentry if raising an error is disabled' do
-        expect(transaction)
-          .to receive(:raise_error?)
-          .and_return(false)
-
-        expect(Raven)
-          .to receive(:capture_exception)
-          .with(an_instance_of(described_class::ThresholdExceededError))
-
-        transaction.act_upon_results
-      end
     end
   end
 
diff --git a/spec/lib/gitlab/query_limiting_spec.rb b/spec/lib/gitlab/query_limiting_spec.rb
index 2eddab0b8c3..42877b1e2dd 100644
--- a/spec/lib/gitlab/query_limiting_spec.rb
+++ b/spec/lib/gitlab/query_limiting_spec.rb
@@ -12,14 +12,16 @@ describe Gitlab::QueryLimiting do
       expect(described_class.enable?).to eq(true)
     end
 
-    it 'returns true on GitLab.com' do
+    it 'returns false on GitLab.com' do
+      expect(Rails.env).to receive(:development?).and_return(false)
+      expect(Rails.env).to receive(:test?).and_return(false)
       allow(Gitlab).to receive(:com?).and_return(true)
 
-      expect(described_class.enable?).to eq(true)
+      expect(described_class.enable?).to eq(false)
     end
 
-    it 'returns true in a non GitLab.com' do
-      expect(Gitlab).to receive(:com?).and_return(false)
+    it 'returns false in a non GitLab.com' do
+      allow(Gitlab).to receive(:com?).and_return(false)
       expect(Rails.env).to receive(:development?).and_return(false)
       expect(Rails.env).to receive(:test?).and_return(false)
 
diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb
index 8b54d72d6f7..a1079e54975 100644
--- a/spec/lib/gitlab/regex_spec.rb
+++ b/spec/lib/gitlab/regex_spec.rb
@@ -18,6 +18,7 @@ describe Gitlab::Regex do
     subject { described_class.environment_name_regex }
 
     it { is_expected.to match('foo') }
+    it { is_expected.to match('a') }
     it { is_expected.to match('foo-1') }
     it { is_expected.to match('FOO') }
     it { is_expected.to match('foo/1') }
@@ -25,6 +26,10 @@ describe Gitlab::Regex do
     it { is_expected.not_to match('9&foo') }
     it { is_expected.not_to match('foo-^') }
     it { is_expected.not_to match('!!()()') }
+    it { is_expected.not_to match('/foo') }
+    it { is_expected.not_to match('foo/') }
+    it { is_expected.not_to match('/foo/') }
+    it { is_expected.not_to match('/') }
   end
 
   describe '.environment_slug_regex' do
diff --git a/spec/lib/gitlab/ssh_public_key_spec.rb b/spec/lib/gitlab/ssh_public_key_spec.rb
index c15e29774b6..93d538141ce 100644
--- a/spec/lib/gitlab/ssh_public_key_spec.rb
+++ b/spec/lib/gitlab/ssh_public_key_spec.rb
@@ -37,41 +37,6 @@ describe Gitlab::SSHPublicKey, lib: true do
     end
   end
 
-  describe '.sanitize(key_content)' do
-    let(:content) { build(:key).key }
-
-    context 'when key has blank space characters' do
-      it 'removes the extra blank space characters' do
-        unsanitized = content.insert(100, "\n")
-          .insert(40, "\r\n")
-          .insert(30, ' ')
-
-        sanitized = described_class.sanitize(unsanitized)
-        _, body = sanitized.split
-
-        expect(sanitized).not_to eq(unsanitized)
-        expect(body).not_to match(/\s/)
-      end
-    end
-
-    context "when key doesn't have blank space characters" do
-      it "doesn't modify the content" do
-        sanitized = described_class.sanitize(content)
-
-        expect(sanitized).to eq(content)
-      end
-    end
-
-    context "when key is invalid" do
-      it 'returns the original content' do
-        unsanitized = "ssh-foo any content=="
-        sanitized = described_class.sanitize(unsanitized)
-
-        expect(sanitized).to eq(unsanitized)
-      end
-    end
-  end
-
   describe '#valid?' do
     subject { public_key }