Merge branch 'bvl-rename-routes-after-user-rename' into 'master'

Set the name of a user-namespace to the user name

See merge request gitlab-org/gitlab-ce!23272

app/models/namespace.rb

@@ -41,8 +41,7 @@ class Namespace < ApplicationRecord
   validates :owner, presence: true, unless: ->(n) { n.type == "Group" }
   validates :name,
     presence: true,
-    length: { maximum: 255 },
-    namespace_name: true
+    length: { maximum: 255 }
   validates :description, length: { maximum: 255 }
   validates :path,

app/models/user.rb

@@ -1117,9 +1117,10 @@ class User < ApplicationRecord
   def ensure_namespace_correct
     if namespace
-      namespace.path = namespace.name = username if username_changed?
+      namespace.path = username if username_changed?
+      namespace.name = name if name_changed?
     else
-      build_namespace(path: username, name: username)
+      build_namespace(path: username, name: name)
     end
   end

app/validators/namespace_name_validator.rb

-# frozen_string_literal: true
-
-# NamespaceNameValidator
-#
-# Custom validator for GitLab namespace name strings.
-class NamespaceNameValidator < ActiveModel::EachValidator
-  def validate_each(record, attribute, value)
-    unless value =~ Gitlab::Regex.namespace_name_regex
-      record.errors.add(attribute, Gitlab::Regex.namespace_name_regex_message)
-    end
-  end
-end

changelogs/unreleased/bvl-rename-routes-after-user-rename.yml

+---
+title: Update a user's routes after updating their name
+merge_request: 23272
+author:
+type: fixed

db/post_migrate/20190628191740_schedule_fixing_names_of_user_namespaces.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class ScheduleFixingNamesOfUserNamespaces < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  class Namespace < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'namespaces'
+
+    scope :user_namespaces, -> { where(type: nil) }
+  end
+
+  class Route < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'routes'
+
+    scope :project_routes, -> { where(source_type: 'Project') }
+  end
+
+  disable_ddl_transaction!
+
+  def up
+    queue_background_migration_jobs_by_range_at_intervals(
+      ScheduleFixingNamesOfUserNamespaces::Namespace.user_namespaces,
+      'FixUserNamespaceNames',
+      60.seconds,
+      batch_size: 5000
+    )
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      ScheduleFixingNamesOfUserNamespaces::Route.project_routes,
+      'FixUserProjectRouteNames',
+      60.seconds,
+      batch_size: 5000
+    )
+  end
+
+  def down
+    # no-op
+  end
+end

lib/gitlab/background_migration/fix_user_namespace_names.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # This migration fixes the namespaces.name for all user-namespaces that have names
+    # that aren't equal to the users name.
+    # Then it uses the updated names of the namespaces to update the associated routes
+    # For more info see https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23272
+    class FixUserNamespaceNames
+      def perform(from_id, to_id)
+        fix_namespace_names(from_id, to_id)
+        fix_namespace_route_names(from_id, to_id)
+      end
+
+      def fix_namespace_names(from_id, to_id)
+        ActiveRecord::Base.connection.execute <<~UPDATE_NAMESPACES
+          WITH namespaces_to_update AS (
+              SELECT
+                  namespaces.id,
+                  users.name AS correct_name
+              FROM
+                  namespaces
+                  INNER JOIN users ON namespaces.owner_id = users.id
+              WHERE
+                  namespaces.type IS NULL
+                  AND namespaces.id BETWEEN #{from_id} AND #{to_id}
+                  AND namespaces.name != users.name
+          )
+          UPDATE
+              namespaces
+          SET
+              name = correct_name
+          FROM
+              namespaces_to_update
+          WHERE
+              namespaces.id = namespaces_to_update.id
+        UPDATE_NAMESPACES
+      end
+
+      def fix_namespace_route_names(from_id, to_id)
+        ActiveRecord::Base.connection.execute <<~ROUTES_UPDATE
+          WITH routes_to_update AS (
+              SELECT
+                  routes.id,
+                  users.name AS correct_name
+              FROM
+                  routes
+                  INNER JOIN namespaces ON routes.source_id = namespaces.id
+                  INNER JOIN users ON namespaces.owner_id = users.id
+              WHERE
+                  namespaces.type IS NULL
+                  AND routes.source_type = 'Namespace'
+                  AND namespaces.id BETWEEN #{from_id} AND #{to_id}
+                  AND (routes.name != users.name OR routes.name IS NULL)
+          )
+          UPDATE
+              routes
+          SET
+              name = correct_name
+          FROM
+              routes_to_update
+          WHERE
+              routes_to_update.id = routes.id
+        ROUTES_UPDATE
+      end
+    end
+  end
+end

lib/gitlab/background_migration/fix_user_project_route_names.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # This migration fixes the routes.name for all user-projects that have names
+    # that don't start with the users name.
+    # For more info see https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23272
+    class FixUserProjectRouteNames
+      def perform(from_id, to_id)
+        ActiveRecord::Base.connection.execute <<~ROUTES_UPDATE
+          WITH routes_to_update AS (
+            SELECT
+                routes.id,
+                users.name || ' / ' || projects.name AS correct_name
+            FROM
+                routes
+                INNER JOIN projects ON routes.source_id = projects.id
+                INNER JOIN namespaces ON projects.namespace_id = namespaces.id
+                INNER JOIN users ON namespaces.owner_id = users.id
+            WHERE
+                routes.source_type = 'Project'
+                AND routes.id BETWEEN #{from_id} AND #{to_id}
+                AND namespaces.type IS NULL
+                AND (routes.name NOT LIKE users.name || '%' OR routes.name IS NULL)
+          )
+          UPDATE
+              routes
+          SET
+              name = routes_to_update.correct_name
+          FROM
+              routes_to_update
+          WHERE
+              routes_to_update.id = routes.id
+        ROUTES_UPDATE
+      end
+    end
+  end
+end

lib/gitlab/regex.rb

@@ -4,14 +4,6 @@ module Gitlab
   module Regex
     extend self
-    def namespace_name_regex
-      @namespace_name_regex ||= /\A[\p{Alnum}\p{Pd}_\. ]*\z/.freeze
-    end
-
-    def namespace_name_regex_message
-      "can contain only letters, digits, '_', '.', dash and space."
-    end
-
     def project_name_regex
       @project_name_regex ||= /\A[\p{Alnum}\u{00A9}-\u{1f9c0}_][\p{Alnum}\p{Pd}\u{00A9}-\u{1f9c0}_\. ]*\z/.freeze
     end

spec/controllers/projects/environments/prometheus_api_controller_spec.rb

@@ -176,7 +176,7 @@ describe Projects::Environments::PrometheusApiController do
   def environment_params(params = {})
     {
       id: environment.id.to_s,
-      namespace_id: project.namespace.name,
+      namespace_id: project.namespace.full_path,
       project_id: project.name,
       proxy_path: 'query',
       query: '1'

spec/features/merge_request/user_creates_merge_request_spec.rb

@@ -78,7 +78,7 @@ describe "User creates a merge request", :js do
       click_button("Submit merge request")
-      expect(page).to have_content(title).and have_content("Request to merge #{user.namespace.name}:#{source_branch} into master")
+      expect(page).to have_content(title).and have_content("Request to merge #{user.namespace.path}:#{source_branch} into master")
     end
   end
 end

spec/features/projects/fork_spec.rb

@@ -50,7 +50,7 @@ describe 'Project fork' do
       click_link('New merge request')
     end
-    expect(current_path).to have_content(/#{user.namespace.name}/i)
+    expect(current_path).to have_content(/#{user.namespace.path}/i)
   end
   it 'shows avatars when Gravatar is disabled' do

spec/lib/gitlab/background_migration/fix_user_namespace_names_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::FixUserNamespaceNames, :migration, schema: 20190620112608 do
+  let(:namespaces) { table(:namespaces) }
+  let(:users) { table(:users) }
+  let(:user) { users.create(name: "The user's full name", projects_limit: 10, username: 'not-null', email: '1') }
+
+  context 'updating the namespace names' do
+    it 'updates a user namespace within range' do
+      user2 = users.create(name: "Other user's full name", projects_limit: 10, username: 'also-not-null', email: '2')
+      user_namespace1 = namespaces.create(
+        id: 2,
+        owner_id: user.id,
+        name: "Should be the user's name",
+        path: user.username
+      )
+      user_namespace2 = namespaces.create(
+        id: 3,
+        owner_id: user2.id,
+        name: "Should also be the user's name",
+        path: user.username
+      )
+
+      described_class.new.perform(1, 5)
+
+      expect(user_namespace1.reload.name).to eq("The user's full name")
+      expect(user_namespace2.reload.name).to eq("Other user's full name")
+    end
+
+    it 'does not update namespaces out of range' do
+      user_namespace = namespaces.create(
+        id: 6,
+        owner_id: user.id,
+        name: "Should be the user's name",
+        path: user.username
+      )
+
+      expect { described_class.new.perform(1, 5) }
+        .not_to change { user_namespace.reload.name }
+    end
+
+    it 'does not update groups owned by the users' do
+      user_group = namespaces.create(
+        id: 2,
+        owner_id: user.id,
+        name: 'A group name',
+        path: 'the-path',
+        type: 'Group'
+      )
+
+      expect { described_class.new.perform(1, 5) }
+        .not_to change { user_group.reload.name }
+    end
+  end
+
+  context 'namespace route names' do
+    let(:routes) { table(:routes) }
+    let(:namespace) do
+      namespaces.create(
+        id: 2,
+        owner_id: user.id,
+        name: "Will be updated to the user's name",
+        path: user.username
+      )
+    end
+
+    it "updates the route name if it didn't match the namespace" do
+      route = routes.create(path: namespace.path, name: 'Incorrect name', source_type: 'Namespace', source_id: namespace.id)
+
+      described_class.new.perform(1, 5)
+
+      expect(route.reload.name).to eq("The user's full name")
+    end
+
+    it 'updates the route name if it was nil match the namespace' do
+      route = routes.create(path: namespace.path, name: nil, source_type: 'Namespace', source_id: namespace.id)
+
+      described_class.new.perform(1, 5)
+
+      expect(route.reload.name).to eq("The user's full name")
+    end
+
+    it "doesn't update group routes" do
+      route = routes.create(path: 'group-path', name: 'Group name', source_type: 'Group', source_id: namespace.id)
+
+      expect { described_class.new.perform(1, 5) }
+        .not_to change { route.reload.name }
+    end
+
+    it "doesn't touch routes for namespaces out of range" do
+      user_namespace = namespaces.create(
+        id: 6,
+        owner_id: user.id,
+        name: "Should be the user's name",
+        path: user.username
+      )
+
+      expect { described_class.new.perform(1, 5) }
+        .not_to change { user_namespace.reload.name }
+    end
+  end
+end

spec/lib/gitlab/background_migration/fix_user_project_route_names_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::FixUserProjectRouteNames, :migration, schema: 20190620112608 do
+  let(:namespaces) { table(:namespaces) }
+  let(:users) { table(:users) }
+  let(:routes) { table(:routes) }
+  let(:projects) { table(:projects) }
+
+  let(:user) { users.create(name: "The user's full name", projects_limit: 10, username: 'not-null', email: '1') }
+
+  let(:namespace) do
+    namespaces.create(
+      owner_id: user.id,
+      name: "Should eventually be the user's name",
+      path: user.username
+    )
+  end
+
+  let(:project) do
+    projects.create(namespace_id: namespace.id, name: 'Project Name')
+  end
+
+  it "updates the route for a project if it did not match the user's name" do
+    route = routes.create(
+      id: 1,
+      path: "#{user.username}/#{project.path}",
+      source_id: project.id,
+      source_type: 'Project',
+      name: 'Completely wrong'
+    )
+
+    described_class.new.perform(1, 5)
+
+    expect(route.reload.name).to eq("The user's full name / Project Name")
+  end
+
+  it 'updates the route for a project if the name was nil' do
+    route = routes.create(
+      id: 1,
+      path: "#{user.username}/#{project.path}",
+      source_id: project.id,
+      source_type: 'Project',
+      name: nil
+    )
+
+    described_class.new.perform(1, 5)
+
+    expect(route.reload.name).to eq("The user's full name / Project Name")
+  end
+
+  it 'does not update routes that were are out of the range' do
+    route = routes.create(
+      id: 6,
+      path: "#{user.username}/#{project.path}",
+      source_id: project.id,
+      source_type: 'Project',
+      name: 'Completely wrong'
+    )
+
+    expect { described_class.new.perform(1, 5) }
+      .not_to change { route.reload.name }
+  end
+
+  it 'does not update routes for projects in groups owned by the user' do
+    group = namespaces.create(
+      owner_id: user.id,
+      name: 'A group',
+      path: 'a-path',
+      type: ''
+    )
+    project = projects.create(namespace_id: group.id, name: 'Project Name')
+    route = routes.create(
+      id: 1,
+      path: "#{group.path}/#{project.path}",
+      source_id: project.id,
+      source_type: 'Project',
+      name: 'Completely wrong'
+    )
+
+    expect { described_class.new.perform(1, 5) }
+      .not_to change { route.reload.name }
+  end
+
+  it 'does not update routes for namespaces' do
+    route = routes.create(
+      id: 1,
+      path: namespace.path,
+      source_id: namespace.id,
+      source_type: 'Namespace',
+      name: 'Completely wrong'
+    )
+
+    expect { described_class.new.perform(1, 5) }
+      .not_to change { route.reload.name }
+  end
+end

spec/models/concerns/routable_spec.rb

@@ -15,23 +15,46 @@ describe Group, 'Routable' do
   end
   describe 'Callbacks' do
-    it 'creates route record on create' do
-      expect(group.route.path).to eq(group.path)
-      expect(group.route.name).to eq(group.name)
-    end
-    it 'updates route record on path change' do
-      group.update(path: 'wow', name: 'much')
-      expect(group.route.path).to eq('wow')
-      expect(group.route.name).to eq('much')
+    context 'for a group' do
+      it 'creates route record on create' do
+        expect(group.route.path).to eq(group.path)
+        expect(group.route.name).to eq(group.name)
+      end
+
+      it 'updates route record on path change' do
+        group.update(path: 'wow', name: 'much')
+        expect(group.route.path).to eq('wow')
+        expect(group.route.name).to eq('much')
+      end
+
+      it 'ensure route path uniqueness across different objects' do
+        create(:group, parent: group, path: 'xyz')
+        duplicate = build(:project, namespace: group, path: 'xyz')
+        expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Path has already been taken')
+      end
     end
-    it 'ensure route path uniqueness across different objects' do
-      create(:group, parent: group, path: 'xyz')
-      duplicate = build(:project, namespace: group, path: 'xyz')
-      expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Path has already been taken')
+    context 'for a user' do
+      let(:user) { create(:user, username: 'jane', name: "Jane Doe") }
+      it 'creates the route for a record on create' do
+        expect(user.namespace.name).to eq('Jane Doe')
+        expect(user.namespace.path).to eq('jane')
+      end
+
+      it 'updates routes and nested routes on name change' do
+        project = create(:project, path: 'work-stuff', name: 'Work stuff', namespace: user.namespace)
+
+        user.update!(username: 'jaen', name: 'Jaen Did')
+        project.reload
+
+        expect(user.namespace.name).to eq('Jaen Did')
+        expect(user.namespace.path).to eq('jaen')
+        expect(project.full_name).to eq('Jaen Did / Work stuff')
+        expect(project.full_path).to eq('jaen/work-stuff')
+      end
     end
   end

spec/models/user_spec.rb

@@ -2931,7 +2931,7 @@ describe User do
       let(:user) { create(:user, username: username) }
       context 'when the user is updated' do
-        context 'when the username is changed' do
+        context 'when the username or name is changed' do
           let(:new_username) { 'bar' }
           it 'changes the namespace (just to compare to when username is not changed)' do
@@ -2942,16 +2942,24 @@ describe User do
             end.to change { user.namespace.updated_at }
           end
-          it 'updates the namespace name' do
+          it 'updates the namespace path when the username was changed' do
             user.update!(username: new_username)
-            expect(user.namespace.name).to eq(new_username)
+            expect(user.namespace.path).to eq(new_username)
           end
-          it 'updates the namespace path' do
-            user.update!(username: new_username)
-            expect(user.namespace.path).to eq(new_username)
+          it 'updates the namespace name if the name was changed' do
+            user.update!(name: 'New name')
+            expect(user.namespace.name).to eq('New name')
+          end
+
+          it 'updates nested routes for the namespace if the name was changed' do
+            project = create(:project, namespace: user.namespace)
+
+            user.update!(name: 'New name')
+
+            expect(project.route.reload.name).to include('New name')
           end
           context 'when there is a validation error (namespace name taken) while updating namespace' do

spec/requests/api/internal_spec.rb

@@ -474,7 +474,7 @@ describe API::Internal do
           'ssh',
           {
             authentication_abilities: [:read_project, :download_code, :push_code],
-            namespace_path: project.namespace.name,
+            namespace_path: project.namespace.path,
             project_path: project.path,
             redirected_path: nil
           }
@@ -753,7 +753,7 @@ describe API::Internal do
   end
   describe 'GET /internal/merge_request_urls' do
-    let(:repo_name) { "#{project.namespace.name}/#{project.path}" }
+    let(:repo_name) { "#{project.full_path}" }
     let(:changes) { URI.escape("#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch") }
     before do
@@ -765,7 +765,7 @@ describe API::Internal do
       expect(json_response).to match [{
         "branch_name" => "new_branch",
-        "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
+        "url" => "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
         "new_merge_request" => true
       }]
     end
@@ -786,7 +786,7 @@ describe API::Internal do
         expect(json_response).to match [{
           "branch_name" => "new_branch",
-          "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
+          "url" => "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
           "new_merge_request" => true
         }]
       end
@@ -927,7 +927,7 @@ describe API::Internal do
       expect(json_response['merge_request_urls']).to match [{
         "branch_name" => branch_name,
-        "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name}",
+        "url" => "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name}",
         "new_merge_request" => true
       }]
     end
@@ -970,7 +970,7 @@ describe API::Internal do
         expect(json_response['merge_request_urls']).to match [{
           'branch_name' => branch_name,
-          'url' => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/1",
+          'url' => "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/1",
           'new_merge_request' => false
         }]
       end

spec/routing/environments_spec.rb

@@ -9,7 +9,7 @@ describe 'environments routing' do
   end
   let(:environments_route) do
-    "#{project.namespace.name}/#{project.name}/environments/"
+    "#{project.full_path}/environments/"
   end
   describe 'routing environment folders' do
@@ -36,13 +36,12 @@ describe 'environments routing' do
   end
   def get_folder(folder)
-    get("#{project.namespace.name}/#{project.name}/" \
-        "environments/folders/#{folder}")
+    get("#{project.full_path}/environments/folders/#{folder}")
   end
   def folder_action(**opts)
-    options = { namespace_id: project.namespace.name,
-                project_id: project.name }
+    options = { namespace_id: project.namespace.path,
+                project_id: project.path }
     ['projects/environments#folder', options.merge(opts)]
   end

spec/serializers/environment_status_entity_spec.rb

@@ -70,7 +70,7 @@ describe EnvironmentStatusEntity do
       it 'returns metrics url' do
         expect(subject[:metrics_url])
-          .to eq("/#{project.namespace.name}/#{project.name}/environments/#{environment.id}/deployments/#{deployment.iid}/metrics")
+          .to eq("/#{project.full_path}/environments/#{environment.id}/deployments/#{deployment.iid}/metrics")
       end
     end

spec/services/merge_requests/get_urls_service_spec.rb

@@ -8,8 +8,8 @@ describe MergeRequests::GetUrlsService do
   let(:project) { create(:project, :public, :repository) }
   let(:service) { described_class.new(project) }
   let(:source_branch) { "merge-test" }
-  let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" }
-  let(:show_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/#{merge_request.iid}" }
+  let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" }
+  let(:show_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/#{merge_request.iid}" }
   let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
   let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" }
   let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
@@ -119,7 +119,7 @@ describe MergeRequests::GetUrlsService do
       let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch" }
       let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/markdown" }
       let(:changes) { "#{new_branch_changes}\n#{existing_branch_changes}" }
-      let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch" }
+      let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch" }
       it 'returns 2 urls for both creating new and showing merge request' do
         result = service.execute(changes)