summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2018-03-28 13:04:16 +0000
committerDouwe Maan <douwe@gitlab.com>2018-03-28 13:04:16 +0000
commitb3fb82a9d28571d90e45220c62dd70d7004a42bd (patch)
tree1ec2e5272c739cbff7614aca81117503305a9ecc
parent11048808528898dc69ecec3fa1a5129fcec1b3ba (diff)
parentdb75057c72e5b3c171fce898bbdeb57bff5a77b7 (diff)
downloadgitlab-ce-b3fb82a9d28571d90e45220c62dd70d7004a42bd.tar.gz
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
-rw-r--r--app/models/redirect_route.rb28
-rw-r--r--app/models/route.rb26
-rw-r--r--changelogs/unreleased/bvl-no-permanent-redirect.yml5
-rw-r--r--db/post_migrate/20180305100050_remove_permanent_from_redirect_routes.rb37
-rw-r--r--db/post_migrate/20180306164012_add_path_index_to_redirect_routes.rb38
-rw-r--r--db/schema.rb1
-rw-r--r--doc/user/project/index.md14
-rw-r--r--lib/gitlab/checks/project_moved.rb20
-rw-r--r--lib/gitlab/database/migration_helpers.rb39
-rw-r--r--lib/gitlab/git_access.rb10
-rw-r--r--lib/tasks/migrate/setup_postgresql.rake4
-rw-r--r--spec/factories/redirect_routes.rb9
-rw-r--r--spec/lib/gitlab/checks/project_moved_spec.rb43
-rw-r--r--spec/lib/gitlab/database/migration_helpers_spec.rb29
-rw-r--r--spec/lib/gitlab/git_access_spec.rb59
-rw-r--r--spec/models/route_spec.rb159
-rw-r--r--spec/models/user_spec.rb45
-rw-r--r--spec/requests/git_http_spec.rb26
-rw-r--r--spec/services/groups/transfer_service_spec.rb42
19 files changed, 236 insertions, 398 deletions
diff --git a/app/models/redirect_route.rb b/app/models/redirect_route.rb
index 20532527346..31de204d824 100644
--- a/app/models/redirect_route.rb
+++ b/app/models/redirect_route.rb
@@ -17,32 +17,4 @@ class RedirectRoute < ActiveRecord::Base
where(wheres, path, "#{sanitize_sql_like(path)}/%")
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
diff --git a/app/models/route.rb b/app/models/route.rb
index 07d96c21cf1..2d609920051 100644
--- a/app/models/route.rb
+++ b/app/models/route.rb
@@ -10,8 +10,6 @@ class Route < ActiveRecord::Base
presence: true,
uniqueness: { case_sensitive: false }
- validate :ensure_permanent_paths, if: :path_changed?
-
before_validation :delete_conflicting_orphaned_routes
after_create :delete_conflicting_redirects
after_update :delete_conflicting_redirects, if: :path_changed?
@@ -45,7 +43,7 @@ class Route < ActiveRecord::Base
# We are not calling route.delete_conflicting_redirects here, in hopes
# of avoiding deadlocks. The parent (self, in this method) already
# 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
@@ -55,31 +53,17 @@ class Route < ActiveRecord::Base
end
def conflicting_redirects
- RedirectRoute.temporary.matching_path_and_descendants(path)
+ RedirectRoute.matching_path_and_descendants(path)
end
- def create_redirect(path, permanent: false)
- RedirectRoute.create(source: source, path: path, permanent: permanent)
+ def create_redirect(path)
+ RedirectRoute.create(source: source, path: path)
end
private
def create_redirect_for_old_path
- create_redirect(path_was, permanent: permanent_redirect?) 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?
+ create_redirect(path_was) if path_changed?
end
def delete_conflicting_orphaned_routes
diff --git a/changelogs/unreleased/bvl-no-permanent-redirect.yml b/changelogs/unreleased/bvl-no-permanent-redirect.yml
new file mode 100644
index 00000000000..c34a3789b58
--- /dev/null
+++ b/changelogs/unreleased/bvl-no-permanent-redirect.yml
@@ -0,0 +1,5 @@
+---
+title: Don't create permanent redirect routes
+merge_request: 17521
+author:
+type: changed
diff --git a/db/post_migrate/20180305100050_remove_permanent_from_redirect_routes.rb b/db/post_migrate/20180305100050_remove_permanent_from_redirect_routes.rb
new file mode 100644
index 00000000000..db5165dbe70
--- /dev/null
+++ b/db/post_migrate/20180305100050_remove_permanent_from_redirect_routes.rb
@@ -0,0 +1,37 @@
+# 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
diff --git a/db/post_migrate/20180306164012_add_path_index_to_redirect_routes.rb b/db/post_migrate/20180306164012_add_path_index_to_redirect_routes.rb
new file mode 100644
index 00000000000..d6fb4f06695
--- /dev/null
+++ b/db/post_migrate/20180306164012_add_path_index_to_redirect_routes.rb
@@ -0,0 +1,38 @@
+# 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
diff --git a/db/schema.rb b/db/schema.rb
index 3bf42080870..77b3d836287 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1605,7 +1605,6 @@ ActiveRecord::Schema.define(version: 20180327101207) do
t.string "path", null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
- t.boolean "permanent"
end
add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path", unique: true, using: :btree
diff --git a/doc/user/project/index.md b/doc/user/project/index.md
index 175a8975ae1..f94e93dd7d8 100644
--- a/doc/user/project/index.md
+++ b/doc/user/project/index.md
@@ -128,11 +128,9 @@ and Git push/pull redirects.
Depending on the situation, different things apply.
-When [renaming a user](../profile/index.md#changing-your-username) or
-[changing a group path](../group/index.md#changing-a-group-s-path):
+When [renaming a user](../profile/index.md#changing-your-username),
+[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
redirect to the new URLs.
- 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
your remote will be displayed instead of rejecting your action.
This means that any automation scripts, or Git clients will continue to
work after a rename, making any transition a lot smoother.
- To avoid pulling from or pushing to an entirely incorrect repository, the old
- path will be reserved.
-
-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.
+- The redirects will be available as long as the original path is not claimed by
+ another group, user or project.
diff --git a/lib/gitlab/checks/project_moved.rb b/lib/gitlab/checks/project_moved.rb
index 3263790a876..3a197078d08 100644
--- a/lib/gitlab/checks/project_moved.rb
+++ b/lib/gitlab/checks/project_moved.rb
@@ -9,20 +9,16 @@ module Gitlab
super(project, user, protocol)
end
- def message(rejected: false)
+ def message
<<~MESSAGE
Project '#{redirected_path}' was moved to '#{project.full_path}'.
Please update your Git remote:
- #{remote_url_message(rejected)}
+ git remote set-url origin #{url_to_repo}
MESSAGE
end
- def permanent_redirect?
- RedirectRoute.permanent.exists?(path: redirected_path)
- end
-
private
attr_reader :redirected_path
@@ -30,18 +26,6 @@ module Gitlab
def self.message_key(user_id, project_id)
"#{REDIRECT_NAMESPACE}:#{user_id}:#{project_id}"
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
diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb
index 44ca434056f..1634fe4e9cb 100644
--- a/lib/gitlab/database/migration_helpers.rb
+++ b/lib/gitlab/database/migration_helpers.rb
@@ -900,11 +900,42 @@ into similar problems in the future (e.g. when new tables are created).
end
end
- # Rails' index_exists? doesn't work when you only give it a table and index
- # name. As such we have to use some extra code to check if an index exists for
- # a given name.
+ # Fetches indexes on a column by name for postgres.
+ #
+ # 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)
- 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
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 6400089a22f..0a2a23e835b 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -53,7 +53,7 @@ module Gitlab
ensure_project_on_push!(cmd, changes)
check_project_accessibility!
- check_project_moved!
+ add_project_moved_message!
check_repository_existence!
case cmd
@@ -125,16 +125,12 @@ module Gitlab
end
end
- def check_project_moved!
+ def add_project_moved_message!
return if redirected_path.nil?
project_moved = Checks::ProjectMoved.new(project, user, protocol, redirected_path)
- if project_moved.permanent_redirect?
- project_moved.add_message
- else
- raise ProjectMovedError, project_moved.message(rejected: true)
- end
+ project_moved.add_message
end
def check_command_disabled!(cmd)
diff --git a/lib/tasks/migrate/setup_postgresql.rake b/lib/tasks/migrate/setup_postgresql.rake
index 1c7a8a90f5c..af30ecb0e9b 100644
--- a/lib/tasks/migrate/setup_postgresql.rake
+++ b/lib/tasks/migrate/setup_postgresql.rake
@@ -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/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/20180113220114_rework_redirect_routes_indexes.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
AddUsersLowerUsernameEmailIndexes.new.up
@@ -17,6 +17,6 @@ task setup_postgresql: :environment do
AddLowerPathIndexToRedirectRoutes.new.up
IndexRedirectRoutesPathForLike.new.up
AddIndexOnNamespacesLowerName.new.up
- ReworkRedirectRoutesIndexes.new.up
UsersNameLowerIndex.new.up
+ AddPathIndexToRedirectRoutes.new.up
end
diff --git a/spec/factories/redirect_routes.rb b/spec/factories/redirect_routes.rb
index c29c81c5df9..774232d0b34 100644
--- a/spec/factories/redirect_routes.rb
+++ b/spec/factories/redirect_routes.rb
@@ -2,14 +2,5 @@ FactoryBot.define do
factory :redirect_route do
sequence(:path) { |n| "redirect#{n}" }
source factory: :group
- permanent false
-
- trait :permanent do
- permanent true
- end
-
- trait :temporary do
- permanent false
- end
end
end
diff --git a/spec/lib/gitlab/checks/project_moved_spec.rb b/spec/lib/gitlab/checks/project_moved_spec.rb
index e263d29656c..8e9386b1ba1 100644
--- a/spec/lib/gitlab/checks/project_moved_spec.rb
+++ b/spec/lib/gitlab/checks/project_moved_spec.rb
@@ -44,44 +44,17 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
end
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"
+ it 'returns a redirect message' do
+ project_moved = described_class.new(project, user, 'http', 'foo/bar')
+ message = <<~MSG
+ Project 'foo/bar' was moved to '#{project.full_path}'.
- expect(project_moved.message(rejected: true)).to eq(message)
- end
- end
+ Please update your Git remote:
- context 'when the push is not rejected' do
- it 'returns a redirect message' 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}\n"
+ git remote set-url origin #{project.http_url_to_repo}
+ MSG
- expect(project_moved.message).to eq(message)
- end
- end
- end
-
- describe '#permanent_redirect?' do
- context 'with a permanent RedirectRoute' do
- 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
- 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
+ expect(project_moved.message).to eq(message)
end
end
end
diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb
index a41b7f4e104..280f799f2ab 100644
--- a/spec/lib/gitlab/database/migration_helpers_spec.rb
+++ b/spec/lib/gitlab/database/migration_helpers_spec.rb
@@ -1211,4 +1211,33 @@ describe Gitlab::Database::MigrationHelpers do
expect(model.perform_background_migration_inline?).to eq(false)
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
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 6f07e423c1b..f8f09d29c73 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -240,14 +240,21 @@ describe Gitlab::GitAccess do
end
shared_examples 'check_project_moved' do
- it 'enqueues a redirected message' do
+ it 'enqueues a redirected message for pushing' do
push_access_check
expect(Gitlab::Checks::ProjectMoved.fetch_message(user.id, project.id)).not_to be_nil
end
+
+ it 'allows push and pull access' do
+ aggregate_failures do
+ expect { push_access_check }.not_to raise_error
+ expect { pull_access_check }.not_to raise_error
+ end
+ end
end
- describe '#check_project_moved!', :clean_gitlab_redis_shared_state do
+ describe '#add_project_moved_message!', :clean_gitlab_redis_shared_state do
before do
project.add_master(user)
end
@@ -261,62 +268,18 @@ describe Gitlab::GitAccess do
end
end
- context 'when a permanent redirect and ssh protocol' do
+ context 'with a redirect and ssh protocol' do
let(:redirected_path) { 'some/other-path' }
- 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'
end
- context 'with a permanent redirect and http protocol' do
+ context 'with a redirect and http protocol' do
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'
end
-
- context 'with a temporal redirect and ssh 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(:protocol) { 'http' }
-
- it 'does not allow to push and pull access' do
- 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
describe '#check_authentication_abilities!' do
diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb
index dfac82b327a..01238a89a81 100644
--- a/spec/models/route_spec.rb
+++ b/spec/models/route_spec.rb
@@ -16,66 +16,6 @@ describe Route do
it { is_expected.to validate_presence_of(:source) }
it { is_expected.to validate_presence_of(:path) }
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
describe 'callbacks' do
@@ -211,43 +151,31 @@ describe Route do
end
context 'when the source is a Project' do
- it 'creates a temporal RedirectRoute' do
+ it 'creates a RedirectRoute' do
project = create(:project)
route = project.route
redirect_route = route.create_redirect('foo')
- expect(redirect_route.permanent?).to be_falsy
+ expect(redirect_route).not_to be_nil
end
end
context 'when the source is not a project' do
- it 'creates a permanent RedirectRoute' do
- redirect_route = route.create_redirect('foo', permanent: true)
- expect(redirect_route.permanent?).to be_truthy
+ it 'creates a RedirectRoute' do
+ redirect_route = route.create_redirect('foo')
+ expect(redirect_route).not_to be_nil
end
end
end
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
- route.create_redirect("#{route.path}/foo")
+ it 'deletes the redirect' do
+ route.create_redirect("#{route.path}/foo")
- expect do
- route.delete_conflicting_redirects
- end.to change { RedirectRoute.count }.by(-1)
- end
+ expect do
+ route.delete_conflicting_redirects
+ end.to change { RedirectRoute.count }.by(-1)
end
context 'when a redirect route with the same path exists' do
@@ -289,31 +217,18 @@ describe Route do
end
describe '#conflicting_redirects' do
+ let(:route) { create(:project).route }
+
it 'returns an ActiveRecord::Relation' do
expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation)
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)
+ it 'returns the redirect routes' do
+ redirect1 = route.create_redirect("#{route.path}/foo")
+ redirect2 = route.create_redirect("#{route.path}/foo/bar")
+ redirect3 = route.create_redirect("#{route.path}/baz/quz")
- 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
- route = create(:project).route
- redirect1 = route.create_redirect("#{route.path}/foo")
- redirect2 = route.create_redirect("#{route.path}/foo/bar")
- redirect3 = route.create_redirect("#{route.path}/baz/quz")
-
- expect(route.conflicting_redirects).to match_array([redirect1, redirect2, redirect3])
- end
+ expect(route.conflicting_redirects).to match_array([redirect1, redirect2, redirect3])
end
context 'when a redirect route with the same path exists' do
@@ -348,44 +263,6 @@ describe Route do
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
context 'when there is a conflicting route' do
let!(:conflicting_group) { create(:group, path: 'foo') }
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index c61674fff13..bbfdda23a31 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -126,23 +126,6 @@ describe User do
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
let(:username) { 'foo' }
let!(:other_user) { create(:user, username: username) }
@@ -2699,27 +2682,19 @@ describe User do
end
end
- describe "#username_previously_taken?" do
- let(:user1) { create(:user, username: 'foo') }
+ context 'changing a username' do
+ let(:user) { create(:user, username: 'foo') }
- context 'when the username has been taken before' do
- before do
- user1.username = 'bar'
- user1.save!
- end
-
- it 'should raise an ActiveRecord::RecordInvalid exception' do
- user2 = build(:user, username: 'foo')
- expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Username has been taken before/)
- end
+ it 'creates a redirect route' do
+ expect { user.update!(username: 'bar') }
+ .to change { RedirectRoute.where(path: 'foo').count }.by(1)
end
- context 'when the username has not been taken before' do
- it 'should be valid' do
- expect(RedirectRoute.count).to eq(0)
- user2 = build(:user, username: 'baz')
- expect(user2).to be_valid
- end
+ it 'deletes the redirect when a user with the old username was created' do
+ user.update!(username: 'bar')
+
+ expect { create(:user, username: 'foo') }
+ .to change { RedirectRoute.where(path: 'foo').count }.by(-1)
end
end
end
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 7d84f8c3c2a..494db30e8e0 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -344,20 +344,11 @@ describe 'Git HTTP requests' do
context 'and the user requests a redirected path' do
let!(:redirect) { project.route.create_redirect('foo/bar') }
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, {})
- 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
@@ -559,20 +550,19 @@ describe 'Git HTTP requests' do
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
end
- it 'downloads get status 404 with "project was moved" message' do
+ it 'downloads get status 200' do
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
it 'uploads get status 404 with "project was moved" message' do
upload(path, env) do |response|
- 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
diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb
index e1c873f8c1e..999677cfaaa 100644
--- a/spec/services/groups/transfer_service_spec.rb
+++ b/spec/services/groups/transfer_service_spec.rb
@@ -222,8 +222,8 @@ describe Groups::TransferService, :postgresql do
expect(new_parent_group.children.first).to eq(group)
end
- it 'should create a permanent redirect for the group' do
- expect(group.redirect_routes.permanent.count).to eq(1)
+ it 'should create a redirect for the group' do
+ expect(group.redirect_routes.count).to eq(1)
end
end
@@ -243,10 +243,10 @@ describe Groups::TransferService, :postgresql do
end
end
- it 'should create permanent redirects for the subgroups' do
- expect(group.redirect_routes.permanent.count).to eq(1)
- expect(subgroup1.redirect_routes.permanent.count).to eq(1)
- expect(subgroup2.redirect_routes.permanent.count).to eq(1)
+ it 'should create redirects for the subgroups' do
+ expect(group.redirect_routes.count).to eq(1)
+ expect(subgroup1.redirect_routes.count).to eq(1)
+ expect(subgroup2.redirect_routes.count).to eq(1)
end
context 'when the new parent has a higher visibility than the children' do
@@ -287,9 +287,9 @@ describe Groups::TransferService, :postgresql do
end
it 'should create permanent redirects for the projects' do
- expect(group.redirect_routes.permanent.count).to eq(1)
- expect(project1.redirect_routes.permanent.count).to eq(1)
- expect(project2.redirect_routes.permanent.count).to eq(1)
+ expect(group.redirect_routes.count).to eq(1)
+ expect(project1.redirect_routes.count).to eq(1)
+ expect(project2.redirect_routes.count).to eq(1)
end
context 'when the new parent has a higher visibility than the projects' do
@@ -338,12 +338,12 @@ describe Groups::TransferService, :postgresql do
end
end
- it 'should create permanent redirect for the subgroups and projects' do
- expect(group.redirect_routes.permanent.count).to eq(1)
- expect(subgroup1.redirect_routes.permanent.count).to eq(1)
- expect(subgroup2.redirect_routes.permanent.count).to eq(1)
- expect(project1.redirect_routes.permanent.count).to eq(1)
- expect(project2.redirect_routes.permanent.count).to eq(1)
+ it 'should create redirect for the subgroups and projects' do
+ expect(group.redirect_routes.count).to eq(1)
+ expect(subgroup1.redirect_routes.count).to eq(1)
+ expect(subgroup2.redirect_routes.count).to eq(1)
+ expect(project1.redirect_routes.count).to eq(1)
+ expect(project2.redirect_routes.count).to eq(1)
end
end
@@ -380,12 +380,12 @@ describe Groups::TransferService, :postgresql do
end
end
- it 'should create permanent redirect for the subgroups and projects' do
- expect(group.redirect_routes.permanent.count).to eq(1)
- expect(project1.redirect_routes.permanent.count).to eq(1)
- expect(subgroup1.redirect_routes.permanent.count).to eq(1)
- expect(nested_subgroup.redirect_routes.permanent.count).to eq(1)
- expect(nested_project.redirect_routes.permanent.count).to eq(1)
+ it 'should create redirect for the subgroups and projects' do
+ expect(group.redirect_routes.count).to eq(1)
+ expect(project1.redirect_routes.count).to eq(1)
+ expect(subgroup1.redirect_routes.count).to eq(1)
+ expect(nested_subgroup.redirect_routes.count).to eq(1)
+ expect(nested_project.redirect_routes.count).to eq(1)
end
end