Remove code for dynamically generating routes

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.

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
 

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

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?

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

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)

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
 

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)
 

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')

changelogs/unreleased/stop-dynamic-routable-creation.yml

+---
+title: Stop dynamically creating project and namespace routes
+merge_request: 20313
+author:
+type: performance

db/migrate/20180702134423_generate_missing_routes.rb

+# 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

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
 

spec/migrations/generate_missing_routes_spec.rb

+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

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

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}"

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
 

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

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)