Automatic merge of gitlab-org/gitlab-ce master

app/models/merge_request_diff_file.rb

@@ -17,7 +17,7 @@ class MergeRequestDiffFile < ApplicationRecord
       if merge_request_diff&.stored_externally?
         merge_request_diff.opening_external_diff do |file|
           file.seek(external_diff_offset)
-          file.read(external_diff_size)
+          force_encode_utf8(file.read(external_diff_size))
         end
       else
         super

changelogs/unreleased/61841-fix-encoding-error-in-mr-diffs.yml

+---
+title: Fix encoding error in MR diffs when using external diffs
+merge_request: 32862
+author: Hiroyuki Sato
+type: fixed

db/post_migrate/20190801072937_add_gitlab_instance_administration_project.rb

@@ -3,8 +3,10 @@
 class AddGitlabInstanceAdministrationProject < ActiveRecord::Migration[5.2]
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
-    Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService.new.execute!
+    BackgroundMigrationWorker.perform_async('AddGitlabInstanceAdministrationProject', [])
   end
 
   def down

lib/gitlab/background_migration/add_gitlab_instance_administration_project.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class AddGitlabInstanceAdministrationProject
+      def perform
+        Rails.logger.info("Creating Gitlab instance administration project") # rubocop:disable Gitlab/RailsLogger
+
+        Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService.new.execute!
+      end
+    end
+  end
+end

rubocop/code_reuse_helpers.rb

@@ -25,7 +25,7 @@ module RuboCop
 
     # Returns the name of a constant node.
     #
-    # Given the AST node `(const nil :Foo)`, this method will return `:Foo`.
+    # Given the AST node `(const nil? :Foo)`, this method will return `:Foo`.
     def name_of_constant(node)
       node.children[1]
     end
@@ -87,7 +87,7 @@ module RuboCop
 
     # Returns the receiver name of a send node.
     #
-    # For the AST node `(send (const nil :Foo) ...)` this would return
+    # For the AST node `(send (const nil? :Foo) ...)` this would return
     # `'Foo'`.
     def name_of_receiver(node)
       name_of_constant(node.children.first).to_s

spec/lib/gitlab/background_migration/add_gitlab_instance_administration_project_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::AddGitlabInstanceAdministrationProject, :migration, schema: 20190725080128 do
+  let(:application_settings) { table(:application_settings) }
+  let(:users)                { table(:users) }
+  let(:projects)             { table(:projects) }
+  let(:namespaces)           { table(:namespaces) }
+  let(:members)              { table(:members) }
+
+  let(:service_class) do
+    Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService
+  end
+
+  let(:prometheus_settings) do
+    {
+      enable: true,
+      listen_address: 'localhost:9090'
+    }
+  end
+
+  before do
+    stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+
+    stub_config(prometheus: prometheus_settings)
+  end
+
+  describe 'perform' do
+    context 'without application_settings' do
+      it 'does not fail' do
+        subject.perform
+
+        expect(Project.count).to eq(0)
+      end
+    end
+
+    context 'without admin users' do
+      let!(:application_setting) { application_settings.create! }
+
+      it 'does not fail' do
+        subject.perform
+
+        expect(Project.count).to eq(0)
+      end
+    end
+
+    context 'with admin users' do
+      let(:project) { Project.last }
+      let(:group) { Group.last }
+      let!(:application_setting) { application_settings.create! }
+      let!(:user) { users.create!(admin: true, email: 'admin1@example.com', projects_limit: 10, state: :active) }
+
+      before do
+        stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
+      end
+
+      shared_examples 'has prometheus service' do |listen_address|
+        it do
+          subject.perform
+
+          prometheus = project.prometheus_service
+          expect(prometheus).to be_persisted
+          expect(prometheus).not_to eq(nil)
+          expect(prometheus.api_url).to eq(listen_address)
+          expect(prometheus.active).to eq(true)
+          expect(prometheus.manual_configuration).to eq(true)
+        end
+      end
+
+      it_behaves_like 'has prometheus service', 'http://localhost:9090'
+
+      it 'creates GitLab Instance Administrator group' do
+        subject.perform
+
+        expect(group).to be_persisted
+        expect(group.name).to eq('GitLab Instance Administrators')
+        expect(group.path).to start_with('gitlab-instance-administrators')
+        expect(group.path.split('-').last.length).to eq(8)
+        expect(group.visibility_level).to eq(service_class::VISIBILITY_LEVEL)
+      end
+
+      it 'creates project with internal visibility' do
+        subject.perform
+
+        expect(project.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL)
+        expect(project).to be_persisted
+      end
+
+      it 'creates project with correct name and description' do
+        subject.perform
+
+        path = 'administration/monitoring/gitlab_instance_administration_project/index'
+        docs_path = Rails.application.routes.url_helpers.help_page_path(path)
+
+        expect(project.name).to eq(service_class::PROJECT_NAME)
+        expect(project.description).to eq(
+          'This project is automatically generated and will be used to help monitor this GitLab instance. ' \
+          "[More information](#{docs_path})"
+        )
+        expect(File).to exist("doc/#{path}.md")
+      end
+
+      it 'adds all admins as maintainers' do
+        admin1 = users.create!(admin: true, email: 'admin2@example.com', projects_limit: 10, state: :active)
+        admin2 = users.create!(admin: true, email: 'admin3@example.com', projects_limit: 10, state: :active)
+        users.create!(email: 'nonadmin1@example.com', projects_limit: 10, state: :active)
+
+        subject.perform
+
+        expect(project.owner).to eq(group)
+        expect(group.members.collect(&:user).collect(&:id)).to contain_exactly(user.id, admin1.id, admin2.id)
+        expect(group.members.collect(&:access_level)).to contain_exactly(
+          Gitlab::Access::OWNER,
+          Gitlab::Access::MAINTAINER,
+          Gitlab::Access::MAINTAINER
+        )
+      end
+
+      it 'saves the project id' do
+        subject.perform
+
+        application_setting.reload
+        expect(application_setting.instance_administration_project_id).to eq(project.id)
+      end
+
+      it 'does not fail when a project already exists' do
+        group = namespaces.create!(
+          path: 'gitlab-instance-administrators',
+          name: 'GitLab Instance Administrators',
+          type: 'Group'
+        )
+        project = projects.create!(
+          namespace_id: group.id,
+          name: 'GitLab Instance Administration'
+        )
+
+        admin1 = users.create!(admin: true, email: 'admin4@example.com', projects_limit: 10, state: :active)
+        admin2 = users.create!(admin: true, email: 'admin5@example.com', projects_limit: 10, state: :active)
+
+        members.create!(
+          user_id: admin1.id,
+          source_id: group.id,
+          source_type: 'Namespace',
+          type: 'GroupMember',
+          access_level: GroupMember::MAINTAINER,
+          notification_level: NotificationSetting.levels[:global]
+        )
+        members.create!(
+          user_id: admin2.id,
+          source_id: group.id,
+          source_type: 'Namespace',
+          type: 'GroupMember',
+          access_level: GroupMember::MAINTAINER,
+          notification_level: NotificationSetting.levels[:global]
+        )
+
+        stub_application_setting(instance_administration_project: project)
+
+        subject.perform
+
+        expect(Project.last.id).to eq(project.id)
+        expect(Group.last.id).to eq(group.id)
+      end
+
+      context 'when local requests from hooks and services are not allowed' do
+        before do
+          stub_application_setting(allow_local_requests_from_web_hooks_and_services: false)
+        end
+
+        it_behaves_like 'has prometheus service', 'http://localhost:9090'
+
+        it 'does not overwrite the existing whitelist' do
+          application_setting.update!(outbound_local_requests_whitelist: ['example.com'])
+
+          subject.perform
+
+          application_setting.reload
+          expect(application_setting.outbound_local_requests_whitelist).to contain_exactly(
+            'example.com', 'localhost'
+          )
+        end
+      end
+
+      context 'with non default prometheus address' do
+        let(:prometheus_settings) do
+          {
+            enable: true,
+            listen_address: 'https://localhost:9090'
+          }
+        end
+
+        it_behaves_like 'has prometheus service', 'https://localhost:9090'
+      end
+
+      context 'when prometheus setting is not present in gitlab.yml' do
+        before do
+          allow(Gitlab.config).to receive(:prometheus).and_raise(Settingslogic::MissingSetting)
+        end
+
+        it 'does not fail' do
+          subject.perform
+
+          expect(project.prometheus_service).to be_nil
+        end
+      end
+
+      context 'when prometheus setting is disabled in gitlab.yml' do
+        let(:prometheus_settings) do
+          {
+            enable: false,
+            listen_address: 'localhost:9090'
+          }
+        end
+
+        it 'does not configure prometheus' do
+          subject.perform
+
+          expect(project.prometheus_service).to be_nil
+        end
+      end
+
+      context 'when prometheus listen address is blank in gitlab.yml' do
+        let(:prometheus_settings) { { enable: true, listen_address: '' } }
+
+        it 'does not configure prometheus' do
+          subject.perform
+
+          expect(project.prometheus_service).to be_nil
+        end
+      end
+    end
+  end
+end

spec/migrations/add_gitlab_instance_administration_project_spec.rb

@@ -6,13 +6,6 @@ require Rails.root.join('db', 'post_migrate', '20190801072937_add_gitlab_instanc
 describe AddGitlabInstanceAdministrationProject, :migration do
   let(:application_settings) { table(:application_settings) }
   let(:users)                { table(:users) }
-  let(:projects)             { table(:projects) }
-  let(:namespaces)           { table(:namespaces) }
-  let(:members)              { table(:members) }
-
-  let(:service_class) do
-    Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService
-  end
 
   let(:prometheus_settings) do
     {
@@ -43,210 +36,4 @@ describe AddGitlabInstanceAdministrationProject, :migration do
       expect(Group.count).to eq(0)
     end
   end
-
-  describe 'up' do
-    context 'without application_settings' do
-      it 'does not fail' do
-        migrate!
-
-        expect(Project.count).to eq(0)
-      end
-    end
-
-    context 'without admin users' do
-      let!(:application_setting) { application_settings.create! }
-
-      it 'does not fail' do
-        migrate!
-
-        expect(Project.count).to eq(0)
-      end
-    end
-
-    context 'with admin users' do
-      let(:project) { Project.last }
-      let(:group) { Group.last }
-      let!(:application_setting) { application_settings.create! }
-      let!(:user) { users.create!(admin: true, email: 'admin1@example.com', projects_limit: 10, state: :active) }
-
-      before do
-        stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
-      end
-
-      shared_examples 'has prometheus service' do |listen_address|
-        it do
-          migrate!
-
-          prometheus = project.prometheus_service
-          expect(prometheus).to be_persisted
-          expect(prometheus).not_to eq(nil)
-          expect(prometheus.api_url).to eq(listen_address)
-          expect(prometheus.active).to eq(true)
-          expect(prometheus.manual_configuration).to eq(true)
-        end
-      end
-
-      it_behaves_like 'has prometheus service', 'http://localhost:9090'
-
-      it 'creates GitLab Instance Administrator group' do
-        migrate!
-
-        expect(group).to be_persisted
-        expect(group.name).to eq('GitLab Instance Administrators')
-        expect(group.path).to start_with('gitlab-instance-administrators')
-        expect(group.path.split('-').last.length).to eq(8)
-        expect(group.visibility_level).to eq(service_class::VISIBILITY_LEVEL)
-      end
-
-      it 'creates project with internal visibility' do
-        migrate!
-
-        expect(project.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL)
-        expect(project).to be_persisted
-      end
-
-      it 'creates project with correct name and description' do
-        migrate!
-
-        path = 'administration/monitoring/gitlab_instance_administration_project/index'
-        docs_path = Rails.application.routes.url_helpers.help_page_path(path)
-
-        expect(project.name).to eq(service_class::PROJECT_NAME)
-        expect(project.description).to eq(
-          'This project is automatically generated and will be used to help monitor this GitLab instance. ' \
-          "[More information](#{docs_path})"
-        )
-        expect(File).to exist("doc/#{path}.md")
-      end
-
-      it 'adds all admins as maintainers' do
-        admin1 = users.create!(admin: true, email: 'admin2@example.com', projects_limit: 10, state: :active)
-        admin2 = users.create!(admin: true, email: 'admin3@example.com', projects_limit: 10, state: :active)
-        users.create!(email: 'nonadmin1@example.com', projects_limit: 10, state: :active)
-
-        migrate!
-
-        expect(project.owner).to eq(group)
-        expect(group.members.collect(&:user).collect(&:id)).to contain_exactly(user.id, admin1.id, admin2.id)
-        expect(group.members.collect(&:access_level)).to contain_exactly(
-          Gitlab::Access::OWNER,
-          Gitlab::Access::MAINTAINER,
-          Gitlab::Access::MAINTAINER
-        )
-      end
-
-      it 'saves the project id' do
-        migrate!
-
-        application_setting.reload
-        expect(application_setting.instance_administration_project_id).to eq(project.id)
-      end
-
-      it 'does not fail when a project already exists' do
-        group = namespaces.create!(
-          path: 'gitlab-instance-administrators',
-          name: 'GitLab Instance Administrators',
-          type: 'Group'
-        )
-        project = projects.create!(
-          namespace_id: group.id,
-          name: 'GitLab Instance Administration'
-        )
-
-        admin1 = users.create!(admin: true, email: 'admin4@example.com', projects_limit: 10, state: :active)
-        admin2 = users.create!(admin: true, email: 'admin5@example.com', projects_limit: 10, state: :active)
-
-        members.create!(
-          user_id: admin1.id,
-          source_id: group.id,
-          source_type: 'Namespace',
-          type: 'GroupMember',
-          access_level: GroupMember::MAINTAINER,
-          notification_level: NotificationSetting.levels[:global]
-        )
-        members.create!(
-          user_id: admin2.id,
-          source_id: group.id,
-          source_type: 'Namespace',
-          type: 'GroupMember',
-          access_level: GroupMember::MAINTAINER,
-          notification_level: NotificationSetting.levels[:global]
-        )
-
-        stub_application_setting(instance_administration_project: project)
-
-        migrate!
-
-        expect(Project.last.id).to eq(project.id)
-        expect(Group.last.id).to eq(group.id)
-      end
-
-      context 'when local requests from hooks and services are not allowed' do
-        before do
-          stub_application_setting(allow_local_requests_from_web_hooks_and_services: false)
-        end
-
-        it_behaves_like 'has prometheus service', 'http://localhost:9090'
-
-        it 'does not overwrite the existing whitelist' do
-          application_setting.update!(outbound_local_requests_whitelist: ['example.com'])
-
-          migrate!
-
-          application_setting.reload
-          expect(application_setting.outbound_local_requests_whitelist).to contain_exactly(
-            'example.com', 'localhost'
-          )
-        end
-      end
-
-      context 'with non default prometheus address' do
-        let(:prometheus_settings) do
-          {
-            enable: true,
-            listen_address: 'https://localhost:9090'
-          }
-        end
-
-        it_behaves_like 'has prometheus service', 'https://localhost:9090'
-      end
-
-      context 'when prometheus setting is not present in gitlab.yml' do
-        before do
-          allow(Gitlab.config).to receive(:prometheus).and_raise(Settingslogic::MissingSetting)
-        end
-
-        it 'does not fail' do
-          migrate!
-
-          expect(project.prometheus_service).to be_nil
-        end
-      end
-
-      context 'when prometheus setting is disabled in gitlab.yml' do
-        let(:prometheus_settings) do
-          {
-            enable: false,
-            listen_address: 'localhost:9090'
-          }
-        end
-
-        it 'does not configure prometheus' do
-          migrate!
-
-          expect(project.prometheus_service).to be_nil
-        end
-      end
-
-      context 'when prometheus listen address is blank in gitlab.yml' do
-        let(:prometheus_settings) { { enable: true, listen_address: '' } }
-
-        it 'does not configure prometheus' do
-          migrate!
-
-          expect(project.prometheus_service).to be_nil
-        end
-      end
-    end
-  end
 end

spec/models/merge_request_diff_file_spec.rb

@@ -4,26 +4,59 @@ require 'spec_helper'
 
 describe MergeRequestDiffFile do
   describe '#diff' do
-    let(:unpacked) { 'unpacked' }
-    let(:packed) { [unpacked].pack('m0') }
+    context 'when diff is not stored' do
+      let(:unpacked) { 'unpacked' }
+      let(:packed) { [unpacked].pack('m0') }
 
-    before do
-      subject.diff = packed
-    end
-
-    context 'when the diff is marked as binary' do
       before do
-        subject.binary = true
+        subject.diff = packed
+      end
+
+      context 'when the diff is marked as binary' do
+        before do
+          subject.binary = true
+        end
+
+        it 'unpacks from base 64' do
+          expect(subject.diff).to eq(unpacked)
+        end
+      end
+
+      context 'when the diff is not marked as binary' do
+        it 'returns the raw diff' do
+          expect(subject.diff).to eq(packed)
+        end
       end
+    end
 
-      it 'unpacks from base 64' do
-        expect(subject.diff).to eq(unpacked)
+    context 'when diff is stored in DB' do
+      let(:file) { create(:merge_request).merge_request_diff.merge_request_diff_files.first }
+
+      it 'returns UTF-8 string' do
+        expect(file.diff.encoding).to eq Encoding::UTF_8
       end
     end
 
-    context 'when the diff is not marked as binary' do
-      it 'returns the raw diff' do
-        expect(subject.diff).to eq(packed)
+    context 'when diff is stored in external storage' do
+      let(:file) { create(:merge_request).merge_request_diff.merge_request_diff_files.first }
+      let(:test_dir) { 'tmp/tests/external-diffs' }
+
+      around do |example|
+        FileUtils.mkdir_p(test_dir)
+
+        begin
+          example.run
+        ensure
+          FileUtils.rm_rf(test_dir)
+        end
+      end
+
+      before do
+        stub_external_diffs_setting(enabled: true, storage_path: test_dir)
+      end
+
+      it 'returns UTF-8 string' do
+        expect(file.diff.encoding).to eq Encoding::UTF_8
       end
     end
   end