summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/helpers/tree_helper.rb4
-rw-r--r--app/models/merge_request_diff.rb3
-rw-r--r--changelogs/unreleased/fix-deadlock-chunked-io.yml5
-rw-r--r--changelogs/unreleased/force-reload-arguments-1.yml5
-rw-r--r--changelogs/unreleased/sh-fix-hash-filename-handling.yml5
-rw-r--r--lib/gitlab/ci/trace/chunked_io.rb29
-rw-r--r--lib/gitlab/http_io.rb8
-rw-r--r--lib/gitlab/profiler.rb39
-rw-r--r--spec/helpers/tree_helper_spec.rb28
-rw-r--r--spec/lib/gitlab/ci/trace/chunked_io_spec.rb63
-rw-r--r--spec/lib/gitlab/profiler_spec.rb42
11 files changed, 182 insertions, 49 deletions
diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb
index 78a11616d4c..e2879bfdcf1 100644
--- a/app/helpers/tree_helper.rb
+++ b/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
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 6f1beede6f9..a3029a54604 100644
--- a/app/models/merge_request_diff.rb
+++ b/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
diff --git a/changelogs/unreleased/fix-deadlock-chunked-io.yml b/changelogs/unreleased/fix-deadlock-chunked-io.yml
new file mode 100644
index 00000000000..def7a59e86e
--- /dev/null
+++ b/changelogs/unreleased/fix-deadlock-chunked-io.yml
@@ -0,0 +1,5 @@
+---
+title: Fix deadlock on ChunkedIO
+merge_request:
+author:
+type: fixed
diff --git a/changelogs/unreleased/force-reload-arguments-1.yml b/changelogs/unreleased/force-reload-arguments-1.yml
new file mode 100644
index 00000000000..29f34b8bdbe
--- /dev/null
+++ b/changelogs/unreleased/force-reload-arguments-1.yml
@@ -0,0 +1,5 @@
+---
+title: Passing an argument to force an association to reload is now deprecated
+merge_request: 23334
+author: Jasper Maes
+type: other
diff --git a/changelogs/unreleased/sh-fix-hash-filename-handling.yml b/changelogs/unreleased/sh-fix-hash-filename-handling.yml
new file mode 100644
index 00000000000..cb32051a4ab
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-hash-filename-handling.yml
@@ -0,0 +1,5 @@
+---
+title: Fix handling of filenames with hash characters in tree view
+merge_request: 23368
+author:
+type: fixed
diff --git a/lib/gitlab/ci/trace/chunked_io.rb b/lib/gitlab/ci/trace/chunked_io.rb
index e9b3199d56e..8c6fd56493f 100644
--- a/lib/gitlab/ci/trace/chunked_io.rb
+++ b/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
diff --git a/lib/gitlab/http_io.rb b/lib/gitlab/http_io.rb
index e768b8adb12..6a9fb85b054 100644
--- a/lib/gitlab/http_io.rb
+++ b/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
diff --git a/lib/gitlab/profiler.rb b/lib/gitlab/profiler.rb
index 4a62f367835..93a9fcf1591 100644
--- a/lib/gitlab/profiler.rb
+++ b/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
diff --git a/spec/helpers/tree_helper_spec.rb b/spec/helpers/tree_helper_spec.rb
index ab4566e261b..4a62e696cd9 100644
--- a/spec/helpers/tree_helper_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/ci/trace/chunked_io_spec.rb b/spec/lib/gitlab/ci/trace/chunked_io_spec.rb
index 6259b952add..546a9e7d0cc 100644
--- a/spec/lib/gitlab/ci/trace/chunked_io_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/profiler_spec.rb b/spec/lib/gitlab/profiler_spec.rb
index 4059188fba1..8bb0c1a0b8a 100644
--- a/spec/lib/gitlab/profiler_spec.rb
+++ b/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(
View Lorry Controller Status