Commit b3fb82a9 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'bvl-no-permanent-redirect' into 'master'

Don't create permanent redirect when renaming a namespace

See merge request gitlab-org/gitlab-ce!17521
parents 11048808 db75057c
...@@ -17,32 +17,4 @@ class RedirectRoute < ActiveRecord::Base ...@@ -17,32 +17,4 @@ class RedirectRoute < ActiveRecord::Base
where(wheres, path, "#{sanitize_sql_like(path)}/%") where(wheres, path, "#{sanitize_sql_like(path)}/%")
end end
scope :permanent, -> do
if column_permanent_exists?
where(permanent: true)
else
none
end
end
scope :temporary, -> do
if column_permanent_exists?
where(permanent: [false, nil])
else
all
end
end
default_value_for :permanent, false
def permanent=(value)
if self.class.column_permanent_exists?
super
end
end
def self.column_permanent_exists?
ActiveRecord::Base.connection.column_exists?(:redirect_routes, :permanent)
end
end end
...@@ -10,8 +10,6 @@ class Route < ActiveRecord::Base ...@@ -10,8 +10,6 @@ class Route < ActiveRecord::Base
presence: true, presence: true,
uniqueness: { case_sensitive: false } uniqueness: { case_sensitive: false }
validate :ensure_permanent_paths, if: :path_changed?
before_validation :delete_conflicting_orphaned_routes before_validation :delete_conflicting_orphaned_routes
after_create :delete_conflicting_redirects after_create :delete_conflicting_redirects
after_update :delete_conflicting_redirects, if: :path_changed? after_update :delete_conflicting_redirects, if: :path_changed?
...@@ -45,7 +43,7 @@ class Route < ActiveRecord::Base ...@@ -45,7 +43,7 @@ class Route < ActiveRecord::Base
# We are not calling route.delete_conflicting_redirects here, in hopes # We are not calling route.delete_conflicting_redirects here, in hopes
# of avoiding deadlocks. The parent (self, in this method) already # of avoiding deadlocks. The parent (self, in this method) already
# called it, which deletes conflicts for all descendants. # called it, which deletes conflicts for all descendants.
route.create_redirect(old_path, permanent: permanent_redirect?) if attributes[:path] route.create_redirect(old_path) if attributes[:path]
end end
end end
end end
...@@ -55,31 +53,17 @@ class Route < ActiveRecord::Base ...@@ -55,31 +53,17 @@ class Route < ActiveRecord::Base
end end
def conflicting_redirects def conflicting_redirects
RedirectRoute.temporary.matching_path_and_descendants(path) RedirectRoute.matching_path_and_descendants(path)
end end
def create_redirect(path, permanent: false) def create_redirect(path)
RedirectRoute.create(source: source, path: path, permanent: permanent) RedirectRoute.create(source: source, path: path)
end end
private private
def create_redirect_for_old_path def create_redirect_for_old_path
create_redirect(path_was, permanent: permanent_redirect?) if path_changed? create_redirect(path_was) if path_changed?
end
def permanent_redirect?
source_type != "Project"
end
def ensure_permanent_paths
return if path.nil?
errors.add(:path, "has been taken before") if conflicting_redirect_exists?
end
def conflicting_redirect_exists?
RedirectRoute.permanent.matching_path_and_descendants(path).exists?
end end
def delete_conflicting_orphaned_routes def delete_conflicting_orphaned_routes
......
---
title: Don't create permanent redirect routes
merge_request: 17521
author:
type: changed
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class RemovePermanentFromRedirectRoutes < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
INDEX_NAME_PERM = "index_redirect_routes_on_path_text_pattern_ops_where_permanent"
INDEX_NAME_TEMP = "index_redirect_routes_on_path_text_pattern_ops_where_temporary"
def up
# These indexes were created on Postgres only in:
# ReworkRedirectRoutesIndexes:
# https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/16211
if Gitlab::Database.postgresql?
disable_statement_timeout
execute "DROP INDEX CONCURRENTLY IF EXISTS #{INDEX_NAME_PERM};"
execute "DROP INDEX CONCURRENTLY IF EXISTS #{INDEX_NAME_TEMP};"
end
remove_column(:redirect_routes, :permanent)
end
def down
add_column(:redirect_routes, :permanent, :boolean)
if Gitlab::Database.postgresql?
disable_statement_timeout
execute("CREATE INDEX CONCURRENTLY #{INDEX_NAME_PERM} ON redirect_routes (lower(path) varchar_pattern_ops) where (permanent);")
execute("CREATE INDEX CONCURRENTLY #{INDEX_NAME_TEMP} ON redirect_routes (lower(path) varchar_pattern_ops) where (not permanent or permanent is null) ;")
end
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddPathIndexToRedirectRoutes < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
disable_ddl_transaction!
INDEX_NAME = 'index_redirect_routes_on_path_unique_text_pattern_ops'
# Indexing on LOWER(path) varchar_pattern_ops speeds up the LIKE query in
# RedirectRoute.matching_path_and_descendants
#
# This same index is also added in the `ReworkRedirectRoutesIndexes` so this
# is a no-op in most cases. But this migration is also called from the
# `setup_postgresql.rake` task when setting up a new database, in which case
# we want to create the index.
def up
return unless Gitlab::Database.postgresql?
disable_statement_timeout
unless index_exists_by_name?(:redirect_routes, INDEX_NAME)
execute("CREATE UNIQUE INDEX CONCURRENTLY #{INDEX_NAME} ON redirect_routes (lower(path) varchar_pattern_ops);")
end
end
def down
# Do nothing in the DOWN. Since the index above is originally created in the
# `ReworkRedirectRoutesIndexes`. This migration wouldn't have actually
# created any new index.
#
# This migration is only here to be called form `setup_postgresql.rake` so
# any newly created database would have this index.
end
end
...@@ -1605,7 +1605,6 @@ ActiveRecord::Schema.define(version: 20180327101207) do ...@@ -1605,7 +1605,6 @@ ActiveRecord::Schema.define(version: 20180327101207) do
t.string "path", null: false t.string "path", null: false
t.datetime "created_at", null: false t.datetime "created_at", null: false
t.datetime "updated_at", null: false t.datetime "updated_at", null: false
t.boolean "permanent"
end end
add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path", unique: true, using: :btree add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path", unique: true, using: :btree
......
...@@ -128,11 +128,9 @@ and Git push/pull redirects. ...@@ -128,11 +128,9 @@ and Git push/pull redirects.
Depending on the situation, different things apply. Depending on the situation, different things apply.
When [renaming a user](../profile/index.md#changing-your-username) or When [renaming a user](../profile/index.md#changing-your-username),
[changing a group path](../group/index.md#changing-a-group-s-path): [changing a group path](../group/index.md#changing-a-group-s-path) or [renaming a repository](settings/index.md#renaming-a-repository):
- **The redirect to the new URL is permanent**, which means that the original
namespace can't be claimed again by any group or user.
- Existing web URLs for the namespace and anything under it (e.g., projects) will - Existing web URLs for the namespace and anything under it (e.g., projects) will
redirect to the new URLs. redirect to the new URLs.
- Starting with GitLab 10.3, existing Git remote URLs for projects under the - Starting with GitLab 10.3, existing Git remote URLs for projects under the
...@@ -141,9 +139,5 @@ When [renaming a user](../profile/index.md#changing-your-username) or ...@@ -141,9 +139,5 @@ When [renaming a user](../profile/index.md#changing-your-username) or
your remote will be displayed instead of rejecting your action. your remote will be displayed instead of rejecting your action.
This means that any automation scripts, or Git clients will continue to This means that any automation scripts, or Git clients will continue to
work after a rename, making any transition a lot smoother. work after a rename, making any transition a lot smoother.
To avoid pulling from or pushing to an entirely incorrect repository, the old - The redirects will be available as long as the original path is not claimed by
path will be reserved. another group, user or project.
When [renaming-a-repository](settings/index.md#renaming-a-repository), the same
things apply, except for the Git push/pull actions which will be rejected with a
warning message to change to the new remote URL.
...@@ -9,20 +9,16 @@ module Gitlab ...@@ -9,20 +9,16 @@ module Gitlab
super(project, user, protocol) super(project, user, protocol)
end end
def message(rejected: false) def message
<<~MESSAGE <<~MESSAGE
Project '#{redirected_path}' was moved to '#{project.full_path}'. Project '#{redirected_path}' was moved to '#{project.full_path}'.
Please update your Git remote: Please update your Git remote:
#{remote_url_message(rejected)} git remote set-url origin #{url_to_repo}
MESSAGE MESSAGE
end end
def permanent_redirect?
RedirectRoute.permanent.exists?(path: redirected_path)
end
private private
attr_reader :redirected_path attr_reader :redirected_path
...@@ -30,18 +26,6 @@ module Gitlab ...@@ -30,18 +26,6 @@ module Gitlab
def self.message_key(user_id, project_id) def self.message_key(user_id, project_id)
"#{REDIRECT_NAMESPACE}:#{user_id}:#{project_id}" "#{REDIRECT_NAMESPACE}:#{user_id}:#{project_id}"
end end
def remote_url_message(rejected)
if rejected
"git remote set-url origin #{url_to_repo} and try again."
else
"git remote set-url origin #{url_to_repo}"
end
end
def url
protocol == 'ssh' ? project.ssh_url_to_repo : project.http_url_to_repo
end
end end
end end
end end
...@@ -900,11 +900,42 @@ into similar problems in the future (e.g. when new tables are created). ...@@ -900,11 +900,42 @@ into similar problems in the future (e.g. when new tables are created).
end end
end end
# Rails' index_exists? doesn't work when you only give it a table and index # Fetches indexes on a column by name for postgres.
# name. As such we have to use some extra code to check if an index exists for #
# a given name. # This will include indexes using an expression on the column, for example:
# `CREATE INDEX CONCURRENTLY index_name ON table (LOWER(column));`
#
# For mysql, it falls back to the default ActiveRecord implementation that
# will not find custom indexes. But it will select by name without passing
# a column.
#
# We can remove this when upgrading to Rails 5 with an updated `index_exists?`:
# - https://github.com/rails/rails/commit/edc2b7718725016e988089b5fb6d6fb9d6e16882
#
# Or this can be removed when we no longer support postgres < 9.5, so we
# can use `CREATE INDEX IF NOT EXISTS`.
def index_exists_by_name?(table, index) def index_exists_by_name?(table, index)
indexes(table).map(&:name).include?(index) # We can't fall back to the normal `index_exists?` method because that
# does not find indexes without passing a column name.
if indexes(table).map(&:name).include?(index.to_s)
true
elsif Gitlab::Database.postgresql?
postgres_exists_by_name?(table, index)
else
false
end
end
def postgres_exists_by_name?(table, name)
index_sql = <<~SQL
SELECT COUNT(*)
FROM pg_index
JOIN pg_class i ON (indexrelid=i.oid)
JOIN pg_class t ON (indrelid=t.oid)
WHERE i.relname = '#{name}' AND t.relname = '#{table}'
SQL
connection.select_value(index_sql).to_i > 0
end end
end end
end end
......
...@@ -53,7 +53,7 @@ module Gitlab ...@@ -53,7 +53,7 @@ module Gitlab
ensure_project_on_push!(cmd, changes) ensure_project_on_push!(cmd, changes)
check_project_accessibility! check_project_accessibility!
check_project_moved! add_project_moved_message!
check_repository_existence! check_repository_existence!
case cmd case cmd
...@@ -125,16 +125,12 @@ module Gitlab ...@@ -125,16 +125,12 @@ module Gitlab
end end
end end
def check_project_moved! def add_project_moved_message!
return if redirected_path.nil? return if redirected_path.nil?
project_moved = Checks::ProjectMoved.new(project, user, protocol, redirected_path) project_moved = Checks::ProjectMoved.new(project, user, protocol, redirected_path)
if project_moved.permanent_redirect?
project_moved.add_message project_moved.add_message
else
raise ProjectMovedError, project_moved.message(rejected: true)
end
end end
def check_command_disabled!(cmd) def check_command_disabled!(cmd)
......
...@@ -7,8 +7,8 @@ task setup_postgresql: :environment do ...@@ -7,8 +7,8 @@ task setup_postgresql: :environment do
require Rails.root.join('db/migrate/20170724214302_add_lower_path_index_to_redirect_routes') require Rails.root.join('db/migrate/20170724214302_add_lower_path_index_to_redirect_routes')
require Rails.root.join('db/migrate/20170503185032_index_redirect_routes_path_for_like') require Rails.root.join('db/migrate/20170503185032_index_redirect_routes_path_for_like')
require Rails.root.join('db/migrate/20171220191323_add_index_on_namespaces_lower_name.rb') require Rails.root.join('db/migrate/20171220191323_add_index_on_namespaces_lower_name.rb')
require Rails.root.join('db/migrate/20180113220114_rework_redirect_routes_indexes.rb')
require Rails.root.join('db/migrate/20180215181245_users_name_lower_index.rb') require Rails.root.join('db/migrate/20180215181245_users_name_lower_index.rb')
require Rails.root.join('db/post_migrate/20180306164012_add_path_index_to_redirect_routes.rb')
NamespacesProjectsPathLowerIndexes.new.up NamespacesProjectsPathLowerIndexes.new.up
AddUsersLowerUsernameEmailIndexes.new.up AddUsersLowerUsernameEmailIndexes.new.up
...@@ -17,6 +17,6 @@ task setup_postgresql: :environment do ...@@ -17,6 +17,6 @@ task setup_postgresql: :environment do
AddLowerPathIndexToRedirectRoutes.new.up AddLowerPathIndexToRedirectRoutes.new.up
IndexRedirectRoutesPathForLike.new.up IndexRedirectRoutesPathForLike.new.up
AddIndexOnNamespacesLowerName.new.up AddIndexOnNamespacesLowerName.new.up
ReworkRedirectRoutesIndexes.new.up
UsersNameLowerIndex.new.up UsersNameLowerIndex.new.up
AddPathIndexToRedirectRoutes.new.up
end end
...@@ -2,14 +2,5 @@ FactoryBot.define do ...@@ -2,14 +2,5 @@ FactoryBot.define do
factory :redirect_route do factory :redirect_route do
sequence(:path) { |n| "redirect#{n}" } sequence(:path) { |n| "redirect#{n}" }
source factory: :group source factory: :group
permanent false
trait :permanent do
permanent true
end
trait :temporary do
permanent false
end
end end
end end
...@@ -44,44 +44,17 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do ...@@ -44,44 +44,17 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
end end
describe '#message' do describe '#message' do
context 'when the push is rejected' do
it 'returns a redirect message telling the user to try again' do
project_moved = described_class.new(project, user, 'http', 'foo/bar')
message = "Project 'foo/bar' was moved to '#{project.full_path}'." +
"\n\nPlease update your Git remote:" +
"\n\n git remote set-url origin #{project.http_url_to_repo} and try again.\n"
expect(project_moved.message(rejected: true)).to eq(message)
end
end
context 'when the push is not rejected' do
it 'returns a redirect message' do it 'returns a redirect message' do
project_moved = described_class.new(project, user, 'http', 'foo/bar') project_moved = described_class.new(project, user, 'http', 'foo/bar')
message = "Project 'foo/bar' was moved to '#{project.full_path}'." + message = <<~MSG
"\n\nPlease update your Git remote:" + Project 'foo/bar' was moved to '#{project.full_path}'.
"\n\n git remote set-url origin #{project.http_url_to_repo}\n"
expect(project_moved.message).to eq(message) Please update your Git remote:
end
end
end
describe '#permanent_redirect?' do git remote set-url origin #{project.http_url_to_repo}
context 'with a permanent RedirectRoute' do MSG
it 'returns true' do
project.route.create_redirect('foo/bar', permanent: true)
project_moved = described_class.new(project, user, 'http', 'foo/bar')
expect(project_moved.permanent_redirect?).to be_truthy
end
end
context 'without a permanent RedirectRoute' do expect(project_moved.message).to eq(message)
it 'returns false' do
project.route.create_redirect('foo/bar')
project_moved = described_class.new(project, user, 'http', 'foo/bar')
expect(project_moved.permanent_redirect?).to be_falsy
end
end end
end end
end end
...@@ -1211,4 +1211,33 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -1211,4 +1211,33 @@ describe Gitlab::Database::MigrationHelpers do
expect(model.perform_background_migration_inline?).to eq(false) expect(model.perform_background_migration_inline?).to eq(false)
end end
end end
describe '#index_exists_by_name?' do
it 'returns true if an index exists' do
expect(model.index_exists_by_name?(:projects, 'index_projects_on_path'))
.to be_truthy
end
it 'returns false if the index does not exist' do
expect(model.index_exists_by_name?(:projects, 'this_does_not_exist'))
.to be_falsy
end
context 'when an index with a function exists', :postgresql do
before do
ActiveRecord::Base.connection.execute(
'CREATE INDEX test_index ON projects (LOWER(path));'
)
end
after do
'DROP INDEX IF EXISTS test_index;'
end
it 'returns true if an index exists' do
expect(model.index_exists_by_name?(:projects, 'test_index'))
.to be_truthy
end
end
end
end end
...@@ -240,19 +240,12 @@ describe Gitlab::GitAccess do ...@@ -240,19 +240,12 @@ describe Gitlab::GitAccess do
end end
shared_examples 'check_project_moved' do shared_examples 'check_project_moved' do
it 'enqueues a redirected message' 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::ProjectMoved.fetch_message(user.id, project.id)).not_to be_nil
end end
end
describe '#check_project_moved!', :clean_gitlab_redis_shared_state do
before do
project.add_master(user)
end
context 'when a redirect was not followed to find the project' do
it 'allows push and pull access' do it 'allows push and pull access' do
aggregate_failures do aggregate_failures do
expect { push_access_check }.not_to raise_error expect { push_access_check }.not_to raise_error
...@@ -261,61 +254,31 @@ describe Gitlab::GitAccess do ...@@ -261,61 +254,31 @@ describe Gitlab::GitAccess do
end end
end end
context 'when a permanent redirect and ssh protocol' do describe '#add_project_moved_message!', :clean_gitlab_redis_shared_state do
let(:redirected_path) { 'some/other-path' }
before do before do
allow_any_instance_of(Gitlab::Checks::ProjectMoved).to receive(:permanent_redirect?).and_return(true) project.add_master(user)
end end
context 'when a redirect was not followed to find the project' do
it 'allows push and pull access' do it 'allows push and pull access' do
aggregate_failures do aggregate_failures do
expect { push_access_check }.not_to raise_error expect { push_access_check }.not_to raise_error
expect { pull_access_check }.not_to raise_error
end end
end end
it_behaves_like 'check_project_moved'
end end
context 'with a permanent redirect and http protocol' do context 'with a redirect and ssh protocol' do
let(:redirected_path) { 'some/other-path' } let(:redirected_path) { 'some/other-path' }
let(:protocol) { 'http' }
before do
allow_any_instance_of(Gitlab::Checks::ProjectMoved).to receive(:permanent_redirect?).and_return(true)
end
it 'allows_push and pull access' do
aggregate_failures do
expect { push_access_check }.not_to raise_error
end
end
it_behaves_like 'check_project_moved' it_behaves_like 'check_project_moved'
end end
context 'with a temporal redirect and ssh protocol' do context 'with a redirect and http protocol' do
let(:redirected_path) { 'some/other-path' }
it 'blocks push and pull access' do
aggregate_failures do
expect { push_access_check }.to raise_error(described_class::ProjectMovedError, /Project '#{redirected_path}' was moved to '#{project.full_path}'/)
expect { push_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.ssh_url_to_repo}/)
expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /Project '#{redirected_path}' was moved to '#{project.full_path}'/)
expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.ssh_url_to_repo}/)
end
end
end
context 'with a temporal redirect and http protocol' do
let(:redirected_path) { 'some/other-path' } let(:redirected_path) { 'some/other-path' }
let(:protocol) { 'http' } let(:protocol) { 'http' }
it 'does not allow to push and pull access' do it_behaves_like 'check_project_moved'
expect { push_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.http_url_to_repo}/)
expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.http_url_to_repo}/)
end
end end
end end
......
...@@ -16,66 +16,6 @@ describe Route do ...@@ -16,66 +16,6 @@ describe Route do
it { is_expected.to validate_presence_of(:source) } it { is_expected.to validate_presence_of(:source) }
it { is_expected.to validate_presence_of(:path) } it { is_expected.to validate_presence_of(:path) }
it { is_expected.to validate_uniqueness_of(:path).case_insensitive } it { is_expected.to validate_uniqueness_of(:path).case_insensitive }
describe '#ensure_permanent_paths' do
context 'when the route is not yet persisted' do
let(:new_route) { described_class.new(path: 'foo', source: build(:group)) }
context 'when permanent conflicting redirects exist' do
it 'is invalid' do
redirect = build(:redirect_route, :permanent, path: 'foo/bar/baz')
redirect.save!(validate: false)
expect(new_route.valid?).to be_falsey
expect(new_route.errors.first[1]).to eq('has been taken before')
end
end
context 'when no permanent conflicting redirects exist' do
it 'is valid' do
expect(new_route.valid?).to be_truthy
end
end
end
context 'when path has changed' do
before do
route.path = 'foo'
end
context 'when permanent conflicting redirects exist' do
it 'is invalid' do
redirect = build(:redirect_route, :permanent, path: 'foo/bar/baz')
redirect.save!(validate: false)
expect(route.valid?).to be_falsey
expect(route.errors.first[1]).to eq('has been taken before')
end
end
context 'when no permanent conflicting redirects exist' do
it 'is valid' do
expect(route.valid?).to be_truthy
end
end
end
context 'when path has not changed' do
context 'when permanent conflicting redirects exist' do
it 'is valid' do
redirect = build(:redirect_route, :permanent, path: 'git_lab/foo/bar')
redirect.save!(validate: false)
expect(route.valid?).to be_truthy
end
end
context 'when no permanent conflicting redirects exist' do
it 'is valid' do
expect(route.valid?).to be_truthy
end
end
end
end
end end
describe 'callbacks' do describe 'callbacks' do
...@@ -211,34 +151,23 @@ describe Route do ...@@ -211,34 +151,23 @@ describe Route do
end end
context 'when the source is a Project' do context 'when the source is a Project' do
it 'creates a temporal RedirectRoute' do it 'creates a RedirectRoute' do
project = create(:project) project = create(:project)
route = project.route route = project.route
redirect_route = route.create_redirect('foo') redirect_route = route.create_redirect('foo')
expect(redirect_route.permanent?).to be_falsy expect(redirect_route).not_to be_nil
end end
end end
context 'when the source is not a project' do context 'when the source is not a project' do
it 'creates a permanent RedirectRoute' do it 'creates a RedirectRoute' do
redirect_route = route.create_redirect('foo', permanent: true) redirect_route = route.create_redirect('foo')
expect(redirect_route.permanent?).to be_truthy expect(redirect_route).not_to be_nil
end end
end end
end end
describe '#delete_conflicting_redirects' do describe '#delete_conflicting_redirects' do
context 'with permanent redirect' do
it 'does not delete the redirect' do
route.create_redirect("#{route.path}/foo", permanent: true)
expect do
route.delete_conflicting_redirects
end.not_to change { RedirectRoute.count }
end
end
context 'with temporal redirect' do
let(:route) { create(:project).route } let(:route) { create(:project).route }
it 'deletes the redirect' do it 'deletes the redirect' do
...@@ -248,7 +177,6 @@ describe Route do ...@@ -248,7 +177,6 @@ describe Route do
route.delete_conflicting_redirects route.delete_conflicting_redirects
end.to change { RedirectRoute.count }.by(-1) end.to change { RedirectRoute.count }.by(-1)
end end
end
context 'when a redirect route with the same path exists' do context 'when a redirect route with the same path exists' do
context 'when the redirect route has matching case' do context 'when the redirect route has matching case' do
...@@ -289,32 +217,19 @@ describe Route do ...@@ -289,32 +217,19 @@ describe Route do
end end
describe '#conflicting_redirects' do describe '#conflicting_redirects' do
let(:route) { create(:project).route }
it 'returns an ActiveRecord::Relation' do it 'returns an ActiveRecord::Relation' do
expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation) expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation)
end end
context 'with permanent redirects' do
it 'does not return anything' do
route.create_redirect("#{route.path}/foo", permanent: true)
route.create_redirect("#{route.path}/foo/bar", permanent: true)
route.create_redirect("#{route.path}/baz/quz", permanent: true)
expect(route.conflicting_redirects).to be_empty
end
end
context 'with temporal redirects' do
let(:route) { create(:project).route }
it 'returns the redirect routes' do it 'returns the redirect routes' do
route = create(:project).route
redirect1 = route.create_redirect("#{route.path}/foo") redirect1 = route.create_redirect("#{route.path}/foo")
redirect2 = route.create_redirect("#{route.path}/foo/bar") redirect2 = route.create_redirect("#{route.path}/foo/bar")
redirect3 = route.create_redirect("#{route.path}/baz/quz") redirect3 = route.create_redirect("#{route.path}/baz/quz")
expect(route.conflicting_redirects).to match_array([redirect1, redirect2, redirect3]) expect(route.conflicting_redirects).to match_array([redirect1, redirect2, redirect3])
end end
end
context 'when a redirect route with the same path exists' do context 'when a redirect route with the same path exists' do
let(:route) { create(:project).route } let(:route) { create(:project).route }
...@@ -348,44 +263,6 @@ describe Route do ...@@ -348,44 +263,6 @@ describe Route do
end end
end end
describe "#conflicting_redirect_exists?" do
context 'when a conflicting redirect exists' do
let(:group1) { create(:group, path: 'foo') }
let(:group2) { create(:group, path: 'baz') }
it 'should not be saved' do
group1.path = 'bar'
group1.save
group2.path = 'foo'
expect(group2.save).to be_falsy
end
it 'should return an error on path' do
group1.path = 'bar'
group1.save
group2.path = 'foo'
group2.valid?
expect(group2.errors[:path]).to eq(['has been taken before'])
end
end
context 'when a conflicting redirect does not exist' do
let(:project1) { create(:project, path: 'foo') }
let(:project2) { create(:project, path: 'baz') }
it 'should be saved' do
project1.path = 'bar'
project1.save
project2.path = 'foo'
expect(project2.save).to be_truthy
end
end
end
describe '#delete_conflicting_orphaned_routes' do describe '#delete_conflicting_orphaned_routes' do
context 'when there is a conflicting route' do context 'when there is a conflicting route' do
let!(:conflicting_group) { create(:group, path: 'foo') } let!(:conflicting_group) { create(:group, path: 'foo') }
......
...@@ -126,23 +126,6 @@ describe User do ...@@ -126,23 +126,6 @@ describe User do
end end
end end
context 'when the username was used by another user before' do
let(:username) { 'foo' }
let!(:other_user) { create(:user, username: username) }
before do
other_user.username = 'bar'
other_user.save!
end
it 'is invalid' do
user = build(:user, username: username)
expect(user).not_to be_valid
expect(user.errors.full_messages).to eq(['Username has been taken before'])
end
end
context 'when the username is in use by another user' do context 'when the username is in use by another user' do
let(:username) { 'foo' } let(:username) { 'foo' }
let!(:other_user) { create(:user, username: username) } let!(:other_user) { create(:user, username: username) }
...@@ -2699,27 +2682,19 @@ describe User do ...@@ -2699,27 +2682,19 @@ describe User do
end end
end end
describe "#username_previously_taken?" do context 'changing a username' do
let(:user1) { create(:user, username: 'foo') } let(:user) { create(:user, username: 'foo') }
context 'when the username has been taken before' do it 'creates a redirect route' do
before do expect { user.update!(username: 'bar') }
user1.username = 'bar' .to change { RedirectRoute.where(path: 'foo').count }.by(1)
user1.save!
end end
it 'should raise an ActiveRecord::RecordInvalid exception' do it 'deletes the redirect when a user with the old username was created' do
user2 = build(:user, username: 'foo') user.update!(username: 'bar')
expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Username has been taken before/)
end
end
context 'when the username has not been taken before' do expect { create(:user, username: 'foo') }
it 'should be valid' do .to change { RedirectRoute.where(path: 'foo').count }.by(-1)
expect(RedirectRoute.count).to eq(0)
user2 = build(:user, username: 'baz')
expect(user2).to be_valid
end
end end
end end
end end
...@@ -344,20 +344,11 @@ describe 'Git HTTP requests' do ...@@ -344,20 +344,11 @@ describe 'Git HTTP requests' do
context 'and the user requests a redirected path' do context 'and the user requests a redirected path' do
let!(:redirect) { project.route.create_redirect('foo/bar') } let!(:redirect) { project.route.create_redirect('foo/bar') }
let(:path) { "#{redirect.path}.git" } let(:path) { "#{redirect.path}.git" }
let(:project_moved_message) do
<<-MSG.strip_heredoc
Project '#{redirect.path}' was moved to '#{project.full_path}'.
Please update your Git remote:
git remote set-url origin #{project.http_url_to_repo} and try again.
MSG
end
it 'downloads get status 404 with "project was moved" message' do it 'downloads get status 200 for redirects' do
clone_get(path, {}) clone_get(path, {})
expect(response).to have_gitlab_http_status(:not_found)
expect(response.body).to match(project_moved_message) expect(response).to have_gitlab_http_status(:ok)
end end
end end
end end
...@@ -559,20 +550,19 @@ describe 'Git HTTP requests' do ...@@ -559,20 +550,19 @@ describe 'Git HTTP requests' do
Please update your Git remote: Please update your Git remote:
git remote set-url origin #{project.http_url_to_repo} and try again. git remote set-url origin #{project.http_url_to_repo}.
MSG MSG
end end
it 'downloads get status 404 with "project was moved" message' do it 'downloads get status 200' do
clone_get(path, env) clone_get(path, env)
expect(response).to have_gitlab_http_status(:not_found)
expect(response.body).to match(project_moved_message) expect(response).to have_gitlab_http_status(:ok)
end end
it 'uploads get status 404 with "project was moved" message' do it 'uploads get status 404 with "project was moved" message' do
upload(path, env) do |response| upload(path, env) do |response|
expect(response).to have_gitlab_http_status(:not_found) expect(response).to have_gitlab_http_status(:ok)
expect(response.body).to match(project_moved_message)
end end
end end
end end
......
...@@ -222,8 +222,8 @@ describe Groups::TransferService, :postgresql do ...@@ -222,8 +222,8 @@ describe Groups::TransferService, :postgresql do
expect(new_parent_group.children.first).to eq(group) expect(new_parent_group.children.first).to eq(group)
end end
it 'should create a permanent redirect for the group' do it 'should create a redirect for the group' do
expect(group.redirect_routes.permanent.count).to eq(1) expect(group.redirect_routes.count).to eq(1)
end end
end end
...@@ -243,10 +243,10 @@ describe Groups::TransferService, :postgresql do ...@@ -243,10 +243,10 @@ describe Groups::TransferService, :postgresql do
end end
end end
it 'should create permanent redirects for the subgroups' do it 'should create redirects for the subgroups' do
expect(group.redirect_routes.permanent.count).to eq(1) expect(group.redirect_routes.count).to eq(1)
expect(subgroup1.redirect_routes.permanent.count).to eq(1) expect(subgroup1.redirect_routes.count).to eq(1)
expect(subgroup2.redirect_routes.permanent.count).to eq(1) expect(subgroup2.redirect_routes.count).to eq(1)
end end
context 'when the new parent has a higher visibility than the children' do context 'when the new parent has a higher visibility than the children' do
...@@ -287,9 +287,9 @@ describe Groups::TransferService, :postgresql do ...@@ -287,9 +287,9 @@ describe Groups::TransferService, :postgresql do
end end
it 'should create permanent redirects for the projects' do it 'should create permanent redirects for the projects' do
expect(group.redirect_routes.permanent.count).to eq(1) expect(group.redirect_routes.count).to eq(1)
expect(project1.redirect_routes.permanent.count).to eq(1) expect(project1.redirect_routes.count).to eq(1)
expect(project2.redirect_routes.permanent.count).to eq(1) expect(project2.redirect_routes.count).to eq(1)
end end
context 'when the new parent has a higher visibility than the projects' do context 'when the new parent has a higher visibility than the projects' do
...@@ -338,12 +338,12 @@ describe Groups::TransferService, :postgresql do ...@@ -338,12 +338,12 @@ describe Groups::TransferService, :postgresql do
end end
end end
it 'should create permanent redirect for the subgroups and projects' do it 'should create redirect for the subgroups and projects' do
expect(group.redirect_routes.permanent.count).to eq(1) expect(group.redirect_routes.count).to eq(1)
expect(subgroup1.redirect_routes.permanent.count).to eq(1) expect(subgroup1.redirect_routes.count).to eq(1)
expect(subgroup2.redirect_routes.permanent.count).to eq(1) expect(subgroup2.redirect_routes.count).to eq(1)
expect(project1.redirect_routes.permanent.count).to eq(1) expect(project1.redirect_routes.count).to eq(1)
expect(project2.redirect_routes.permanent.count).to eq(1) expect(project2.redirect_routes.count).to eq(1)
end end
end end
...@@ -380,12 +380,12 @@ describe Groups::TransferService, :postgresql do ...@@ -380,12 +380,12 @@ describe Groups::TransferService, :postgresql do
end end
end end
it 'should create permanent redirect for the subgroups and projects' do it 'should create redirect for the subgroups and projects' do
expect(group.redirect_routes.permanent.count).to eq(1) expect(group.redirect_routes.count).to eq(1)
expect(project1.redirect_routes.permanent.count).to eq(1) expect(project1.redirect_routes.count).to eq(1)
expect(subgroup1.redirect_routes.permanent.count).to eq(1) expect(subgroup1.redirect_routes.count).to eq(1)
expect(nested_subgroup.redirect_routes.permanent.count).to eq(1) expect(nested_subgroup.redirect_routes.count).to eq(1)
expect(nested_project.redirect_routes.permanent.count).to eq(1) expect(nested_project.redirect_routes.count).to eq(1)
end end
end end
......
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