summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRobert Speicher <robert@gitlab.com>2016-10-11 14:04:31 +0000
committerRobert Speicher <robert@gitlab.com>2016-10-11 14:04:31 +0000
commit5c6cbfa6cbc6cb82ba20e615c27f98649265b4ce (patch)
treee8e91e7201426324eb0e0e55dbee5099ca59e801
parente28623cc6e54fdcce8b4b8432c767c085a2b7bc0 (diff)
parent1022456bb15d18b05c14fe344950fb75c7c69f48 (diff)
downloadgitlab-ce-5c6cbfa6cbc6cb82ba20e615c27f98649265b4ce.tar.gz
Merge branch 'atom-routes' into 'master'
Allow browsing branches that end with '.atom' ## What does this MR do? 1. Simplify the regex capture in the routing for the CommitsController to not exclude the '.atom' suffix. That's a perfectly valid git branch name, so we shouldn't blow up if we get it. 2. Because Rails now can't automatically detect the request format, add some code to do so in `ExtractPath` when there is no path. This means that, given branches 'foo' and 'foo.atom', the Atom feed for the former is unroutable. To fix this: don't do that! Give the branches different names! ## Why was this MR needed? Creating a branch or tag name ending in '.atom' would cause some 500s on that repo. ## What are the relevant issue numbers? Closes #21955. Related to !5994. See merge request !6750
-rw-r--r--CHANGELOG1
-rw-r--r--config/routes/project.rb2
-rw-r--r--lib/extracts_path.rb33
-rw-r--r--spec/controllers/projects/commits_controller_spec.rb41
-rw-r--r--spec/lib/extracts_path_spec.rb87
-rw-r--r--spec/routing/project_routing_spec.rb2
6 files changed, 152 insertions, 14 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 2095e4fc0fd..0c5955eb59a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -20,6 +20,7 @@ v 8.13.0 (unreleased)
- Don't include archived projects when creating group milestones. !4940 (Jeroen Jacobs)
- Add tag shortcut from the Commit page. !6543
- Keep refs for each deployment
+ - Allow browsing branches that end with '.atom'
- Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller)
- Add more tests for calendar contribution (ClemMakesApps)
- Update Gitlab Shell to fix some problems with moving projects between storages
diff --git a/config/routes/project.rb b/config/routes/project.rb
index e8807ef06a7..4c1da5c4df5 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -159,7 +159,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only:
get(
'/commits/*id',
to: 'commits#show',
- constraints: { id: /(?:[^.]|\.(?!atom$))+/, format: /atom/ },
+ constraints: { id: /.+/, format: false },
as: :commits
)
end
diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb
index a4558d157c0..e4d996a3fb6 100644
--- a/lib/extracts_path.rb
+++ b/lib/extracts_path.rb
@@ -52,8 +52,7 @@ module ExtractsPath
# Append a trailing slash if we only get a ref and no file path
id += '/' unless id.ends_with?('/')
- valid_refs = @project.repository.ref_names
- valid_refs.select! { |v| id.start_with?("#{v}/") }
+ valid_refs = ref_names.select { |v| id.start_with?("#{v}/") }
if valid_refs.length == 0
# No exact ref match, so just try our best
@@ -74,6 +73,19 @@ module ExtractsPath
pair
end
+ # If we have an ID of 'foo.atom', and the controller provides Atom and HTML
+ # formats, then we have to check if the request was for the Atom version of
+ # the ID without the '.atom' suffix, or the HTML version of the ID including
+ # the suffix. We only check this if the version including the suffix doesn't
+ # match, so it is possible to create a branch which has an unroutable Atom
+ # feed.
+ def extract_ref_without_atom(id)
+ id_without_atom = id.sub(/\.atom$/, '')
+ valid_refs = ref_names.select { |v| "#{id_without_atom}/".start_with?("#{v}/") }
+
+ valid_refs.max_by(&:length)
+ end
+
# Assigns common instance variables for views working with Git tree-ish objects
#
# Assignments are:
@@ -86,6 +98,10 @@ module ExtractsPath
# If the :id parameter appears to be requesting a specific response format,
# that will be handled as well.
#
+ # If there is no path and the ref doesn't exist in the repo, try to resolve
+ # the ref without an '.atom' suffix. If _that_ ref is found, set the request's
+ # format to Atom manually.
+ #
# Automatically renders `not_found!` if a valid tree path could not be
# resolved (e.g., when a user inserts an invalid path or ref).
def assign_ref_vars
@@ -103,6 +119,13 @@ module ExtractsPath
@commit = @repo.commit(@options[:extended_sha1])
end
+ if @path.empty? && !@commit
+ @id = @ref = extract_ref_without_atom(@id)
+ @commit = @repo.commit(@ref)
+
+ request.format = :atom if @commit
+ end
+
raise InvalidPathError unless @commit
@hex_path = Digest::SHA1.hexdigest(@path)
@@ -125,4 +148,10 @@ module ExtractsPath
id += "/" + params[:path] unless params[:path].blank?
id
end
+
+ def ref_names
+ return [] unless @project
+
+ @ref_names ||= @project.repository.ref_names
+ end
end
diff --git a/spec/controllers/projects/commits_controller_spec.rb b/spec/controllers/projects/commits_controller_spec.rb
index 2518a48e336..1ac7e03a2db 100644
--- a/spec/controllers/projects/commits_controller_spec.rb
+++ b/spec/controllers/projects/commits_controller_spec.rb
@@ -10,15 +10,38 @@ describe Projects::CommitsController do
end
describe "GET show" do
- context "as atom feed" do
- it "renders as atom" do
- get(:show,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: "master",
- format: "atom")
- expect(response).to be_success
- expect(response.content_type).to eq('application/atom+xml')
+ context "when the ref name ends in .atom" do
+ render_views
+
+ context "when the ref does not exist with the suffix" do
+ it "renders as atom" do
+ get(:show,
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ id: "master.atom")
+
+ expect(response).to be_success
+ expect(response.content_type).to eq('application/atom+xml')
+ end
+ end
+
+ context "when the ref exists with the suffix" do
+ before do
+ commit = project.repository.commit('master')
+
+ allow_any_instance_of(Repository).to receive(:commit).and_call_original
+ allow_any_instance_of(Repository).to receive(:commit).with('master.atom').and_return(commit)
+
+ get(:show,
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ id: "master.atom")
+ end
+
+ it "renders as HTML" do
+ expect(response).to be_success
+ expect(response.content_type).to eq('text/html')
+ end
end
end
end
diff --git a/spec/lib/extracts_path_spec.rb b/spec/lib/extracts_path_spec.rb
index e10c1f5c547..0e85e302f29 100644
--- a/spec/lib/extracts_path_spec.rb
+++ b/spec/lib/extracts_path_spec.rb
@@ -6,6 +6,7 @@ describe ExtractsPath, lib: true do
include Gitlab::Routing.url_helpers
let(:project) { double('project') }
+ let(:request) { double('request') }
before do
@project = project
@@ -15,9 +16,10 @@ describe ExtractsPath, lib: true do
allow(project).to receive(:repository).and_return(repo)
allow(project).to receive(:path_with_namespace).
and_return('gitlab/gitlab-ci')
+ allow(request).to receive(:format=)
end
- describe '#assign_ref' do
+ describe '#assign_ref_vars' do
let(:ref) { sample_commit[:id] }
let(:params) { { path: sample_commit[:line_code_path], ref: ref } }
@@ -61,6 +63,75 @@ describe ExtractsPath, lib: true do
expect(@id).to eq(get_id)
end
end
+
+ context 'ref only exists without .atom suffix' do
+ context 'with a path' do
+ let(:params) { { ref: 'v1.0.0.atom', path: 'README.md' } }
+
+ it 'renders a 404' do
+ expect(self).to receive(:render_404)
+
+ assign_ref_vars
+ end
+ end
+
+ context 'without a path' do
+ let(:params) { { ref: 'v1.0.0.atom' } }
+ before { assign_ref_vars }
+
+ it 'sets the un-suffixed version as @ref' do
+ expect(@ref).to eq('v1.0.0')
+ end
+
+ it 'sets the request format to Atom' do
+ expect(request).to have_received(:format=).with(:atom)
+ end
+ end
+ end
+
+ context 'ref exists with .atom suffix' do
+ context 'with a path' do
+ let(:params) { { ref: 'master.atom', path: 'README.md' } }
+
+ before do
+ repository = @project.repository
+ allow(repository).to receive(:commit).and_call_original
+ allow(repository).to receive(:commit).with('master.atom').and_return(repository.commit('master'))
+
+ assign_ref_vars
+ end
+
+ it 'sets the suffixed version as @ref' do
+ expect(@ref).to eq('master.atom')
+ end
+
+ it 'does not change the request format' do
+ expect(request).not_to have_received(:format=)
+ end
+ end
+
+ context 'without a path' do
+ let(:params) { { ref: 'master.atom' } }
+
+ before do
+ repository = @project.repository
+ allow(repository).to receive(:commit).and_call_original
+ allow(repository).to receive(:commit).with('master.atom').and_return(repository.commit('master'))
+ end
+
+ it 'sets the suffixed version as @ref' do
+ assign_ref_vars
+
+ expect(@ref).to eq('master.atom')
+ end
+
+ it 'does not change the request format' do
+ expect(request).not_to receive(:format=)
+
+ assign_ref_vars
+ end
+ end
+ end
end
describe '#extract_ref' do
@@ -115,4 +186,18 @@ describe ExtractsPath, lib: true do
end
end
end
+
+ describe '#extract_ref_without_atom' do
+ it 'ignores any matching refs suffixed with atom' do
+ expect(extract_ref_without_atom('master.atom')).to eq('master')
+ end
+
+ it 'returns the longest matching ref' do
+ expect(extract_ref_without_atom('release/app/v1.0.0.atom')).to eq('release/app/v1.0.0')
+ end
+
+ it 'returns nil if there are no matching refs' do
+ expect(extract_ref_without_atom('foo.atom')).to eq(nil)
+ end
+ end
end
diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb
index 77842057a10..2322430d212 100644
--- a/spec/routing/project_routing_spec.rb
+++ b/spec/routing/project_routing_spec.rb
@@ -337,7 +337,7 @@ describe Projects::CommitsController, 'routing' do
end
it 'to #show' do
- expect(get('/gitlab/gitlabhq/commits/master.atom')).to route_to('projects/commits#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master', format: 'atom')
+ expect(get('/gitlab/gitlabhq/commits/master.atom')).to route_to('projects/commits#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master.atom')
end
end