Skip to content
Snippets Groups Projects
Commit 3c2bf586 authored by Jerry Seto's avatar Jerry Seto :two:
Browse files

Merge branch...

Merge branch '20526-impossible-to-browse-a-branch-if-a-tag-exists-with-the-same-name-be-2' into '20526-impossible-to-browse-a-branch-if-a-tag-exists-with-the-same-name-be'

Draft: Update redirects and commit lookup code for controllers

See merge request gitlab-org/gitlab!122237



Merged-by: default avatarJerry Seto <jseto@gitlab.com>
parents 613c4b50 11d851c5
No related branches found
No related tags found
No related merge requests found
# frozen_string_literal: true
module RedirectsForMissingPathOnTree
def redirect_to_tree_root_for_missing_path(project, ref, path)
redirect_to project_tree_path(project, ref), notice: missing_path_on_ref(path, ref)
def redirect_to_tree_root_for_missing_path(project, ref, path, ref_type: nil)
redirect_to project_tree_path(project, ref, ref_type: ref_type), notice: missing_path_on_ref(path, ref)
end
private
......
......@@ -161,6 +161,8 @@ def blob
end
def check_for_ambiguous_ref
return if Feature.enabled?(:redirect_with_ref_type, @project)
@ref_type = ref_type
if @ref_type == ExtractsRef::BRANCH_REF_TYPE && ambiguous_ref?(@project, @ref)
......@@ -170,7 +172,17 @@ def check_for_ambiguous_ref
end
def commit
@commit ||= @repository.commit(@ref)
if Feature.enabled?(:redirect_with_ref_type, @project)
response = ::ExtractsRef::RequestedRef.new(@repository, ref_type: ref_type, ref: @ref).find
@commit = response[:commit]
@ref_type = response[:ref_type]
if response[:ambiguous]
return redirect_to(project_blob_path(@project, File.join(@ref_type ? @ref : @commit.id, @path), ref_type: @ref_type))
end
else
@commit ||= @repository.commit(@ref)
end
return render_404 unless @commit
end
......
......@@ -12,6 +12,7 @@ class Projects::TreeController < Projects::ApplicationController
before_action :require_non_empty_project, except: [:new, :create]
before_action :assign_ref_vars
before_action :find_requested_ref, only: [:show]
before_action :assign_dir_vars, only: [:create_dir]
before_action :authorize_read_code!
before_action :authorize_edit_tree!, only: [:create_dir]
......@@ -29,18 +30,20 @@ class Projects::TreeController < Projects::ApplicationController
def show
return render_404 unless @commit
@ref_type = ref_type
if @ref_type == BRANCH_REF_TYPE && ambiguous_ref?(@project, @ref)
branch = @project.repository.find_branch(@ref)
if branch
redirect_to project_tree_path(@project, branch.target)
return
unless Feature.enabled?(:redirect_with_ref_type, @project)
@ref_type = ref_type
if @ref_type == BRANCH_REF_TYPE && ambiguous_ref?(@project, @ref)
branch = @project.repository.find_branch(@ref)
if branch
redirect_to project_tree_path(@project, branch.target)
return
end
end
end
if tree.entries.empty?
if @repository.blob_at(@commit.id, @path)
redirect_to project_blob_path(@project, File.join(@ref, @path))
redirect_to project_blob_path(@project, File.join(@ref, @path), ref_type: @ref_type)
elsif @path.present?
redirect_to_tree_root_for_missing_path(@project, @ref, @path)
end
......@@ -60,6 +63,23 @@ def create_dir
private
def find_requested_ref
return unless Feature.enabled?(:redirect_with_ref_type, @project)
@ref_type = ref_type
if @ref_type.present?
@tree = @repo.tree(@ref, @path, ref_type: @ref_type)
else
response = ExtractsPath::RequestedRef.new(@repository, ref_type: nil, ref: @ref).find
@ref_type = response[:ref_type]
@commit = response[:commit]
if response[:ambiguous]
redirect_to(project_tree_path(@project, File.join(@ref_type ? @ref : @commit.id, @path), ref_type: @ref_type))
end
end
end
def redirect_renamed_default_branch?
action_name == 'show'
end
......
......@@ -172,7 +172,9 @@ def show
flash.now[:alert] = _("Project '%{project_name}' queued for deletion.") % { project_name: @project.name }
end
if ambiguous_ref?(@project, @ref)
if Feature.enabled?(:redirect_with_ref_type, @project)
@ref_type = 'heads'
elsif ambiguous_ref?(@project, @ref)
branch = @project.repository.find_branch(@ref)
# The files view would render a ref other than the default branch
......
---
name: redirect_with_ref_type
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/122237
rollout_issue_url:
milestone: '16.1'
type: development
group: group::source code
default_enabled: false
......@@ -80,7 +80,7 @@ def assign_ref_vars
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def tree
@tree ||= @repo.tree(@commit.id, @path) # rubocop:disable Gitlab/ModuleWithInstanceVariables
@tree ||= @repo.tree(@commit.id, @path, ref_type: ref_type) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def extract_ref_path
......
# frozen_string_literal: true
module ExtractsRef
class RequestedRef
include Gitlab::Utils::StrongMemoize
SYMBOLIC_REF_PREFIX = %r{((refs/)?(heads|tags)/)+}
def initialize(repository, ref_type:, ref:)
@ref_type = ref_type
@ref = ref
@repository = repository
end
attr_reader :repository, :ref_type, :ref
def find
case ref_type
when 'tags'
{ ref_type: ref_type, commit: tag }
when 'heads'
{ ref_type: ref_type, commit: branch }
else
commit_without_ref_type
end
end
private
def commit_without_ref_type
return { ref_type: nil, commit: nil } if commit.nil?
# ref is probably complete 40 character sha
return { ref_type: nil, commit: commit } if commit.id == ref
if tag.present?
{ ref_type: 'tags', commit: tag, ambiguous: branch.present? }
elsif branch.present?
{ ref_type: 'heads', commit: branch }
elsif ref =~ SYMBOLIC_REF_PREFIX
{ ref_type: nil, commit: commit, ambiguous: true }
else
{ ref_type: nil, commit: commit }
end
end
def commit
repository.commit(ref)
end
strong_memoize_attr :commit
def tag
raw_commit = repository.find_tag(ref)&.dereferenced_target
::Commit.new(raw_commit, repository.container) if raw_commit
end
strong_memoize_attr :tag
def branch
raw_commit = repository.find_branch(ref)&.dereferenced_target
::Commit.new(raw_commit, repository.container) if raw_commit
end
strong_memoize_attr :branch
end
end
......@@ -9,11 +9,18 @@
let(:previous_default_branch) { nil }
describe "GET show" do
let(:params) { { namespace_id: project.namespace, project_id: project, id: id } }
let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
let(:ref_type) { 'heads' }
let(:request) do
get(:show, params: params)
end
let(:redirect_with_ref_type) { true }
before do
stub_feature_flags(redirect_with_ref_type: redirect_with_ref_type)
end
render_views
context 'with file path' do
......@@ -28,21 +35,34 @@
let(:ref) { 'ambiguous_ref' }
let(:path) { 'README.md' }
let(:id) { "#{ref}/#{path}" }
let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
context 'and explicitly requesting a branch' do
let(:ref_type) { 'heads' }
context 'and the redirect_with_ref_type flag is disabled' do
let(:redirect_with_ref_type) { false }
context 'and explicitly requesting a branch' do
let(:ref_type) { 'heads' }
it 'redirects to blob#show with sha for the branch' do
expect(response).to redirect_to(project_blob_path(project, "#{RepoHelpers.another_sample_commit.id}/#{path}"))
end
end
context 'and explicitly requesting a tag' do
let(:ref_type) { 'tags' }
it 'redirects to blob#show with sha for the branch' do
expect(response).to redirect_to(project_blob_path(project, "#{RepoHelpers.another_sample_commit.id}/#{path}"))
it 'responds with success' do
expect(response).to be_ok
end
end
end
context 'and explicitly requesting a tag' do
let(:ref_type) { 'tags' }
context 'and the redirect_with_ref_type flag is enabled' do
context 'when the ref_type is nil' do
let(:ref_type) { nil }
it 'responds with success' do
expect(response).to be_ok
it 'redirects to the tag' do
expect(response).to redirect_to(project_blob_path(project, id, ref_type: 'tags'))
end
end
end
end
......@@ -100,7 +120,7 @@
let(:id) { 'master/README.md' }
before do
get :show, params: { namespace_id: project.namespace, project_id: project, id: id }, format: :json
get :show, params: params, format: :json
end
it do
......@@ -114,7 +134,7 @@
let(:id) { 'master/README.md' }
before do
get :show, params: { namespace_id: project.namespace, project_id: project, id: id, viewer: 'none' }, format: :json
get :show, params: { namespace_id: project.namespace, project_id: project, id: id, ref_type: 'heads', viewer: 'none' }, format: :json
end
it do
......@@ -127,7 +147,7 @@
context 'with tree path' do
before do
get :show, params: { namespace_id: project.namespace, project_id: project, id: id }
get :show, params: params
controller.instance_variable_set(:@blob, nil)
end
......
......@@ -6,6 +6,7 @@
let(:project) { create(:project, :repository, previous_default_branch: previous_default_branch) }
let(:previous_default_branch) { nil }
let(:user) { create(:user) }
let(:redirect_with_ref_type) { true }
before do
sign_in(user)
......@@ -17,10 +18,14 @@
describe "GET show" do
let(:params) do
{
namespace_id: project.namespace.to_param, project_id: project, id: id
namespace_id: project.namespace.to_param, project_id: project, id: id, ref_type: ref_type
}
end
let(:request) { get :show, params: params }
let(:ref_type) { 'heads' }
# Make sure any errors accessing the tree in our views bubble up to this spec
render_views
......@@ -28,26 +33,77 @@
expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
project.repository.add_tag(project.creator, 'ambiguous_ref', RepoHelpers.sample_commit.id)
project.repository.add_branch(project.creator, 'ambiguous_ref', RepoHelpers.another_sample_commit.id)
get :show, params: params
stub_feature_flags(redirect_with_ref_type: redirect_with_ref_type)
end
context 'when the ref is ambiguous' do
let(:id) { 'ambiguous_ref' }
let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
context 'when the redirect_with_ref_type flag is disabled' do
let(:redirect_with_ref_type) { false }
context 'when there is a ref and tag with the same name' do
let(:id) { 'ambiguous_ref' }
let(:ref_type) { nil }
let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
context 'and explicitly requesting a branch' do
let(:ref_type) { 'heads' }
context 'and explicitly requesting a branch' do
let(:ref_type) { 'heads' }
it 'redirects to blob#show with sha for the branch' do
expect(response).to redirect_to(project_tree_path(project, RepoHelpers.another_sample_commit.id))
it 'redirects to blob#show with sha for the branch' do
request
expect(response).to redirect_to(project_tree_path(project, RepoHelpers.another_sample_commit.id))
end
end
context 'and explicitly requesting a tag' do
let(:ref_type) { 'tags' }
it 'responds with success' do
request
expect(response).to be_ok
end
end
end
end
context 'and explicitly requesting a tag' do
let(:ref_type) { 'tags' }
describe 'delegating to ExtractsRef::RequestedRef' do
context 'when there is a ref and tag with the same name' do
let(:id) { 'ambiguous_ref' }
let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
it 'responds with success' do
expect(response).to be_ok
let(:requested_ref_double) { ExtractsRef::RequestedRef.new(project.repository, ref_type: ref_type, ref: id) }
before do
allow(ExtractsRef::RequestedRef).to receive(:new).with(kind_of(Repository), ref_type: ref_type, ref: id).and_return(requested_ref_double)
end
context 'and explicitly requesting a branch' do
let(:ref_type) { 'heads' }
it 'finds the branch' do
expect(requested_ref_double).not_to receive(:find)
request
expect(response).to be_ok
end
end
context 'and explicitly requesting a tag' do
let(:ref_type) { 'tags' }
it 'finds the tag' do
expect(requested_ref_double).not_to receive(:find)
request
expect(response).to be_ok
end
end
context 'and not specifying a ref_type' do
let(:ref_type) { nil }
it 'finds the tags and redirects' do
expect(requested_ref_double).to receive(:find).and_call_original
request
expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{id}/?ref_type=tags")
end
end
end
end
......@@ -55,19 +111,26 @@
context "valid branch, no path" do
let(:id) { 'master' }
it { is_expected.to respond_with(:success) }
it 'responds with success' do
request
expect(response).to be_ok
end
end
context "valid branch, valid path" do
let(:id) { 'master/encoding/' }
it { is_expected.to respond_with(:success) }
it 'responds with success' do
request
expect(response).to be_ok
end
end
context "valid branch, invalid path" do
let(:id) { 'master/invalid-path/' }
it 'redirects' do
request
expect(subject)
.to redirect_to("/#{project.full_path}/-/tree/master")
end
......@@ -76,54 +139,91 @@
context "invalid branch, valid path" do
let(:id) { 'invalid-branch/encoding/' }
it { is_expected.to respond_with(:not_found) }
it 'responds with not_found' do
request
expect(subject).to respond_with(:not_found)
end
end
context "renamed default branch, valid file" do
let(:id) { 'old-default-branch/encoding/' }
let(:previous_default_branch) { 'old-default-branch' }
it { is_expected.to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/encoding/") }
it 'redirects' do
request
expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/encoding/")
end
end
context "renamed default branch, invalid file" do
let(:id) { 'old-default-branch/invalid-path/' }
let(:previous_default_branch) { 'old-default-branch' }
it { is_expected.to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/invalid-path/") }
it 'redirects' do
request
expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/invalid-path/")
end
end
context "valid empty branch, invalid path" do
let(:id) { 'empty-branch/invalid-path/' }
it 'redirects' do
expect(subject)
.to redirect_to("/#{project.full_path}/-/tree/empty-branch")
request
expect(subject).to redirect_to("/#{project.full_path}/-/tree/empty-branch")
end
end
context "valid empty branch" do
let(:id) { 'empty-branch' }
it { is_expected.to respond_with(:success) }
it 'responds with success' do
request
expect(response).to be_ok
end
end
context "invalid SHA commit ID" do
let(:id) { 'ff39438/.gitignore' }
it { is_expected.to respond_with(:not_found) }
it 'responds with not_found' do
request
expect(subject).to respond_with(:not_found)
end
end
context "valid SHA commit ID" do
let(:ref_type) { nil }
let(:id) { '6d39438' }
it { is_expected.to respond_with(:success) }
it 'responds with success' do
request
expect(response).to be_ok
end
context 'and there is a tag with the same name' do
before do
project.repository.add_tag(project.creator, id, RepoHelpers.sample_commit.id)
end
it 'responds with success' do
request
# This uses the tag
# TODO: Should we redirect in this case?
expect(response).to be_ok
end
end
end
context "valid SHA commit ID with path" do
let(:ref_type) { nil }
let(:id) { '6d39438/.gitignore' }
it { expect(response).to have_gitlab_http_status(:found) }
it 'responds with found' do
request
expect(response).to have_gitlab_http_status(:found)
end
end
end
......@@ -149,7 +249,7 @@
before do
get :show, params: {
namespace_id: project.namespace.to_param, project_id: project, id: id
namespace_id: project.namespace.to_param, project_id: project, id: id, ref_type: 'heads'
}
end
......@@ -157,7 +257,7 @@
let(:id) { 'master/README.md' }
it 'redirects' do
redirect_url = "/#{project.full_path}/-/blob/master/README.md"
redirect_url = "/#{project.full_path}/-/blob/master/README.md?ref_type=heads"
expect(subject).to redirect_to(redirect_url)
end
end
......
......@@ -164,65 +164,71 @@ def get_activity(project)
end
end
context 'when there is a tag with the same name as the default branch' do
let_it_be(:tagged_project) { create(:project, :public, :custom_repo, files: ['somefile']) }
let(:tree_with_default_branch) do
branch = tagged_project.repository.find_branch(tagged_project.default_branch)
project_tree_path(tagged_project, branch.target)
end
context 'when redirect_with_ref_type is disabled' do
before do
tagged_project.repository.create_file(
tagged_project.creator,
'file_for_tag',
'content for file',
message: "Automatically created file",
branch_name: 'branch-to-tag'
)
tagged_project.repository.add_tag(
tagged_project.creator,
tagged_project.default_branch, # tag name
'branch-to-tag' # target
)
stub_feature_flags(redirect_with_ref_type: false)
end
it 'redirects to tree view for the default branch' do
get :show, params: { namespace_id: tagged_project.namespace, id: tagged_project }
expect(response).to redirect_to(tree_with_default_branch)
end
end
context 'when the default branch name can resolve to another ref' do
let!(:project_with_default_branch) do
create(:project, :public, :custom_repo, files: ['somefile']).tap do |p|
p.repository.create_branch("refs/heads/refs/heads/#{other_ref}", 'master')
p.change_head("refs/heads/#{other_ref}")
end.reload
end
context 'when there is a tag with the same name as the default branch' do
let_it_be(:tagged_project) { create(:project, :public, :custom_repo, files: ['somefile']) }
let(:tree_with_default_branch) do
branch = tagged_project.repository.find_branch(tagged_project.default_branch)
project_tree_path(tagged_project, branch.target)
end
let(:other_ref) { 'branch-name' }
before do
tagged_project.repository.create_file(
tagged_project.creator,
'file_for_tag',
'content for file',
message: "Automatically created file",
branch_name: 'branch-to-tag'
)
tagged_project.repository.add_tag(
tagged_project.creator,
tagged_project.default_branch, # tag name
'branch-to-tag' # target
)
end
context 'but there is no other ref' do
it 'responds with ok' do
get :show, params: { namespace_id: project_with_default_branch.namespace, id: project_with_default_branch }
expect(response).to be_ok
it 'redirects to tree view for the default branch' do
get :show, params: { namespace_id: tagged_project.namespace, id: tagged_project }
expect(response).to redirect_to(tree_with_default_branch)
end
end
context 'and that other ref exists' do
let(:tree_with_default_branch) do
branch = project_with_default_branch.repository.find_branch(project_with_default_branch.default_branch)
project_tree_path(project_with_default_branch, branch.target)
context 'when the default branch name can resolve to another ref' do
let!(:project_with_default_branch) do
create(:project, :public, :custom_repo, files: ['somefile']).tap do |p|
p.repository.create_branch("refs/heads/refs/heads/#{other_ref}", 'master')
p.change_head("refs/heads/#{other_ref}")
end.reload
end
before do
project_with_default_branch.repository.create_branch(other_ref, 'master')
let(:other_ref) { 'branch-name' }
context 'but there is no other ref' do
it 'responds with ok' do
get :show, params: { namespace_id: project_with_default_branch.namespace, id: project_with_default_branch }
expect(response).to be_ok
end
end
it 'redirects to tree view for the default branch' do
get :show, params: { namespace_id: project_with_default_branch.namespace, id: project_with_default_branch }
expect(response).to redirect_to(tree_with_default_branch)
context 'and that other ref exists' do
let(:tree_with_default_branch) do
branch = project_with_default_branch.repository.find_branch(project_with_default_branch.default_branch)
project_tree_path(project_with_default_branch, branch.target)
end
before do
project_with_default_branch.repository.create_branch(other_ref, 'master')
end
it 'redirects to tree view for the default branch' do
get :show, params: { namespace_id: project_with_default_branch.namespace, id: project_with_default_branch }
expect(response).to redirect_to(tree_with_default_branch)
end
end
end
end
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe ExtractsRef::RequestedRef, feature_category: :source_code_management do
describe '#find' do
subject { described_class.new(project.repository, ref_type: ref_type, ref: ref).find }
let_it_be(:project) { create(:project, :repository) }
let(:ref_type) { nil }
let(:existing_tag_name) { 'v1.0.0' }
let(:existing_branch_name) { 'feature' }
shared_examples 'RequestedRef when ref_type is specified' do
context 'when ref_type is heads' do
let(:ref_type) { 'heads' }
it 'returns the branch commit' do
expect(subject[:ref_type]).to eq('heads')
expect(subject[:commit].id).to eq(project.repository.find_branch(ref)&.dereferenced_target&.id)
end
end
context 'when ref_type is tags' do
let(:ref_type) { 'tags' }
it 'returns the tag commit' do
expect(subject[:ref_type]).to eq('tags')
expect(subject[:commit].id).to eq(project.repository.find_tag(ref)&.dereferenced_target&.id)
end
end
end
context 'when the ref is the sha for a commit' do
let(:ref) { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' }
before(:all) do
project.repository.create_branch('570e7b2abdd848b95f2f578043fc23bd6f6fd24d')
project.repository.add_tag(project.owner, '570e7b2abdd848b95f2f578043fc23bd6f6fd24d', 'master')
end
it_behaves_like 'RequestedRef when ref_type is specified'
it 'returns the commit' do
expect(subject[:ref_type]).to be_nil
expect(subject[:commit].id).to eq(project.repository.commit(ref).id)
end
end
context 'when ref is for a tag' do
let(:ref) { existing_tag_name }
it 'returns the tag commit' do
expect(subject[:ref_type]).to eq('tags')
expect(subject[:commit].id).to eq(project.repository.find_tag(ref)&.dereferenced_target&.id)
end
context 'and there is a branch with the same name' do
before do
project.repository.create_branch(existing_tag_name)
end
it_behaves_like 'RequestedRef when ref_type is specified'
it 'returns the tag commit' do
expect(subject[:ref_type]).to eq('tags')
expect(subject[:commit].id).to eq(project.repository.find_tag(ref)&.dereferenced_target&.id)
expect(subject[:ambiguous]).to be_truthy
end
end
end
context 'when ref is only for a branch' do
let(:ref) { existing_branch_name }
it 'returns the tag commit' do
expect(subject[:ref_type]).to eq('heads')
expect(subject[:commit].id).to eq(project.repository.find_branch(ref)&.dereferenced_target&.id)
end
end
context 'when ref is an abbreviated commit sha' do
let(:ref) { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d'.first(8) }
it 'returns the commit' do
expect(subject[:ref_type]).to be_nil
expect(subject[:commit].id).to eq(project.repository.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d').id)
end
end
context 'when ref does not exist' do
let(:ref) { SecureRandom.uuid }
it 'returns the commit' do
expect(subject[:ref_type]).to be_nil
expect(subject[:commit]).to be_nil
end
end
context 'when ref is symbolic' do
let(:ref) { "heads/#{existing_branch_name}" }
it 'returns the commit' do
expect(subject[:ref_type]).to be_nil
expect(subject[:commit].id).to eq(project.repository.find_branch(existing_branch_name)&.dereferenced_target&.id)
expect(subject[:ambiguous]).to be_truthy
end
end
end
end
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment