Remove code for dynamically generating routesstop-dynamic-routable-creation

This adds a database migration that creates routes for any projects and
namespaces that don't already have one. We also remove the runtime code
for dynamically creating routes, as this is no longer necessary.

17 files changed, 265 insertions, 104 deletions

diff --git a/app/finders/admin/projects_finder.rb b/app/finders/admin/projects_finder.rb
index 53b77f5fed9..543bf1a1415 100644
--- a/app/finders/admin/projects_finder.rb
+++ b/app/finders/admin/projects_finder.rb
@@ -7,7 +7,7 @@ class Admin::ProjectsFinder
   end
 
   def execute
-    items = Project.without_deleted.with_statistics
+    items = Project.without_deleted.with_statistics.with_route
     items = by_namespace_id(items)
     items = by_visibilty_level(items)
     items = by_with_push(items)
@@ -16,7 +16,7 @@ class Admin::ProjectsFinder
     items = by_archived(items)
     items = by_personal(items)
     items = by_name(items)
-    items = items.includes(namespace: [:owner])
+    items = items.includes(namespace: [:owner, :route])
     sort(items).page(params[:page])
   end
 
diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index 0176a12a131..cb91f8fbac8 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -90,34 +90,17 @@ module Routable
   end
 
   def full_name
-    if route && route.name.present?
-      @full_name ||= route.name # rubocop:disable Gitlab/ModuleWithInstanceVariables
-    else
-      update_route if persisted?
-
-      build_full_name
-    end
+    route&.name || build_full_name
   end
 
-  # Every time `project.namespace.becomes(Namespace)` is called for polymorphic_path,
-  # a new instance is instantiated, and we end up duplicating the same query to retrieve
-  # the route. Caching this per request ensures that even if we have multiple instances,
-  # we will not have to duplicate work, avoiding N+1 queries in some cases.
   def full_path
-    return uncached_full_path unless RequestStore.active? && persisted?
-
-    RequestStore[full_path_key] ||= uncached_full_path
+    route&.path || build_full_path
   end
 
   def full_path_components
     full_path.split('/')
   end
 
-  def expires_full_path_cache
-    RequestStore.delete(full_path_key) if RequestStore.active?
-    @full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables
-  end
-
   def build_full_path
     if parent && path
       parent.full_path + '/' + path
@@ -138,16 +121,6 @@ module Routable
     self.errors[:path].concat(route_path_errors) if route_path_errors
   end
 
-  def uncached_full_path
-    if route && route.path.present?
-      @full_path ||= route.path # rubocop:disable Gitlab/ModuleWithInstanceVariables
-    else
-      update_route if persisted?
-
-      build_full_path
-    end
-  end
-
   def full_name_changed?
     name_changed? || parent_changed?
   end
@@ -156,10 +129,6 @@ module Routable
     path_changed? || parent_changed?
   end
 
-  def full_path_key
-    @full_path_key ||= "routable/full_path/#{self.class.name}/#{self.id}"
-  end
-
   def build_full_name
     if parent && name
       parent.human_name + ' / ' + name
@@ -168,18 +137,9 @@ module Routable
     end
   end
 
-  def update_route
-    return if Gitlab::Database.read_only?
-
-    prepare_route
-    route.save
-  end
-
   def prepare_route
     route || build_route(source: self)
     route.path = build_full_path
     route.name = build_full_name
-    @full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables
-    @full_name = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables
   end
 end
diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb
index f66bdd529f1..d61ad19b43d 100644
--- a/app/models/concerns/storage/legacy_namespace.rb
+++ b/app/models/concerns/storage/legacy_namespace.rb
@@ -11,8 +11,6 @@ module Storage
                      Namespace.find(parent_id_was) # raise NotFound early if needed
                    end
 
-      expires_full_path_cache
-
       move_repositories
 
       if parent_changed?
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 7034c633268..c1dc2f55346 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -304,7 +304,6 @@ class Namespace < ActiveRecord::Base
 
   def write_projects_repository_config
     all_projects.find_each do |project|
-      project.expires_full_path_cache # we need to clear cache to validate renames correctly
       project.write_repository_config
     end
   end
diff --git a/app/models/project.rb b/app/models/project.rb
index f880d728839..2034e752635 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1233,8 +1233,6 @@ class Project < ActiveRecord::Base
     return true if skip_disk_validation
     return false unless repository_storage
 
-    expires_full_path_cache # we need to clear cache to validate renames correctly
-
     # Check if repository with same path already exists on disk we can
     # skip this for the hashed storage because the path does not change
     if legacy_storage? && repository_with_same_path_already_exists?
@@ -1613,7 +1611,6 @@ class Project < ActiveRecord::Base
     # When we import a project overwriting the original project, there
     # is a move operation. In that case we don't want to send the instructions.
     send_move_instructions(full_path_was) unless import_started?
-    expires_full_path_cache
 
     self.old_path_with_namespace = full_path_was
     SystemHooksService.new.execute_hooks_for(self, :rename)
diff --git a/app/serializers/pipeline_serializer.rb b/app/serializers/pipeline_serializer.rb
index 4a33160afa1..3205578b83e 100644
--- a/app/serializers/pipeline_serializer.rb
+++ b/app/serializers/pipeline_serializer.rb
@@ -11,10 +11,15 @@ class PipelineSerializer < BaseSerializer
         :retryable_builds,
         :cancelable_statuses,
         :trigger_requests,
-        :project,
         :manual_actions,
         :artifacts,
-        { pending_builds: :project }
+        {
+          pending_builds: :project,
+          project: [:route, { namespace: :route }],
+          artifacts: {
+            project: [:route, { namespace: :route }]
+          }
+        }
       ])
     end
 
diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb
index a4a66330546..c2a0c5fa7f3 100644
--- a/app/services/projects/transfer_service.rb
+++ b/app/services/projects/transfer_service.rb
@@ -77,7 +77,6 @@ module Projects
         Gitlab::PagesTransfer.new.move_project(project.path, @old_namespace.full_path, @new_namespace.full_path)
 
         project.old_path_with_namespace = @old_path
-        project.expires_full_path_cache
 
         write_repository_config(@new_path)
 
diff --git a/app/views/admin/projects/_projects.html.haml b/app/views/admin/projects/_projects.html.haml
index 00933d726d9..fdaacc098e0 100644
--- a/app/views/admin/projects/_projects.html.haml
+++ b/app/views/admin/projects/_projects.html.haml
@@ -17,7 +17,7 @@
             - if project.archived
               %span.badge.badge-warning archived
           .title
-            = link_to [:admin, project.namespace.becomes(Namespace), project] do
+            = link_to(admin_namespace_project_path(project.namespace, project)) do
               .dash-project-avatar
                 .avatar-container.s40
                   = project_icon(project, alt: '', class: 'avatar project-avatar s40')
diff --git a/changelogs/unreleased/stop-dynamic-routable-creation.yml b/changelogs/unreleased/stop-dynamic-routable-creation.yml
new file mode 100644
index 00000000000..8bfcb5b2d11
--- /dev/null
+++ b/changelogs/unreleased/stop-dynamic-routable-creation.yml
@@ -0,0 +1,5 @@
+---
+title: Stop dynamically creating project and namespace routes
+merge_request: 20313
+author:
+type: performance
diff --git a/db/migrate/20180702134423_generate_missing_routes.rb b/db/migrate/20180702134423_generate_missing_routes.rb
new file mode 100644
index 00000000000..994725f9bd1
--- /dev/null
+++ b/db/migrate/20180702134423_generate_missing_routes.rb
@@ -0,0 +1,143 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+# This migration generates missing routes for any projects and namespaces that
+# don't already have a route.
+#
+# On GitLab.com this would insert 611 project routes, and 0 namespace routes.
+# The exact number could vary per instance, so we take care of both just in
+# case.
+class GenerateMissingRoutes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    self.table_name = 'users'
+  end
+
+  class Route < ActiveRecord::Base
+    self.table_name = 'routes'
+  end
+
+  module Routable
+    def build_full_path
+      if parent && path
+        parent.build_full_path + '/' + path
+      else
+        path
+      end
+    end
+
+    def build_full_name
+      if parent && name
+        parent.human_name + ' / ' + name
+      else
+        name
+      end
+    end
+
+    def human_name
+      build_full_name
+    end
+
+    def attributes_for_insert
+      time = Time.zone.now
+
+      {
+        # We can't use "self.class.name" here as that would include the
+        # migration namespace.
+        source_type: source_type_for_route,
+        source_id: id,
+        created_at: time,
+        updated_at: time,
+        name: build_full_name,
+
+        # The route path might already be taken. Instead of trying to generate a
+        # new unique name on every conflict, we just append the row ID to the
+        # route path.
+        path: "#{build_full_path}-#{id}"
+      }
+    end
+  end
+
+  class Project < ActiveRecord::Base
+    self.table_name = 'projects'
+
+    include EachBatch
+    include GenerateMissingRoutes::Routable
+
+    belongs_to :namespace, class_name: 'GenerateMissingRoutes::Namespace'
+
+    has_one :route,
+      as: :source,
+      inverse_of: :source,
+      class_name: 'GenerateMissingRoutes::Route'
+
+    alias_method :parent, :namespace
+    alias_attribute :parent_id, :namespace_id
+
+    def self.without_routes
+      where(
+        'NOT EXISTS (
+          SELECT 1
+          FROM routes
+          WHERE source_type = ?
+          AND source_id = projects.id
+        )',
+        'Project'
+      )
+    end
+
+    def source_type_for_route
+      'Project'
+    end
+  end
+
+  class Namespace < ActiveRecord::Base
+    self.table_name = 'namespaces'
+
+    include EachBatch
+    include GenerateMissingRoutes::Routable
+
+    belongs_to :parent, class_name: 'GenerateMissingRoutes::Namespace'
+    belongs_to :owner, class_name: 'GenerateMissingRoutes::User'
+
+    has_one :route,
+      as: :source,
+      inverse_of: :source,
+      class_name: 'GenerateMissingRoutes::Route'
+
+    def self.without_routes
+      where(
+        'NOT EXISTS (
+          SELECT 1
+          FROM routes
+          WHERE source_type = ?
+          AND source_id = namespaces.id
+        )',
+        'Namespace'
+      )
+    end
+
+    def source_type_for_route
+      'Namespace'
+    end
+  end
+
+  def up
+    [Namespace, Project].each do |model|
+      model.without_routes.each_batch(of: 100) do |batch|
+        rows = batch.map(&:attributes_for_insert)
+
+        Gitlab::Database.bulk_insert(:routes, rows)
+      end
+    end
+  end
+
+  def down
+    # Removing routes we previously generated makes no sense.
+  end
+end
diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb
index 290fcd4f8e6..d89716b1b50 100644
--- a/spec/controllers/projects/pipelines_controller_spec.rb
+++ b/spec/controllers/projects/pipelines_controller_spec.rb
@@ -55,10 +55,8 @@ describe Projects::PipelinesController do
         stub_feature_flags(ci_pipeline_persisted_stages: false)
       end
 
-      it 'returns JSON with serialized pipelines', :request_store do
-        queries = ActiveRecord::QueryRecorder.new do
-          get_pipelines_index_json
-        end
+      it 'returns JSON with serialized pipelines' do
+        get_pipelines_index_json
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(response).to match_response_schema('pipeline')
@@ -73,8 +71,14 @@ describe Projects::PipelinesController do
         json_response.dig('pipelines', 0, 'details', 'stages').tap do |stages|
           expect(stages.count).to eq 3
         end
+      end
+
+      it 'does not execute N+1 queries' do
+        queries = ActiveRecord::QueryRecorder.new do
+          get_pipelines_index_json
+        end
 
-        expect(queries.count).to be_within(5).of(30)
+        expect(queries.count).to be <= 36
       end
     end
 
diff --git a/spec/migrations/generate_missing_routes_spec.rb b/spec/migrations/generate_missing_routes_spec.rb
new file mode 100644
index 00000000000..32515d353b0
--- /dev/null
+++ b/spec/migrations/generate_missing_routes_spec.rb
@@ -0,0 +1,84 @@
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20180702134423_generate_missing_routes.rb')
+
+describe GenerateMissingRoutes, :migration do
+  describe '#up' do
+    let(:namespaces) { table(:namespaces) }
+    let(:projects) { table(:projects) }
+    let(:routes) { table(:routes) }
+
+    it 'creates routes for projects without a route' do
+      namespace = namespaces.create!(name: 'GitLab', path: 'gitlab')
+
+      routes.create!(
+        path: 'gitlab',
+        source_type: 'Namespace',
+        source_id: namespace.id
+      )
+
+      project = projects.create!(
+        name: 'GitLab CE',
+        path: 'gitlab-ce',
+        namespace_id: namespace.id
+      )
+
+      described_class.new.up
+
+      route = routes.where(source_type: 'Project').take
+
+      expect(route.source_id).to eq(project.id)
+      expect(route.path).to eq("gitlab/gitlab-ce-#{project.id}")
+    end
+
+    it 'creates routes for namespaces without a route' do
+      namespace = namespaces.create!(name: 'GitLab', path: 'gitlab')
+
+      described_class.new.up
+
+      route = routes.where(source_type: 'Namespace').take
+
+      expect(route.source_id).to eq(namespace.id)
+      expect(route.path).to eq("gitlab-#{namespace.id}")
+    end
+
+    it 'does not create routes for namespaces that already have a route' do
+      namespace = namespaces.create!(name: 'GitLab', path: 'gitlab')
+
+      routes.create!(
+        path: 'gitlab',
+        source_type: 'Namespace',
+        source_id: namespace.id
+      )
+
+      described_class.new.up
+
+      expect(routes.count).to eq(1)
+    end
+
+    it 'does not create routes for projects that already have a route' do
+      namespace = namespaces.create!(name: 'GitLab', path: 'gitlab')
+
+      routes.create!(
+        path: 'gitlab',
+        source_type: 'Namespace',
+        source_id: namespace.id
+      )
+
+      project = projects.create!(
+        name: 'GitLab CE',
+        path: 'gitlab-ce',
+        namespace_id: namespace.id
+      )
+
+      routes.create!(
+        path: 'gitlab/gitlab-ce',
+        source_type: 'Project',
+        source_id: project.id
+      )
+
+      described_class.new.up
+
+      expect(routes.count).to eq(2)
+    end
+  end
+end
diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb
index ed3e28fbeca..565266321d3 100644
--- a/spec/models/concerns/routable_spec.rb
+++ b/spec/models/concerns/routable_spec.rb
@@ -12,16 +12,6 @@ describe Group, 'Routable' do
     it { is_expected.to have_many(:redirect_routes).dependent(:destroy) }
   end
 
-  describe 'GitLab read-only instance' do
-    it 'does not save route if route is not present' do
-      group.route.path = ''
-      allow(Gitlab::Database).to receive(:read_only?).and_return(true)
-      expect(group).to receive(:update_route).and_call_original
-
-      expect { group.full_path }.to change { Route.count }.by(0)
-    end
-  end
-
   describe 'Callbacks' do
     it 'creates route record on create' do
       expect(group.route.path).to eq(group.path)
@@ -131,29 +121,6 @@ describe Group, 'Routable' do
 
     it { expect(group.full_path).to eq(group.path) }
     it { expect(nested_group.full_path).to eq("#{group.full_path}/#{nested_group.path}") }
-
-    context 'with RequestStore active', :request_store do
-      it 'does not load the route table more than once' do
-        group.expires_full_path_cache
-        expect(group).to receive(:uncached_full_path).once.and_call_original
-
-        3.times { group.full_path }
-        expect(group.full_path).to eq(group.path)
-      end
-    end
-  end
-
-  describe '#expires_full_path_cache' do
-    context 'with RequestStore active', :request_store do
-      it 'expires the full_path cache' do
-        expect(group.full_path).to eq('foo')
-
-        group.route.update(path: 'bar', name: 'bar')
-        group.expires_full_path_cache
-
-        expect(group.full_path).to eq('bar')
-      end
-    end
   end
 
   describe '#full_name' do
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index c1b385aaf76..f56b79119d3 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -295,6 +295,16 @@ describe Namespace do
 
       parent.update(path: 'mygroup_new')
 
+      # Routes are loaded when creating the projects, so we need to manually
+      # reload them for the below code to be aware of the above UPDATE.
+      [
+        project_in_parent_group,
+        hashed_project_in_subgroup,
+        legacy_project_in_subgroup
+      ].each do |project|
+        project.route.reload
+      end
+
       expect(project_rugged(project_in_parent_group).config['gitlab.fullpath']).to eq "mygroup_new/#{project_in_parent_group.path}"
       expect(project_rugged(hashed_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{hashed_project_in_subgroup.path}"
       expect(project_rugged(legacy_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{legacy_project_in_subgroup.path}"
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index b0ec725bf70..deaaaed424b 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2942,8 +2942,6 @@ describe Project do
 
         expect(project).to receive(:expire_caches_before_rename)
 
-        expect(project).to receive(:expires_full_path_cache)
-
         project.rename_repo
       end
 
@@ -3103,8 +3101,6 @@ describe Project do
 
         expect(project).to receive(:expire_caches_before_rename)
 
-        expect(project).to receive(:expires_full_path_cache)
-
         project.rename_repo
       end
 
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index eb4235e3ee6..cf57776346a 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/spec/serializers/pipeline_serializer_spec.rb
@@ -125,7 +125,7 @@ describe PipelineSerializer do
         it 'verifies number of queries', :request_store do
           recorded = ActiveRecord::QueryRecorder.new { subject }
 
-          expect(recorded.count).to be_within(2).of(27)
+          expect(recorded.count).to be_within(2).of(31)
           expect(recorded.cached_count).to eq(0)
         end
       end
@@ -144,7 +144,7 @@ describe PipelineSerializer do
           # pipeline. With the same ref this check is cached but if refs are
           # different then there is an extra query per ref
           # https://gitlab.com/gitlab-org/gitlab-ce/issues/46368
-          expect(recorded.count).to be_within(2).of(30)
+          expect(recorded.count).to be_within(2).of(34)
           expect(recorded.cached_count).to eq(0)
         end
       end
diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb
index 7e85f599afb..1a85c52fc97 100644
--- a/spec/services/projects/transfer_service_spec.rb
+++ b/spec/services/projects/transfer_service_spec.rb
@@ -31,12 +31,6 @@ describe Projects::TransferService do
       transfer_project(project, user, group)
     end
 
-    it 'expires full_path cache' do
-      expect(project).to receive(:expires_full_path_cache)
-
-      transfer_project(project, user, group)
-    end
-
     it 'invalidates the user\'s personal_project_count cache' do
       expect(user).to receive(:invalidate_personal_projects_count)