summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/finders/admin/projects_finder.rb4
-rw-r--r--app/models/concerns/routable.rb44
-rw-r--r--app/models/concerns/storage/legacy_namespace.rb2
-rw-r--r--app/models/namespace.rb1
-rw-r--r--app/models/project.rb3
-rw-r--r--app/serializers/pipeline_serializer.rb9
-rw-r--r--app/services/projects/transfer_service.rb1
-rw-r--r--app/views/admin/projects/_projects.html.haml2
-rw-r--r--changelogs/unreleased/stop-dynamic-routable-creation.yml5
-rw-r--r--db/migrate/20180702134423_generate_missing_routes.rb143
-rw-r--r--spec/controllers/projects/pipelines_controller_spec.rb14
-rw-r--r--spec/migrations/generate_missing_routes_spec.rb84
-rw-r--r--spec/models/concerns/routable_spec.rb33
-rw-r--r--spec/models/namespace_spec.rb10
-rw-r--r--spec/models/project_spec.rb4
-rw-r--r--spec/serializers/pipeline_serializer_spec.rb4
-rw-r--r--spec/services/projects/transfer_service_spec.rb6
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)