Merge branch 'ce-to-ee-2018-11-26' into 'master'

CE upstream - 2018-11-26 23:21 UTC

See merge request gitlab-org/gitlab-ee!8598

app/helpers/tree_helper.rb

@@ -37,13 +37,13 @@ module TreeHelper
   # Using Rails `*_path` methods can be slow, especially when generating
   # many paths, as with a repository tree that has thousands of items.
   def fast_project_blob_path(project, blob_path)
-    Addressable::URI.escape(
+    ActionDispatch::Journey::Router::Utils.escape_path(
       File.join(relative_url_root, project.path_with_namespace, 'blob', blob_path)
     )
   end
 
   def fast_project_tree_path(project, tree_path)
-    Addressable::URI.escape(
+    ActionDispatch::Journey::Router::Utils.escape_path(
       File.join(relative_url_root, project.path_with_namespace, 'tree', tree_path)
     )
   end

app/models/merge_request_diff.rb

@@ -313,7 +313,8 @@ class MergeRequestDiff < ActiveRecord::Base
 
     # merge_request_diff_commits.reload is preferred way to reload associated
     # objects but it returns cached result for some reason in this case
-    commits = merge_request_diff_commits(true)
+    # we can circumvent that by specifying that we need an uncached reload
+    commits = self.class.uncached { merge_request_diff_commits.reload }
     self.commits_count = commits.size
   end
 

changelogs/unreleased/fix-deadlock-chunked-io.yml

+---
+title: Fix deadlock on ChunkedIO
+merge_request:
+author:
+type: fixed

changelogs/unreleased/force-reload-arguments-1.yml

+---
+title: Passing an argument to force an association to reload is now deprecated
+merge_request: 23334
+author: Jasper Maes
+type: other

changelogs/unreleased/sh-fix-hash-filename-handling.yml

+---
+title: Fix handling of filenames with hash characters in tree view
+merge_request: 23368
+author:
+type: fixed

lib/gitlab/ci/trace/chunked_io.rb

@@ -13,7 +13,7 @@ module Gitlab
 
         attr_reader :build
         attr_reader :tell, :size
-        attr_reader :chunk, :chunk_range
+        attr_reader :chunk_data, :chunk_range
 
         alias_method :pos, :tell
 
@@ -75,14 +75,14 @@ module Gitlab
 
           until length <= 0 || eof?
             data = chunk_slice_from_offset
-            break if data.empty?
+            raise FailedToGetChunkError if data.empty?
 
             chunk_bytes = [CHUNK_SIZE - chunk_offset, length].min
-            chunk_data = data.byteslice(0, chunk_bytes)
+            chunk_data_slice = data.byteslice(0, chunk_bytes)
 
-            out << chunk_data
-            @tell += chunk_data.bytesize
-            length -= chunk_data.bytesize
+            out << chunk_data_slice
+            @tell += chunk_data_slice.bytesize
+            length -= chunk_data_slice.bytesize
           end
 
           out = out.join
@@ -100,11 +100,14 @@ module Gitlab
 
           until eof?
             data = chunk_slice_from_offset
+            raise FailedToGetChunkError if data.empty?
+
             new_line = data.index("\n")
 
             if !new_line.nil?
-              out << data[0..new_line]
-              @tell += new_line + 1
+              raw_data = data[0..new_line]
+              out << raw_data
+              @tell += raw_data.bytesize
               break
             else
               out << data
@@ -121,13 +124,13 @@ module Gitlab
           while tell < start_pos + data.bytesize
             # get slice from current offset till the end where it falls into chunk
             chunk_bytes = CHUNK_SIZE - chunk_offset
-            chunk_data = data.byteslice(tell - start_pos, chunk_bytes)
+            data_slice = data.byteslice(tell - start_pos, chunk_bytes)
 
             # append data to chunk, overwriting from that point
-            ensure_chunk.append(chunk_data, chunk_offset)
+            ensure_chunk.append(data_slice, chunk_offset)
 
             # move offsets within buffer
-            @tell += chunk_data.bytesize
+            @tell += data_slice.bytesize
             @size = [size, tell].max
           end
 
@@ -183,12 +186,12 @@ module Gitlab
             current_chunk.tap do |chunk|
               raise FailedToGetChunkError unless chunk
 
-              @chunk = chunk.data
+              @chunk_data = chunk.data
               @chunk_range = chunk.range
             end
           end
 
-          @chunk[chunk_offset..CHUNK_SIZE]
+          @chunk_data.byteslice(chunk_offset, CHUNK_SIZE)
         end
 
         def chunk_offset

lib/gitlab/http_io.rb

@@ -85,11 +85,11 @@ module Gitlab
         break if data.empty?
 
         chunk_bytes = [BUFFER_SIZE - chunk_offset, length].min
-        chunk_data = data.byteslice(0, chunk_bytes)
+        data_slice = data.byteslice(0, chunk_bytes)
 
-        out << chunk_data
-        @tell += chunk_data.bytesize
-        length -= chunk_data.bytesize
+        out << data_slice
+        @tell += data_slice.bytesize
+        length -= data_slice.bytesize
       end
 
       out = out.join

lib/gitlab/profiler.rb

@@ -36,7 +36,6 @@ module Gitlab
     #
     # - private_token: instead of providing a user instance, the token can be
     #   given as a string. Takes precedence over the user option.
-    # rubocop: disable CodeReuse/ActiveRecord
     def self.profile(url, logger: nil, post_data: nil, user: nil, private_token: nil)
       app = ActionDispatch::Integration::Session.new(Rails.application)
       verb = :get
@@ -47,12 +46,11 @@ module Gitlab
         headers['Content-Type'] = 'application/json'
       end
 
-      if user
-        private_token ||= user.personal_access_tokens.active.pluck(:token).first
-        raise 'Your user must have a personal_access_token' unless private_token
+      if private_token
+        headers['Private-Token'] = private_token
+        user = nil # private_token overrides user
       end
 
-      headers['Private-Token'] = private_token if private_token
       logger = create_custom_logger(logger, private_token: private_token)
 
       RequestStore.begin!
@@ -70,7 +68,9 @@ module Gitlab
       app.get('/api/v4/users')
 
       result = with_custom_logger(logger) do
-        RubyProf.profile { app.public_send(verb, url, post_data, headers) } # rubocop:disable GitlabSecurity/PublicSend
+        with_user(user) do
+          RubyProf.profile { app.public_send(verb, url, post_data, headers) } # rubocop:disable GitlabSecurity/PublicSend
+        end
       end
 
       RequestStore.end!
@@ -79,7 +79,6 @@ module Gitlab
 
       result
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     def self.create_custom_logger(logger, private_token: nil)
       return unless logger
@@ -130,13 +129,29 @@ module Gitlab
         ActionController::Base.logger = logger
       end
 
-      result = yield
+      yield.tap do
+        ActiveSupport::LogSubscriber.colorize_logging = original_colorize_logging
+        ActiveRecord::Base.logger = original_activerecord_logger
+        ActionController::Base.logger = original_actioncontroller_logger
+      end
+    end
+
+    def self.with_user(user)
+      if user
+        API::Helpers::CommonHelpers.send(:define_method, :find_current_user!) { user } # rubocop:disable GitlabSecurity/PublicSend
+        ApplicationController.send(:define_method, :current_user) { user } # rubocop:disable GitlabSecurity/PublicSend
+        ApplicationController.send(:define_method, :authenticate_user!) { } # rubocop:disable GitlabSecurity/PublicSend
+      end
 
-      ActiveSupport::LogSubscriber.colorize_logging = original_colorize_logging
-      ActiveRecord::Base.logger = original_activerecord_logger
-      ActionController::Base.logger = original_actioncontroller_logger
+      yield.tap do
+        remove_method(API::Helpers::CommonHelpers, :find_current_user!)
+        remove_method(ApplicationController, :current_user)
+        remove_method(ApplicationController, :authenticate_user!)
+      end
+    end
 
-      result
+    def self.remove_method(klass, meth)
+      klass.send(:remove_method, meth) if klass.instance_methods(false).include?(meth) # rubocop:disable GitlabSecurity/PublicSend
     end
 
     # rubocop: disable CodeReuse/ActiveRecord

spec/helpers/tree_helper_spec.rb

@@ -5,6 +5,16 @@ describe TreeHelper do
   let(:repository) { project.repository }
   let(:sha) { 'c1c67abbaf91f624347bb3ae96eabe3a1b742478' }
 
+  def create_file(filename)
+    project.repository.create_file(
+      project.creator,
+      filename,
+      'test this',
+      message: "Automatically created file #{filename}",
+      branch_name: 'master'
+    )
+  end
+
   describe '.render_tree' do
     before do
       @id = sha
@@ -57,6 +67,15 @@ describe TreeHelper do
 
       expect(fast_path).to start_with('/gitlab/root')
     end
+
+    it 'encodes files starting with #' do
+      filename = '#test-file'
+      create_file(filename)
+
+      fast_path = fast_project_blob_path(project, filename)
+
+      expect(fast_path).to end_with('%23test-file')
+    end
   end
 
   describe '.fast_project_tree_path' do
@@ -73,6 +92,15 @@ describe TreeHelper do
 
       expect(fast_path).to start_with('/gitlab/root')
     end
+
+    it 'encodes files starting with #' do
+      filename = '#test-file'
+      create_file(filename)
+
+      fast_path = fast_project_tree_path(project, filename)
+
+      expect(fast_path).to end_with('%23test-file')
+    end
   end
 
   describe 'flatten_tree' do

spec/lib/gitlab/ci/trace/chunked_io_spec.rb

@@ -116,6 +116,19 @@ describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do
         chunked_io.each_line { |line| }
       end
     end
+
+    context 'when buffer consist of many empty lines' do
+      let(:sample_trace_raw) { Array.new(10, "   ").join("\n") }
+
+      before do
+        build.trace.set(sample_trace_raw)
+      end
+
+      it 'yields lines' do
+        expect { |b| chunked_io.each_line(&b) }
+          .to yield_successive_args(*string_io.each_line.to_a)
+      end
+    end
   end
 
   context "#read" do
@@ -143,6 +156,22 @@ describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do
       end
     end
 
+    context 'when chunk is missing data' do
+      let(:length) { nil }
+
+      before do
+        stub_buffer_size(1024)
+        build.trace.set(sample_trace_raw)
+
+        # make second chunk to not have data
+        build.trace_chunks.second.append('', 0)
+      end
+
+      it 'raises an error' do
+        expect { subject }.to raise_error described_class::FailedToGetChunkError
+      end
+    end
+
     context 'when read only first 100 bytes' do
       let(:length) { 100 }
 
@@ -266,6 +295,40 @@ describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do
         expect(chunked_io.readline).to eq(string_io.readline)
       end
     end
+
+    context 'when chunk is missing data' do
+      let(:length) { nil }
+
+      before do
+        build.trace.set(sample_trace_raw)
+
+        # make first chunk to have invalid data
+        build.trace_chunks.first.append('data', 0)
+      end
+
+      it 'raises an error' do
+        expect { subject }.to raise_error described_class::FailedToGetChunkError
+      end
+    end
+
+    context 'when utf-8 is being used' do
+      let(:sample_trace_raw) { sample_trace_raw_utf8.force_encoding(Encoding::BINARY) }
+      let(:sample_trace_raw_utf8) { "😺\n😺\n😺\n😺" }
+
+      before do
+        stub_buffer_size(3) # the utf-8 character has 4 bytes
+
+        build.trace.set(sample_trace_raw_utf8)
+      end
+
+      it 'has known length' do
+        expect(sample_trace_raw_utf8.bytesize).to eq(4 * 4 + 3 * 1)
+        expect(sample_trace_raw.bytesize).to eq(4 * 4 + 3 * 1)
+        expect(chunked_io.size).to eq(4 * 4 + 3 * 1)
+      end
+
+      it_behaves_like 'all line matching'
+    end
   end
 
   context "#write" do

spec/lib/gitlab/profiler_spec.rb

@@ -43,31 +43,16 @@ describe Gitlab::Profiler do
 
     it 'uses the user for auth if given' do
       user = double(:user)
-      user_token = 'user'
 
-      allow(user).to receive_message_chain(:personal_access_tokens, :active, :pluck, :first).and_return(user_token)
-
-      expect(app).to receive(:get).with('/', nil, 'Private-Token' => user_token)
-      expect(app).to receive(:get).with('/api/v4/users')
+      expect(described_class).to receive(:with_user).with(user)
 
       described_class.profile('/', user: user)
     end
 
-    context 'when providing a user without a personal access token' do
-      it 'raises an error' do
-        user = double(:user)
-        allow(user).to receive_message_chain(:personal_access_tokens, :active, :pluck).and_return([])
-
-        expect { described_class.profile('/', user: user) }.to raise_error('Your user must have a personal_access_token')
-      end
-    end
-
     it 'uses the private_token for auth if both it and user are set' do
       user = double(:user)
-      user_token = 'user'
-
-      allow(user).to receive_message_chain(:personal_access_tokens, :active, :pluck, :first).and_return(user_token)
 
+      expect(described_class).to receive(:with_user).with(nil).and_call_original
       expect(app).to receive(:get).with('/', nil, 'Private-Token' => private_token)
       expect(app).to receive(:get).with('/api/v4/users')
 
@@ -210,6 +195,29 @@ describe Gitlab::Profiler do
     end
   end
 
+  describe '.with_user' do
+    context 'when the user is set' do
+      let(:user) { double(:user) }
+
+      it 'overrides auth in ApplicationController to use the given user' do
+        expect(described_class.with_user(user) { ApplicationController.new.current_user }).to eq(user)
+      end
+
+      it 'cleans up ApplicationController afterwards' do
+        expect { described_class.with_user(user) { } }
+          .to not_change { ActionController.instance_methods(false) }
+      end
+    end
+
+    context 'when the user is nil' do
+      it 'does not define methods on ApplicationController' do
+        expect(ApplicationController).not_to receive(:define_method)
+
+        described_class.with_user(nil) { }
+      end
+    end
+  end
+
   describe '.log_load_times_by_model' do
     it 'logs the model, query count, and time by slowest first' do
       expect(null_logger).to receive(:load_times_by_model).and_return(