Commit 6c612d69 authored by Markus Koller's avatar Markus Koller Committed by Matthias Käppler

Support repository moved message with all container types

Rename the `ProjectMoved` push check to `ContainerMoved`, and use the
repository identifier to store messages in Redis instead of the project
ID.

Previously this only worked in a project context, but now supports all
repository containers and also adds the container type in the push
message.

To handle the transition from the previous message key to the new one,
we query and delete both keys in Redis.

Changelog: changed
parent a6b764b6
...@@ -7,6 +7,8 @@ class Wiki ...@@ -7,6 +7,8 @@ class Wiki
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
include GlobalID::Identification include GlobalID::Identification
extend ActiveModel::Naming
MARKUPS = { # rubocop:disable Style/MultilineIfModifier MARKUPS = { # rubocop:disable Style/MultilineIfModifier
'Markdown' => :markdown, 'Markdown' => :markdown,
'RDoc' => :rdoc, 'RDoc' => :rdoc,
......
...@@ -32,11 +32,11 @@ class PostReceiveService ...@@ -32,11 +32,11 @@ class PostReceiveService
response.add_alert_message(broadcast_message) response.add_alert_message(broadcast_message)
response.add_merge_request_urls(merge_request_urls) response.add_merge_request_urls(merge_request_urls)
# Neither User nor Project are guaranteed to be returned; an orphaned write deploy # Neither User nor Repository are guaranteed to be returned; an orphaned write deploy
# key could be used # key could be used
if user && project if user && repository
redirect_message = Gitlab::Checks::ProjectMoved.fetch_message(user.id, project.id) redirect_message = Gitlab::Checks::ContainerMoved.fetch_message(user, repository)
project_created_message = Gitlab::Checks::ProjectCreated.fetch_message(user.id, project.id) project_created_message = Gitlab::Checks::ProjectCreated.fetch_message(user, repository)
response.add_basic_message(redirect_message) response.add_basic_message(redirect_message)
response.add_basic_message(project_created_message) response.add_basic_message(project_created_message)
...@@ -94,6 +94,8 @@ class PostReceiveService ...@@ -94,6 +94,8 @@ class PostReceiveService
end end
def record_onboarding_progress def record_onboarding_progress
return unless project
OnboardingProgressService.new(project.namespace).execute(action: :git_write) OnboardingProgressService.new(project.namespace).execute(action: :git_write)
end end
end end
......
...@@ -20,9 +20,9 @@ module EE ...@@ -20,9 +20,9 @@ module EE
override :check_container! override :check_container!
def check_container! def check_container!
return check_group! if group?
super super
check_group! if group?
end end
override :check_push_access! override :check_push_access!
......
...@@ -8,7 +8,7 @@ module EE ...@@ -8,7 +8,7 @@ module EE
return super unless License.feature_available?(:project_aliases) return super unless License.feature_available?(:project_aliases)
if project_alias = ProjectAlias.find_by_name(project_path) if project_alias = ProjectAlias.find_by_name(project_path)
[project_alias.project, nil] project_alias.project
else else
super super
end end
......
...@@ -3,6 +3,19 @@ ...@@ -3,6 +3,19 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Gitlab::RepoPath do RSpec.describe Gitlab::RepoPath do
describe '.parse' do
let_it_be(:wiki) { create(:group_wiki) }
let_it_be(:redirect) { wiki.group.route.create_redirect('foo/bar/baz') }
it 'parses a group wiki repository path' do
expect(described_class.parse(wiki.repository.full_path)).to eq([wiki, nil, Gitlab::GlRepository::WIKI, nil])
end
it 'parses a redirected group wiki repository path' do
expect(described_class.parse("#{redirect.path}.wiki.git")).to eq([wiki, nil, Gitlab::GlRepository::WIKI, "#{redirect.path}.wiki"])
end
end
describe '.find_project' do describe '.find_project' do
let(:project) { create(:project) } let(:project) { create(:project) }
...@@ -11,7 +24,7 @@ RSpec.describe Gitlab::RepoPath do ...@@ -11,7 +24,7 @@ RSpec.describe Gitlab::RepoPath do
let(:project_alias) { create(:project_alias, project: project) } let(:project_alias) { create(:project_alias, project: project) }
it 'does not return a project' do it 'does not return a project' do
expect(described_class.find_project(project_alias.name)).to eq([nil, nil]) expect(described_class.find_project(project_alias.name)).to be_nil
end end
end end
end end
...@@ -25,20 +38,20 @@ RSpec.describe Gitlab::RepoPath do ...@@ -25,20 +38,20 @@ RSpec.describe Gitlab::RepoPath do
let(:project_alias) { create(:project_alias, project: project) } let(:project_alias) { create(:project_alias, project: project) }
it 'returns the project' do it 'returns the project' do
expect(described_class.find_project(project_alias.name)).to eq([project, nil]) expect(described_class.find_project(project_alias.name)).to eq(project)
end end
end end
context 'project_path does not match a project alias' do context 'project_path does not match a project alias' do
context 'project path matches project full path' do context 'project path matches project full path' do
it 'returns the project' do it 'returns the project' do
expect(described_class.find_project(project.full_path)).to eq([project, nil]) expect(described_class.find_project(project.full_path)).to eq(project)
end end
end end
context 'project path does not match an existing project full path' do context 'project path does not match an existing project full path' do
it 'returns nil' do it 'returns nil' do
expect(described_class.find_project('some-project')).to eq([nil, nil]) expect(described_class.find_project('some-project')).to be_nil
end end
end end
end end
......
...@@ -54,6 +54,16 @@ RSpec.describe Gitlab::GitAccessWiki do ...@@ -54,6 +54,16 @@ RSpec.describe Gitlab::GitAccessWiki do
expect { pull_changes(changes) }.not_to raise_error expect { pull_changes(changes) }.not_to raise_error
end end
end end
context 'when the group is renamed' do
let(:redirected_path) { 'some/other-path' }
it 'enqueues a redirected message for pushing' do
subject
expect(Gitlab::Checks::ContainerMoved.fetch_message(user, wiki.repository)).not_to be_nil
end
end
end end
context 'when user cannot :create_wiki' do context 'when user cannot :create_wiki' do
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
module Gitlab module Gitlab
module Checks module Checks
class ProjectMoved < PostPushMessage class ContainerMoved < PostPushMessage
REDIRECT_NAMESPACE = "redirect_namespace" REDIRECT_NAMESPACE = "redirect_namespace"
def initialize(repository, user, protocol, redirected_path) def initialize(repository, user, protocol, redirected_path)
...@@ -13,7 +13,7 @@ module Gitlab ...@@ -13,7 +13,7 @@ module Gitlab
def message def message
<<~MESSAGE <<~MESSAGE
Project '#{redirected_path}' was moved to '#{project.full_path}'. #{container.class.model_name.human} '#{redirected_path}' was moved to '#{container.full_path}'.
Please update your Git remote: Please update your Git remote:
...@@ -25,8 +25,16 @@ module Gitlab ...@@ -25,8 +25,16 @@ module Gitlab
attr_reader :redirected_path attr_reader :redirected_path
def self.message_key(user_id, project_id) def self.message_key(user, repository)
"#{REDIRECT_NAMESPACE}:#{user_id}:#{project_id}" "#{REDIRECT_NAMESPACE}:#{user.id}:#{repository.gl_repository}"
end
# TODO: Remove in the next release
# https://gitlab.com/gitlab-org/gitlab/-/issues/292030
def self.legacy_message_key(user, repository)
return unless repository.project
"#{REDIRECT_NAMESPACE}:#{user.id}:#{repository.project.id}"
end end
end end
end end
......
...@@ -9,21 +9,32 @@ module Gitlab ...@@ -9,21 +9,32 @@ module Gitlab
@protocol = protocol @protocol = protocol
end end
def self.fetch_message(user_id, project_id) def self.fetch_message(user, repository)
key = message_key(user_id, project_id) key = message_key(user, repository)
# Also check for messages in the legacy key
# TODO: Remove in the next release
# https://gitlab.com/gitlab-org/gitlab/-/issues/292030
legacy_key = legacy_message_key(user, repository) if respond_to?(:legacy_message_key)
Gitlab::Redis::SharedState.with do |redis| Gitlab::Redis::SharedState.with do |redis|
message = redis.get(key) message = redis.get(key)
redis.del(key) redis.del(key)
message
if legacy_key
legacy_message = redis.get(legacy_key)
redis.del(legacy_key)
end
legacy_message || message
end end
end end
def add_message def add_message
return unless user.present? && project.present? return unless user && repository
Gitlab::Redis::SharedState.with do |redis| Gitlab::Redis::SharedState.with do |redis|
key = self.class.message_key(user.id, project.id) key = self.class.message_key(user, repository)
redis.setex(key, 5.minutes, message) redis.setex(key, 5.minutes, message)
end end
end end
...@@ -39,7 +50,7 @@ module Gitlab ...@@ -39,7 +50,7 @@ module Gitlab
delegate :project, to: :repository, allow_nil: true delegate :project, to: :repository, allow_nil: true
delegate :container, to: :repository, allow_nil: false delegate :container, to: :repository, allow_nil: false
def self.message_key(user_id, project_id) def self.message_key(user, repository)
raise NotImplementedError raise NotImplementedError
end end
......
...@@ -21,8 +21,16 @@ module Gitlab ...@@ -21,8 +21,16 @@ module Gitlab
private private
def self.message_key(user_id, project_id) def self.message_key(user, repository)
"#{PROJECT_CREATED}:#{user_id}:#{project_id}" "#{PROJECT_CREATED}:#{user.id}:#{repository.gl_repository}"
end
# TODO: Remove in the next release
# https://gitlab.com/gitlab-org/gitlab/-/issues/292030
def self.legacy_message_key(user, repository)
return unless repository.project
"#{PROJECT_CREATED}:#{user.id}:#{repository.project.id}"
end end
def project_url def project_url
......
...@@ -9,7 +9,6 @@ module Gitlab ...@@ -9,7 +9,6 @@ module Gitlab
ForbiddenError = Class.new(StandardError) ForbiddenError = Class.new(StandardError)
NotFoundError = Class.new(StandardError) NotFoundError = Class.new(StandardError)
TimeoutError = Class.new(StandardError) TimeoutError = Class.new(StandardError)
ProjectMovedError = Class.new(NotFoundError)
# Use the magic string '_any' to indicate we do not know what the # Use the magic string '_any' to indicate we do not know what the
# changes are. This is also what gitlab-shell does. # changes are. This is also what gitlab-shell does.
...@@ -148,11 +147,11 @@ module Gitlab ...@@ -148,11 +147,11 @@ module Gitlab
raise NotFoundError, not_found_message if container.nil? raise NotFoundError, not_found_message if container.nil?
check_project! if project? check_project! if project?
add_container_moved_message!
end end
def check_project! def check_project!
check_project_accessibility! check_project_accessibility!
add_project_moved_message!
end end
def check_custom_action def check_custom_action
...@@ -221,12 +220,12 @@ module Gitlab ...@@ -221,12 +220,12 @@ module Gitlab
error_message(:project_not_found) error_message(:project_not_found)
end end
def add_project_moved_message! def add_container_moved_message!
return if redirected_path.nil? return if redirected_path.nil?
project_moved = Checks::ProjectMoved.new(repository, user, protocol, redirected_path) container_moved = Checks::ContainerMoved.new(repository, user, protocol, redirected_path)
project_moved.add_message container_moved.add_message
end end
def check_command_disabled! def check_command_disabled!
......
...@@ -13,7 +13,6 @@ module Gitlab ...@@ -13,7 +13,6 @@ module Gitlab
# @returns [HasRepository, Project, String, String] # @returns [HasRepository, Project, String, String]
def self.parse(path) def self.parse(path)
repo_path = path.delete_prefix('/').delete_suffix('.git') repo_path = path.delete_prefix('/').delete_suffix('.git')
redirected_path = nil
# Detect the repo type based on the path, the first one tried is the project # Detect the repo type based on the path, the first one tried is the project
# type, which does not have a suffix. # type, which does not have a suffix.
...@@ -26,9 +25,11 @@ module Gitlab ...@@ -26,9 +25,11 @@ module Gitlab
# Removing the suffix (.wiki, .design, ...) from the project path # Removing the suffix (.wiki, .design, ...) from the project path
full_path = repo_path.chomp(type.path_suffix) full_path = repo_path.chomp(type.path_suffix)
container, project, redirected_path = find_container(type, full_path) container, project = find_container(type, full_path)
next unless container
return [container, project, type, redirected_path] if container redirected_path = repo_path if redirected?(container, repo_path)
return [container, project, type, redirected_path]
end end
# When a project did not exist, the parsed repo_type would be empty. # When a project did not exist, the parsed repo_type would be empty.
...@@ -40,32 +41,28 @@ module Gitlab ...@@ -40,32 +41,28 @@ module Gitlab
# Returns an array containing: # Returns an array containing:
# - The repository container # - The repository container
# - The related project (if available) # - The related project (if available)
# - The original container path (if redirected)
# #
# @returns [HasRepository, Project, String] # @returns [HasRepository, Project, String]
def self.find_container(type, full_path) def self.find_container(type, full_path)
return [nil, nil, nil] if full_path.blank? return [nil, nil] if full_path.blank?
if type.snippet? if type.snippet?
snippet, redirected_path = find_snippet(full_path) snippet = find_snippet(full_path)
[snippet, snippet&.project, redirected_path] [snippet, snippet&.project]
elsif type.wiki? elsif type.wiki?
wiki, redirected_path = find_wiki(full_path) wiki = find_wiki(full_path)
[wiki, wiki.try(:project), redirected_path] [wiki, wiki.try(:project)]
else else
project, redirected_path = find_project(full_path) project = find_project(full_path)
[project, project, redirected_path] [project, project]
end end
end end
def self.find_project(project_path) def self.find_project(project_path)
project = Project.find_by_full_path(project_path, follow_redirects: true) Project.find_by_full_path(project_path, follow_redirects: true)
redirected_path = project_path if redirected?(project, project_path)
[project, redirected_path]
end end
def self.redirected?(container, container_path) def self.redirected?(container, container_path)
...@@ -77,11 +74,11 @@ module Gitlab ...@@ -77,11 +74,11 @@ module Gitlab
# - h5bp/html5-boilerplate/snippets/53 # - h5bp/html5-boilerplate/snippets/53
def self.find_snippet(snippet_path) def self.find_snippet(snippet_path)
snippet_id, project_path = extract_snippet_info(snippet_path) snippet_id, project_path = extract_snippet_info(snippet_path)
return [nil, nil] unless snippet_id return unless snippet_id
project, redirected_path = find_project(project_path) if project_path project = find_project(project_path) if project_path
[Snippet.find_by_id_and_project(id: snippet_id, project: project), redirected_path] Snippet.find_by_id_and_project(id: snippet_id, project: project)
end end
# Wiki path can be either: # Wiki path can be either:
...@@ -93,10 +90,9 @@ module Gitlab ...@@ -93,10 +90,9 @@ module Gitlab
# - group/subgroup # - group/subgroup
def self.find_wiki(container_path) def self.find_wiki(container_path)
container = Routable.find_by_full_path(container_path, follow_redirects: true) container = Routable.find_by_full_path(container_path, follow_redirects: true)
redirected_path = container_path if redirected?(container, container_path)
# In CE, Group#wiki is not available so this will return nil for a group path. # In CE, Group#wiki is not available so this will return nil for a group path.
[container&.try(:wiki), redirected_path] container&.try(:wiki)
end end
def self.extract_snippet_info(snippet_path) def self.extract_snippet_info(snippet_path)
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do RSpec.describe Gitlab::Checks::ContainerMoved, :clean_gitlab_redis_shared_state do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:project) { create(:project, :repository, :wiki_repo, namespace: user.namespace) } let_it_be(:project) { create(:project, :repository, :wiki_repo, namespace: user.namespace) }
...@@ -14,27 +14,48 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do ...@@ -14,27 +14,48 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
subject { described_class.new(repository, git_user, protocol, redirect_path) } subject { described_class.new(repository, git_user, protocol, redirect_path) }
describe '.fetch_message' do describe '.fetch_message' do
let(:key) { "redirect_namespace:#{user.id}:#{project.repository.gl_repository}" }
let(:legacy_key) { "redirect_namespace:#{user.id}:#{project.id}" }
context 'with a redirect message queue' do context 'with a redirect message queue' do
before do before do
subject.add_message subject.add_message
end end
it 'returns the redirect message' do it 'returns the redirect message' do
expect(described_class.fetch_message(user.id, project.id)).to eq(subject.message) expect(described_class.fetch_message(user, project.repository)).to eq(subject.message)
end end
it 'deletes the redirect message from redis' do it 'deletes the redirect message from redis' do
expect(Gitlab::Redis::SharedState.with { |redis| redis.get("redirect_namespace:#{user.id}:#{project.id}") }).not_to be_nil expect(Gitlab::Redis::SharedState.with { |redis| redis.get(key) }).not_to be_nil
described_class.fetch_message(user.id, project.id) described_class.fetch_message(user, project.repository)
expect(Gitlab::Redis::SharedState.with { |redis| redis.get("redirect_namespace:#{user.id}:#{project.id}") }).to be_nil expect(Gitlab::Redis::SharedState.with { |redis| redis.get(key) }).to be_nil
end
context 'with a message in the legacy key' do
before do
Gitlab::Redis::SharedState.with do |redis|
redis.set(legacy_key, 'legacy message')
end
end
it 'returns and deletes the legacy message' do
expect(Gitlab::Redis::SharedState.with { |redis| redis.get(key) }).not_to be_nil
expect(Gitlab::Redis::SharedState.with { |redis| redis.get(legacy_key) }).not_to be_nil
expect(described_class.fetch_message(user, project.repository)).to eq('legacy message')
expect(Gitlab::Redis::SharedState.with { |redis| redis.get(key) }).to be_nil
expect(Gitlab::Redis::SharedState.with { |redis| redis.get(legacy_key) }).to be_nil
end
end end
end end
context 'with no redirect message queue' do context 'with no redirect message queue' do
it 'returns nil' do it 'returns nil' do
expect(described_class.fetch_message(1, 2)).to be_nil expect(described_class.fetch_message(user, project.repository)).to be_nil
end end
end end
end end
...@@ -58,7 +79,7 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do ...@@ -58,7 +79,7 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
shared_examples 'returns redirect message' do shared_examples 'returns redirect message' do
it do it do
message = <<~MSG message = <<~MSG
Project '#{redirect_path}' was moved to '#{project.full_path}'. #{container_label} '#{redirect_path}' was moved to '#{repository.container.full_path}'.
Please update your Git remote: Please update your Git remote:
...@@ -86,6 +107,7 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do ...@@ -86,6 +107,7 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
context 'with project' do context 'with project' do
it_behaves_like 'errors per protocol' do it_behaves_like 'errors per protocol' do
let(:container_label) { 'Project' }
let(:http_url_to_repo) { project.http_url_to_repo } let(:http_url_to_repo) { project.http_url_to_repo }
let(:ssh_url_to_repo) { project.ssh_url_to_repo } let(:ssh_url_to_repo) { project.ssh_url_to_repo }
end end
...@@ -95,6 +117,7 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do ...@@ -95,6 +117,7 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
let(:repository) { project.wiki.repository } let(:repository) { project.wiki.repository }
it_behaves_like 'errors per protocol' do it_behaves_like 'errors per protocol' do
let(:container_label) { 'Project wiki' }
let(:http_url_to_repo) { project.wiki.http_url_to_repo } let(:http_url_to_repo) { project.wiki.http_url_to_repo }
let(:ssh_url_to_repo) { project.wiki.ssh_url_to_repo } let(:ssh_url_to_repo) { project.wiki.ssh_url_to_repo }
end end
...@@ -106,6 +129,7 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do ...@@ -106,6 +129,7 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
let(:repository) { snippet.repository } let(:repository) { snippet.repository }
it_behaves_like 'errors per protocol' do it_behaves_like 'errors per protocol' do
let(:container_label) { 'Project snippet' }
let(:http_url_to_repo) { snippet.http_url_to_repo } let(:http_url_to_repo) { snippet.http_url_to_repo }
let(:ssh_url_to_repo) { snippet.ssh_url_to_repo } let(:ssh_url_to_repo) { snippet.ssh_url_to_repo }
end end
...@@ -116,8 +140,10 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do ...@@ -116,8 +140,10 @@ RSpec.describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
let(:repository) { snippet.repository } let(:repository) { snippet.repository }
it 'returns nil' do it_behaves_like 'errors per protocol' do
expect(subject.add_message).to be_nil let(:container_label) { 'Personal snippet' }
let(:http_url_to_repo) { snippet.http_url_to_repo }
let(:ssh_url_to_repo) { snippet.ssh_url_to_repo }
end end
end end
end end
......
...@@ -13,27 +13,48 @@ RSpec.describe Gitlab::Checks::ProjectCreated, :clean_gitlab_redis_shared_state ...@@ -13,27 +13,48 @@ RSpec.describe Gitlab::Checks::ProjectCreated, :clean_gitlab_redis_shared_state
subject { described_class.new(repository, git_user, 'http') } subject { described_class.new(repository, git_user, 'http') }
describe '.fetch_message' do describe '.fetch_message' do
let(:key) { "project_created:#{user.id}:#{project.repository.gl_repository}" }
let(:legacy_key) { "project_created:#{user.id}:#{project.id}" }
context 'with a project created message queue' do context 'with a project created message queue' do
before do before do
subject.add_message subject.add_message
end end
it 'returns project created message' do it 'returns project created message' do
expect(described_class.fetch_message(user.id, project.id)).to eq(subject.message) expect(described_class.fetch_message(user, project.repository)).to eq(subject.message)
end end
it 'deletes the project created message from redis' do it 'deletes the project created message from redis' do
expect(Gitlab::Redis::SharedState.with { |redis| redis.get("project_created:#{user.id}:#{project.id}") }).not_to be_nil expect(Gitlab::Redis::SharedState.with { |redis| redis.get(key) }).not_to be_nil
described_class.fetch_message(user, project.repository)
expect(Gitlab::Redis::SharedState.with { |redis| redis.get(key) }).to be_nil
end
context 'with a message in the legacy key' do
before do
Gitlab::Redis::SharedState.with do |redis|
redis.set(legacy_key, 'legacy message')
end
end
it 'returns and deletes the legacy message' do
expect(Gitlab::Redis::SharedState.with { |redis| redis.get(key) }).not_to be_nil
expect(Gitlab::Redis::SharedState.with { |redis| redis.get(legacy_key) }).not_to be_nil
described_class.fetch_message(user.id, project.id) expect(described_class.fetch_message(user, project.repository)).to eq('legacy message')
expect(Gitlab::Redis::SharedState.with { |redis| redis.get("project_created:#{user.id}:#{project.id}") }).to be_nil expect(Gitlab::Redis::SharedState.with { |redis| redis.get(key) }).to be_nil
expect(Gitlab::Redis::SharedState.with { |redis| redis.get(legacy_key) }).to be_nil
end
end end
end end
context 'with no project created message queue' do context 'with no project created message queue' do
it 'returns nil' do it 'returns nil' do
expect(described_class.fetch_message(1, 2)).to be_nil expect(described_class.fetch_message(user, project.repository)).to be_nil
end end
end end
end end
......
...@@ -265,7 +265,7 @@ RSpec.describe Gitlab::GitAccess do ...@@ -265,7 +265,7 @@ RSpec.describe Gitlab::GitAccess do
it 'enqueues a redirected message for pushing' do it 'enqueues a redirected message for pushing' do
push_access_check push_access_check
expect(Gitlab::Checks::ProjectMoved.fetch_message(user.id, project.id)).not_to be_nil expect(Gitlab::Checks::ContainerMoved.fetch_message(user, project.repository)).not_to be_nil
end end
it 'allows push and pull access' do it 'allows push and pull access' do
......
...@@ -13,11 +13,11 @@ RSpec.describe ::Gitlab::RepoPath do ...@@ -13,11 +13,11 @@ RSpec.describe ::Gitlab::RepoPath do
describe '.parse' do describe '.parse' do
context 'a repository storage path' do context 'a repository storage path' do
it 'parses a full repository project path' do it 'parses a full project repository path' do
expect(described_class.parse(project.repository.full_path)).to eq([project, project, Gitlab::GlRepository::PROJECT, nil]) expect(described_class.parse(project.repository.full_path)).to eq([project, project, Gitlab::GlRepository::PROJECT, nil])
end end
it 'parses a full wiki project path' do it 'parses a full project wiki repository path' do
expect(described_class.parse(project.wiki.repository.full_path)).to eq([project.wiki, project, Gitlab::GlRepository::WIKI, nil]) expect(described_class.parse(project.wiki.repository.full_path)).to eq([project.wiki, project, Gitlab::GlRepository::WIKI, nil])
end end
...@@ -49,7 +49,7 @@ RSpec.describe ::Gitlab::RepoPath do ...@@ -49,7 +49,7 @@ RSpec.describe ::Gitlab::RepoPath do
end end
it 'parses a relative wiki path' do it 'parses a relative wiki path' do
expect(described_class.parse(redirect.path + '.wiki.git')).to eq([project.wiki, project, Gitlab::GlRepository::WIKI, redirect_route]) expect(described_class.parse(redirect.path + '.wiki.git')).to eq([project.wiki, project, Gitlab::GlRepository::WIKI, "#{redirect_route}.wiki"])
end end
it 'parses a relative path starting with /' do it 'parses a relative path starting with /' do
...@@ -57,7 +57,7 @@ RSpec.describe ::Gitlab::RepoPath do ...@@ -57,7 +57,7 @@ RSpec.describe ::Gitlab::RepoPath do
end end
it 'parses a redirected project snippet repository path' do it 'parses a redirected project snippet repository path' do
expect(described_class.parse(redirect.path + "/snippets/#{project_snippet.id}.git")).to eq([project_snippet, project, Gitlab::GlRepository::SNIPPET, redirect_route]) expect(described_class.parse(redirect.path + "/snippets/#{project_snippet.id}.git")).to eq([project_snippet, project, Gitlab::GlRepository::SNIPPET, "#{redirect_route}/snippets/#{project_snippet.id}"])
end end
end end
end end
...@@ -70,8 +70,8 @@ RSpec.describe ::Gitlab::RepoPath do ...@@ -70,8 +70,8 @@ RSpec.describe ::Gitlab::RepoPath do
describe '.find_project' do describe '.find_project' do
context 'when finding a project by its canonical path' do context 'when finding a project by its canonical path' do
context 'when the cases match' do context 'when the cases match' do
it 'returns the project and nil' do it 'returns the project' do
expect(described_class.find_project(project.full_path)).to eq([project, nil]) expect(described_class.find_project(project.full_path)).to eq(project)
end end
end end
...@@ -80,45 +80,45 @@ RSpec.describe ::Gitlab::RepoPath do ...@@ -80,45 +80,45 @@ RSpec.describe ::Gitlab::RepoPath do
# easy and safe to redirect someone to the correctly-cased URL. For git # easy and safe to redirect someone to the correctly-cased URL. For git
# requests, we should accept wrongly-cased URLs because it is a pain to # requests, we should accept wrongly-cased URLs because it is a pain to
# block people's git operations and force them to update remote URLs. # block people's git operations and force them to update remote URLs.
it 'returns the project and nil' do it 'returns the project' do
expect(described_class.find_project(project.full_path.upcase)).to eq([project, nil]) expect(described_class.find_project(project.full_path.upcase)).to eq(project)
end end
end end
end end
context 'when finding a project via a redirect' do context 'when finding a project via a redirect' do
it 'returns the project and nil' do it 'returns the project' do
expect(described_class.find_project(redirect.path)).to eq([project, redirect.path]) expect(described_class.find_project(redirect.path)).to eq(project)
end end
end end
end end
describe '.find_snippet' do describe '.find_snippet' do
it 'extracts path and id from personal snippet route' do it 'extracts path and id from personal snippet route' do
expect(described_class.find_snippet("snippets/#{personal_snippet.id}")).to eq([personal_snippet, nil]) expect(described_class.find_snippet("snippets/#{personal_snippet.id}")).to eq(personal_snippet)
end end
it 'extracts path and id from project snippet route' do it 'extracts path and id from project snippet route' do
expect(described_class.find_snippet("#{project.full_path}/snippets/#{project_snippet.id}")).to eq([project_snippet, nil]) expect(described_class.find_snippet("#{project.full_path}/snippets/#{project_snippet.id}")).to eq(project_snippet)
end end
it 'returns nil for invalid snippet paths' do it 'returns nil for invalid snippet paths' do
aggregate_failures do aggregate_failures do
expect(described_class.find_snippet("snippets/#{project_snippet.id}")).to eq([nil, nil]) expect(described_class.find_snippet("snippets/#{project_snippet.id}")).to be_nil
expect(described_class.find_snippet("#{project.full_path}/snippets/#{personal_snippet.id}")).to eq([nil, nil]) expect(described_class.find_snippet("#{project.full_path}/snippets/#{personal_snippet.id}")).to be_nil
expect(described_class.find_snippet('')).to eq([nil, nil]) expect(described_class.find_snippet('')).to be_nil
end end
end end
it 'returns nil for snippets not associated with the project' do it 'returns nil for snippets not associated with the project' do
snippet = create(:project_snippet) snippet = create(:project_snippet)
expect(described_class.find_snippet("#{project.full_path}/snippets/#{snippet.id}")).to eq([nil, nil]) expect(described_class.find_snippet("#{project.full_path}/snippets/#{snippet.id}")).to be_nil
end end
context 'when finding a project snippet via a redirect' do context 'when finding a project snippet via a redirect' do
it 'returns the project and true' do it 'returns the project snippet' do
expect(described_class.find_snippet("#{redirect.path}/snippets/#{project_snippet.id}")).to eq([project_snippet, redirect.path]) expect(described_class.find_snippet("#{redirect.path}/snippets/#{project_snippet.id}")).to eq(project_snippet)
end end
end end
end end
......
...@@ -1176,59 +1176,68 @@ RSpec.describe API::Internal::Base do ...@@ -1176,59 +1176,68 @@ RSpec.describe API::Internal::Base do
allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(user) allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(user)
end end
context 'with Project' do shared_examples 'runs post-receive hooks' do
it 'executes PostReceiveService' do let(:gl_repository) { container.repository.gl_repository }
message = <<~MESSAGE.strip let(:messages) { [] }
To create a merge request for #{branch_name}, visit:
http://#{Gitlab.config.gitlab.host}/#{project.full_path}/-/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name}
MESSAGE
it 'executes PostReceiveService' do
subject subject
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to eq({ expect(json_response).to eq({
'messages' => [{ 'message' => message, 'type' => 'basic' }], 'messages' => messages,
'reference_counter_decreased' => true 'reference_counter_decreased' => true
}) })
end end
it 'tries to notify that the container has moved' do
expect(Gitlab::Checks::ContainerMoved).to receive(:fetch_message).with(user, container.repository)
subject
end
it_behaves_like 'storing arguments in the application context' do it_behaves_like 'storing arguments in the application context' do
let(:expected_params) { { user: user.username, project: project.full_path } } let(:expected_params) { expected_context }
end end
end end
context 'with PersonalSnippet' do context 'with Project' do
let(:gl_repository) { "snippet-#{personal_snippet.id}" } it_behaves_like 'runs post-receive hooks' do
let(:container) { project }
it 'executes PostReceiveService' do let(:expected_context) { { user: user.username, project: project.full_path } }
subject
expect(json_response).to eq({ let(:messages) do
'messages' => [], [
'reference_counter_decreased' => true {
}) 'message' => <<~MESSAGE.strip,
To create a merge request for #{branch_name}, visit:
http://#{Gitlab.config.gitlab.host}/#{project.full_path}/-/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name}
MESSAGE
'type' => 'basic'
}
]
end
end end
end
it_behaves_like 'storing arguments in the application context' do context 'with PersonalSnippet' do
let(:expected_params) { { user: key.user.username } } it_behaves_like 'runs post-receive hooks' do
let(:gl_repository) { "snippet-#{personal_snippet.id}" } let(:container) { personal_snippet }
let(:expected_context) { { user: key.user.username } }
end end
end end
context 'with ProjectSnippet' do context 'with ProjectSnippet' do
let(:gl_repository) { "snippet-#{project_snippet.id}" } it_behaves_like 'runs post-receive hooks' do
let(:container) { project_snippet }
it 'executes PostReceiveService' do let(:expected_context) { { user: key.user.username, project: project_snippet.project.full_path } }
subject
expect(json_response).to eq({
'messages' => [],
'reference_counter_decreased' => true
})
end end
end
it_behaves_like 'storing arguments in the application context' do context 'with ProjectWiki' do
let(:expected_params) { { user: key.user.username, project: project_snippet.project.full_path } } it_behaves_like 'runs post-receive hooks' do
let(:gl_repository) { "snippet-#{project_snippet.id}" } let(:container) { project.wiki }
let(:expected_context) { { user: key.user.username, project: project.full_path } }
end end
end end
...@@ -1236,7 +1245,7 @@ RSpec.describe API::Internal::Base do ...@@ -1236,7 +1245,7 @@ RSpec.describe API::Internal::Base do
it 'does not try to notify that project moved' do it 'does not try to notify that project moved' do
allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(nil) allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(nil)
expect(Gitlab::Checks::ProjectMoved).not_to receive(:fetch_message) expect(Gitlab::Checks::ContainerMoved).not_to receive(:fetch_message)
subject subject
...@@ -1244,33 +1253,17 @@ RSpec.describe API::Internal::Base do ...@@ -1244,33 +1253,17 @@ RSpec.describe API::Internal::Base do
end end
end end
context 'when project is nil' do context 'when container is nil' do
context 'with Project' do let(:gl_repository) { 'project-foo' }
let(:gl_repository) { 'project-foo' }
it 'does not try to notify that project moved' do
allow(Gitlab::GlRepository).to receive(:parse).and_return([nil, nil, Gitlab::GlRepository::PROJECT])
expect(Gitlab::Checks::ProjectMoved).not_to receive(:fetch_message)
subject
expect(response).to have_gitlab_http_status(:ok)
end
end
context 'with PersonalSnippet' do
let(:gl_repository) { "snippet-#{personal_snippet.id}" }
it 'does not try to notify that project moved' do it 'does not try to notify that project moved' do
allow(Gitlab::GlRepository).to receive(:parse).and_return([personal_snippet, nil, Gitlab::GlRepository::SNIPPET]) allow(Gitlab::GlRepository).to receive(:parse).and_return([nil, nil, Gitlab::GlRepository::PROJECT])
expect(Gitlab::Checks::ProjectMoved).not_to receive(:fetch_message) expect(Gitlab::Checks::ContainerMoved).not_to receive(:fetch_message)
subject subject
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
end
end end
end end
end end
......
...@@ -283,7 +283,7 @@ RSpec.describe PostReceiveService do ...@@ -283,7 +283,7 @@ RSpec.describe PostReceiveService do
context 'with a redirected data' do context 'with a redirected data' do
it 'returns redirected message on the response' do it 'returns redirected message on the response' do
project_moved = Gitlab::Checks::ProjectMoved.new(project.repository, user, 'http', 'foo/baz') project_moved = Gitlab::Checks::ContainerMoved.new(project.repository, user, 'http', 'foo/baz')
project_moved.add_message project_moved.add_message
expect(subject).to include(build_basic_message(project_moved.message)) expect(subject).to include(build_basic_message(project_moved.message))
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment