Read about our upcoming Code of Conduct on this issue

Commit 3b22743c authored by Yorick Peterse's avatar Yorick Peterse
Browse files

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.
parent 7114d69d922e
......@@ -7,7 +7,7 @@ def initialize(params:, current_user:)
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 @@ def execute
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
......
......@@ -90,34 +90,17 @@ def where_full_path_in(paths)
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 @@ def set_path_errors
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 @@ def full_path_changed?
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 @@ def build_full_name
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
......@@ -11,8 +11,6 @@ def move_dir
Namespace.find(parent_id_was) # raise NotFound early if needed
end
expires_full_path_cache
move_repositories
if parent_changed?
......
......@@ -304,7 +304,6 @@ def force_share_with_group_lock_on_descendants
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
......
......@@ -1233,8 +1233,6 @@ def check_repository_path_availability
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 @@ def rename_repo_notify!
# 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)
......
......@@ -11,10 +11,15 @@ def represent(resource, opts = {})
: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
......
......@@ -77,7 +77,6 @@ def attempt_transfer_transaction
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)
......
......@@ -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')
......
---
title: Stop dynamically creating project and namespace routes
merge_request: 20313
author:
type: performance
# 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
......@@ -55,10 +55,8 @@
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 @@
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
......
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
......@@ -12,16 +12,6 @@
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 @@
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
......
......@@ -295,6 +295,16 @@ def expect_project_directories_at(namespace_path)
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}"
......
......@@ -2942,8 +2942,6 @@ def enable_lfs
expect(project).to receive(:expire_caches_before_rename)
expect(project).to receive(:expires_full_path_cache)
project.rename_repo
end
......@@ -3103,8 +3101,6 @@ def enable_lfs
expect(project).to receive(:expire_caches_before_rename)
expect(project).to receive(:expires_full_path_cache)
project.rename_repo
end
......
......@@ -125,7 +125,7 @@
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 @@ def ref
# 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
......
......@@ -31,12 +31,6 @@
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)
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment